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

Handle nested code actions #753

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
20 changes: 12 additions & 8 deletions src/OmniSharp.Abstractions/Extensions/ReflectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@ public static Lazy<MethodInfo> LazyGetMethod(this Lazy<Type> lazyType, string me

return new Lazy<MethodInfo>(() =>
{
var methodInfo = lazyType.Value.GetMethod(methodName);
var type = lazyType.Value;
var methodInfo = type.GetMethod(methodName);

if (methodInfo == null)
{
throw new InvalidOperationException($"Could not get type '{methodName}'");
throw new InvalidOperationException($"Could not get method '{methodName}' on type '{type.FullName}'");
}

return methodInfo;
Expand All @@ -54,11 +55,12 @@ public static Lazy<MethodInfo> LazyGetMethod(this Lazy<Type> lazyType, string me

return new Lazy<MethodInfo>(() =>
{
var methodInfo = lazyType.Value.GetMethod(methodName, bindingFlags);
var type = lazyType.Value;
var methodInfo = type.GetMethod(methodName, bindingFlags);

if (methodInfo == null)
{
throw new InvalidOperationException($"Could not get type '{methodName}'");
throw new InvalidOperationException($"Could not get method '{methodName}' on type '{type.FullName}'");
}

return methodInfo;
Expand All @@ -72,11 +74,12 @@ public static MethodInfo GetMethod(this Lazy<Type> lazyType, string methodName)
throw new ArgumentNullException(nameof(lazyType));
}

var methodInfo = lazyType.Value.GetMethod(methodName);
var type = lazyType.Value;
var methodInfo = type.GetMethod(methodName);

if (methodInfo == null)
{
throw new InvalidOperationException($"Could not get type '{methodName}'");
throw new InvalidOperationException($"Could not get method '{methodName}' on type '{type.FullName}'");
}

return methodInfo;
Expand All @@ -89,11 +92,12 @@ public static MethodInfo GetMethod(this Lazy<Type> lazyType, string methodName,
throw new ArgumentNullException(nameof(lazyType));
}

var methodInfo = lazyType.Value.GetMethod(methodName, bindingFlags);
var type = lazyType.Value;
var methodInfo = type.GetMethod(methodName, bindingFlags);

if (methodInfo == null)
{
throw new InvalidOperationException($"Could not get type '{methodName}'");
throw new InvalidOperationException($"Could not get method '{methodName}' on type '{type.FullName}'");
}

return methodInfo;
Expand Down
12 changes: 0 additions & 12 deletions src/OmniSharp.Roslyn.CSharp/Extensions/CodeActionExtensions.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
using System;
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;

namespace OmniSharp.Roslyn.CSharp.Services.Refactoring.V2
{
public class AvailableCodeAction
{
public CodeAction CodeAction { get; }
public CodeAction ParentCodeAction { get; }

public AvailableCodeAction(CodeAction codeAction, CodeAction parentCodeAction = null)
{
if (codeAction == null)
{
throw new ArgumentNullException(nameof(codeAction));
}

this.CodeAction = codeAction;
this.ParentCodeAction = parentCodeAction;
}

public string GetIdentifier()
{
return CodeAction.EquivalenceKey ?? GetTitle();
}

public string GetTitle()
{
return ParentCodeAction != null
? $"{ParentCodeAction.Title} -> {CodeAction.Title}"
: CodeAction.Title;
}

public Task<ImmutableArray<CodeActionOperation>> GetOperationsAsync(CancellationToken cancellationToken)
{
return CodeAction.GetOperationsAsync(cancellationToken);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
Expand All @@ -11,9 +12,10 @@
using Microsoft.CodeAnalysis.Text;
using Microsoft.Extensions.Logging;
using OmniSharp.Models.V2;
using OmniSharp.Roslyn.CSharp.Services.CodeActions;
using OmniSharp.Services;

namespace OmniSharp.Roslyn.CSharp.Services.CodeActions
namespace OmniSharp.Roslyn.CSharp.Services.Refactoring.V2
{
public abstract class BaseCodeActionService<TRequest, TResponse> : RequestHandler<TRequest, TResponse>
{
Expand All @@ -23,41 +25,63 @@ public abstract class BaseCodeActionService<TRequest, TResponse> : RequestHandle

private readonly CodeActionHelper _helper;

private readonly MethodInfo _getNestedCodeActions;

protected BaseCodeActionService(OmniSharpWorkspace workspace, CodeActionHelper helper, IEnumerable<ICodeActionProvider> providers, ILogger logger)
{
this.Workspace = workspace;
this.Providers = providers;
this.Logger = logger;
this._helper = helper;

// Sadly, the CodeAction.NestedCodeActions property is still internal.
Copy link
Member

Choose a reason for hiding this comment

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

👎 (at it still being internal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed making it public awhile back, but we tabled the issue because folks didn't like the word "Nested" or "Composite" because it's really about grouping related code actions together rather than making up a larger code action out of little ones. I need to bring this back up with the team again.

var nestedCodeActionsProperty = typeof(CodeAction).GetProperty("NestedCodeActions", BindingFlags.NonPublic | BindingFlags.Instance);
if (nestedCodeActionsProperty == null)
{
throw new InvalidOperationException("Could not find CodeAction.NestedCodeActions property.");
}

this._getNestedCodeActions = nestedCodeActionsProperty.GetGetMethod(nonPublic: true);
if (this._getNestedCodeActions == null)
{
throw new InvalidOperationException("Could not retrieve 'get' method for CodeAction.NestedCodeActions property.");
}
}

public abstract Task<TResponse> Handle(TRequest request);


protected async Task<IEnumerable<CodeAction>> GetActionsAsync(ICodeActionRequest request)
protected async Task<IEnumerable<AvailableCodeAction>> GetAvailableCodeActions(ICodeActionRequest request)
{
var actions = new List<CodeAction>();
var originalDocument = this.Workspace.GetDocument(request.FileName);
if (originalDocument == null)
{
return actions;
return Array.Empty<AvailableCodeAction>();
}

var refactoringContext = await GetRefactoringContext(originalDocument, request, actions);
if (refactoringContext != null)
{
await CollectRefactoringActions(refactoringContext.Value);
}
var actions = new List<CodeAction>();

var codeFixContext = await GetCodeFixContext(originalDocument, request, actions);
if (codeFixContext != null)
{
await CollectCodeFixActions(codeFixContext.Value);
}

var refactoringContext = await GetRefactoringContext(originalDocument, request, actions);
if (refactoringContext != null)
{
await CollectRefactoringActions(refactoringContext.Value);
}

// TODO: Determine good way to order code actions.
actions.Reverse();
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on this (besides just reversing the order like we do today?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some notion of ordering in Roslyn using attributes but it's messy. At some point, I intend to order them similarly in OmniSharp, but it's down the priority list a bit. For now, I just wanted to add a TODO to indicate that it wasn't ideal. 😄


return actions;
// Be sure to filter out any code actions that inherit from CodeActionWithOptions.
// This isn't a great solution and might need changing later, but every Roslyn code action
// derived from this type tries to display a dialog. For now, this is a reasonable solution.
var availableActions = ConvertToAvailableCodeAction(actions)
.Where(a => !a.CodeAction.GetType().GetTypeInfo().IsSubclassOf(typeof(CodeActionWithOptions)));

return availableActions;
}

private async Task<CodeRefactoringContext?> GetRefactoringContext(Document originalDocument, ICodeActionRequest request, List<CodeAction> actionsDestination)
Expand Down Expand Up @@ -124,7 +148,7 @@ private async Task CollectCodeFixActions(CodeFixContext context)
// TODO: This is a horrible hack! However, remove unnecessary usings only
// responds for diagnostics that are produced by its diagnostic analyzer.
// We need to provide a *real* diagnostic engine to address this.
if (codeFix.ToString() != CodeActionHelper.RemoveUnnecessaryUsingsProviderName)
if (codeFix.GetType().FullName != CodeActionHelper.RemoveUnnecessaryUsingsProviderName)
{
if (!diagnosticIds.Any(id => codeFix.FixableDiagnosticIds.Contains(id)))
{
Expand Down Expand Up @@ -166,5 +190,35 @@ private async Task CollectRefactoringActions(CodeRefactoringContext context)
}
}
}

private IEnumerable<AvailableCodeAction> ConvertToAvailableCodeAction(IEnumerable<CodeAction> actions)
{
var codeActions = new List<AvailableCodeAction>();

foreach (var action in actions)
{
var handledNestedActions = false;

// Roslyn supports "nested" code actions in order to allow submenus in the VS light bulb menu.
// For now, we'll just expand nested code actions in place.
var nestedActions = this._getNestedCodeActions.Invoke<ImmutableArray<CodeAction>>(action, null);
if (nestedActions.Length > 0)
{
foreach (var nestedAction in nestedActions)
{
codeActions.Add(new AvailableCodeAction(nestedAction, action));
}

handledNestedActions = true;
}

if (!handledNestedActions)
{
codeActions.Add(new AvailableCodeAction(action));
}
}

return codeActions;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using Microsoft.Extensions.Logging;
using OmniSharp.Mef;
using OmniSharp.Models.V2;
using OmniSharp.Roslyn.CSharp.Extensions;
using OmniSharp.Roslyn.CSharp.Services.CodeActions;
using OmniSharp.Services;

Expand All @@ -27,12 +26,17 @@ public GetCodeActionsService(

public override async Task<GetCodeActionsResponse> Handle(GetCodeActionsRequest request)
{
var actions = await GetActionsAsync(request);
var availableActions = await GetAvailableCodeActions(request);

return new GetCodeActionsResponse
{
CodeActions = actions.Select(a => new OmniSharpCodeAction(a.GetIdentifier(), a.Title))
CodeActions = availableActions.Select(ConvertToOmniSharpCodeAction)
};
}

private static OmniSharpCodeAction ConvertToOmniSharpCodeAction(AvailableCodeAction availableAction)
{
return new OmniSharpCodeAction(availableAction.GetIdentifier(), availableAction.GetTitle());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using Microsoft.Extensions.Logging;
using OmniSharp.Mef;
using OmniSharp.Models;
using OmniSharp.Roslyn.CSharp.Extensions;
using OmniSharp.Roslyn.CSharp.Services.CodeActions;
using OmniSharp.Services;

Expand All @@ -33,16 +32,16 @@ public RunCodeActionService(

public override async Task<RunCodeActionResponse> Handle(RunCodeActionRequest request)
{
var actions = await GetActionsAsync(request);
var action = actions.FirstOrDefault(a => a.GetIdentifier().Equals(request.Identifier));
if (action == null)
var availableActions = await GetAvailableCodeActions(request);
var availableAction = availableActions.FirstOrDefault(a => a.GetIdentifier().Equals(request.Identifier));
if (availableAction == null)
{
return new RunCodeActionResponse();
}

Logger.LogInformation($"Applying code action: {action.Title}");
Logger.LogInformation($"Applying code action: {availableAction.GetTitle()}");

var operations = await action.GetOperationsAsync(CancellationToken.None);
var operations = await availableAction.GetOperationsAsync(CancellationToken.None);

var solution = this.Workspace.CurrentSolution;
var changes = new List<ModifiedFileResponse>();
Expand Down