Semantic Snippets - Remove reflection and call the LanguageServerSnippetExpander directly by akhera99 · Pull Request #61491 · dotnet/roslyn

akhera99 · 2022-05-24T18:24:29Z

Needed to update the FoldingRangeTests to account for changes from FoldingRangeKind being an enum to being a struct.

Need to remove the TestRoslynLanguageServerSnippetExpander as well as the RoslynLSPSnippetExpander files as they are no longer necessary.

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

.

sharwell · 2022-05-25T16:51:25Z

eng/Versions.props

+    <MicrosoftVisualStudioLanguageServerClientVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientVersion>
+    <MicrosoftVisualStudioLanguageServerClientImplementationVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientImplementationVersion>


💡 Can we define a new variable MicrosoftVisualStudioLanguageServerClientPackagesVersion to cover both of these?

sharwell · 2022-05-25T16:52:54Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CommitManager.cs

                var lspSnippetText = change.Properties[SnippetCompletionItem.LSPSnippetKey];

-                _roslynLSPSnippetExpander.Expand(change.TextChange.Span, lspSnippetText, _textView, triggerSnapshot);
+                if (!_languageServerSnippetExpander.TryExpand(lspSnippetText!, triggerSnapshotSpan, _textView))


💡 I think I'd rather have Contract.ThrowIfNull(lspSnippetText) on the previous line

sharwell · 2022-05-25T16:55:33Z

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

+    <!-- CA1416 can go away when we target net6.0-macos -->
+    <NoWarn>$(NoWarn);NU1701;</NoWarn>


❗ This is covered by suppressions on the specific PackageReference lines, and not here (transitive dependencies which trigger this can be explicitly included with their own NoWarn attributes)

I have a NoWarn on here:
<PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client.Implementation" Version="$(MicrosoftVisualStudioLanguageServerClientImplementationVersion)" PrivateAssets="all" NoWarn="NU1701" />

I was still getting issues because the packages seemingly had implicit dependencies.

I had to deal with similar in #61234 and can help update this.

sharwell · 2022-05-25T16:56:13Z

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

    <PackageReference Include="Microsoft.VisualStudio.Language" Version="$(MicrosoftVisualStudioLanguageVersion)" />
    <!-- No warn on NU1701 until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->
    <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client" Version="$(MicrosoftVisualStudioLanguageServerClientVersion)" PrivateAssets="all" NoWarn="NU1701" />
+    <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client.Implementation" Version="$(MicrosoftVisualStudioLanguageServerClientImplementationVersion)" PrivateAssets="all" NoWarn="NU1701" />


❔ What API are we using from this, and why can't it be relocated to the WPF assembly?

the LanguageServerSnippetExpander, we need to use the TryExpand method. We are specifically calling it in the CommitManager, so not sure if it can be relocated to the WPF assembly?

Unfortunately, this code will probably also eventually be needed in VS Mac.

I think moving to EditorFeatures.WPF makes sense, even if we have to mirror the change in EditorFeatures.Cocoa (and ideally that would be done at the same time), as it prevents someone else from later adding some new usage of a Windows-only aspect of the implementation assembly without realizing it.

sharwell · 2022-05-25T16:59:29Z

src/Features/Lsif/GeneratorTest/FoldingRangeTests.vb

    Public NotInheritable Class FoldingRangeTests
        Private Const TestProjectAssemblyName As String = "TestProject"

+        ' Expected 'FoldingRangeKind' argument would likely change for some of the tests when


😕 Is this intended to be part of this change?

#61496

This was discussed here, it's already been changed in a PR David made. I'm waiting for the above PR to go in prior to fixing this one up.

…r via the EditorFeatures layer

…ve_reflection

JoeRobich · 2022-05-28T00:48:09Z

The failing MacOS test has previously been reported as flaky - #61474

jmarolf · 2022-05-31T23:59:08Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CommitManager.cs

        private readonly IGlobalOptionService _globalOptions;
        private readonly IThreadingContext _threadingContext;
-        private readonly RoslynLSPSnippetExpander _roslynLSPSnippetExpander;
+        private readonly ILanguageServerSnippetExpander? _languageServerSnippetExpander;


Why is this nullable? won't this always be imported successfully if the VS version is new enough?

No, it is exported by a higher layer (EditorFeatures is WPF+Cocoa, but this is only exported for WPF). It will likely become required in a future change to move this implementation down to a cross-platform layer.

jmarolf · 2022-06-01T00:02:43Z

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

-    <!-- No warn on NU1701 until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->
-    <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client" Version="$(MicrosoftVisualStudioLanguageServerClientVersion)" PrivateAssets="all" NoWarn="NU1701" />
-    <!-- Explicit reference to Shell.15.0 et. al. with NU1701 only until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->
-    <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />


u love to see it!

jmarolf

akhera99 added 2 commits May 24, 2022 10:57

many changes to update versions

59c65d6

wip

b81a439

ghost added the Area-IDE label May 24, 2022

removed reflection classes

7cad46e

akhera99 marked this pull request as ready for review May 24, 2022 19:47

akhera99 requested review from a team as code owners May 24, 2022 19:47

fixed comment

63e7097

davidwengier previously approved these changes May 24, 2022

View reviewed changes

davidwengier reviewed May 24, 2022

View reviewed changes

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj Outdated Show resolved Hide resolved

akhera99 mentioned this pull request May 24, 2022

Update versions to use most recent LanguageServer changes. #61496

Merged

sharwell reviewed May 25, 2022

View reviewed changes

akhera99 requested review from a team as code owners May 25, 2022 23:23

akhera99 requested a review from a team May 25, 2022 23:23

akhera99 requested a review from a team as a code owner May 25, 2022 23:23

akhera99 changed the base branch from features/semantic-snippets to main May 26, 2022 01:52

akhera99 changed the base branch from main to features/semantic-snippets May 26, 2022 01:53

jmarolf self-assigned this May 26, 2022

akhera99 force-pushed the features/snippets_remove_reflection branch 4 times, most recently from d55b83b to fb46e1d Compare May 26, 2022 18:16

sharwell added 2 commits May 26, 2022 11:24

Simplify LSP reference update

8b62f34

Implement an adapter interface to access LanguageServerSnippetExpande…

b61c0dd

…r via the EditorFeatures layer

sharwell force-pushed the features/snippets_remove_reflection branch from fb46e1d to b61c0dd Compare May 26, 2022 18:25

get file from main

40b519e

akhera99 changed the base branch from features/semantic-snippets to main May 26, 2022 20:24

akhera99 changed the base branch from main to features/semantic-snippets May 26, 2022 20:24

akhera99 and others added 2 commits May 26, 2022 13:27

Merge branch 'features/semantic-snippets' into features/snippets_remo…

9335809

…ve_reflection

trying to fix issues

7fdc86f

runfoapp bot mentioned this pull request May 27, 2022

Syntax_Provider_Doesnt_Attribute_Incorrect_Timing flaky on MacOS #61474

Closed

Fix nullability issue

b2d0561

akhera99 requested review from davidwengier and sharwell May 31, 2022 23:54

jmarolf reviewed May 31, 2022

View reviewed changes

jmarolf reviewed Jun 1, 2022

View reviewed changes

jmarolf approved these changes Jun 1, 2022

View reviewed changes

akhera99 merged commit 83d3426 into dotnet:features/semantic-snippets Jun 1, 2022

		<MicrosoftVisualStudioLanguageServerClientVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientVersion>
		<MicrosoftVisualStudioLanguageServerClientImplementationVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientImplementationVersion>

		<!-- CA1416 can go away when we target net6.0-macos -->
		<NoWarn>$(NoWarn);NU1701;</NoWarn>

Conversation

akhera99 commented May 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhera99 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhera99 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeRobich commented May 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarolf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akhera99 May 25, 2022 •

edited

Loading

akhera99 May 25, 2022 •

edited

Loading

sharwell May 31, 2022 •

edited

Loading