Stream: t-compiler/wg-rls-2.0

Topic: VS Code syntax


Laurențiu Nicola (May 21 2019 at 09:53, on Zulip):

Hello! Do you have any plans regarding the syntax highlighting support for Code? Right now it seems a bit worse than the default TextMate syntax

Laurențiu Nicola (May 21 2019 at 09:53, on Zulip):

I do have a WIP patch that improves things a little, but it won't be enough

matklad (May 21 2019 at 09:54, on Zulip):

I've opened https://github.com/rust-analyzer/rust-analyzer/issues/1294 just now :-)

Laurențiu Nicola (May 21 2019 at 09:54, on Zulip):

Yeah, I added the ThemeColor thing

Laurențiu Nicola (May 21 2019 at 09:55, on Zulip):

and a light theme that more or less matches the default one

matklad (May 21 2019 at 09:55, on Zulip):

We should improve highlighintg quite a bit, both in terms of how we interract with VS Code and what the user sees, as well as internally, in terms of how syntax highlighting is implemented

Laurențiu Nicola (May 21 2019 at 09:55, on Zulip):

one remaining issue is that it doesn't highlight types

matklad (May 21 2019 at 09:56, on Zulip):

one remaining issue is that it doesn't highlight types

There's so many things we can highlight better

matklad (May 21 2019 at 09:56, on Zulip):

the top one for me would be to underlying mut variables and variables with &mut type

Laurențiu Nicola (May 21 2019 at 09:56, on Zulip):

yeah, that would be interesting

Laurențiu Nicola (May 21 2019 at 09:56, on Zulip):

maybe unsafe too

Laurențiu Nicola (May 21 2019 at 09:57, on Zulip):

pasted image

Laurențiu Nicola (May 21 2019 at 09:58, on Zulip):

here's a quick preview

matklad (May 21 2019 at 10:02, on Zulip):

Looks good! Can you send a PR with ThemeColor?

If you are interested in improving the internal implementation, I can write some mentoring instructions as well!

Laurențiu Nicola (May 21 2019 at 10:03, on Zulip):

I'll file one when it's in a better shape. It's still missing a high contrast set of colors and I'm pretty bad at picking them

matklad (May 21 2019 at 10:06, on Zulip):

Thanks!

Laurențiu Nicola (May 21 2019 at 11:07, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/1299

Laurențiu Nicola (May 21 2019 at 13:14, on Zulip):

Can I get rid of the background color? I don't see where it's used

matklad (May 21 2019 at 13:16, on Zulip):

I think you can: iirc, it's the editor how controls the background

Laurențiu Nicola (May 21 2019 at 13:19, on Zulip):

the one here? https://github.com/rust-analyzer/rust-analyzer/blob/master/editors/code/src/highlighting.ts#L22

Laurențiu Nicola (May 21 2019 at 13:19, on Zulip):

I thought they are emitted by RA

Laurențiu Nicola (May 21 2019 at 13:20, on Zulip):

I'm also not sure what builtin is

Laurențiu Nicola (May 21 2019 at 13:51, on Zulip):

can I check what a NAME_REF points to (function, type, variable)?

matklad (May 21 2019 at 13:51, on Zulip):

Yeah, and that's the big part of "better highlighting"

matklad (May 21 2019 at 13:52, on Zulip):

The rule should be that NAME_REF are colored into the same color as the thing they point to

matklad (May 21 2019 at 13:53, on Zulip):

That's not a simple change of the current code. See how goto_definition works, to learn about the API

matklad (May 21 2019 at 13:53, on Zulip):

The TL;DR: is that we should go from syntax to hir, and do resolution there

Florian Diebold (May 21 2019 at 13:54, on Zulip):

that will require doing name resolution for highlighting though -- should there be separate 'levels'? i.e. VSCode probably does simple regex-based (lexer level) highlighting for keywords, then we can do syntax based highlighting, and then semantic

matklad (May 21 2019 at 13:55, on Zulip):

Yeah, that's the other bit of the plan: do highlighting in several phases: lexical, syntactical, semantic/diagnosict

matklad (May 21 2019 at 13:56, on Zulip):

And yet another bit of a puzzle is region-based highlighting: we should highilght only the vewiport

Laurențiu Nicola (May 21 2019 at 14:16, on Zulip):

I tried to cast the node to a NameRef, invoke the go to definition machinery, get a NavigationTarget, but I'm not sure what to do now (:

Laurențiu Nicola (May 21 2019 at 14:37, on Zulip):

ah, nice

Laurențiu Nicola (May 21 2019 at 14:37, on Zulip):

pasted image

matklad (May 21 2019 at 14:50, on Zulip):

@Laurențiu Nicola you shouldn't invoke go to definition machinery exactly, you should use the HIR API directly:

    if let Some(path) = name_ref.syntax().ancestors().find_map(ast::Path::cast) {
        if let Some(resolved) = analyzer.resolve_path(db, path) {
            match resolved {
                hir::PathResolution::Def(def) => return Exact(NavigationTarget::from_def(db, def)),
                hir::PathResolution::LocalBinding(pat) => {
matklad (May 21 2019 at 14:50, on Zulip):

maybe we can extract some of that glue code into a module which is shared by both goto defenition and highlighting, but highlighting shouldn't be invoking goto def directly

Laurențiu Nicola (May 21 2019 at 14:51, on Zulip):

right, that makes sense

Laurențiu Nicola (May 21 2019 at 14:52, on Zulip):

can I just make a new SourceAnalyzer?

Laurențiu Nicola (May 21 2019 at 14:53, on Zulip):

I was using reference_definition

matklad (May 21 2019 at 14:53, on Zulip):

You can make something that wraps SourceAnalyzer and has methods like classify_nameref

Laurențiu Nicola (May 21 2019 at 18:00, on Zulip):
      10571ms - handle_code_action
              0ms - diagnostics
Laurențiu Nicola (May 21 2019 at 18:00, on Zulip):

we need a way to disable these

Laurențiu Nicola (May 21 2019 at 18:01, on Zulip):

actually, it was just loading this time, I guess. but the diagnostics are slow at times

Laurențiu Nicola (May 23 2019 at 12:00, on Zulip):

@matklad The issue with highlighting NAME as function is that it will apply equally to function, type and struct names. I would prefer to have type names and references to them to have the same color

Laurențiu Nicola (May 23 2019 at 12:00, on Zulip):

pasted image

matklad (May 23 2019 at 12:01, on Zulip):

Yeah, we should use something more precise than just "Name"

Laurențiu Nicola (May 23 2019 at 12:01, on Zulip):

It's probably fine short-term, though

matklad (May 23 2019 at 12:01, on Zulip):

"function"

matklad (May 23 2019 at 12:01, on Zulip):

but short-term, "function" is better than "text"

Laurențiu Nicola (May 23 2019 at 12:02, on Zulip):

same for things like

                    BIND_PAT@[3091; 3105)
                      NAME@[3091; 3105)
                        IDENT@[3091; 3105) "TYPE_ALIAS_DEF"
matklad (May 23 2019 at 12:06, on Zulip):

Yeah, I thik we need something like "classify_name" as welll

Laurențiu Nicola (May 23 2019 at 12:07, on Zulip):

Next time :-). I'll update the test and hopefully we're good to go with this

Laurențiu Nicola (May 23 2019 at 12:14, on Zulip):

Gah, I wonder when the Debug comma change will hit stable

matklad (May 23 2019 at 12:15, on Zulip):

This friday? Or is it in the next reelase?

Laurențiu Nicola (May 23 2019 at 12:19, on Zulip):

Yeah, it's in beta

Laurențiu Nicola (May 23 2019 at 13:37, on Zulip):

re #1314, we'll still need to handle the case when classify_name_ref fails

matklad (May 23 2019 at 13:40, on Zulip):

Indeed... I think we need some variants like UnresolvedPathSegment, UnresolvedFieldAccess, etc. Actually, for FieldAccess we can do FieldAccess(Option<hir::StructField>),, that is, we can move None inside some of the variants

Laurențiu Nicola (May 23 2019 at 13:41, on Zulip):

That sounds like a good idea

Laurențiu Nicola (May 23 2019 at 13:42, on Zulip):

Also, this https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_ide_api/src/name_ref_kind.rs#L26-L71 looks a bit weird

Laurențiu Nicola (May 23 2019 at 13:42, on Zulip):

Shouldn't it try to resolve a path first, then handle function calls, macros, and the other things?

Laurențiu Nicola (May 23 2019 at 13:44, on Zulip):

I would expect both f() and m::f() to go through the path logic, with f getting resolved as a function call at the end

matklad (May 23 2019 at 13:45, on Zulip):

That what actually happens

matklad (May 23 2019 at 13:45, on Zulip):

the only special case is method calls

matklad (May 23 2019 at 13:45, on Zulip):

x.foo() is not the same as (x.foo)()

matklad (May 23 2019 at 13:45, on Zulip):

so, you need to special case that

Laurențiu Nicola (May 23 2019 at 13:46, on Zulip):

So it's for methods (not functions) and fields?

Laurențiu Nicola (May 23 2019 at 13:46, on Zulip):

But there's also a case for macros

matklad (May 23 2019 at 13:47, on Zulip):

Hm, yeah, I think macros can be hadled slightly better

matklad (May 23 2019 at 13:47, on Zulip):

We should first fetch a path

matklad (May 23 2019 at 13:47, on Zulip):

and only then check path's parent and call either "resovle_path" or "resolve_macro"

Laurențiu Nicola (May 23 2019 at 13:49, on Zulip):

Yeah, something like that

Laurențiu Nicola (May 23 2019 at 18:56, on Zulip):

I wonder if parts of completion_context.rs could reuse the same logic

Laurențiu Nicola (May 23 2019 at 18:56, on Zulip):

That one also has a classify_name_ref, but it seems to operate only on the AST

matklad (May 24 2019 at 12:52, on Zulip):

@Laurențiu Nicola I wondered that as well!

I think though that completion is a pretty unique subsystem, and it might want to do things differently (for example, it has to process two files, original one and the one with a fake identifier). So, while it seems like there's a duplication there, I think we shouldn't try to agressively remove it

matklad (May 24 2019 at 12:52, on Zulip):

We definitelly want to employ classify_name_ref in hover though!

Laurențiu Nicola (May 29 2019 at 06:09, on Zulip):

Is there a reason why name resolution doesn't seem to work for i32?

Laurențiu Nicola (May 29 2019 at 06:11, on Zulip):

I mean, I imagine why, but is it tracked somewhere?

matklad (May 29 2019 at 06:39, on Zulip):

i32 is a primitive type, so there's nothing we can resolve it to.

matklad (May 29 2019 at 06:45, on Zulip):

OTOH, yeah, the resolver can returns something like "primitive type" here...

Laurențiu Nicola (May 29 2019 at 06:48, on Zulip):

there's this issue that's visible in the HTML colorization, where types get assigned two tags

Laurențiu Nicola (May 29 2019 at 06:48, on Zulip):
<span class="keyword">fn</span> <span class="function">foo</span>&lt;<span class="type type">T</span>&gt;() -&gt; <span class="type">T</span> {
Laurențiu Nicola (May 29 2019 at 06:51, on Zulip):

I think that's because they're handled twice, once as TYPE_PARAM, and once as a NAME

Laurențiu Nicola (May 29 2019 at 06:52, on Zulip):
            TYPE_ALIAS_DEF | TYPE_ARG | TYPE_PARAM => "type",
matklad (May 29 2019 at 06:52, on Zulip):

Yeah, that's a common bug in this kind of code

matklad (May 29 2019 at 06:52, on Zulip):

Some logic for prioritization should be added

matklad (May 29 2019 at 06:53, on Zulip):

Or, we should just explicitly handle all named thigs

Laurențiu Nicola (May 29 2019 at 06:53, on Zulip):

And it makes sense to remove that line, since (I imagine) we want to highlight terminals (if that's the correct term)

Laurențiu Nicola (May 29 2019 at 06:53, on Zulip):

and TYPE_PARAM isn't one

matklad (May 29 2019 at 06:53, on Zulip):

Thats, we don't color name when we visit name, we color it when we visit type alias, function definition, struct definition, etc

Laurențiu Nicola (May 29 2019 at 06:53, on Zulip):

but then things like i32 won't be highlighted

matklad (May 29 2019 at 06:54, on Zulip):

but my gut feeling is that removing TYPE_ALIAS_DEF | TYPE_ARG | TYPE_PARAM => "type", is ok

Laurențiu Nicola (May 29 2019 at 06:54, on Zulip):

yeah, I meant the other way around (highlight names, not definitions)

Laurențiu Nicola (May 29 2019 at 07:13, on Zulip):

So I remember a BUILTIN NodeKind that is not used, was it supposed to be used for primitive types or something else?

Laurențiu Nicola (May 29 2019 at 07:14, on Zulip):

And I think there's no UNION_DEF

matklad (May 29 2019 at 07:14, on Zulip):

I wouldn't say that original kinds were really supposed to be used for anything. I think I've just copy-pasted them from fall, so, if something doesn't make sense, it probably indeed is senseless :)

Laurențiu Nicola (May 29 2019 at 07:15, on Zulip):

Fair enough

matklad (May 29 2019 at 07:15, on Zulip):

I am not sure if we should color primitive types like ixx, fxx str and char specially

Laurențiu Nicola (May 29 2019 at 07:15, on Zulip):

Like types, maybe? Or keywords

matklad (May 29 2019 at 07:16, on Zulip):

I think they should be just types

matklad (May 29 2019 at 07:16, on Zulip):

more specifically, structs

Laurențiu Nicola (May 29 2019 at 07:16, on Zulip):

Yup

matklad (May 29 2019 at 07:16, on Zulip):

(I think we can color enums and structs differently, though I don't know if we nececssary should)

Laurențiu Nicola (May 29 2019 at 07:17, on Zulip):

And that should be handled by type resolution, not in the grammar, right?

Laurențiu Nicola (May 29 2019 at 07:17, on Zulip):

The primitives

matklad (May 29 2019 at 07:17, on Zulip):

Hm

matklad (May 29 2019 at 07:17, on Zulip):

not by the grammar, but not by the type resolution either

matklad (May 29 2019 at 07:18, on Zulip):

I think this is the job of name resolution

matklad (May 29 2019 at 07:18, on Zulip):

(although, I guess, currently this logic wrongly lives in type inference code, let me take a look)

Florian Diebold (May 29 2019 at 07:24, on Zulip):

Yeah, the primitive type resolution currently lives in type inference, I think because the name resolution doesn't know about types and there's nothing 'intermediate' the primitives can resolve to

matklad (May 29 2019 at 07:26, on Zulip):

TIL: this is valid rust: pub use i32 as int;

matklad (May 29 2019 at 07:27, on Zulip):

So, name resolution needs to learn about i32 and friends

matklad (May 29 2019 at 08:35, on Zulip):

submitted https://github.com/rust-analyzer/rust-analyzer/issues/1340

Florian Diebold (May 29 2019 at 09:10, on Zulip):

maybe we should just give them a StructId?

matklad (May 29 2019 at 09:12, on Zulip):

I was thinking about that, and builtins seems cleaner: with StructId, we'd have to assign them to a particular crate, handle source, etc

matklad (May 29 2019 at 09:13, on Zulip):

Or you mean, assigning StructId, but not allowing to create a Struct of them?

Florian Diebold (May 29 2019 at 09:13, on Zulip):

no, if we assign a StructId we need to allow creating a Struct... but yeah, maybe it's more trouble than it's worth

matklad (May 29 2019 at 10:07, on Zulip):

This is nicely handled in Go. In the stdlib, they have stub like extern type int;, so primitive types have a real definition in stdlib

matklad (May 29 2019 at 10:07, on Zulip):

this won't work for rust due to no_core

Laurențiu Nicola (Aug 20 2019 at 14:37, on Zulip):

An update from Code: https://github.com/microsoft/vscode/issues/77140

Jeremy Kolb (Aug 20 2019 at 15:44, on Zulip):

Oh neat. I haven't seen tree-sitter before.

matklad (Aug 20 2019 at 15:49, on Zulip):

Tree-sitter is an absolute :fire: !

Daniel Mcnab (Aug 20 2019 at 15:52, on Zulip):

Is that a good :fire: or a :trash_can: :fire:? The use of the indefinite article suggests the latter

matklad (Aug 20 2019 at 15:53, on Zulip):

a good one!

Daniel Mcnab (Aug 20 2019 at 15:54, on Zulip):

Fair enough!

Last update: Nov 19 2019 at 18:35UTC