Port semantic tokens range endpoint to cohost server #9761

davidwengier · 2023-12-24T10:41:00Z

Brings semantic tokens to cohosting, so there is an easy visual indicator if things are working.

Right now colours are disco on first open of a file, but are fixed with an edit. This is actually a good thing, as it will help me diagnose and improve the document system in cohosting.

In some respects this is a temporary thing until we get proper options support in cohosting, but in other respects having the service just take the one option value it actually needs is much nicer

alexgav · 2023-12-25T20:50:44Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

-    private readonly RazorLSPOptionsMonitor _razorLSPOptionsMonitor;
-    private readonly IClientConnection _clientConnection;
-    private readonly ILogger _logger;
+    private readonly IRazorDocumentMappingService _documentMappingService = documentMappingService ?? throw new ArgumentNullException(nameof(documentMappingService));


_documentMappingService

Nit: what do you think of suggestions here

https://stackoverflow.com/questions/77556444/null-checking-with-primary-constructor-in-c-sharp-12

Couple of interesting thoughts there. I personally like the idea of re-using primary constructor parameter name for class field name to shadow it. That way you won't have both "documentMappingService" and "_documentMappingService" in your completion list. Also that helper methods seems nice, though it becomes another "magic" method someone not familiar with codebase won't immediately understand when looking at the code. So not sure about the helper, but re-using parameter names seems nice.

Probably something we should discuss, and maybe vote on, in a meeting early next year. Personally I'm a fan of having separate fields because a) they're readonly, and b) its obvious in a PR diff whether something is a field because of the underscore. If a primary constructor parameter is captured in a field, then the compiler will warn if the parameter is used in a method body, so its not really possible to mix the two styles.

As for the extension method, I don't have any issue with it. We already have one, .AssumeNotNull() which we could use, though it throws a different exception type. Don't know if that matters. My real personal preference there is to get rid of the null checks altogether and just rely on the MEF composition exceptions.

I don't have strong feelings here and I also like having underscores for readability, but then should we always have private (readonly when appropriate) field starting with underscore for primary constructor parameters used in class methods? Definitely something we should agree on to be consistent. Seems like half of the clean-up from primary constructors came from getting rid of fields, then of course naming becomes a bit confusing, especially in long files when you are looking at a name and you don't know if it's a local defined somewhere above in the method or primary constructor parameter. If we do have fields for each parameter, do we even need to switch to primary constructors?

I'm good with relying on MEF composition exceptions in MEF parts, the code wouldn't even get to throwing ANE exception in that case, so the check is pointless.

Might be nice to have ANE thrown instead of IOE, so maybe adding ArgumentNotNull would be nice in places where we keep null checking for arguments. Not asking to add in this PR :). I was looking at this mostly in the context of switching to primary constructors and having each field initializer doing the whole "name ?? throw new ANE(nameof(name))" everywhere to see if there is something more succinct we could do.

alexgav · 2023-12-25T21:08:18Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

-    private readonly ILogger _logger;
+    private readonly IRazorDocumentMappingService _documentMappingService = documentMappingService ?? throw new ArgumentNullException(nameof(documentMappingService));
+    private readonly LanguageServerFeatureOptions _languageServerFeatureOptions = languageServerFeatureOptions ?? throw new ArgumentNullException(nameof(languageServerFeatureOptions));
+    private readonly ILogger _logger = loggerFactory.CreateLogger<RazorSemanticTokensInfoService>();


loggerFactory.CreateLogger()

nit: maybe

_logger = (loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory)).CreateLogger();

If logger factory is null, this line would throw anyway, so not sure its worth it?

I don't care too much, maybe a little nicer to have the name of the argument captured? Feel free to punt.

alexgav · 2023-12-26T01:39:34Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

+
+        var semanticTokens = await GetSemanticTokensAsync(clientConnection, textDocumentIdentifier, range, documentContext, correlationId, colorBackground, cancellationToken).ConfigureAwait(false);
+
+        var amount = semanticTokens is null ? "no" : (semanticTokens.Data.Length / 5).ToString(Thread.CurrentThread.CurrentCulture);


5

Should this be the "TokenSize" const already defined in this class? Same with two usages of magic number "5" below :)

Fair call, I just copied this as is. Will update

Yep, I saw it first in the original code location and was like "what's the magic number 5 and where does it come from"? :) The constant in the target class made sense.

alexgav · 2023-12-26T02:10:29Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/SemanticTokensRangeEndpoint.cs

-    {
-        _telemetryReporter = telemetryReporter;
-    }
+    private readonly IRazorSemanticTokensInfoService _semanticTokensInfoService = semanticTokensInfoService;


_semanticTokensInfoService

Why the field instead of using primary constructor parameter in the methods?

See reply above, but TL;DR I like the underscore :)

I like the underscores too :) Also see my response above. I don't have strong feeling either way, other than - do the primary constructors even make sense then?

alexgav

maryamariyan · 2024-01-02T21:48:50Z

...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/IServiceCollectionExtensions.cs

+            services.AddHandlerWithCapabilities<SemanticTokensRangeEndpoint>();
+            // Ensure that we don't add the default service if something else has added one.
+            services.TryAddSingleton<IRazorSemanticTokensInfoService, RazorSemanticTokensInfoService>();
+        }


Given that there is no else case for this if (!featureOptions.UseRazorCohostServer) condition,

I wonder where is the cohost setup happening and why we don't add IRazorSemanticTokensInfoService for when UseRazorCohostServer is set to true.

Cohosting uses MEF for DI, so the setup is all done via attributes and not by manually adding things to service collections

...icrosoft.VisualStudio.LanguageServerClient.Razor/Cohost/CohostSemanticTokensRangeEndpoint.cs

Part of #9519 Now that the initial cohost PR has been merged, we don't need to use the feature branch any more. That means this PR, which is big, but all of the code in it has been reviewed, in #9761, #9766 and #9767.

davidwengier added 9 commits December 23, 2023 22:03

Use constructor injection

5819e8c

Use primary constructor

93f886d

Move logic from endpoint into service

c44a260

Move IClientConnection to a parameter to prepare for cohosting

de081ab

Use primary constructor

0755188

Create cohost endpoint

d216f26

Disable semantic tokens in regular server

0e6bd17

Move options retreival code out into the endpoints

16615ce

In some respects this is a temporary thing until we get proper options support in cohosting, but in other respects having the service just take the one option value it actually needs is much nicer

Update tests and benchmarks

0e9d0b3

davidwengier requested a review from a team as a code owner December 24, 2023 10:41

davidwengier added 2 commits December 24, 2023 21:52

Merge branch 'features/cohost' into dev/dawengie/SemanticTokensCohost

7f258ab

Updates after merge

bf504f3

davidwengier force-pushed the dev/dawengie/SemanticTokensCohost branch from 59e2394 to bf504f3 Compare December 25, 2023 10:17

alexgav reviewed Dec 25, 2023

View reviewed changes

Merge branch 'features/cohost' into dev/dawengie/SemanticTokensCohost

ad2fe27

alexgav reviewed Dec 26, 2023

View reviewed changes

alexgav approved these changes Dec 26, 2023

View reviewed changes

davidwengier merged commit be23375 into features/cohost Dec 27, 2023
12 checks passed

davidwengier deleted the dev/dawengie/SemanticTokensCohost branch December 27, 2023 08:27

maryamariyan reviewed Jan 2, 2024

View reviewed changes

davidwengier mentioned this pull request Jan 4, 2024

Merge Cohost feature branch into main #9786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port semantic tokens range endpoint to cohost server #9761

Port semantic tokens range endpoint to cohost server #9761

davidwengier commented Dec 24, 2023

alexgav Dec 25, 2023

davidwengier Dec 26, 2023

alexgav Dec 26, 2023

alexgav Dec 25, 2023 •

edited

Loading

davidwengier Dec 26, 2023

alexgav Dec 26, 2023

alexgav Dec 26, 2023

davidwengier Dec 26, 2023

alexgav Dec 26, 2023

alexgav Dec 26, 2023 •

edited

Loading

davidwengier Dec 26, 2023

alexgav Dec 26, 2023

alexgav left a comment

maryamariyan Jan 2, 2024

davidwengier Jan 2, 2024 •

edited

Loading


		var semanticTokens = await GetSemanticTokensAsync(clientConnection, textDocumentIdentifier, range, documentContext, correlationId, colorBackground, cancellationToken).ConfigureAwait(false);

		var amount = semanticTokens is null ? "no" : (semanticTokens.Data.Length / 5).ToString(Thread.CurrentThread.CurrentCulture);

Port semantic tokens range endpoint to cohost server #9761

Port semantic tokens range endpoint to cohost server #9761

Conversation

davidwengier commented Dec 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgav Dec 25, 2023 • 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

alexgav Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

alexgav Dec 25, 2023 •

edited

Loading

alexgav Dec 26, 2023 •

edited

Loading

davidwengier Jan 2, 2024 •

edited

Loading