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

[DRAFT] Add Code Action Provider Name to CodeAction.Data #48951

Closed
wants to merge 11 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ protected static LSP.VSCompletionItem CreateCompletionItem(
return item;
}

private protected static CodeActionResolveData CreateCodeActionResolveData(string uniqueIdentifier, LSP.Location location)
=> new CodeActionResolveData(uniqueIdentifier, location.Range, CreateTextDocumentIdentifier(location.Uri));
private protected static CodeActionResolveData CreateCodeActionResolveData(string uniqueIdentifier, LSP.Location location, string providerName = null)
=> new CodeActionResolveData(uniqueIdentifier, providerName ?? string.Empty, location.Range, CreateTextDocumentIdentifier(location.Uri));

/// <summary>
/// Creates a solution with a document.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private static async Task<Document> AddAwaitAsync(
private class MyCodeAction : CodeAction.DocumentChangeAction
{
public MyCodeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument)
: base(title, createChangedDocument)
: base(title, createChangedDocument, providerName: PredefinedCodeRefactoringProviderNames.AddAwait)
{
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand All @@ -25,6 +26,8 @@ private class ConstructorDelegatingCodeAction : CodeAction
private readonly State _state;
private readonly bool _addNullChecks;

internal override string ProviderName => PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers;

public ConstructorDelegatingCodeAction(
AbstractGenerateConstructorFromMembersCodeRefactoringProvider service,
Document document,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Shared.Extensions;

Expand All @@ -24,6 +25,8 @@ private class FieldDelegatingCodeAction : CodeAction
private readonly State _state;
private readonly bool _addNullChecks;

internal override string ProviderName => PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers;

public FieldDelegatingCodeAction(
AbstractGenerateConstructorFromMembersCodeRefactoringProvider service,
Document document,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.GenerateFromMembers;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.PickMembers;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
Expand All @@ -29,6 +30,8 @@ private class GenerateConstructorWithDialogCodeAction : CodeActionWithOptions

private bool? _addNullCheckOptionValue;

internal override string ProviderName => PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers;

public override string Title => FeaturesResources.Generate_constructor;

public GenerateConstructorWithDialogCodeAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ private static string GetCodeActionId(string assemblyName, string abstractTypeFu
private class MyCodeAction : CodeAction.DocumentChangeAction
{
public MyCodeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string id)
: base(title, createChangedDocument, id)
: base(title, createChangedDocument, id, providerName: PredefinedCodeFixProviderNames.ImplementAbstractClass)
{
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.ImplementType;
Expand All @@ -35,7 +36,7 @@ internal partial class ImplementInterfaceCodeAction : CodeAction
protected readonly State State;
protected readonly AbstractImplementInterfaceService Service;
private readonly string _equivalenceKey;

internal override string ProviderName => PredefinedCodeFixProviderNames.ImplementInterface;
internal ImplementInterfaceCodeAction(
AbstractImplementInterfaceService service,
Document document,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures.Text" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures.Wpf" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.Razor" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.InteractiveEditorFeatures" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.InteractiveFeatures" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.LanguageServer.Protocol" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ private static VSCodeAction GenerateVSCodeAction(
Priority = UnifiedSuggestedActionSetPriorityToPriorityLevel(setPriority),
Group = $"Roslyn{currentSetNumber}",
ApplicableRange = applicableRange,
Data = new CodeActionResolveData(currentTitle, request.Range, request.TextDocument)
Data = new CodeActionResolveData(currentTitle, codeAction.ProviderName, request.Range, request.TextDocument)
};

static VSCodeAction[] GenerateNestedVSCodeActions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,24 @@ internal class CodeActionResolveData
/// </remarks>
public string UniqueIdentifier { get; }

/// <summary>
/// Identifies the Code Action Provider which produced this Code Action.
/// </summary>
/// <remarks>
/// The unique identifier is currently set as:
/// name of the code action provider
/// e.g. 'Add Await Code Action Provider'
/// </remarks>
public string ProviderName { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be added to published API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I don't think we need the ProviderName property in this file actually. The properties in CodeActionResolveData are just used to carry over necessary information from our CodeActionHandler class to our CodeActionResolveHandler class. Since CodeActionHandler is the class that tells LSP what code actions are available, I don't think we need to carry over the ProviderName information to the resolve handler, since all the resolve handler does is populate the code action edits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't we need this property here to ensure the CodeAction.Data payload actually carries the provider name?

The properties in CodeActionResolveData are just used to carry over necessary information from our CodeActionHandler class to our CodeActionResolveHandler class.

In-between Razor get's the CodeActionResolveData and that's where I was intending to inspect the payload to consume the ProviderName. Is there a better way to ensure razor get's the provider name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be misunderstanding how things work here. Could you explain what Razor is doing in between Roslyn's CodeActionsHandler and CodeActionResolveHandler?

My initial impression was that per our conversation with Miguel, Razor would provide Roslyn with a list ProviderNames through a custom property to Roslyn's CodeActionsHandler. Roslyn would then filter code actions based on these provider names, and only return back a filtered set of code actions. Later, Roslyn would get code action resolve requests for each of the filtered code actions, and then would populate these code actions with the edit data.
^Is this correct? Please feel free to correct me if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be misunderstanding how things work here. Could you explain what Razor is doing in between Roslyn's CodeActionsHandler and CodeActionResolveHandler?

  1. Razor sends a textDocument/codeAction request to the Roslyn CodeActionsHandler.
  2. Roslyn runs its analyzers & [NEW in this PR] annotates the response CodeAction.CodeActionResolveData with the ProviderName property.
  3. Razor receives the response from Update links in README.md with ported wiki content #2, inspects the CodeAction.CodeActionResolveData.ProviderName to identify the code action, determine whether or not it is supported, and take the appropriate action to display it to the user in a razor compatible manner.
  4. [Optional]: Should it be necessary to resolve the code action (ex. user selects it), then razor sends a textDocument/codeActionResolve request to Roslyn's CodeActionResolveHandler with the initial CodeAction.CodeActionResolveData.
  5. Roslyn returns resolved edits, razor remaps those edits & displays them to the user.

My initial impression was that per our conversation with Miguel, Razor would provide Roslyn with a list ProviderNames through a custom property to Roslyn's CodeActionsHandler. Roslyn would then filter code actions based on these provider names, and only return back a filtered set of code actions. Later, Roslyn would get code action resolve requests for each of the filtered code actions, and then would populate these code actions with the edit data.

This was definitely one of the proposals we were discussing, but we ended up deciding against it for now in the interest of simplicity and reduce coupling. This also required potential LSP Spec changes, whereas annotating the CodeActionResolveData payload does not as the spec just specifies an object Data;. The goal of this PR was to take the approach steps detailed above. I hope that helps clarify things, let me know if there are any other questions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That was a great explanation, thank you! In that case, yeah, I definitely think the data payload needs the provider name.


public LSP.Range Range { get; }

public LSP.TextDocumentIdentifier TextDocument { get; }

public CodeActionResolveData(string uniqueIdentifier, LSP.Range range, LSP.TextDocumentIdentifier textDocument)
public CodeActionResolveData(string uniqueIdentifier, string providerName, LSP.Range range, LSP.TextDocumentIdentifier textDocument)
{
UniqueIdentifier = uniqueIdentifier;
ProviderName = providerName;
Range = range;
TextDocument = textDocument;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.CodeFixes;

namespace Microsoft.CodeAnalysis.ExternalAccess.Razor
{
internal static class CSharpPredefinedCodefixProviderNames
{
public static string ImplementAbstractClass => PredefinedCodeFixProviderNames.ImplementAbstractClass;
public static string ImplementInterface => PredefinedCodeFixProviderNames.ImplementInterface;
public static string SpellCheck => PredefinedCodeFixProviderNames.SpellCheck;
public static string FullyQualify => PredefinedCodeFixProviderNames.FullyQualify;
public static string AddImport => PredefinedCodeFixProviderNames.AddImport;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.CodeRefactorings;

namespace Microsoft.CodeAnalysis.ExternalAccess.Razor
{
internal static class CSharpPredefinedCodeRefactoringProviderNames
{
public static string AddAwait => PredefinedCodeRefactoringProviderNames.AddAwait;
public static string GenerateConstructorFromMembers => PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers;
public static string GenerateDefaultConstructors => PredefinedCodeRefactoringProviderNames.GenerateDefaultConstructors;
public static string GenerateEqualsAndGetHashCodeFromMembers => PredefinedCodeRefactoringProviderNames.GenerateEqualsAndGetHashCodeFromMembers;

}
}
16 changes: 13 additions & 3 deletions src/Workspaces/Core/Portable/CodeActions/CodeAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ public abstract class CodeAction
/// </remarks>
public virtual string? EquivalenceKey => null;

/// <summary>
/// Identifies the Code Action Provider which produced this Code Action.
/// </summary>
/// <remarks>
/// e.g. 'Add Await Code Action Provider'
/// </remarks>
internal virtual string? ProviderName => null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to consider exposing this property as a public API - this is something that can be made part of the code action telemetry and we can resolve the long outstanding issue #4919. @allisonchou do you mind bringing it to the design meeting?


internal virtual bool IsInlinable => false;

internal virtual CodeActionPriority Priority => CodeActionPriority.Medium;
Expand Down Expand Up @@ -358,14 +366,16 @@ public static CodeAction Create(string title, ImmutableArray<CodeAction> nestedA

internal abstract class SimpleCodeAction : CodeAction
{
public SimpleCodeAction(string title, string? equivalenceKey)
public SimpleCodeAction(string title, string? equivalenceKey, string? providerName = null)
{
Title = title;
EquivalenceKey = equivalenceKey;
ProviderName = providerName;
}

public sealed override string Title { get; }
public sealed override string? EquivalenceKey { get; }
internal sealed override string? ProviderName { get; }
}

internal class CodeActionWithNestedActions : SimpleCodeAction
Expand Down Expand Up @@ -410,8 +420,8 @@ internal class DocumentChangeAction : SimpleCodeAction
{
private readonly Func<CancellationToken, Task<Document>> _createChangedDocument;

public DocumentChangeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string? equivalenceKey = null)
: base(title, equivalenceKey)
public DocumentChangeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string? equivalenceKey = null, string? providerName = null)
: base(title, equivalenceKey, providerName)
{
_createChangedDocument = createChangedDocument;
}
Expand Down