Support highlighting reassigned local variables in IntelliJ

[davidmorgan]

For Java, IntelliJ offers color/font/highlighting for local variables based on whether they are ever reassigned.

(Settings->Editor->Colors & Fonts->Java, Variables->Local variable, Variables->Reassigned local variable).

Please add equivalent support for Dart.

Reasoning: some (me included!) currently write "final" instead of "var" where possible to highlight what can be reassigned. The IDE should be configurable to do this for us, so we can just write "var" everywhere.

Thanks!

[mit-mit]

Adding a screenshot (poor color choice mine):

[davidmorgan]

Any ideas if/when this might happen, please?

Alternatively, if someone can point me in the right direction I might take a shot at it, since I seem to be the person who wants it most :)

[mit-mit]

cc @devoncarew

[dave26199]

Any pointers please? If this is easy to implement I'd like to take a look.

[mit-mit]

@devoncarew @pq @stevemessick can any of you offer some pointers for how to about this?

[devoncarew]

I think this would qualify as semantic highlighting - highlighting info decided on in the analysis server and sent over to IntelliJ. See hereabouts for the spec: https://htmlpreview.github.io/?https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/doc/api.html#type_HighlightRegionType

@scheglov has the most experience with this on the analysis server side and @alexander-doroshko on the IntelliJ side.

[scheglov]

See the semantic highlighting computer. Because in general adding new highlighting types is not backward compatible, you will need to create another copy and activate it only when --useAnalysisHighlight3 is passed to the Analysis Server.

[davidmorgan]

Is there a guide somewhere to working on the analyzer code please? I tried downloading the SDK from github, and running "dart test/integration/analysis/highlights_test2.dart"; but I get errors about missing import "analyzer-0.28.1/lib/dart/ast/standard_resolution_map.dart" -- no idea what's going on here.

If I can get it running I guess the code change isn't too hard. How does adding a new highlight type work from the IntelliJ side, please? Is there work needed there to support a new type / to pass --useAnalysisHighlight3?

[scheglov]

I think you ran pub get in one of the packages. You should not do this. SDK already comes with the .packages file, which is used to keep packages compatible with each other and third_party.

Yes, you need to change the Dart plugin for IDEA. You need to add new identifiers to the configuration, and use them to process semantic highlighting from Analysis Server.

[davidmorgan]

That was it, thanks. Removing the wrong .packages allowed me to run the test.

Thanks for the plugin links.

[bwilkerson]

If we're going to add a useAnalysisHighlight3 option, then I want us to spend some time thinking about whether there are other highlighting changes we would like to add. I don't want to add a new flag every time we add a single highlighting kind.

@jwren @devoncarew @alexander-doroshko Can you think of any additional highlighting information we'd like to have server report (even if we don't have time to implement it now)?

[alexander-doroshko]

I've found 2 more requests submitted by @zoechi:

• 26727 (static field reference)

• 28314 (about constructors, class references, and annotations)

[davidmorgan]

Implementation question -- I'm sure this is obvious to anyone used to working on AST visitors :)

Highlighting reassigned locals is going to need some kind of lookahead -- because we need to highlight it where it's declared/used based on what happens later.

Is there a standard pattern for doing this? I guess something like:

• Visitor that builds a list of reassigned identifiers
• Whenever we enter a scope (method declaration, constructor declaration, ...) call this visitor and store the results ... where? In DartUnitHighlightsComputer3?

Thanks!

[bwilkerson]

I'm sure this is obvious to anyone used to working on AST visitors

I wish that were true :-)

I don't know that it's best, but I would probably extend _DartUnitHighlightsComputerVisitor3 to keep track of the current local variable scope, allowing scopes to nest. (There isn't a class that will do exactly what you need, so you'd have to write this yourself.) Each scope would remember the variables defined in that scope, and for each variable the information you need in order to produce highlighting information (such as the offset of the variable and whether it was re-assigned). You can then produce the highlighting regions when exiting a scope (because you know that you've now seen everything you need to see about the variables).

[scheglov]

We might already have information you need. See potentiallyMutatedInScope.

[davidmorgan]

@scheglov Thanks! That does indeed make it very easy to implement.

I realized that to try it I can use one of the other highlights that I don't need -- dynamic locals -- to avoid having to change the protocol. So, currently giving this highlight a try to confirm that it's useful enough to be worth all the hassle... will also see if others are interested.

[davidmorgan]

User interface question: it looks like we're doing to have to double up the existing set of "local variable" highlights, leading to 8 highlights for them:

(reassigned/not) x (dynamic/not) x (declaration/reference) local variable

One alternative that comes to mind is we put the highlight on the symbol before the variable name, i.e. on the "var" or the type annotation. Then we'd have one new highlight: "type annotation (or var) of reassigned local". I'd have to try this to see if it can be implemented in a reasonable way -- and if it's as nice to use.

Any other suggestions? Or should we just live with eight "local variable" highlights?

Thanks!

[bwilkerson]

Personally, I don't think I'd like having type annotations highlighted differently based on whether the variable being defined is reassigned, but I'd have to see it to know for sure.

I will point out that Dart allows you to declare multiple variables in any given declaration

var x, y, z;

and therefore the keyword / type annotation isn't always associated with a single variable, which could lead to confusion. How would you highlight the above if x and z are only assigned once, but y is reassigned?

[alexander-doroshko]

For Java support in IntelliJ this is solved by using a composition of 2 or more highlighting types for the same region. For example, the fact that this is a variable causes certain foreground color and the fact that it is reassigned adds underscore effect. To avoid N x M x K matrix of colors analysis server could probably provide several highlighting types for the same region and clients should be ready for this composition. Though not sure if current spec allows it. Maybe you'll need to define in the spec that some highlighting types are 'base' and others are additional.

[bwilkerson]

The spec says

Note that the highlight regions that are returned can overlap other highlight regions if there is more than one meaning associated with a particular region.

but doesn't specify what clients should do when this happens. I suspect that server isn't currently returning such data and that it's untested on all clients, but it is a viable alternative.

[alexander-doroshko]

I suspect that server isn't currently returning such data

Thanks for the reminder! I remembered one such case, it is annotations. We have a bigger region of ANNOTATION type and inside there's a smaller one of e.g. TOP_LEVEL_GETTER_REFERENCE. Currently, the inner region wins, though not sure I like it For example, annotation highlighting in Java looks better to me. Though this looks more like a client-side question, not server-side.

[bwilkerson]

Unless server just shouldn't be generating those highlights at all...

[davidmorgan]

I played with the overlapping regions options a bit.

There seems to be some arbitrary order about which highlights win if they overlap. This matters particularly if the regions are not overlapping but actually the same, as would be the case here.

I managed to implement the highlighting one symbol over ("var" in "var x", instead of "x"); I'll see what this is like to use. Re: what happens if you declare multiple variables in one go; I'd just highlight if any of them is reassigned.