From 63842852a7793fe6614d4f78fba111eb8e0d3bf9 Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 8 Jun 2023 17:56:20 +0200 Subject: [PATCH 1/4] Democratizing code fixes - part one --- .../CodeFix/AddInstanceMemberParameter.fs | 20 ++++-- .../FSharp.Editor/CodeFix/ChangeToUpcast.fs | 71 +++++++++++-------- .../CodeFix/ConvertToAnonymousRecord.fs | 50 +++++++------ .../FSharp.Editor/CodeFix/IFSharpCodeFix.fs | 11 +++ .../src/FSharp.Editor/FSharp.Editor.fsproj | 1 + .../AddInstanceMemberParameterTests.fs | 33 +++++++++ .../CodeFixes/ChangeToUpcastTests.fs | 66 +++++++++++++++++ .../CodeFixes/CodeFixTestFramework.fs | 46 ++++++++++++ .../ConvertToAnonymousRecordTests.fs | 31 ++++++++ .../FSharp.Editor.Tests.fsproj | 5 ++ .../Helpers/RoslynHelpers.fs | 10 +++ 11 files changed, 286 insertions(+), 58 deletions(-) create mode 100644 vsintegration/src/FSharp.Editor/CodeFix/IFSharpCodeFix.fs create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddInstanceMemberParameterTests.fs create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ChangeToUpcastTests.fs create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ConvertToAnonymousRecordTests.fs diff --git a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs index 28915bd0af7..e7378074e48 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs @@ -9,17 +9,25 @@ open System.Collections.Immutable open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.CodeFixes +open CancellableTasks + [] type internal FSharpAddInstanceMemberParameterCodeFixProvider() = inherit CodeFixProvider() - static let title = SR.AddMissingInstanceMemberParameter() + interface IFSharpCodeFix with + member _.GetChangesAsync _ span = + cancellableTask { + let title = SR.AddMissingInstanceMemberParameter() + let changes = [ TextChange(TextSpan(span.Start, 0), "x.") ] + return title, changes + } override _.FixableDiagnosticIds = ImmutableArray.Create("FS0673") - override _.RegisterCodeFixesAsync context : Task = - asyncMaybe { - do context.RegisterFsharpFix(CodeFix.AddInstanceMemberParameter, title, [| TextChange(TextSpan(context.Span.Start, 0), "x.") |]) + override this.RegisterCodeFixesAsync context : Task = + cancellableTask { + let! title, changes = (this :> IFSharpCodeFix).GetChangesAsync context.Document context.Span + context.RegisterFsharpFix(CodeFix.AddInstanceMemberParameter, title, changes) } - |> Async.Ignore - |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken) + |> CancellableTask.startAsTask context.CancellationToken diff --git a/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs b/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs index 39279cde5d5..572fcd3f62c 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs @@ -9,40 +9,53 @@ open System.Collections.Immutable open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.CodeFixes +open CancellableTasks + [] type internal FSharpChangeToUpcastCodeFixProvider() = inherit CodeFixProvider() override _.FixableDiagnosticIds = ImmutableArray.Create("FS3198") + interface IFSharpCodeFix with + member _.GetChangesAsync document span = + cancellableTask { + let! cancellationToken = CancellableTask.getCurrentCancellationToken () + + let! sourceText = document.GetTextAsync(cancellationToken) + let text = sourceText.GetSubText(span).ToString() + + // Only works if it's one or the other + let isDowncastOperator = text.Contains(":?>") + let isDowncastKeyword = text.Contains("downcast") + + let changes = + Option.guard ( + (isDowncastOperator || isDowncastKeyword) + && not (isDowncastOperator && isDowncastKeyword) + ) + |> Option.map (fun _ -> + let replacement = + if isDowncastOperator then + text.Replace(":?>", ":>") + else + text.Replace("downcast", "upcast") + + [ TextChange(span, replacement) ]) + |> Option.defaultValue [] + + let title = + if isDowncastOperator then + SR.UseUpcastOperator() + else + SR.UseUpcastKeyword() + + return title, changes + } + override this.RegisterCodeFixesAsync context : Task = - asyncMaybe { - let! sourceText = context.Document.GetTextAsync(context.CancellationToken) - let text = sourceText.GetSubText(context.Span).ToString() - - // Only works if it's one or the other - let isDowncastOperator = text.Contains(":?>") - let isDowncastKeyword = text.Contains("downcast") - - do! - Option.guard ( - (isDowncastOperator || isDowncastKeyword) - && not (isDowncastOperator && isDowncastKeyword) - ) - - let replacement = - if isDowncastOperator then - text.Replace(":?>", ":>") - else - text.Replace("downcast", "upcast") - - let title = - if isDowncastOperator then - SR.UseUpcastOperator() - else - SR.UseUpcastKeyword() - - do context.RegisterFsharpFix(CodeFix.ChangeToUpcast, title, [| TextChange(context.Span, replacement) |]) + cancellableTask { + let! title, changes = (this :> IFSharpCodeFix).GetChangesAsync context.Document context.Span + context.RegisterFsharpFix(CodeFix.ChangeToUpcast, title, changes) } - |> Async.Ignore - |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken) + |> CancellableTask.startAsTask context.CancellationToken diff --git a/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs b/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs index 808dcac5ef9..71b96edefc7 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs @@ -3,45 +3,49 @@ namespace Microsoft.VisualStudio.FSharp.Editor open System.Composition -open System.Threading open System.Threading.Tasks open System.Collections.Immutable open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.CodeFixes -open Microsoft.CodeAnalysis.CodeActions + +open CancellableTasks [] type internal FSharpConvertToAnonymousRecordCodeFixProvider [] () = inherit CodeFixProvider() - static let title = SR.ConvertToAnonymousRecord() - override _.FixableDiagnosticIds = ImmutableArray.Create("FS0039") - override _.RegisterCodeFixesAsync context : Task = - asyncMaybe { - let document = context.Document + interface IFSharpCodeFix with + member _.GetChangesAsync document span = + cancellableTask { + let! cancellationToken = CancellableTask.getCurrentCancellationToken () - let! parseResults = - document.GetFSharpParseResultsAsync(nameof (FSharpConvertToAnonymousRecordCodeFixProvider)) - |> liftAsync + let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpConvertToAnonymousRecordCodeFixProvider)) - let! sourceText = context.Document.GetTextAsync(context.CancellationToken) + let! sourceText = document.GetTextAsync(cancellationToken) - let errorRange = - RoslynHelpers.TextSpanToFSharpRange(document.FilePath, context.Span, sourceText) + let errorRange = + RoslynHelpers.TextSpanToFSharpRange(document.FilePath, span, sourceText) - let! recordRange = parseResults.TryRangeOfRecordExpressionContainingPos errorRange.Start - let! recordSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, recordRange) + let changes = + parseResults.TryRangeOfRecordExpressionContainingPos errorRange.Start + |> Option.bind (fun range -> RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, range)) + |> Option.map (fun span -> + [ + TextChange(TextSpan(span.Start + 1, 0), "|") + TextChange(TextSpan(span.End - 1, 0), "|") + ]) + |> Option.defaultValue [] - let changes = - [ - TextChange(TextSpan(recordSpan.Start + 1, 0), "|") - TextChange(TextSpan(recordSpan.End, 0), "|") - ] + let title = SR.ConvertToAnonymousRecord() + return title, changes + } - context.RegisterFsharpFix(CodeFix.ConvertToAnonymousRecord, title, changes) + override this.RegisterCodeFixesAsync context : Task = + cancellableTask { + let! title, changes = (this :> IFSharpCodeFix).GetChangesAsync context.Document context.Span + return context.RegisterFsharpFix(CodeFix.ConvertToAnonymousRecord, title, changes) } - |> Async.Ignore - |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken) + |> CancellableTask.startAsTask context.CancellationToken diff --git a/vsintegration/src/FSharp.Editor/CodeFix/IFSharpCodeFix.fs b/vsintegration/src/FSharp.Editor/CodeFix/IFSharpCodeFix.fs new file mode 100644 index 00000000000..e0a4fa23a6e --- /dev/null +++ b/vsintegration/src/FSharp.Editor/CodeFix/IFSharpCodeFix.fs @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +namespace Microsoft.VisualStudio.FSharp.Editor + +open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Text + +open CancellableTasks + +type IFSharpCodeFix = + abstract member GetChangesAsync: document: Document -> span: TextSpan -> CancellableTask diff --git a/vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj b/vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj index 598c72145aa..a2673c2d559 100644 --- a/vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj +++ b/vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj @@ -101,6 +101,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddInstanceMemberParameterTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddInstanceMemberParameterTests.fs new file mode 100644 index 00000000000..1fbd6b4a39d --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddInstanceMemberParameterTests.fs @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.AddInstanceMemberParameterTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = FSharpAddInstanceMemberParameterCodeFixProvider() +let private diagnostic = 0673 // This instance member needs a parameter to represent the object being invoked... + +[] +let ``Fixes FS0673`` () = + let code = + """ +type UsefulTestHarness() = + member FortyTwo = 42 +""" + + let expected = + { + Title = "Add missing instance member parameter" + FixedCode = + """ +type UsefulTestHarness() = + member x.FortyTwo = 42 +""" + } + + let actual = codeFix |> fix code diagnostic + + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ChangeToUpcastTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ChangeToUpcastTests.fs new file mode 100644 index 00000000000..9b0ead993d7 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ChangeToUpcastTests.fs @@ -0,0 +1,66 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.ChangeToUpcastTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = FSharpChangeToUpcastCodeFixProvider() +let private diagnostic = 3198 // The conversion is an upcast, not a downcast... + +// Test cases are taken from the original PR: +// https://github.com/dotnet/fsharp/pull/10463 + +[] +let ``Fixes FS3198 - operator`` () = + let code = + """ +type IFoo = abstract member Bar : unit -> unit +type Foo() = interface IFoo with member __.Bar () = () + +let Thing : IFoo = Foo() :?> IFoo +""" + + let expected = + { + Title = "Use ':>' operator" + FixedCode = + """ +type IFoo = abstract member Bar : unit -> unit +type Foo() = interface IFoo with member __.Bar () = () + +let Thing : IFoo = Foo() :> IFoo +""" + } + + let actual = codeFix |> fix code diagnostic + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS3198 - keyword`` () = + let code = + """ +type IFoo = abstract member Bar : unit -> unit +type Foo() = interface IFoo with member __.Bar () = () + +let Thing : IFoo = downcast Foo() +""" + + let expected = + { + Title = "Use 'upcast'" + FixedCode = + """ +type IFoo = abstract member Bar : unit -> unit +type Foo() = interface IFoo with member __.Bar () = () + +let Thing : IFoo = upcast Foo() +""" + } + + let actual = codeFix |> fix code diagnostic + + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs new file mode 100644 index 00000000000..04998935ea8 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.CodeFixTestFramework + +open System.Threading + +open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Text +open Microsoft.VisualStudio.FSharp.Editor +open Microsoft.VisualStudio.FSharp.Editor.CancellableTasks + +open FSharp.Editor.Tests.Helpers + +type TestCodeFix = { Title: string; FixedCode: string } + +let getRelevantDiagnostic (document: Document) errorNumber = + cancellableTask { + let! _, checkFileResults = document.GetFSharpParseAndCheckResultsAsync "test" + + return + checkFileResults.Diagnostics + |> Seq.where (fun d -> d.ErrorNumber = errorNumber) + |> Seq.exactlyOne + } + +let fix (code: string) diagnostic (fixProvider: IFSharpCodeFix) = + cancellableTask { + let sourceText = SourceText.From code + let document = RoslynTestHelpers.GetFsDocument code + + let! diagnostic = getRelevantDiagnostic document diagnostic + + let diagnosticSpan = + RoslynHelpers.FSharpRangeToTextSpan(sourceText, diagnostic.Range) + + let! title, changes = fixProvider.GetChangesAsync document diagnosticSpan + let fixedSourceText = sourceText.WithChanges changes + + return + { + Title = title + FixedCode = fixedSourceText.ToString() + } + } + |> CancellableTask.start CancellationToken.None + |> fun task -> task.Result diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ConvertToAnonymousRecordTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ConvertToAnonymousRecordTests.fs new file mode 100644 index 00000000000..9f8008356bb --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/ConvertToAnonymousRecordTests.fs @@ -0,0 +1,31 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.ConvertToAnonymousRecordTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = FSharpConvertToAnonymousRecordCodeFixProvider() +let private diagnostic = 0039 // The record label is not defined... + +[] +let ``Fixes FS0039`` () = + let code = + """ +let band = { Name = "The Velvet Underground" } +""" + + let expected = + { + Title = "Convert to Anonymous Record" + FixedCode = + """ +let band = {| Name = "The Velvet Underground" |} +""" + } + + let actual = codeFix |> fix code diagnostic + + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index e1aae0a95a1..232e394455c 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -32,6 +32,10 @@ + + + + @@ -55,6 +59,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs index 17ab1d90f4c..3fed33dc983 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs @@ -337,3 +337,13 @@ type RoslynTestHelpers private () = options |> RoslynTestHelpers.SetProjectOptions projId solution solution, checker + + static member GetFsDocument code = + // without this lib some symbols are not loaded + let options = + { RoslynTestHelpers.DefaultProjectOptions with + OtherOptions = [| "--targetprofile:netcore" |] + } + + RoslynTestHelpers.CreateSolution(code, options = options) + |> RoslynTestHelpers.GetSingleDocument From 7555fe491a1e01418c74d5f544f503af5042f246 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 13 Jun 2023 14:33:29 +0200 Subject: [PATCH 2/4] Up --- .../src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs | 3 ++- .../src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs index e7378074e48..db2cba010f4 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs @@ -15,10 +15,11 @@ open CancellableTasks type internal FSharpAddInstanceMemberParameterCodeFixProvider() = inherit CodeFixProvider() + static let title = SR.AddMissingInstanceMemberParameter() + interface IFSharpCodeFix with member _.GetChangesAsync _ span = cancellableTask { - let title = SR.AddMissingInstanceMemberParameter() let changes = [ TextChange(TextSpan(span.Start, 0), "x.") ] return title, changes } diff --git a/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs b/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs index 71b96edefc7..da39b93e9c3 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/ConvertToAnonymousRecord.fs @@ -15,6 +15,8 @@ open CancellableTasks type internal FSharpConvertToAnonymousRecordCodeFixProvider [] () = inherit CodeFixProvider() + static let title = SR.ConvertToAnonymousRecord() + override _.FixableDiagnosticIds = ImmutableArray.Create("FS0039") interface IFSharpCodeFix with @@ -39,7 +41,6 @@ type internal FSharpConvertToAnonymousRecordCodeFixProvider [ Option.defaultValue [] - let title = SR.ConvertToAnonymousRecord() return title, changes } From 399d78afb76f05877071a2a543b97c71c94e5711 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 13 Jun 2023 16:49:20 +0200 Subject: [PATCH 3/4] Update AddInstanceMemberParameter.fs --- .../src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs index db2cba010f4..e98976184a0 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/AddInstanceMemberParameter.fs @@ -19,10 +19,8 @@ type internal FSharpAddInstanceMemberParameterCodeFixProvider() = interface IFSharpCodeFix with member _.GetChangesAsync _ span = - cancellableTask { - let changes = [ TextChange(TextSpan(span.Start, 0), "x.") ] - return title, changes - } + let changes = [ TextChange(TextSpan(span.Start, 0), "x.") ] + CancellableTask.singleton (title, changes) override _.FixableDiagnosticIds = ImmutableArray.Create("FS0673") From 517debf4461ccbd9a9c2938a0b17d270cec87e53 Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 14 Jun 2023 14:13:10 +0200 Subject: [PATCH 4/4] Update ChangeToUpcast.fs --- .../FSharp.Editor/CodeFix/ChangeToUpcast.fs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs b/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs index 572fcd3f62c..7136009a928 100644 --- a/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs +++ b/vsintegration/src/FSharp.Editor/CodeFix/ChangeToUpcast.fs @@ -30,19 +30,19 @@ type internal FSharpChangeToUpcastCodeFixProvider() = let isDowncastKeyword = text.Contains("downcast") let changes = - Option.guard ( - (isDowncastOperator || isDowncastKeyword) - && not (isDowncastOperator && isDowncastKeyword) - ) - |> Option.map (fun _ -> - let replacement = - if isDowncastOperator then - text.Replace(":?>", ":>") - else - text.Replace("downcast", "upcast") - - [ TextChange(span, replacement) ]) - |> Option.defaultValue [] + [ + if + (isDowncastOperator || isDowncastKeyword) + && not (isDowncastOperator && isDowncastKeyword) + then + let replacement = + if isDowncastOperator then + text.Replace(":?>", ":>") + else + text.Replace("downcast", "upcast") + + TextChange(span, replacement) + ] let title = if isDowncastOperator then