From 3cad2305a06c7fa86f522fc8dde22d010bc77c7b Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 10 Aug 2022 00:10:24 -0700 Subject: [PATCH 1/2] Record whether a CodeAction is a fix or not Omnisharp-roslyn side of fixing https://github.com/OmniSharp/omnisharp-vscode/issues/5335. --- .../Models/v2/CodeActions/CodeAction.cs | 4 +- .../Refactoring/V2/AvailableCodeAction.cs | 4 +- .../Refactoring/V2/BaseCodeActionService.cs | 22 +++--- .../Refactoring/V2/GetCodeActionsService.cs | 2 +- .../CodeActionsV2Facts.cs | 37 ++++----- .../CodeActionsV2Facts.cs | 78 +++++++++---------- .../CodeActionsWithOptionsFacts.cs | 6 +- .../AbstractCodeActionsTestFixture.cs | 4 +- 8 files changed, 81 insertions(+), 76 deletions(-) diff --git a/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs index 5ef5297230..837c7a2d77 100644 --- a/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs +++ b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs @@ -2,13 +2,15 @@ namespace OmniSharp.Models.V2.CodeActions { public class OmniSharpCodeAction { - public OmniSharpCodeAction(string identifier, string name) + public OmniSharpCodeAction(string identifier, string name, bool isCodeFix) { Identifier = identifier; Name = name; + IsCodeFix = isCodeFix; } public string Identifier { get; } public string Name { get; } + public bool IsCodeFix { get; } } } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs index 26a3d2c6ba..749c62e3c0 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs @@ -10,11 +10,13 @@ public class AvailableCodeAction { public CodeAction CodeAction { get; } public CodeAction ParentCodeAction { get; } + public bool IsCodeFix { get; } - public AvailableCodeAction(CodeAction codeAction, CodeAction parentCodeAction = null) + public AvailableCodeAction(CodeAction codeAction, bool isCodeFix, CodeAction parentCodeAction = null) { this.CodeAction = codeAction ?? throw new ArgumentNullException(nameof(codeAction)); this.ParentCodeAction = parentCodeAction; + this.IsCodeFix = isCodeFix; } public string GetIdentifier() diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs index f2f072a128..01fd63acb3 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs @@ -78,7 +78,7 @@ protected async Task> GetAvailableCodeActions(I return Array.Empty(); } - var codeActions = new List(); + var codeActions = new List<(CodeAction CodeAction, bool IsCodeFix)>(); var sourceText = await document.GetTextAsync(); var span = GetTextSpan(request, sourceText); @@ -86,7 +86,7 @@ protected async Task> GetAvailableCodeActions(I await CollectCodeFixesActions(document, span, codeActions); await CollectRefactoringActions(document, span, codeActions); - var distinctActions = codeActions.GroupBy(x => x.Title).Select(x => x.First()); + var distinctActions = codeActions.GroupBy(x => x.CodeAction.Title).Select(x => x.First()); var availableActions = ConvertToAvailableCodeAction(distinctActions); @@ -117,7 +117,7 @@ private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText) return new TextSpan(position, length: 0); } - private async Task CollectCodeFixesActions(Document document, TextSpan span, List codeActions) + private async Task CollectCodeFixesActions(Document document, TextSpan span, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) { var diagnosticsWithProjects = await _diagnostics.GetDiagnostics(ImmutableArray.Create(document.FilePath)); @@ -135,7 +135,7 @@ private async Task CollectCodeFixesActions(Document document, TextSpan span, Lis } } - private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable diagnostics, List codeActions) + private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable diagnostics, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) { var codeActionOptions = CodeActionOptionsFactory.Create(Options); @@ -149,7 +149,7 @@ private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerabl document, span, fixableDiagnostics, - (a, _) => codeActions.Add(a), + (a, _) => codeActions.Add((a, IsCodeFix: true)), codeActionOptions, CancellationToken.None); @@ -182,7 +182,7 @@ private bool HasFix(CodeFixProvider codeFixProvider, string diagnosticId) || (customDiagVsFixMap.ContainsKey(diagnosticId) && codeFixProvider.FixableDiagnosticIds.Any(id => id == customDiagVsFixMap[diagnosticId])); } - private async Task CollectRefactoringActions(Document document, TextSpan span, List codeActions) + private async Task CollectRefactoringActions(Document document, TextSpan span, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) { var codeActionOptions = CodeActionOptionsFactory.Create(Options); var availableRefactorings = OrderedCodeRefactoringProviders.Value; @@ -194,7 +194,7 @@ private async Task CollectRefactoringActions(Document document, TextSpan span, L var context = OmniSharpCodeFixContextFactory.CreateCodeRefactoringContext( document, span, - (a, _) => codeActions.Add(a), + (a, _) => codeActions.Add((a, IsCodeFix: false)), codeActionOptions, CancellationToken.None); @@ -207,17 +207,17 @@ private async Task CollectRefactoringActions(Document document, TextSpan span, L } } - private IEnumerable ConvertToAvailableCodeAction(IEnumerable actions) + private IEnumerable ConvertToAvailableCodeAction(IEnumerable<(CodeAction CodeAction, bool IsCodeFix)> actions) { return actions.SelectMany(action => { - var nestedActions = action.GetNestedCodeActions(); + var nestedActions = action.CodeAction.GetNestedCodeActions(); if (!nestedActions.IsDefaultOrEmpty) { - return nestedActions.Select(nestedAction => new AvailableCodeAction(nestedAction, action)); + return nestedActions.Select(nestedAction => new AvailableCodeAction(nestedAction, action.IsCodeFix, action.CodeAction)); } - return new[] { new AvailableCodeAction(action) }; + return new[] { new AvailableCodeAction(action.CodeAction, action.IsCodeFix) }; }); } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs index dafa236f62..6c9011192c 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs @@ -41,7 +41,7 @@ public override async Task Handle(GetCodeActionsRequest private static OmniSharpCodeAction ConvertToOmniSharpCodeAction(AvailableCodeAction availableAction) { - return new OmniSharpCodeAction(availableAction.GetIdentifier(), availableAction.GetTitle()); + return new OmniSharpCodeAction(availableAction.GetIdentifier(), availableAction.GetTitle(), availableAction.IsCodeFix); } } } diff --git a/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs b/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs index dd9f797ac5..d2f2e71501 100644 --- a/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs +++ b/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs @@ -10,6 +10,7 @@ using OmniSharp.Models.UpdateBuffer; using OmniSharp.Models.V2; using OmniSharp.Models.V2.CodeActions; +using Roslyn.Test.Utilities; using TestUtility; using Xunit; using Xunit.Abstractions; @@ -30,7 +31,7 @@ public async Task Can_get_code_actions_from_roslyn() const string code = "var regex = new Reg[||]ex();"; var refactorings = await FindRefactoringNamesAsync(code); - Assert.Contains("using System.Text.RegularExpressions;", refactorings); + Assert.Contains(("using System.Text.RegularExpressions;", IsCodeFix: true), refactorings); } [Fact] @@ -46,7 +47,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code); - Assert.Contains("Extract method", refactorings); + Assert.Contains(("Extract method", IsCodeFix: false), refactorings); } [Fact] @@ -62,21 +63,21 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code); - var expected = new List + var expected = new List<(string, bool IsCodeFix)> { - "using System.Text.RegularExpressions;", - "System.Text.RegularExpressions.Regex", - "Extract method", - "Extract local function", - "Introduce local for 'Regex.Match(\"foo\", \"bar\")'", - "Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", - "Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", - "Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into new overload", - "Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", - "Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", - "Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into new overload" + ("using System.Text.RegularExpressions;", IsCodeFix: true), + ("System.Text.RegularExpressions.Regex", IsCodeFix: true), + ("Extract method", IsCodeFix: false), + ("Extract local function", IsCodeFix: false), + ("Introduce local for 'Regex.Match(\"foo\", \"bar\")'", IsCodeFix: false), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", IsCodeFix: false), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", IsCodeFix: false), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into new overload", IsCodeFix: false), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", IsCodeFix: false), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", IsCodeFix: false), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into new overload", IsCodeFix: false) }; - Assert.Equal(expected, refactorings); + AssertEx.Equal(expected, refactorings); } [Fact] @@ -122,7 +123,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code); - Assert.Empty(refactorings.Where(x => x.StartsWith("Rename file to"))); + Assert.Empty(refactorings.Where(x => x.Name.StartsWith("Rename file to"))); } private async Task RunRefactoringAsync(string code, string refactoringName) @@ -134,11 +135,11 @@ private async Task RunRefactoringAsync(string code, strin return await RunRefactoringsAsync(code, identifier); } - private async Task> FindRefactoringNamesAsync(string code) + private async Task> FindRefactoringNamesAsync(string code) { var codeActions = await FindRefactoringsAsync(code); - return codeActions.Select(a => a.Name); + return codeActions.Select(a => (a.Name, a.IsCodeFix)); } private async Task> FindRefactoringsAsync(string code) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs index b4735cf372..fa57d31d73 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs @@ -3,9 +3,9 @@ using System.Linq; using System.Threading.Tasks; using OmniSharp.Models; -using OmniSharp.Models.V2; using OmniSharp.Models.V2.CodeActions; using OmniSharp.Roslyn.CSharp.Services.Refactoring.V2; +using Roslyn.Test.Utilities; using TestUtility; using Xunit; using Xunit.Abstractions; @@ -34,7 +34,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - Assert.Contains("using System;", refactorings); + Assert.Contains(("using System;", IsCodeFix: true), refactorings); } [Theory] @@ -105,7 +105,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - Assert.Contains("Extract method", refactorings); + Assert.Contains(("Extract method", IsCodeFix: false), refactorings); } [Theory] @@ -124,51 +124,51 @@ public void Whatever() var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - List expected = roslynAnalyzersEnabled ? new List + var expected = roslynAnalyzersEnabled ? new List<(string Name, bool IsCodeFix)> { - "Fix formatting", - "using System;", + ("Fix formatting", IsCodeFix: true), + ("using System;", IsCodeFix: true), #if NETCOREAPP - "using Internal;", - "Fully qualify 'Console' -> Internal.Console", - "Fully qualify 'Console' -> System.Console", + ("using Internal;", IsCodeFix: true), + ("Fully qualify 'Console' -> Internal.Console", IsCodeFix: true), + ("Fully qualify 'Console' -> System.Console", IsCodeFix: true), #else - "System.Console", + ("System.Console", IsCodeFix: true), #endif - "Generate variable 'Console' -> Generate property 'Console'", - "Generate variable 'Console' -> Generate field 'Console'", - "Generate variable 'Console' -> Generate read-only field 'Console'", - "Generate variable 'Console' -> Generate local 'Console'", - "Generate variable 'Console' -> Generate parameter 'Console'", - "Generate type 'Console' -> Generate class 'Console' in new file", - "Generate type 'Console' -> Generate class 'Console'", - "Generate type 'Console' -> Generate nested class 'Console'", - "Extract local function", - "Extract method", - "Introduce local for 'Console.Write(\"should be using System;\")'" - } : new List + ("Generate variable 'Console' -> Generate property 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate field 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate read-only field 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate local 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate parameter 'Console'", IsCodeFix: true), + ("Generate type 'Console' -> Generate class 'Console' in new file", IsCodeFix: true), + ("Generate type 'Console' -> Generate class 'Console'", IsCodeFix: true), + ("Generate type 'Console' -> Generate nested class 'Console'", IsCodeFix: true), + ("Extract local function", IsCodeFix: false), + ("Extract method", IsCodeFix: false), + ("Introduce local for 'Console.Write(\"should be using System;\")'", IsCodeFix: false) + } : new List<(string Name, bool IsCodeFix)> { - "using System;", + ("using System;", IsCodeFix: true), #if NETCOREAPP - "using Internal;", - "Fully qualify 'Console' -> Internal.Console", - "Fully qualify 'Console' -> System.Console", + ("using Internal;", IsCodeFix: true), + ("Fully qualify 'Console' -> Internal.Console", IsCodeFix: true), + ("Fully qualify 'Console' -> System.Console", IsCodeFix: true), #else - "System.Console", + ("System.Console", IsCodeFix: true), #endif - "Generate variable 'Console' -> Generate property 'Console'", - "Generate variable 'Console' -> Generate field 'Console'", - "Generate variable 'Console' -> Generate read-only field 'Console'", - "Generate variable 'Console' -> Generate local 'Console'", - "Generate variable 'Console' -> Generate parameter 'Console'", - "Generate type 'Console' -> Generate class 'Console' in new file", - "Generate type 'Console' -> Generate class 'Console'", - "Generate type 'Console' -> Generate nested class 'Console'", - "Extract local function", - "Extract method", - "Introduce local for 'Console.Write(\"should be using System;\")'" + ("Generate variable 'Console' -> Generate property 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate field 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate read-only field 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate local 'Console'", IsCodeFix: true), + ("Generate variable 'Console' -> Generate parameter 'Console'", IsCodeFix: true), + ("Generate type 'Console' -> Generate class 'Console' in new file", IsCodeFix: true), + ("Generate type 'Console' -> Generate class 'Console'", IsCodeFix: true), + ("Generate type 'Console' -> Generate nested class 'Console'", IsCodeFix: true), + ("Extract local function", IsCodeFix: false), + ("Extract method", IsCodeFix: false), + ("Introduce local for 'Console.Write(\"should be using System;\")'", IsCodeFix: false) }; - Assert.Equal(expected.OrderBy(x => x), refactorings.OrderBy(x => x)); + AssertEx.Equal(expected.OrderBy(x => x.Name), refactorings.OrderBy(x => x.Name)); } [Theory] diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs index be5eb92f5c..20221794ed 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs @@ -174,7 +174,7 @@ public void Foo(string a[||], string b) var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain("Change signature...", response); + Assert.DoesNotContain(("Change signature...", IsCodeFix: false), response); } [Fact] @@ -188,7 +188,7 @@ public async Task Blacklists_generate_type_with_UI() var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain("Generate type 'NonExistentBaseType' -> Generate new type...", response); + Assert.DoesNotContain(("Generate type 'NonExistentBaseType' -> Generate new type...", IsCodeFix: false), response); } [Fact] @@ -206,7 +206,7 @@ public class BaseClass {}"; var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain("Pull 'Foo' up -> Pull members up to base type...", response); + Assert.DoesNotContain(("Pull 'Foo' up -> Pull members up to base type...", IsCodeFix: false), response); } [Fact] diff --git a/tests/TestUtility/AbstractCodeActionsTestFixture.cs b/tests/TestUtility/AbstractCodeActionsTestFixture.cs index a7d8ff8232..0b50e9c284 100644 --- a/tests/TestUtility/AbstractCodeActionsTestFixture.cs +++ b/tests/TestUtility/AbstractCodeActionsTestFixture.cs @@ -64,11 +64,11 @@ protected async Task RunRefactoringAsync(string code, str return await RunRefactoringsAsync(code, identifier, wantsChanges); } - protected async Task> FindRefactoringNamesAsync(string code, bool isAnalyzersEnabled = true, bool analyzeOpenDocumentsOnly = false) + protected async Task> FindRefactoringNamesAsync(string code, bool isAnalyzersEnabled = true, bool analyzeOpenDocumentsOnly = false) { var codeActions = await FindRefactoringsAsync(code, TestHelpers.GetConfigurationDataWithAnalyzerConfig(isAnalyzersEnabled, analyzeOpenDocumentsOnly: analyzeOpenDocumentsOnly)); - return codeActions.Select(a => a.Name); + return codeActions.Select(a => (a.Name, a.IsCodeFix)); } protected async Task> FindRefactoringsAsync(string code, IConfiguration configurationData = null) From bd462d5c97ad51f04bb75bf67f049203db943883 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 10 Aug 2022 23:06:14 -0700 Subject: [PATCH 2/2] Switch to using strings for code action kinds, instead of a bool. Move LSP handler code for this into the main handler, and more accurately classify quickfix actions. --- .../Models/v2/CodeActions/CodeAction.cs | 12 ++- .../Models/v2/CodeActions/CodeActionKind.cs | 10 +++ .../Handlers/OmniSharpCodeActionHandler.cs | 27 ++++--- .../Refactoring/V2/AvailableCodeAction.cs | 6 +- .../Refactoring/V2/BaseCodeActionService.cs | 34 ++++++--- .../Refactoring/V2/GetCodeActionsService.cs | 2 +- .../CodeActionsV2Facts.cs | 32 ++++---- .../CodeActionsV2Facts.cs | 74 +++++++++---------- .../CodeActionsWithOptionsFacts.cs | 7 +- .../AbstractCodeActionsTestFixture.cs | 4 +- 10 files changed, 124 insertions(+), 84 deletions(-) create mode 100644 src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeActionKind.cs diff --git a/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs index 837c7a2d77..7e7589f801 100644 --- a/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs +++ b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeAction.cs @@ -1,16 +1,22 @@ +using System.Diagnostics; + namespace OmniSharp.Models.V2.CodeActions { public class OmniSharpCodeAction { - public OmniSharpCodeAction(string identifier, string name, bool isCodeFix) + public OmniSharpCodeAction(string identifier, string name, string codeActionKind) { + Debug.Assert(codeActionKind is CodeActions.CodeActionKind.QuickFix + or CodeActions.CodeActionKind.Refactor + or CodeActions.CodeActionKind.RefactorExtract + or CodeActions.CodeActionKind.RefactorInline); Identifier = identifier; Name = name; - IsCodeFix = isCodeFix; + CodeActionKind = codeActionKind; } public string Identifier { get; } public string Name { get; } - public bool IsCodeFix { get; } + public string CodeActionKind { get; } } } diff --git a/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeActionKind.cs b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeActionKind.cs new file mode 100644 index 0000000000..fde25f25de --- /dev/null +++ b/src/OmniSharp.Abstractions/Models/v2/CodeActions/CodeActionKind.cs @@ -0,0 +1,10 @@ +namespace OmniSharp.Models.V2.CodeActions +{ + public static class CodeActionKind + { + public const string QuickFix = nameof(QuickFix); + public const string Refactor = nameof(Refactor); + public const string RefactorInline = nameof(RefactorInline); + public const string RefactorExtract = nameof(RefactorExtract); + } +} diff --git a/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpCodeActionHandler.cs b/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpCodeActionHandler.cs index 4453359080..5522a9008c 100644 --- a/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpCodeActionHandler.cs +++ b/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpCodeActionHandler.cs @@ -5,7 +5,6 @@ using System.Threading.Tasks; using MediatR; using Microsoft.CodeAnalysis; -using Newtonsoft.Json.Linq; using OmniSharp.Extensions.JsonRpc; using OmniSharp.Extensions.LanguageServer.Protocol; using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities; @@ -14,8 +13,11 @@ using OmniSharp.Models.V2.CodeActions; using OmniSharp.Extensions.LanguageServer.Protocol.Document; using OmniSharp.Extensions.LanguageServer.Protocol.Workspace; -using OmniSharp.Models; using Diagnostic = OmniSharp.Extensions.LanguageServer.Protocol.Models.Diagnostic; +using CodeActionKind = OmniSharp.Extensions.LanguageServer.Protocol.Models.CodeActionKind; +using OmniSharpCodeActionKind = OmniSharp.Models.V2.CodeActions.CodeActionKind; +using System; +using Range = OmniSharp.Extensions.LanguageServer.Protocol.Models.Range; namespace OmniSharp.LanguageServerProtocol.Handlers { @@ -80,18 +82,12 @@ public override async Task Handle(CodeActionParams foreach (var ca in omnisharpResponse.CodeActions) { - CodeActionKind kind; - if (ca.Identifier.StartsWith("using ")) { kind = CodeActionKind.QuickFix; } - else if (ca.Identifier.StartsWith("Inline ")) { kind = CodeActionKind.RefactorInline; } - else if (ca.Identifier.StartsWith("Extract ")) { kind = CodeActionKind.RefactorExtract; } - else if (ca.Identifier.StartsWith("Change ")) { kind = CodeActionKind.QuickFix; } - else { kind = CodeActionKind.Refactor; } codeActions.Add( new CodeAction { Title = ca.Name, - Kind = kind, + Kind = OmniSharpCodeActionHandler.FromOmniSharpCodeActionKind(ca.CodeActionKind), Diagnostics = new Container(), Edit = new WorkspaceEdit(), Command = Command.Create("omnisharp/executeCodeAction") @@ -168,6 +164,16 @@ ExecuteCommandRegistrationOptions IRegistration omnisharpCodeAction switch + { + OmniSharpCodeActionKind.QuickFix => CodeActionKind.QuickFix, + OmniSharpCodeActionKind.Refactor => CodeActionKind.Refactor, + OmniSharpCodeActionKind.RefactorInline => CodeActionKind.RefactorInline, + OmniSharpCodeActionKind.RefactorExtract => CodeActionKind.RefactorExtract, + _ => throw new InvalidOperationException($"Unexpected code action kind {omnisharpCodeAction}") + }; + protected override CodeActionRegistrationOptions CreateRegistrationOptions(CodeActionCapability capability, ClientCapabilities clientCapabilities) { return new CodeActionRegistrationOptions() @@ -176,7 +182,8 @@ protected override CodeActionRegistrationOptions CreateRegistrationOptions(CodeA CodeActionKinds = new Container( CodeActionKind.SourceOrganizeImports, CodeActionKind.Refactor, - CodeActionKind.RefactorExtract), + CodeActionKind.RefactorExtract, + CodeActionKind.RefactorInline), }; } } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs index 749c62e3c0..b36dada976 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/AvailableCodeAction.cs @@ -10,13 +10,13 @@ public class AvailableCodeAction { public CodeAction CodeAction { get; } public CodeAction ParentCodeAction { get; } - public bool IsCodeFix { get; } + public string CodeActionKind { get; } - public AvailableCodeAction(CodeAction codeAction, bool isCodeFix, CodeAction parentCodeAction = null) + public AvailableCodeAction(CodeAction codeAction, string codeActionKind, CodeAction parentCodeAction = null) { this.CodeAction = codeAction ?? throw new ArgumentNullException(nameof(codeAction)); this.ParentCodeAction = parentCodeAction; - this.IsCodeFix = isCodeFix; + this.CodeActionKind = codeActionKind; } public string GetIdentifier() diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs index 01fd63acb3..afd8544778 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs @@ -78,7 +78,7 @@ protected async Task> GetAvailableCodeActions(I return Array.Empty(); } - var codeActions = new List<(CodeAction CodeAction, bool IsCodeFix)>(); + var codeActions = new List<(CodeAction CodeAction, string CodeActionKind)>(); var sourceText = await document.GetTextAsync(); var span = GetTextSpan(request, sourceText); @@ -117,7 +117,7 @@ private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText) return new TextSpan(position, length: 0); } - private async Task CollectCodeFixesActions(Document document, TextSpan span, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) + private async Task CollectCodeFixesActions(Document document, TextSpan span, List<(CodeAction CodeAction, string CodeActionKind)> codeActions) { var diagnosticsWithProjects = await _diagnostics.GetDiagnostics(ImmutableArray.Create(document.FilePath)); @@ -135,7 +135,7 @@ private async Task CollectCodeFixesActions(Document document, TextSpan span, Lis } } - private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable diagnostics, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) + private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable diagnostics, List<(CodeAction CodeAction, string CodeActionKind)> codeActions) { var codeActionOptions = CodeActionOptionsFactory.Create(Options); @@ -149,7 +149,7 @@ private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerabl document, span, fixableDiagnostics, - (a, _) => codeActions.Add((a, IsCodeFix: true)), + (a, _) => codeActions.Add((a, CodeActionKind.QuickFix)), codeActionOptions, CancellationToken.None); @@ -182,7 +182,7 @@ private bool HasFix(CodeFixProvider codeFixProvider, string diagnosticId) || (customDiagVsFixMap.ContainsKey(diagnosticId) && codeFixProvider.FixableDiagnosticIds.Any(id => id == customDiagVsFixMap[diagnosticId])); } - private async Task CollectRefactoringActions(Document document, TextSpan span, List<(CodeAction CodeAction, bool IsCodeFix)> codeActions) + private async Task CollectRefactoringActions(Document document, TextSpan span, List<(CodeAction CodeAction, string CodeActionKind)> codeActions) { var codeActionOptions = CodeActionOptionsFactory.Create(Options); var availableRefactorings = OrderedCodeRefactoringProviders.Value; @@ -194,7 +194,23 @@ private async Task CollectRefactoringActions(Document document, TextSpan span, L var context = OmniSharpCodeFixContextFactory.CreateCodeRefactoringContext( document, span, - (a, _) => codeActions.Add((a, IsCodeFix: false)), + (a, _) => + { + string kind; + if (a.Title.StartsWith("Inline ")) + { + kind = CodeActionKind.RefactorInline; + } + else if (a.Title.StartsWith("Extract ")) + { + kind = CodeActionKind.RefactorExtract; + } + else + { + kind = CodeActionKind.Refactor; + } + codeActions.Add((a, kind)); + }, codeActionOptions, CancellationToken.None); @@ -207,17 +223,17 @@ private async Task CollectRefactoringActions(Document document, TextSpan span, L } } - private IEnumerable ConvertToAvailableCodeAction(IEnumerable<(CodeAction CodeAction, bool IsCodeFix)> actions) + private IEnumerable ConvertToAvailableCodeAction(IEnumerable<(CodeAction CodeAction, string CodeActionKind)> actions) { return actions.SelectMany(action => { var nestedActions = action.CodeAction.GetNestedCodeActions(); if (!nestedActions.IsDefaultOrEmpty) { - return nestedActions.Select(nestedAction => new AvailableCodeAction(nestedAction, action.IsCodeFix, action.CodeAction)); + return nestedActions.Select(nestedAction => new AvailableCodeAction(nestedAction, action.CodeActionKind, action.CodeAction)); } - return new[] { new AvailableCodeAction(action.CodeAction, action.IsCodeFix) }; + return new[] { new AvailableCodeAction(action.CodeAction, action.CodeActionKind) }; }); } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs index 6c9011192c..9fc4d259e8 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/GetCodeActionsService.cs @@ -41,7 +41,7 @@ public override async Task Handle(GetCodeActionsRequest private static OmniSharpCodeAction ConvertToOmniSharpCodeAction(AvailableCodeAction availableAction) { - return new OmniSharpCodeAction(availableAction.GetIdentifier(), availableAction.GetTitle(), availableAction.IsCodeFix); + return new OmniSharpCodeAction(availableAction.GetIdentifier(), availableAction.GetTitle(), availableAction.CodeActionKind); } } } diff --git a/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs b/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs index d2f2e71501..8ee9d18642 100644 --- a/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs +++ b/tests/OmniSharp.Cake.Tests/CodeActionsV2Facts.cs @@ -31,7 +31,7 @@ public async Task Can_get_code_actions_from_roslyn() const string code = "var regex = new Reg[||]ex();"; var refactorings = await FindRefactoringNamesAsync(code); - Assert.Contains(("using System.Text.RegularExpressions;", IsCodeFix: true), refactorings); + Assert.Contains(("using System.Text.RegularExpressions;", CodeActionKind.QuickFix), refactorings); } [Fact] @@ -47,7 +47,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code); - Assert.Contains(("Extract method", IsCodeFix: false), refactorings); + Assert.Contains(("Extract method", CodeActionKind.RefactorExtract), refactorings); } [Fact] @@ -63,19 +63,19 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code); - var expected = new List<(string, bool IsCodeFix)> + var expected = new List<(string, string CodeActionKind)> { - ("using System.Text.RegularExpressions;", IsCodeFix: true), - ("System.Text.RegularExpressions.Regex", IsCodeFix: true), - ("Extract method", IsCodeFix: false), - ("Extract local function", IsCodeFix: false), - ("Introduce local for 'Regex.Match(\"foo\", \"bar\")'", IsCodeFix: false), - ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", IsCodeFix: false), - ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", IsCodeFix: false), - ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into new overload", IsCodeFix: false), - ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", IsCodeFix: false), - ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", IsCodeFix: false), - ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into new overload", IsCodeFix: false) + ("using System.Text.RegularExpressions;", CodeActionKind.QuickFix), + ("System.Text.RegularExpressions.Regex", CodeActionKind.QuickFix), + ("Extract method", CodeActionKind.RefactorExtract), + ("Extract local function", CodeActionKind.RefactorExtract), + ("Introduce local for 'Regex.Match(\"foo\", \"bar\")'", CodeActionKind.Refactor), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", CodeActionKind.Refactor), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", CodeActionKind.Refactor), + ("Introduce parameter for 'Regex.Match(\"foo\", \"bar\")' -> into new overload", CodeActionKind.Refactor), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> and update call sites directly", CodeActionKind.Refactor), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into extracted method to invoke at call sites", CodeActionKind.Refactor), + ("Introduce parameter for all occurrences of 'Regex.Match(\"foo\", \"bar\")' -> into new overload", CodeActionKind.Refactor) }; AssertEx.Equal(expected, refactorings); } @@ -135,11 +135,11 @@ private async Task RunRefactoringAsync(string code, strin return await RunRefactoringsAsync(code, identifier); } - private async Task> FindRefactoringNamesAsync(string code) + private async Task> FindRefactoringNamesAsync(string code) { var codeActions = await FindRefactoringsAsync(code); - return codeActions.Select(a => (a.Name, a.IsCodeFix)); + return codeActions.Select(a => (a.Name, a.CodeActionKind)); } private async Task> FindRefactoringsAsync(string code) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs index fa57d31d73..9ea22b7098 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs @@ -34,7 +34,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - Assert.Contains(("using System;", IsCodeFix: true), refactorings); + Assert.Contains(("using System;", CodeActionKind.QuickFix), refactorings); } [Theory] @@ -105,7 +105,7 @@ public void Whatever() }"; var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - Assert.Contains(("Extract method", IsCodeFix: false), refactorings); + Assert.Contains(("Extract method", CodeActionKind.RefactorExtract), refactorings); } [Theory] @@ -124,49 +124,49 @@ public void Whatever() var refactorings = await FindRefactoringNamesAsync(code, roslynAnalyzersEnabled); - var expected = roslynAnalyzersEnabled ? new List<(string Name, bool IsCodeFix)> + var expected = roslynAnalyzersEnabled ? new List<(string Name, string CodeActionKind)> { - ("Fix formatting", IsCodeFix: true), - ("using System;", IsCodeFix: true), + ("Fix formatting", CodeActionKind.QuickFix), + ("using System;", CodeActionKind.QuickFix), #if NETCOREAPP - ("using Internal;", IsCodeFix: true), - ("Fully qualify 'Console' -> Internal.Console", IsCodeFix: true), - ("Fully qualify 'Console' -> System.Console", IsCodeFix: true), + ("using Internal;", CodeActionKind.QuickFix), + ("Fully qualify 'Console' -> Internal.Console", CodeActionKind.QuickFix), + ("Fully qualify 'Console' -> System.Console", CodeActionKind.QuickFix), #else - ("System.Console", IsCodeFix: true), + ("System.Console", CodeActionKind.QuickFix), #endif - ("Generate variable 'Console' -> Generate property 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate field 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate read-only field 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate local 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate parameter 'Console'", IsCodeFix: true), - ("Generate type 'Console' -> Generate class 'Console' in new file", IsCodeFix: true), - ("Generate type 'Console' -> Generate class 'Console'", IsCodeFix: true), - ("Generate type 'Console' -> Generate nested class 'Console'", IsCodeFix: true), - ("Extract local function", IsCodeFix: false), - ("Extract method", IsCodeFix: false), - ("Introduce local for 'Console.Write(\"should be using System;\")'", IsCodeFix: false) - } : new List<(string Name, bool IsCodeFix)> + ("Generate variable 'Console' -> Generate property 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate field 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate read-only field 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate local 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate parameter 'Console'", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate class 'Console' in new file", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate class 'Console'", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate nested class 'Console'", CodeActionKind.QuickFix), + ("Extract local function", CodeActionKind.RefactorExtract), + ("Extract method", CodeActionKind.RefactorExtract), + ("Introduce local for 'Console.Write(\"should be using System;\")'", CodeActionKind.Refactor) + } : new List<(string Name, string CodeActionKind)> { - ("using System;", IsCodeFix: true), + ("using System;", CodeActionKind.QuickFix), #if NETCOREAPP - ("using Internal;", IsCodeFix: true), - ("Fully qualify 'Console' -> Internal.Console", IsCodeFix: true), - ("Fully qualify 'Console' -> System.Console", IsCodeFix: true), + ("using Internal;", CodeActionKind.QuickFix), + ("Fully qualify 'Console' -> Internal.Console", CodeActionKind.QuickFix), + ("Fully qualify 'Console' -> System.Console", CodeActionKind.QuickFix), #else - ("System.Console", IsCodeFix: true), + ("System.Console", CodeActionKind.QuickFix), #endif - ("Generate variable 'Console' -> Generate property 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate field 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate read-only field 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate local 'Console'", IsCodeFix: true), - ("Generate variable 'Console' -> Generate parameter 'Console'", IsCodeFix: true), - ("Generate type 'Console' -> Generate class 'Console' in new file", IsCodeFix: true), - ("Generate type 'Console' -> Generate class 'Console'", IsCodeFix: true), - ("Generate type 'Console' -> Generate nested class 'Console'", IsCodeFix: true), - ("Extract local function", IsCodeFix: false), - ("Extract method", IsCodeFix: false), - ("Introduce local for 'Console.Write(\"should be using System;\")'", IsCodeFix: false) + ("Generate variable 'Console' -> Generate property 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate field 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate read-only field 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate local 'Console'", CodeActionKind.QuickFix), + ("Generate variable 'Console' -> Generate parameter 'Console'", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate class 'Console' in new file", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate class 'Console'", CodeActionKind.QuickFix), + ("Generate type 'Console' -> Generate nested class 'Console'", CodeActionKind.QuickFix), + ("Extract local function", CodeActionKind.RefactorExtract), + ("Extract method", CodeActionKind.RefactorExtract), + ("Introduce local for 'Console.Write(\"should be using System;\")'", CodeActionKind.Refactor) }; AssertEx.Equal(expected.OrderBy(x => x.Name), refactorings.OrderBy(x => x.Name)); } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs index 20221794ed..6e3e7ea6c9 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsWithOptionsFacts.cs @@ -2,6 +2,7 @@ using System.Reflection; using System.Threading.Tasks; using OmniSharp.Models; +using OmniSharp.Models.V2.CodeActions; using TestUtility; using Xunit; using Xunit.Abstractions; @@ -174,7 +175,7 @@ public void Foo(string a[||], string b) var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain(("Change signature...", IsCodeFix: false), response); + Assert.DoesNotContain(("Change signature...", CodeActionKind.Refactor), response); } [Fact] @@ -188,7 +189,7 @@ public async Task Blacklists_generate_type_with_UI() var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain(("Generate type 'NonExistentBaseType' -> Generate new type...", IsCodeFix: false), response); + Assert.DoesNotContain(("Generate type 'NonExistentBaseType' -> Generate new type...", CodeActionKind.Refactor), response); } [Fact] @@ -206,7 +207,7 @@ public class BaseClass {}"; var response = await FindRefactoringNamesAsync(code); - Assert.DoesNotContain(("Pull 'Foo' up -> Pull members up to base type...", IsCodeFix: false), response); + Assert.DoesNotContain(("Pull 'Foo' up -> Pull members up to base type...", CodeActionKind.Refactor), response); } [Fact] diff --git a/tests/TestUtility/AbstractCodeActionsTestFixture.cs b/tests/TestUtility/AbstractCodeActionsTestFixture.cs index 0b50e9c284..cd1282c971 100644 --- a/tests/TestUtility/AbstractCodeActionsTestFixture.cs +++ b/tests/TestUtility/AbstractCodeActionsTestFixture.cs @@ -64,11 +64,11 @@ protected async Task RunRefactoringAsync(string code, str return await RunRefactoringsAsync(code, identifier, wantsChanges); } - protected async Task> FindRefactoringNamesAsync(string code, bool isAnalyzersEnabled = true, bool analyzeOpenDocumentsOnly = false) + protected async Task> FindRefactoringNamesAsync(string code, bool isAnalyzersEnabled = true, bool analyzeOpenDocumentsOnly = false) { var codeActions = await FindRefactoringsAsync(code, TestHelpers.GetConfigurationDataWithAnalyzerConfig(isAnalyzersEnabled, analyzeOpenDocumentsOnly: analyzeOpenDocumentsOnly)); - return codeActions.Select(a => (a.Name, a.IsCodeFix)); + return codeActions.Select(a => (a.Name, a.CodeActionKind)); } protected async Task> FindRefactoringsAsync(string code, IConfiguration configurationData = null)