Implement constructor tearoffs support in syntax highlighters

[eernstg]

This issue tracks support of the constructor tearoffs feature. See the enclosing meta-issue for details.

• https://github.com/dart-lang/dart-syntax-highlight @DanTup
• LSP semantic tokens @DanTup
• https://github.com/codemirror/CodeMirror @domesticmouse
• https://github.com/highlightjs/highlight.js @srawlins

[devoncarew]

The three repos to update:

• https://github.com/dart-lang/dart-syntax-highlight/
• https://github.com/highlightjs/highlight.js
• https://github.com/codemirror/CodeMirror/

[devoncarew]

Note that this issue currently does not have an owner.

[eernstg]

@srawlins and @keertip, could you suggest an owner for this issue?

[srawlins]

Good question. I think Ivan has previously updated the internal parser for Code Search. I've previously contributed to CodeMirror and highlight.js. I'm not sure who would do VSCode, GitHub, IntelliJ, ...

[devoncarew]

@DanTup could probably take a look at https://github.com/dart-lang/dart-syntax-highlight/; it's likely it won't need changes. That powers both github and vs code.

[DanTup]

Neat, I'll take a look! LSP semantic tokens (and analysis server highlights) might also need tweaking - though I think
#46020 might need to be completed first.

[bwilkerson]

I suspect that the underlying implementation is far enough along to support any work that needs to be done. The AST structure is defined and I believe it's fully resolved in the cases where it's being produced. However, unless we want to start highlighting tear-offs differently (and we haven't done that for other kinds of tear-offs, though it's an interesting idea), it isn't clear to me that there's any work to be done. We'll need to confirm that either way, of course.

[keertip]

@emmanuel-p, for code search support

[DanTup]

@bwilkerson what's the correct way to enable this for (manual) testing? When I try to copy the sample code I get an error saying I need to use an SDK constraint of >=2.15.0 for the feature to be enabled, but if I try that I get complaints running pub get because my SDK isn't high enough (it's 2.15.0-edge... which is semantically lower than 2.15.0). If I try using 2.15.0-0 as the constraint, I get the original error about needing >=2.15.0.

nevermind, I found I can add this to analysis_options.yaml:

analyzer:
  enable-experiment:
    - constructor-tearoffs

[bwilkerson]

Yep.

[DanTup]

For the textmate grammar, torn-off constructors are treated like fields, and then functions where they've invoked:

I'm not sure if there's a way to improve this (since the colouring is basic and just uses the presence of the parens to colour the item).

With LSP semantic tokens enabled, they become coloured like constructors, and also have the custom constructor modifier:

Although I'm not certain whether I'd expect aaa and bbb here to be yellow where they've invoked (they're currently coloured as variables). What's shown here is consistent with other functions (eg. eee in eee() is also coloured as a variable and not a function/method) though I'm not sure whether that's also accidental or deliberate. @bwilkerson any thoughts?

[devoncarew]

Thanks all; I've updated the first comment on this issue to break out the anticipated work. I've also assigned speculative owners, so we know this is tracking forward; cc @DanTup @srawlins @domesticmouse. Note that some of the grammars are simple enough that the work here might not be implementation but just validation that things look reasonable.

[devoncarew]

Note that out timeframe is to have constructor tear-offs feature complete by early October.

[DanTup]

I've opened https://dart-review.googlesource.com/c/sdk/+/213263/ to map the new Highlight Kinds in LSP so the colouring still works. We're not doing anything special for tear-offs so they'll just be coloured as standard functions/methods/constructors (we can easily add a custom modifier to colour them differently, but I don't know if that's a current goal?).

I think that addresses the second item in the task list above (unless we do want to colour them differently?), and I don't think there's anything to do for the first since it doesn't use semantics for colouring (although I've verified things look by my expectations).

[domesticmouse]

Can I get clarity on what we want CodeMirror to do for constructor tearoffs? I've looked at what the above code does in DartPad and it looks sane to me (after ignoring the analysis errors):

[srawlins]

Personally I think that looks fine.

[bwilkerson]

Can I get clarity on what we want CodeMirror to do for constructor tearoffs?

I don't have a definitive answer. The design principle I'd apply is that code should be highlighted differently if doing so aids the user's understanding of the code. In the case of tear-offs I think that means that they should be highlighted differently if

• it's important to the understanding of the code to know whether a constructor/method/function is being invoked vs being torn off (which I think it is) and
• the syntactic difference (sometimes as little as ()) and type errors aren't sufficient to make that distinction clear.

I don't know whether the syntactic difference and type errors are sufficient (I don't have any practical experience with it, nor have we done any user studies), but I suspect that they probably are.

My recommendation is to leave them highlighted as is for now and we can change it if it proves to be necessary.

[devoncarew]

@srawlins - I'm going to speculatively assume that we're good wrt highlight.js; I don't believe this is a real change to the grammar for what that highlighter is trying to do.