Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semantic Snippets - Remove reflection and call the LanguageServerSnippetExpander directly #61491

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@
<MicrosoftVisualStudioExtensibilityTestingVersion>0.1.132-beta</MicrosoftVisualStudioExtensibilityTestingVersion>
<!-- CodeStyleAnalyzerVersion should we updated together with version of dotnet-format in dotnet-tools.json -->
<CodeStyleAnalyzerVersion>4.2.0-2.final</CodeStyleAnalyzerVersion>
<VisualStudioEditorPackagesVersion>17.2.3194</VisualStudioEditorPackagesVersion>
<VisualStudioEditorPackagesVersion>17.3.37-preview</VisualStudioEditorPackagesVersion>
<ILAsmPackageVersion>5.0.0-alpha1.19409.1</ILAsmPackageVersion>
<ILDAsmPackageVersion>5.0.0-preview.1.20112.8</ILDAsmPackageVersion>
<MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>17.3.5</MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>
<MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>17.3.2017</MicrosoftVisualStudioLanguageServerProtocolPackagesVersion>
<MicrosoftVisualStudioShellPackagesVersion>17.2.32505.113</MicrosoftVisualStudioShellPackagesVersion>
<RefOnlyMicrosoftBuildPackagesVersion>16.5.0</RefOnlyMicrosoftBuildPackagesVersion>
<!-- The version of Roslyn we build Source Generators against that are built in this
Expand Down Expand Up @@ -153,7 +153,8 @@
<MicrosoftVisualStudioLanguageServerProtocolVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolVersion>
<MicrosoftVisualStudioLanguageServerProtocolExtensionsVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolExtensionsVersion>
<MicrosoftVisualStudioLanguageServerProtocolInternalVersion>$(MicrosoftVisualStudioLanguageServerProtocolPackagesVersion)</MicrosoftVisualStudioLanguageServerProtocolInternalVersion>
<MicrosoftVisualStudioLanguageServerClientVersion>17.0.5165</MicrosoftVisualStudioLanguageServerClientVersion>
<MicrosoftVisualStudioLanguageServerClientVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientVersion>
<MicrosoftVisualStudioLanguageServerClientImplementationVersion>17.3.2062-preview</MicrosoftVisualStudioLanguageServerClientImplementationVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<MicrosoftVisualStudioLanguageStandardClassificationVersion>$(VisualStudioEditorPackagesVersion)</MicrosoftVisualStudioLanguageStandardClassificationVersion>
<MicrosoftVisualStudioLiveShareVersion>2.18.6</MicrosoftVisualStudioLiveShareVersion>
<MicrosoftVisualStudioLiveShareLanguageServicesVersion>3.0.6</MicrosoftVisualStudioLiveShareLanguageServicesVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ public abstract class AbstractCSharpSnippetCompletionProviderTests : AbstractCSh
{
protected abstract string ItemToCommit { get; }

protected override TestComposition GetComposition()
=> base.GetComposition()
.AddExcludedPartTypes(typeof(IRoslynLSPSnippetExpander))
.AddParts(typeof(TestRoslynLanguageServerSnippetExpander));

internal override Type GetCompletionProviderType()
=> typeof(CSharpSnippetCompletionProvider);
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.Text.Shared.Extensions;
using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion;
using Microsoft.VisualStudio.LanguageServer.Client.Snippets;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Threading;
Expand All @@ -39,7 +40,7 @@ internal sealed class CommitManager : IAsyncCompletionCommitManager
private readonly ITextView _textView;
private readonly IGlobalOptionService _globalOptions;
private readonly IThreadingContext _threadingContext;
private readonly RoslynLSPSnippetExpander _roslynLSPSnippetExpander;
private readonly LanguageServerSnippetExpander _languageServerSnippetExpander;

public IEnumerable<char> PotentialCommitCharacters
{
Expand All @@ -62,13 +63,13 @@ internal CommitManager(
RecentItemsManager recentItemsManager,
IGlobalOptionService globalOptions,
IThreadingContext threadingContext,
RoslynLSPSnippetExpander roslynLSPSnippetExpander)
LanguageServerSnippetExpander languageServerSnippetExpander)
{
_globalOptions = globalOptions;
_threadingContext = threadingContext;
_recentItemsManager = recentItemsManager;
_textView = textView;
_roslynLSPSnippetExpander = roslynLSPSnippetExpander;
_languageServerSnippetExpander = languageServerSnippetExpander;
}

/// <summary>
Expand Down Expand Up @@ -241,20 +242,24 @@ private AsyncCompletionData.CommitResult Commit(
return new AsyncCompletionData.CommitResult(isHandled: true, AsyncCompletionData.CommitBehavior.None);
}

// Specifically for snippets, we use reflection to try and invoke the LanguageServerSnippetExpander's
// TryExpand method and determine if it succeeded or not.
var textChange = change.TextChange;
var triggerSnapshotSpan = new SnapshotSpan(triggerSnapshot, textChange.Span.ToSpan());
var mappedSpan = triggerSnapshotSpan.TranslateTo(subjectBuffer.CurrentSnapshot, SpanTrackingMode.EdgeInclusive);

// Specifically for snippets, we check to see if the associated completion item is a snippet,
// and if so, we call upon the LanguageServerSnippetExpander's TryExpand to insert the snippet.
if (SnippetCompletionItem.IsSnippet(roslynItem))
{
var lspSnippetText = change.Properties[SnippetCompletionItem.LSPSnippetKey];

_roslynLSPSnippetExpander.Expand(change.TextChange.Span, lspSnippetText, _textView, triggerSnapshot);
if (!_languageServerSnippetExpander.TryExpand(lspSnippetText!, triggerSnapshotSpan, _textView))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
FatalError.ReportAndCatch(new InvalidOperationException("The invoked LSP snippet expander came back as false."), ErrorSeverity.Critical);
}

return new AsyncCompletionData.CommitResult(isHandled: true, AsyncCompletionData.CommitBehavior.None);
}

var textChange = change.TextChange;
var triggerSnapshotSpan = new SnapshotSpan(triggerSnapshot, textChange.Span.ToSpan());
var mappedSpan = triggerSnapshotSpan.TranslateTo(subjectBuffer.CurrentSnapshot, SpanTrackingMode.EdgeInclusive);

using (var edit = subjectBuffer.CreateEdit(EditOptions.DefaultMinimalChange, reiteratedVersionNumber: null, editTag: null))
{
edit.Replace(mappedSpan.Span, change.TextChange.NewText);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Snippets;
using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion;
using Microsoft.VisualStudio.LanguageServer.Client.Snippets;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Utilities;

Expand All @@ -23,20 +24,20 @@ internal class CommitManagerProvider : IAsyncCompletionCommitManagerProvider
private readonly IThreadingContext _threadingContext;
private readonly RecentItemsManager _recentItemsManager;
private readonly IGlobalOptionService _globalOptions;
private readonly RoslynLSPSnippetExpander _roslynLSPSnippetExpander;
private readonly LanguageServerSnippetExpander _languageServerSnippetExpander;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public CommitManagerProvider(
IThreadingContext threadingContext,
RecentItemsManager recentItemsManager,
IGlobalOptionService globalOptions,
RoslynLSPSnippetExpander roslynLSPSnippetExpander)
LanguageServerSnippetExpander languageServerSnippetExpander)
{
_threadingContext = threadingContext;
_recentItemsManager = recentItemsManager;
_globalOptions = globalOptions;
_roslynLSPSnippetExpander = roslynLSPSnippetExpander;
_languageServerSnippetExpander = languageServerSnippetExpander;
}

IAsyncCompletionCommitManager? IAsyncCompletionCommitManagerProvider.GetOrCreate(ITextView textView)
Expand All @@ -46,7 +47,7 @@ public CommitManagerProvider(
return null;
}

return new CommitManager(textView, _recentItemsManager, _globalOptions, _threadingContext, _roslynLSPSnippetExpander);
return new CommitManager(textView, _recentItemsManager, _globalOptions, _threadingContext, _languageServerSnippetExpander);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
<DefineConstants>$(DefineConstants);EDITOR_FEATURES</DefineConstants>
<ApplyNgenOptimization Condition="'$(TargetFramework)' == 'netstandard2.0'">full</ApplyNgenOptimization>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗ 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)

Copy link
Member Author

@akhera99 akhera99 May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<!-- NuGet -->
<PackageId>Microsoft.CodeAnalysis.EditorFeatures.Common</PackageId>
<IsPackable>true</IsPackable>
Expand Down Expand Up @@ -39,6 +42,7 @@
<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" />
davidwengier marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@akhera99 akhera99 May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<!-- 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" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u love to see it!

<PackageReference Include="Microsoft.VisualStudio.Shell.Framework" Version="$(MicrosoftVisualStudioShellFrameworkVersion)" PrivateAssets="all" ExcludeAssets="all" NoWarn="NU1701" />
Expand Down
77 changes: 0 additions & 77 deletions src/EditorFeatures/Core/Snippets/RoslynLSPSnippetExpander.cs

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ internal class CSharpSnippetCompletionProvider : AbstractSnippetCompletionProvid
{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public CSharpSnippetCompletionProvider(IRoslynLSPSnippetExpander roslynLSPSnippetExpander)
: base(roslynLSPSnippetExpander)
public CSharpSnippetCompletionProvider()
{
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@ namespace Microsoft.CodeAnalysis.Completion.Providers.Snippets
{
internal abstract class AbstractSnippetCompletionProvider : CompletionProvider
{
private readonly IRoslynLSPSnippetExpander _roslynLSPSnippetExpander;

public AbstractSnippetCompletionProvider(IRoslynLSPSnippetExpander roslynLSPSnippetExpander)
{
_roslynLSPSnippetExpander = roslynLSPSnippetExpander;
}

public override async Task<CompletionChange> GetChangeAsync(Document document, CompletionItem item, char? commitKey = null, CancellationToken cancellationToken = default)
{
// This retrieves the document without the text used to invoke completion
Expand Down Expand Up @@ -58,32 +51,29 @@ public override async Task<CompletionChange> GetChangeAsync(Document document, C

public override async Task ProvideCompletionsAsync(CompletionContext context)
{
if (_roslynLSPSnippetExpander.CanExpandSnippet())
var document = context.Document;
var cancellationToken = context.CancellationToken;
var position = context.Position;
var service = document.GetLanguageService<ISnippetService>();

if (service == null)
{
return;
}

var (strippedDocument, newPosition) = await GetDocumentWithoutInvokingTextAsync(document, position, cancellationToken).ConfigureAwait(false);

var snippets = await service.GetSnippetsAsync(strippedDocument, newPosition, cancellationToken).ConfigureAwait(false);

foreach (var snippetData in snippets)
{
var document = context.Document;
var cancellationToken = context.CancellationToken;
var position = context.Position;
var service = document.GetLanguageService<ISnippetService>();

if (service == null)
{
return;
}

var (strippedDocument, newPosition) = await GetDocumentWithoutInvokingTextAsync(document, position, cancellationToken).ConfigureAwait(false);

var snippets = await service.GetSnippetsAsync(strippedDocument, newPosition, cancellationToken).ConfigureAwait(false);

foreach (var snippetData in snippets)
{
var completionItem = SnippetCompletionItem.Create(
displayText: snippetData.DisplayName,
displayTextSuffix: "",
position: position,
snippetIdentifier: snippetData.SnippetIdentifier,
glyph: Glyph.Snippet);
context.AddItem(completionItem);
}
var completionItem = SnippetCompletionItem.Create(
displayText: snippetData.DisplayName,
displayTextSuffix: "",
position: position,
snippetIdentifier: snippetData.SnippetIdentifier,
glyph: Glyph.Snippet);
context.AddItem(completionItem);
}
}

Expand Down
29 changes: 20 additions & 9 deletions src/Features/Lsif/GeneratorTest/FoldingRangeTests.vb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ Namespace Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.UnitTests
Public NotInheritable Class FoldingRangeTests
Private Const TestProjectAssemblyName As String = "TestProject"

' Expected 'FoldingRangeKind' argument would likely change for some of the tests when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 Is this intended to be part of this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

' https://github.com/dotnet/roslyn/projects/45#card-20049168 is implemented.
<Theory>
<InlineData("
using {|foldingRange:System;
using System.Linq;|}")>
<InlineData("
using {|foldingRange:S = System.String;
using System.Linq;|}")>
Public Async Function TestFoldingRangesImports(code As String) As Task
Await TestFoldingRanges(code, rangeKind:=FoldingRangeKind.Imports)
End Function

' Expected 'FoldingRangeKind' argument would likely change for some of the tests when
' https://github.com/dotnet/roslyn/projects/45#card-20049168 is implemented.
<Theory>
Expand All @@ -30,19 +43,17 @@ class C{|foldingRange:
M();
}|}
}|}
}|}", Nothing)>
}|}")>
<InlineData("
{|foldingRange:#region
#endregion|}", Nothing)>
<InlineData("
using {|foldingRange:System;
using System.Linq;|}", FoldingRangeKind.Imports)>
<InlineData("
using {|foldingRange:S = System.String;
using System.Linq;|}", FoldingRangeKind.Imports)>
#endregion|}")>
<InlineData("
{|foldingRange:// Comment Line 1
// Comment Line 2|}", Nothing)>
// Comment Line 2|}")>
Public Async Function TestFoldingRangesStandard(code As String) As Task
Await TestFoldingRanges(code, rangeKind:=Nothing)
End Function

Public Async Function TestFoldingRanges(code As String, rangeKind As FoldingRangeKind?) As Task
Using workspace = TestWorkspace.CreateWorkspace(
<Workspace>
Expand Down