[LSP] Modify semanticTokens `isFinalized` logic #60484

allisonchou · 2022-03-30T18:51:45Z

The isFinalized variable is used by Razor to determine whether to cache C# tokens on their end. C#'s tokens may not always be finalized since we utilize frozen partial semantics.
Today, the current check to set isFinalized's value involves calling TryGetCompilation. However, we've run into problems since TryGetCompilation is never guaranteed to actually return a compilation.
As an alternative for 17.2, instead of TryGetCompilation we now call IsFullyLoaded and check to see if we have a full compilation. This isn't guaranteed to return finalized tokens but has been determined to be an appropriate workaround until workspace/semanticTokens/refresh messages are implemented in Roslyn and Razor.

dibarbet

will need to target 17.2

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs

sharwell · 2022-03-30T19:22:18Z

since we utilize frozen partial semantics.

This part is not true prior to #60485

…to AddIsFullyLoaded

sharwell · 2022-03-30T20:26:09Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

+            // If the project's compilation isn't yet available, kick off a task in the background to
+            // hopefully make it available faster since we'll need it later to compute tokens.
+            if (!context.Document.Project.TryGetCompilation(out _))
+            {
+                _ = context.Document.Project.GetCompilationAsync(cancellationToken).ConfigureAwait(false);
+            }


This is supposed to be handled by BackgroundCompiler and CompilationAvailableTaggerEventSource. Is there a reason we want it here separately?

There was an issue in Razor where document.Project.TryGetCompilation(out var _) was returning false even after ~5 mins of waiting. We thought kicking off a separate task at the beginning might solve this problem and allow compilation to return sooner.

cc @CyrusNajmabadi is this the correct way to handle things?

sharwell · 2022-03-30T20:28:43Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs

-            var isFinalized = document.Project.TryGetCompilation(out var compilation) && compilation == semanticModel.Compilation;
-            document = frozenDocument;
+            document = document.WithFrozenPartialSemantics(cancellationToken);
+            options = options with { ForceFrozenPartialSemanticsForCrossProcessOperations = true };


❔ Do we want to use frozen partial semantics with isFinalized?

Hmm, I wouldn't think so? I believe we want to call TryGetCompilation on the complete document. The only other place the frozen document would be used with isFinalized is when retrieving the WorkspaceStatusService, and unless I'm mistaken it doesn't seem like frozen partial semantics would be useful for that.

…to AddIsFullyLoaded

…yLoaded

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

dibarbet

yeah this looks mostly good - just one potential race condition we should fix (should hopefully be straightforward with a lock)

dibarbet · 2022-04-01T17:36:18Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

+                // If the project's compilation isn't yet available, kick off a task in the background to
+                // hopefully make it available faster since we'll need it later to compute tokens.
+                var newCompilationTask = project.GetCompilationAsync(cancellationToken);
+                _projectIdToCompilation.Add(project.Id, newCompilationTask);


this can technically race and lead to multiple calls to _projectIdToCompilation.Add which would throw -

2 semantic tokens requests come in for the same project (could be diff docs).

They run in parallel since semantic tokens isn't mutating

Running in parallel, they both hit the if !_projectIdToCompilation.ContainsKey(project.Id), and they both say it is not present in the dictionary (because neither has created the task yet).

Now both will try to add to the dictionary and potentially throw.

I think you'll want to guard access to the _projectIdToCompilation with a lock

Done! Thanks

* Simplify * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Excluded Task.CompletionTask from completion list in async member declarations * Fix merge * Use existing assert * Use correct assert * Use isInferred: false when creating BoundArrayInitialization in CreateUTF8ByteRepresentation * Move few 'CodeFixProvider's to Analyzers layer * Fix * Fix * Remove ForegroundThreadAffinitizedObject usage * fix * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Remove ForegroundThreadAffinitizedObject usage * Revert bad changes * Fix errors and remove a hard CodeFixProvider back to Features * Update projitems * Move tests * Remove unused abstract property * Remove the single unused read of CodeFixCategory * Remove overrides in Analyzers * Remove overrides in Features * Fixed char recomendations test * Got rid of AsyncModifierName constants * Remove unnecessary `<Compile Remove` * Fixed tests for all other keywords * Cleaned up keyword tests * Move formatting analyzer to shared Analyzers layer * Move tests to shared Analyzers layer and more other fixes * Few more fixes * Fix bad changes * Fix bad changes * Fix bad changes * Remove IVTs of deleted test projects * Address review comment * Fix failing test * Cleanup unused resources in Features layer * Update Roslyn.Diagnostics.Analyzers and remove RS0005 suppressions * Move to SyntaxEditorBasedCodeFixProvider * Remove some 'MyCodeAction's * Remove more 'MyCodeAction's * One more that's not named MyCodeAction * Fix up changes between main and main-vs-deps * Colorize async as keyword in some cases * Few fixes * Added a bunch of tests, where async can never be a keyword, moved logic one level up * Fix typo * Note auto-default merged in feature status doc (#60564) * Update PublishData.json for 17.3 P1 (#60559) * Fix formatting * Added case with equals syntax, moved logic to a separate method * Add Rebuild badge to README (#60298) * Avoid using --blame-crash with CollectDumps This flag causes the test harness to attach ProcDump, which removes our ability to collect heap dumps on crash using Windows' built-in functionality. * Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation * Update src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/RemoteDocumentHighlightsService.cs * Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs * Revert name * Update tests * Fix * Add comment * Add comment * Update src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs * fix grammar * Formatting * Revert * revert * Revert * Fix the Compiler VSIX extension so it works with CPS projects again This has been broken for quite some time. We were MEF importing a type that was no longer a MEF part, and referencing very old reference assemblies that needed to be updated as a part of responding to that original breaking change. * Make NotificationOption2 a serializable struct (#60573) * Update status for UTF8 literals and checked operators (#60587) * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800 * Simplify * Simplify * Simplify * Simplify * [LSP] Modify semanticTokens `isFinalized` logic (#60484) * try/finally * Simplify * Move comment * Simplify * Doc * Doc * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033 * Workspace config options (#59790) * REnames * Expand tuple * Rename parameter * Use deconstruction * Fix merge from main * Do not assert we are on the main thread in the constructor * Add support for unsafe local functions in IDE0062 code fix (#59297) * Track "numeric IntPtr" feature (#60579) * Address review comment * Add helpers * Remove unneeded IVTs * Simplify * Simplify * Pause computation of navbar items when a buffer is not visible * Rename * PR feedback part 1 * Change insertion branch * Update PublishData.json * PR feedback part 2 * lint * Implement handling of rename intent (#59410) * Implement handling of rename intent * Use documentId instead of URI * Move record definition up top * feedback * Address feedback * Remove unused files in CodeStyle layer * Remove unused resources in CodeStyle layer * Changed algorithm of detecting IAsyncEnumerable and IAsyncEnumerator, reverted some refactoring * Got rid of IsSystemRuntimeOrMscorlibAssembly method * Fix crash when combining reordered arguments with params array (#60622) * Fix string * Always use text edits in completion for correctness (#60466) * Filter down to task-like types when in an async context * Update providers * Cleanup * Revert * Revert * Revert * Simplify * Restore logic * Update test * Pull feature-specific APIs out of ISyntaxFacts * Extract out IBlockFacts out of ISyntaxFacts * Simplify * Instrument ReferenceCachingCS for flakiness (#60585) * Fix our INavigableSymbol to not root an entire solution-snapshot * Remove the TEmporaryWorkspace entirely from the RemoteWorkspace code. * Lint * Fixed adding a redundant blank line when suppressing with local attribute * [main] Update dependencies from dotnet/source-build-externals (#60359) [main] Update dependencies from dotnet/source-build-externals * Fix * Added more tests & refactoring * Clean up syntax context * Clean up syntax context * Clean up syntax context * Clean up syntax context * Simplify * Delay starting the work to scan for todo-items * move properties * Cleanup * Cleanup * Update tests * Fixup * Run continuation to dispose of cancellation token source (#60653) * Run continuation to dispose of cancellation token source * Remove cancellation token and execute synchronously * Update src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> * Global indentation options - take 2 (#60565) * Revert "Revert "Global indentation options (#59679)" (#60199)" This reverts commit ab57ce8. * Move IFormattingInteractionService to Editor Features and add inferred indentation detection to its implementation. Move GetFormattingChangesOnTypedCharacterAsync, GetFormattingChangesOnPasteAsync to ISyntaxFormattingService - these do not depend on the editor. * Test fix * Update struct field definite assignment tests * Restore CodeStyle test projects * Lint * Fallout * Pass options to FixAllAsync, simplify CodeAction registration (#60665) * Trim unnessasary leading lines when removing usings (#60672) * Formatting and code generation options (#60127) * Remove duplicate package references (#60658) * Remove duplicate package references * Delete additional Tools.props * Update for definite assignment changes Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com> Co-authored-by: DoctorKrolic <mapmyp03@gmail.com> Co-authored-by: David Barbet <dabarbet@microsoft.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: Youssef1313 <youssefvictor00@gmail.com> Co-authored-by: DoctorKrolic <70431552+DoctorKrolic@users.noreply.github.com> Co-authored-by: Manish Vasani <mavasani@microsoft.com> Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com> Co-authored-by: Rikki Gibson <rigibson@microsoft.com> Co-authored-by: Allison Chou <allichou@microsoft.com> Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com> Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com> Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com> Co-authored-by: Gérald Barré <meziantou@users.noreply.github.com> Co-authored-by: Jonathon Marolf <jmarolf@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com> Co-authored-by: Fredric Silberberg <frsilb@microsoft.com>

allisonchou added 2 commits March 30, 2022 11:22

Modify isFinalized logic

c6d83aa

Add comment

2f569b8

allisonchou added the LSP issues related to the roslyn language server protocol implementation label Mar 30, 2022

allisonchou requested review from NTaylorMullen, CyrusNajmabadi, dibarbet and ryanbrandenburg March 30, 2022 18:51

allisonchou requested a review from a team as a code owner March 30, 2022 18:51

dotnet-issue-labeler bot added the Area-IDE label Mar 30, 2022

dibarbet reviewed Mar 30, 2022

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs Outdated Show resolved Hide resolved

Code review feedback

eae6fa3

allisonchou added 3 commits March 30, 2022 13:20

Review feedback

1828ce3

Add options

20cf491

Merge branch 'release/dev17.2' of https://github.com/dotnet/roslyn in…

4740f18

…to AddIsFullyLoaded

allisonchou changed the base branch from main to release/dev17.2 March 30, 2022 20:25

sharwell reviewed Mar 30, 2022

View reviewed changes

sharwell mentioned this pull request Mar 30, 2022

Update LSP classification paths to use frozen partial semantics #60485

Closed

Merge branch 'release/dev17.2' into AddIsFullyLoaded

8606db0

This was referenced Mar 30, 2022

Remove isFinalized semantic tokens logic dotnet/razor#6194

Closed

[LSP] Remove isFinalized from semantic tokens logic #60322

Closed

allisonchou added 3 commits March 31, 2022 13:59

Update logic

fe6307f

Merge branch 'release/dev17.2' of https://github.com/dotnet/roslyn in…

ef16c05

…to AddIsFullyLoaded

Merge remote-tracking branch 'origin/AddIsFullyLoaded' into AddIsFull…

d3274e2

…yLoaded

allisonchou requested a review from dibarbet March 31, 2022 22:13

Add null check

3af6c20

dibarbet reviewed Mar 31, 2022

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs Outdated Show resolved Hide resolved

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs Outdated Show resolved Hide resolved

Review feedback

f96cf89

dibarbet reviewed Apr 1, 2022

View reviewed changes

allisonchou added 2 commits April 1, 2022 12:02

Add lock

fb287ff

Merge branch 'release/dev17.2' into AddIsFullyLoaded

9bc049f

dibarbet approved these changes Apr 1, 2022

View reviewed changes

allisonchou added the Needs Shiproom Approval label Apr 1, 2022

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Apr 5, 2022

allisonchou merged commit 0167599 into dotnet:release/dev17.2 Apr 5, 2022

allisonchou deleted the AddIsFullyLoaded branch April 5, 2022 20:09

allisonchou mentioned this pull request Apr 18, 2022

[LSP] Semantic tokens isFinalized property continually returning false on document open #59777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LSP] Modify semanticTokens `isFinalized` logic #60484

[LSP] Modify semanticTokens `isFinalized` logic #60484

allisonchou commented Mar 30, 2022 •

edited

Loading

dibarbet left a comment

sharwell commented Mar 30, 2022

sharwell Mar 30, 2022

allisonchou Mar 30, 2022

sharwell Mar 30, 2022

allisonchou Mar 30, 2022

dibarbet left a comment

dibarbet Apr 1, 2022

allisonchou Apr 1, 2022

[LSP] Modify semanticTokens isFinalized logic #60484

[LSP] Modify semanticTokens isFinalized logic #60484

Conversation

allisonchou commented Mar 30, 2022 • edited Loading

dibarbet left a comment

Choose a reason for hiding this comment

sharwell commented Mar 30, 2022

sharwell Mar 30, 2022

Choose a reason for hiding this comment

allisonchou Mar 30, 2022

Choose a reason for hiding this comment

sharwell Mar 30, 2022

Choose a reason for hiding this comment

allisonchou Mar 30, 2022

Choose a reason for hiding this comment

dibarbet left a comment

Choose a reason for hiding this comment

dibarbet Apr 1, 2022

Choose a reason for hiding this comment

allisonchou Apr 1, 2022

Choose a reason for hiding this comment

[LSP] Modify semanticTokens `isFinalized` logic #60484

[LSP] Modify semanticTokens `isFinalized` logic #60484

allisonchou commented Mar 30, 2022 •

edited

Loading