Add support for draft of semantic tokens LSIF spec to LSIF generator#65860
Conversation
38ebc65 to
61a1bb3
Compare
|
|
||
| AssertEx.EqualOrDiff( | ||
| "{""hoverProvider"":true,""declarationProvider"":false,""definitionProvider"":true,""referencesProvider"":true,""typeDefinitionProvider"":false,""documentSymbolProvider"":false,""foldingRangeProvider"":true,""diagnosticProvider"":false,""id"":1,""type"":""vertex"",""label"":""capabilities""} | ||
| "{""hoverProvider"":true,""declarationProvider"":false,""definitionProvider"":true,""referencesProvider"":true,""typeDefinitionProvider"":false,""documentSymbolProvider"":false,""foldingRangeProvider"":true,""diagnosticProvider"":false,""semanticTokensProvider"":{""tokenTypes"":[""namespace"",""type"",""class"",""enum"",""interface"",""struct"",""typeParameter"",""parameter"",""variable"",""property"",""enumMember"",""event"",""function"",""method"",""macro"",""keyword"",""modifier"",""comment"",""string"",""number"",""regexp"",""operator"",""class name"",""constant name"",""delegate name"",""enum member name"",""enum name"",""event name"",""excluded code"",""extension method name"",""field name"",""interface name"",""json - array"",""json - comment"",""json - constructor name"",""json - keyword"",""json - number"",""json - object"",""json - operator"",""json - property name"",""json - punctuation"",""json - string"",""json - text"",""keyword - control"",""label name"",""local name"",""method name"",""module name"",""namespace name"",""operator - overloaded"",""parameter name"",""preprocessor keyword"",""preprocessor text"",""property name"",""punctuation"",""record class name"",""record struct name"",""regex - alternation"",""regex - anchor"",""regex - character class"",""regex - comment"",""regex - grouping"",""regex - other escape"",""regex - quantifier"",""regex - self escaped character"",""regex - text"",""string - escape character"",""string - verbatim"",""struct name"",""text"",""type parameter name"",""whitespace"",""xml doc comment - attribute name"",""xml doc comment - attribute quotes"",""xml doc comment - attribute value"",""xml doc comment - cdata section"",""xml doc comment - comment"",""xml doc comment - delimiter"",""xml doc comment - entity reference"",""xml doc comment - name"",""xml doc comment - processing instruction"",""xml doc comment - text"",""xml literal - attribute name"",""xml literal - attribute quotes"",""xml literal - attribute value"",""xml literal - cdata section"",""xml literal - comment"",""xml literal - delimiter"",""xml literal - embedded expression"",""xml literal - entity reference"",""xml literal - name"",""xml literal - processing instruction"",""xml literal - text""],""tokenModifiers"":[]},""id"":1,""type"":""vertex"",""label"":""capabilities""} |
There was a problem hiding this comment.
I worry here that any time we update the list of semantic token types we'll have to go update this test. Can we update this test to itself grab the list of semantic token types and construct the expected string that way?
There was a problem hiding this comment.
I think there's value to having at least one test that checks the actual list of token types because 1) there's a shared code path between LSP and LSIF here and 2) LSP and LSIF have slightly different expectations of which tokens get returned.
I want to make sure that evolution of LSP doesn't accidentally regress the returning of all token types in LSIF.
How about if I replace these two instances of the list with SemanticTokensHelpers.AllTokenTypes? That type is internal though so it will require IVT.
There was a problem hiding this comment.
How about if I replace these two instances of the list with SemanticTokensHelpers.AllTokenTypes? That type is internal though so it will require IVT.
That'd be perfect; adding IVT is fine here.
There was a problem hiding this comment.
Are you willing to go without this one? Assembling correctly indented JSON from a combination of strings and a list of strings is going to be sort of a pain though it's easy enough to do for the regular LSIF output.
| ""documentSymbolProvider"": false, | ||
| ""foldingRangeProvider"": true, | ||
| ""diagnosticProvider"": false, | ||
| ""semanticTokensProvider"": { |
There was a problem hiding this comment.
Ditto -- it might worth finding a way not trying to hard code this since it'll be unfortunate having to change this any time we add a new type.
| ' - Empty file - generates empty data | ||
| ' - Single line | ||
| ' - Multi-line | ||
| ' - Roslyn custom token type ('x' in 'int x' uses a Roslyn custom token type. | ||
| ' - Token in last line with no trailing newline (ensure sufficient range was passed to LSP generator). |
There was a problem hiding this comment.
Can we line these comments up with the InlineDatas?
There was a problem hiding this comment.
What do you mean? Adjust the indentation? The comments are within the function, so it seems more stylistically consistent to match the indentation of the code within, no?
There was a problem hiding this comment.
So my assumption reading this comment was there's a 1:1 mapping to the InlineData arguments, so the comments could just be mixed together with the InlineDatas rather than having the lists in two places. And actually I'm seeing there's not a 1:1 mapping since there's five bullets here and four InlineDatas, so something seems funky.
If trying to smash the comments together is too hard, consider using a MemberData where another method can provide the test cases here.
There was a problem hiding this comment.
there's five bullets here and four InlineDatas, so something seems funky
The last inline data covers both cases. The point of this comment though is to describe things that are determined specifically by the LSIF generator, not the shared SemanticTokensHelper code.
There was a problem hiding this comment.
Ah that wasn't clear at all. Can we break this into a MemberData w/ the applicable comments before each?
61a1bb3 to
5abce19
Compare
e3f5601 to
f64e453
Compare
| using System.Threading.Tasks; | ||
| using Newtonsoft.Json; | ||
|
|
||
| internal sealed class SemanticTokensCapabilities |
There was a problem hiding this comment.
Should we be using the SemanticTokensLegend type that already exists in LSP for this?
There was a problem hiding this comment.
I see two possible outcomes over the evolution of this tool:
-
We never add any more semantic tokens capabilities - in that case, yes, using SemanticTokensLegend would eliminate one boilerplate class.
-
We add more semantic tokens capabilities - these will almost definitely be different than the LSP ones requiring a custom type anyways.
I don't think it's worth spending any more time on this. When we standardize Semantic Tokens in the LSIF spec, this will likely require changes regardless.
There was a problem hiding this comment.
Can we track this question in the spec somewhere if it's not already? Or does the spec not care and this becomes a issue for the LSP library?
There was a problem hiding this comment.
Yeah this will be part of the Semantic Tokens LSIF spec proposal. Currently this exists as just a draft in my document.
I'll send a spec addition proposal on the LSIF site once I have finished validating the approach works.
I need an LSIF generator NuPkg for that :)
| ' - Empty file - generates empty data | ||
| ' - Single line | ||
| ' - Multi-line | ||
| ' - Roslyn custom token type ('x' in 'int x' uses a Roslyn custom token type. | ||
| ' - Token in last line with no trailing newline (ensure sufficient range was passed to LSP generator). |
There was a problem hiding this comment.
So my assumption reading this comment was there's a 1:1 mapping to the InlineData arguments, so the comments could just be mixed together with the InlineDatas rather than having the lists in two places. And actually I'm seeing there's not a 1:1 mapping since there's five bullets here and four InlineDatas, so something seems funky.
If trying to smash the comments together is too hard, consider using a MemberData where another method can provide the test cases here.
| ""scope"": ""project"", | ||
| ""data"": 2, | ||
| ""id"": 11, | ||
| ""id"": 13, |
There was a problem hiding this comment.
if we added semantic tokens to the end, these woudln't have to update, right?
f64e453 to
fc20dcf
Compare
There was a problem hiding this comment.
Signing off, but there are still some test cleanups I'd appreciate:
- Trying to make the tests not hardcode the classification list.
- Clarifying the comments in TestSemanticTokensData for which scenarios apply to what tests.
Obviously both of these can be follow-ups. The first I suspect will be forced if/when the moment @CyrusNajmabadi wants to add another embedded language. We're getting close to the holidays, so I presume that's imminent.
82b51ff to
6f4a036
Compare
c600f45 to
e97fbe6
Compare
e97fbe6 to
b9a0e72
Compare
703a3db to
b4e97fd
Compare
|
Looks like another infrastructure issue? The integration tests failed. |
Adds support for draft of Semantic Tokens LSIF protocol to the LSIF generator.
This adds two additional bits of content to the LSIF output:
{ "hoverProvider": true, "declarationProvider": false, "definitionProvider": true, "referencesProvider": true, "typeDefinitionProvider": false, "documentSymbolProvider": false, "foldingRangeProvider": true, "diagnosticProvider": false, "semanticTokensProvider": [ "tokenTypes": [ "namespace", "type", "class", "enum", "interface", "struct", ... ] ], "id": 1, "type": "vertex", "label": "capabilities" }{ "outV":4, "inV":12, "id":15, "type":"edge", "label":"textDocument/semanticTokens/full" } { "result": { "data": [0,0,20,17,0,1,0,5,15,0,0,6,6,48,0,0,6,1,54,0,1,0,5,15,0,0,6,6,48,0,0,6,1,21,0,0,1,10,48,0,0,10,1,54,0,1,0,1,54,0,0,1,8,15,0,0,8,1,54,0,0,2,6,15,0,0,6,2,21,0,0,2,6,48,0,0,6,1,21,0,0,1,7,48,0,0,7,1,21,0,0,1,10,48,0,0,10,1,21,0,0,1,24,22,0,0,24,1,54,0,0,1,30,18,0,0,30,1,54,0,0,2,20,53,0,0,21,1,21,0,0,2,22,18,0,0,22,1,54,0,0,1,1,54,0] }, "id":12, "type":"vertex", "label": "semanticTokensResult" }These changes increase the size of the LSIF output by approximately 6.5 percent in my tests generating LSIF on the LSP client repository and increases the generation duration by 15 percent.