Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Difference in behavior with encodedSemanticClassifications-full in TS 4.1 compared to using VS Code plugin #41262

Closed
mjbvz opened this issue Oct 27, 2020 · 6 comments · Fixed by #42438
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Oct 27, 2020

TypeScript Version: 4.1.0-dev.20201026

Repro

  1. Using TS 4.1.0-dev.20201026 and a build of VS Code that explicitly disables the typescript-vscode-sh-plugin plugin

    Here's the change in the VS Code repo that disables the plugin:

    diff --git a/extensions/typescript-language-features/package.json b/extensions/typescript-language-features/package.json
    index dfd3f6295a..aa3763aaf7 100644
    --- a/extensions/typescript-language-features/package.json
    +++ b/extensions/typescript-language-features/package.json
    @@ -1114,12 +1114,6 @@
            }
            ]
        }
    -    ],
    -    "typescriptServerPlugins": [
    -      {
    -        "name": "typescript-vscode-sh-plugin",
    -        "enableForWorkspaceTypeScriptVersions": true
    -      }
        ]
    }
    }
  2. Use the TS File:

    class Foo {}
    
    const a = Foo;
    
    console.log(a);
  3. Look at semantic highlighting

Bug
With TS 4.1 and the plugin disabled, I see this response:

[Trace  - 00:50:46.802] <semantic> Response received: encodedSemanticClassifications-full (17). Request took 1673 ms. Success: true 
Result: {
    "spans": [
        6,
        3,
        11,
        24,
        3,
        11
    ],
    "endOfLineState": 0
}

With the plugin enabled, I see:

[Trace  - 00:50:59.813] <semantic> Response received: encodedSemanticClassifications-full (16). Request took 980 ms. Success: true 
Result: {
    "spans": [
        6,
        3,
        257,
        20,
        1,
        257,
        24,
        3,
        256,
        30,
        7,
        2064,
        38,
        3,
        3088,
        42,
        1,
        256
    ],
    "endOfLineState": 0
}

As you can see, the plugin seems to be providing a lot more information

@orta Is there any work we need to do on the VS Code side to adopt the new semantic work in TS 4.1?

/cc @aeschli

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Nov 4, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.2.0 milestone Nov 4, 2020
@orta
Copy link
Contributor

orta commented Nov 4, 2020

Ah yeah, I made it default to the old behavior so that VS wasn't forced to make changes!

The API for getSemanticClassifications in the language service now has an optional third param which is a 'format':

getSemanticClassifications(fileName: string, span: TextSpan, format: SemanticClassificationFormat): ClassifiedSpan[] | ClassifiedSpan2020[];

Which you can put "2020":

export const enum SemanticClassificationFormat {
         Original = "original",
         TwentyTwenty = "2020"
     }

To trigger the new formatter, I expect that's just an extra param somewhere on the vscode side?

@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 20, 2020

Thanks @orta. I gave this a try but can't get it working. The problems I ran into:

  • I'm not sure how I can pass the format parameter to the server. I tried adding it to the request but that doesn't seem to work

  • I don't see the new semantic request types in the server's protocol.d.ts

@orta
Copy link
Contributor

orta commented Nov 24, 2020

I've sent a PR updating the 4.0 branch with the param, will just d the same thing for the master branch now

@mjbvz
Copy link
Contributor Author

mjbvz commented Dec 2, 2020

Thanks @orta. Just tested this on 4.2.0-dev.20201201 and still seeing issues (although I'm not 100% that the changes have been picked up):

  • SemanticDiagnosticsSyncRequestArgs Does not include a format argument in the protocol typings

  • SemanticDiagnosticsSyncRequestArgs does not seem to take a range for the file. In encodedSemanticClassifications-full, we currently do pass a range

  • Even if I try force add format: '2020' to the request, I still don't see the more complete results returned

@orta
Copy link
Contributor

orta commented Dec 2, 2020

Do you have a WIP vscode branch? I can try get that up and running and work out the kinks?

@mjbvz
Copy link
Contributor Author

mjbvz commented Dec 5, 2020

@orta Here's a branch with the changes: https://github.com/microsoft/vscode/tree/dev/mjbvz/semantic-ts. And the relevant change

I actually was using the wrong types in my earlier comments. SemanticDiagnostics is not what we want. Instead it looks like SemanticClassifications types are missing from the protocol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants