Extract out subsystem for allowing embedded languages to export language services.#25985
Conversation
| { | ||
| Debug.Assert(token.Kind() == SyntaxKind.StringLiteralToken); | ||
|
|
||
| var languageProvider = CSharpEmbeddedLanguageProvider.Instance; |
There was a problem hiding this comment.
Same thing here. Should we try to prune to the right language?
Or could we at least bail out as soon as one matched?
There was a problem hiding this comment.
looking to see what we could do better here.
There was a problem hiding this comment.
we now bail out after one language succeeds.
|
|
||
| Public Overrides ReadOnly Property SyntaxTokenKinds As ImmutableArray(Of Integer) = ImmutableArray.Create(Of Integer)(SyntaxKind.StringLiteralToken) | ||
|
|
||
| Public Overrides Sub AddClassifications(workspace As Workspace, token As SyntaxToken, semanticModel As SemanticModel, result As ArrayBuilder(Of ClassifiedSpan), cancellationToken As CancellationToken) |
There was a problem hiding this comment.
Consider breaking this extra long line, and maybe the line above too.
There was a problem hiding this comment.
Maybe this method could be pulled into the parent type (AbstractSyntaxClassifier) and only having a language-specific implementation for VisualBasicEmbeddedLanguageProvider.Instance?
Never mind. AbstractSyntaxClassifier should not have logic for embedded languages. And it doesn't seem worthwhile to introduce another abstract type just for this.
There was a problem hiding this comment.
Maybe this method could be pulled into the parent type (AbstractSyntaxClassifier)
So, unfortunately "AbstractSyntaxClassifier" is not what is sounds like. it's not an abstract impl shared across VB and C#. Instead, there are two AbstractSyntaxClassifiers. Once defined in VB and one defined in C#. They're more appropriately thought of as AbstractCSharpSyntaxClassifier and AbstractVisualBasicSyntaxClassifier. They don't have anything in common to share.
There was a problem hiding this comment.
ok. i think i can make this better. give me a few minutes.
There was a problem hiding this comment.
Ok. i my understanding was correct, but the design here was due to the classifier stuff being written long before we had any shared compiler code between C#/VB. Turns out there was no need for separate langauge versions, so i merged them and just took your good suggestion. Thanks!
|
@dotnet/roslyn-ide Please review. These small feature-branch PRs make will help make it easier to get in the json and regex support into the IDE. Thanks! |
|
The PR seems to be garbled with a bunch of other changes from merge. |
|
Ooops. Merged master into this. darnit. |
b5a5019 to
c324363
Compare
|
Ok. undid bad merge. |
|
@dotnet/roslyn-infrastructure
|
|
@dotnet-bot test ubuntu_16_debug |
|
@dotnet-bot test ubuntu_16_debug_prtest |
|
@jmarolf can you take a look? I've used this for both the Regex and Json features, and it's worked great, allowing me to only have to update each roslyn feature (like brace-matching, classification,etc.) once, instead of having to make each one know about regex and json. |
| CSharpVSResources.Report_invalid_placeholders_in_string_dot_format_calls; | ||
| CSharpVSResources.Report_invalid_placeholders_in_string_dot_format_calls; | ||
| } | ||
| } |
There was a problem hiding this comment.
Doesn't look like there are any non-whitespace changes in this file
|
Comment added @jmarolf Feel free to merge when convenient! |
|
ok @jmarolf . ready to merge in. thanks! |
|
Thanks! |
[jcouv]
Implements part of #25939 (embedded json)
Implements part of #25940 (embedded regex)