Import the semantic highlighter from typescript-vscode-sh-plugin #39119

orta · 2020-06-17T19:59:40Z

Part 1 of 2 - Re: #38435

Migrates the codebase over, almost entirely free of semantic changes
Adds all their test
Ensures all our older semantic tests also run on the new output

sandersn

Seems reasonable. The style of the code is kind of strange in places, but might be OK for now. The only change I'd make is to invert the if so that the original classifier is the fallback default.

src/services/classifierVscode.ts

src/services/services.ts

src/harness/fourslashImpl.ts

src/compiler/types.ts

tests/baselines/reference/api/tsserverlibrary.d.ts

src/services/services.ts

orta · 2020-06-22T19:40:26Z

Thanks folks - this has been updated with all the feedback

src/harness/fourslashImpl.ts

src/services/classifier2020.ts

orta · 2020-06-23T15:09:11Z

Re: naming, this is tricky - this isn't an LSP standard or anything, and it's vscode specific.

Some options for name differentiation:

tokenAndModifiers
semanticTokens

orta · 2020-06-25T17:07:42Z

Another option for a name to differentiate:

refined (there are less options in the new one)

orta · 2020-07-01T15:39:21Z

We could assume that the format will make it into the LSP spec, and call the format "lsp"?

DanielRosenwasser · 2020-07-01T18:51:37Z

I suppose let's just use the year. We can @deprecate it if we come up with a better name.

…nstead of numbers

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

orta · 2020-07-07T15:35:51Z

OK, this is rebased, updated to use 2020 everywhere and should be good to go - @sheetalkamat any chance I could get another look over?

src/services/classifier2020.ts

sheetalkamat · 2020-07-07T18:07:51Z

src/services/classifier2020.ts

+
+    function classifySymbol(symbol: Symbol, meaning: SemanticMeaning): TokenType | undefined {
+        const flags = symbol.getFlags();
+        if (flags & SymbolFlags.Class) {


Do you need early return if

if ((flags & SymbolFlags.Classifiable) === SymbolFlags.None)

It could, but I'd need to make a new SymbolFlag for it as there are things like functions and variable declarations in this classifier also, do you have an opinion either way?

No but on that note. Is this more additive compared to our original classifier? If so why not use this all the time and filter out things in original format mode ?

The results sent back are different, and conform to a WIP LSP spec. The current one is used by VS, so we basically need to support both. I think this test shows the differences in the results quite well:

var c = classification; const c = classification("original"); verify.syntacticClassificationsAre( c.comment(firstCommentText), c.keyword("function"), c.identifier("myFunction"), c.punctuation("("), c.comment("/* x */"), c.parameterName("x"), c.punctuation(":"), c.keyword("any"), c.punctuation(")"), c.punctuation("{"), c.keyword("var"), c.identifier("y"), c.operator("="), c.identifier("x"), c.operator("?"), c.identifier("x"), c.operator("++"), c.operator(":"), c.operator("++"), c.identifier("x"), c.punctuation(";"), c.punctuation("}"), c.comment("// end of file")); c.comment("// end of file")); const c2 = classification("2020"); verify.semanticClassificationsAre("2020", c2.semanticToken("function.declaration", "myFunction"), c2.semanticToken("parameter.declaration", "x"), c2.semanticToken("variable.declaration.local", "y"), c2.semanticToken("parameter", "x"), c2.semanticToken("parameter", "x"), c2.semanticToken("parameter", "x"), );

The first format (original) includes everything form comments to braces, the second (LSP) is quite an explicit a subset of those results

But if thats the case isnt it better to classify things in one place as function. variable, comments etc and filter out and swithc things depending on format ?

Eg. returned "function.declaration" becomes identifier in original.
punctuation, comment is ignored in 2020 etc

That not an unreasonable idea! However the current implementation is a scanner, but I need to use an AST to get resolved information for things which are outside the scope of the current file. They do end up with quite different semantics given that one works entirely via an AST because it's a semantic vs syntactic.

The subsequent PR from this orta@58e830c#diff-830691d11bb3d3f0f7ca90ef0ee364afR266 speeds up the processing but requires the AST ahead of time, and while it could be implemented on top of the current classifier scanner - there's quite a mis-match between what they're doing

Poke on ^ - is that reasonable?

src/services/classifier2020.ts

sheetalkamat · 2020-07-07T18:11:16Z

tests/baselines/reference/api/typescript.d.ts

@@ -5557,7 +5571,7 @@ declare namespace ts {
    }
    interface ClassifiedSpan {
        textSpan: TextSpan;
-        classificationType: ClassificationTypeNames;
+        classificationType: ClassificationTypeNames | number;


I think this is going to be breaking change.. Everyone who uses this type (even if they arent using the new classification) are going to have to handle this and that doesnt seem necessary

Why does this have to be number and not string just like before?

If this has to be number then we need overloads that return the new type with number (should definitely use enum instead as what does number mean to API user ?) only if format is specified

Great point, I can think I can make this not a breaking change with some overloads and a separate type. The number makes sense in context (one goal of the new system is to be much less chattier, so it sends something like 6701 instead of class.declaration)

I took a stab at this in orta#4 but haven't got it compiling yet

Definitely needs enum in that case

Calling it an enum instead of a number is tricky because it represents an encoded mix of both ts.classifier.v2020.TokenType and ts.classifier.v2020.TokenModifier. It's not a straight forward 1 to 1 map.

I could make a new type like type ClassifierTokenTypeSpan = number which describes what it does?

src/services/classifier2020.ts

src/services/types.ts

WIP - don't provide a breaking change

… semantic_2

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>

orta · 2020-09-09T14:59:08Z

Alright, all feedback in both PRs have been applied, it's up to date with master and shouldn't be a breaking change for API consumers 👍🏻

rbuckton

Overall this seems fine. I have a small nitpick and one perf-related question in the comments.

src/services/classifier2020.ts

rbuckton · 2020-09-10T22:41:20Z

src/services/classifier2020.ts

+        return (isQualifiedName(node.parent) && node.parent.right === node) || (isPropertyAccessExpression(node.parent) && node.parent.name === node);
+    }
+
+    const tokenFromDeclarationMapping: { [name: string]: TokenType } = {


Should this be a Map (nee. ESMap internally)? If it's an object and the property accesses on the use sites above are possible inline cache misses, it can result in deoptimizations.

Thanks 0 I've used a new Map as I found a few of those in the codebase and I assume this is fine?

sheetalkamat · 2020-09-14T18:26:26Z

tests/baselines/reference/api/typescript.d.ts

@@ -5568,6 +5584,10 @@ declare namespace ts {
        textSpan: TextSpan;
        classificationType: ClassificationTypeNames;
    }
+    interface ClassifiedSpan2020 {
+        textSpan: TextSpan;
+        classificationType: number;


This should definitely be not number but enum.. How does API user know what number means which classification otherwise?

typescript-bot assigned orta Jun 17, 2020

typescript-bot added the Author: Team label Jun 17, 2020

orta mentioned this pull request Jun 18, 2020

Help TS team to assume ownership of semantic highlighting in TS tooling microsoft/vscode#92789

Closed

orta requested review from weswigham and sandersn June 19, 2020 11:49

sandersn approved these changes Jun 22, 2020

View reviewed changes

src/services/classifierVscode.ts Outdated Show resolved Hide resolved

src/services/services.ts Outdated Show resolved Hide resolved

src/harness/fourslashImpl.ts Outdated Show resolved Hide resolved

src/harness/fourslashImpl.ts Outdated Show resolved Hide resolved

sheetalkamat requested changes Jun 22, 2020

View reviewed changes

src/compiler/types.ts Outdated Show resolved Hide resolved

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

src/services/services.ts Outdated Show resolved Hide resolved

orta added a commit to orta/TypeScript that referenced this pull request Jun 22, 2020

Handle feedback from microsoft#39119

b812367

orta requested a review from sheetalkamat June 22, 2020 22:18

DanielRosenwasser reviewed Jun 22, 2020

View reviewed changes

src/harness/fourslashImpl.ts Show resolved Hide resolved

DanielRosenwasser reviewed Jun 22, 2020

View reviewed changes

src/harness/fourslashImpl.ts Outdated Show resolved Hide resolved

DanielRosenwasser reviewed Jun 23, 2020

View reviewed changes

aeschli mentioned this pull request Jun 29, 2020

[semantic][typescript] Semantic highlighting for imports should be optional microsoft/vscode#93017

Closed

orta added a commit to orta/TypeScript that referenced this pull request Jun 29, 2020

Handle feedback from microsoft#39119

5841110

orta force-pushed the semantic_2 branch from 87bbcdc to c57193c Compare June 29, 2020 20:19

orta added a commit to orta/TypeScript that referenced this pull request Jun 29, 2020

Handle feedback from microsoft#39119

a25badb

orta added a commit to orta/TypeScript that referenced this pull request Jul 6, 2020

Handle feedback from microsoft#39119

d2057c8

orta force-pushed the semantic_2 branch from 0c45a0f to 6ba0d3f Compare July 6, 2020 17:32

orta and others added 7 commits July 7, 2020 11:34

Initial import of the vscode semantic highlight code

1d3a728

Adds the ability to test modern semantic classification via strings i…

fa8a499

…nstead of numbers

Adds existing tests

8a5c3b3

Port over the semantic classification tests

2a4113b

Update baselines

8728a1e

Update src/harness/fourslashImpl.ts

78ad1d7

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

Handle feedback from microsoft#39119

c43374b

orta and others added 5 commits July 7, 2020 11:34

Update baselines

524e475

Apply suggestions from code review

e1ce709

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

Update src/harness/fourslashImpl.ts

7e35e53

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

Reafactor after comments

8a7596a

Use 2020 everywhere

b8742c3

orta force-pushed the semantic_2 branch from 6ba0d3f to b8742c3 Compare July 7, 2020 15:34

sheetalkamat requested changes Jul 7, 2020

View reviewed changes

orta added 2 commits July 8, 2020 08:14

Handle feedback

473f651

WIP - don't provide a breaking change

2b663a8

orta mentioned this pull request Jul 8, 2020

WIP - don't provide a breaking change orta/TypeScript#4

Merged

sheetalkamat requested changes Aug 17, 2020

View reviewed changes

src/services/classifier2020.ts Outdated Show resolved Hide resolved

src/services/types.ts Outdated Show resolved Hide resolved

sandersn assigned sheetalkamat Sep 4, 2020

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 4, 2020

sandersn unassigned orta Sep 4, 2020

orta and others added 5 commits September 9, 2020 08:59

Fix all build errors

219c64a

Merge pull request #4 from orta/semantic_2_2

415b30f

WIP - don't provide a breaking change

Merge branch 'master' of https://github.com/microsoft/TypeScript into…

1747860

… semantic_2

Update baselines

a9035d6

Update src/services/classifier2020.ts

8739cbf

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>

rbuckton requested changes Sep 10, 2020

View reviewed changes

Addresses Ron's feedback

ff748a5

rbuckton approved these changes Sep 11, 2020

View reviewed changes

orta merged commit db5368d into microsoft:master Sep 11, 2020

sheetalkamat reviewed Sep 14, 2020

View reviewed changes

andrewbranch mentioned this pull request Mar 22, 2022

Supporting Efficient Semantic Highlighting #38435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import the semantic highlighter from typescript-vscode-sh-plugin #39119

Import the semantic highlighter from typescript-vscode-sh-plugin #39119

orta commented Jun 17, 2020 •

edited

Loading

sandersn left a comment

orta commented Jun 22, 2020

orta commented Jun 23, 2020

orta commented Jun 25, 2020

orta commented Jul 1, 2020

DanielRosenwasser commented Jul 1, 2020

orta commented Jul 7, 2020

sheetalkamat Jul 7, 2020

orta Jul 8, 2020

sheetalkamat Jul 8, 2020

orta Jul 9, 2020

sheetalkamat Jul 9, 2020

orta Jul 14, 2020 •

edited

Loading

orta Aug 6, 2020

sheetalkamat Jul 7, 2020

sheetalkamat Jul 7, 2020

sheetalkamat Jul 7, 2020

orta Jul 8, 2020

orta Jul 8, 2020

sheetalkamat Jul 8, 2020

orta Sep 9, 2020 •

edited

Loading

orta commented Sep 9, 2020

rbuckton left a comment

rbuckton Sep 10, 2020

orta Sep 11, 2020

sheetalkamat Sep 14, 2020

Import the semantic highlighter from typescript-vscode-sh-plugin #39119

Import the semantic highlighter from typescript-vscode-sh-plugin #39119

Conversation

orta commented Jun 17, 2020 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

orta commented Jun 22, 2020

orta commented Jun 23, 2020

orta commented Jun 25, 2020

orta commented Jul 1, 2020

DanielRosenwasser commented Jul 1, 2020

orta commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

orta commented Sep 9, 2020

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta commented Jun 17, 2020 •

edited

Loading

orta Jul 14, 2020 •

edited

Loading

orta Sep 9, 2020 •

edited

Loading