From c40e13415bd48b0f44aa044a94c0fa57afda96bc Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 29 Aug 2023 13:42:20 +0200 Subject: [PATCH 1/3] Improving AddOpen code fix provider --- .../CodeFixes/AddOpenCodeFixProvider.fs | 322 ++++++++------ .../FSharp.Editor/Options/EditorOptions.fs | 2 + .../CodeFixes/AddOpenOnTopOffTests.fs | 406 ++++++++++++++++++ .../CodeFixes/AddOpenOnTopOnTests.fs | 401 +++++++++++++++++ .../CodeFixes/CodeFixTestFramework.fs | 3 + .../FSharp.Editor.Tests.fsproj | 2 + .../Helpers/RoslynHelpers.fs | 30 +- 7 files changed, 1026 insertions(+), 140 deletions(-) create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index 08ce2ae91c6..a1093b188dc 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -2,40 +2,107 @@ namespace Microsoft.VisualStudio.FSharp.Editor +open System open System.Composition -open System.Threading.Tasks -open System.Threading open System.Collections.Immutable open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.CodeFixes open FSharp.Compiler.EditorServices +open FSharp.Compiler.Syntax open FSharp.Compiler.Text + open CancellableTasks [] -type internal AddOpenCodeFixProvider [] (assemblyContentProvider: AssemblyContentProvider) = +type internal AddOpenCodeFixProvider [] (assemblyContentProvider: AssemblyContentProvider) = inherit CodeFixProvider() + static let br = Environment.NewLine + let fixUnderscoresInMenuText (text: string) = text.Replace("_", "__") let qualifySymbolFix (context: CodeFixContext) (fullName, qualifier) = - context.RegisterFsharpFix(CodeFix.AddOpen, fixUnderscoresInMenuText fullName, [| TextChange(context.Span, qualifier) |]) - - let openNamespaceFix (context: CodeFixContext) ctx name ns multipleNames sourceText = - let displayText = "open " + ns + (if multipleNames then " (" + name + ")" else "") - let newText, _ = OpenDeclarationHelper.insertOpenDeclaration sourceText ctx ns - let changes = newText.GetTextChanges(sourceText) + { + Name = CodeFix.AddOpen + Message = fixUnderscoresInMenuText fullName + Changes = [ TextChange(context.Span, qualifier) ] + } - context.RegisterFsharpFix(CodeFix.AddOpen, fixUnderscoresInMenuText displayText, changes) + // Hey, I know what you're thinking: this is a horrible hack. + // Indeed it is, this is a better (but still bad) version of the OpenDeclarionHelper. + // The things should be actually fixed in the InsertionContext, it's bugged. + // But currently CompletionProvider also depends on InsertionContext and it's not tested enough. + // So fixing InsertionContext or OpenDeclarationHelper might break completion which would be bad. + // The hack below is at least heavily tested. + // And at least it shows what should be fixed down the line. + let getOpenDeclaration (sourceText: SourceText) (ctx: InsertionContext) (ns: string) = + // insertion context counts from 2, make the world sane + let insertionLineNumber = ctx.Pos.Line - 2 + let margin = String(' ', ctx.Pos.Column) + + let startLineNumber, openDeclaration = + match ctx.ScopeKind with + | ScopeKind.TopModule -> + match sourceText.Lines[insertionLineNumber].ToString().Trim() with + + // explicit top level module + | line when line.StartsWith "module" && not (line.EndsWith "=") -> + insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" + + // nested module, shouldn't be here + | line when line.StartsWith "module" -> + insertionLineNumber, $"{margin}open {ns}{br}{br}" + + // attribute, shouldn't be here + | line when line.StartsWith "[<" && line.EndsWith ">]" -> + let moduleDeclLineNumber = + sourceText.Lines + |> Seq.skip insertionLineNumber + |> Seq.findIndex (fun line -> line.ToString().Contains "module") + |> (+) insertionLineNumber + + let moduleDeclLineText = sourceText.Lines[moduleDeclLineNumber].ToString().Trim() + if moduleDeclLineText.EndsWith "=" then + insertionLineNumber, $"{margin}open {ns}{br}{br}" + else + moduleDeclLineNumber + 2, $"{margin}open {ns}{br}{br}" + + // something else, shot in the dark + | _ -> + insertionLineNumber, $"{margin}open {ns}{br}{br}" + + | ScopeKind.Namespace -> + insertionLineNumber + 3, $"{margin}open {ns}{br}{br}" + | ScopeKind.NestedModule -> + insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" + | ScopeKind.OpenDeclaration -> + insertionLineNumber + 1, $"{margin}open {ns}{br}" + + // So far I don't know how to get here + | ScopeKind.HashDirective -> + insertionLineNumber + 1, $"open {ns}{br}{br}" + + let start = sourceText.Lines[startLineNumber].Start + TextChange(TextSpan(start, 0), openDeclaration) + + let openNamespaceFix ctx name ns multipleNames sourceText = + let displayText = $"open {ns}" + (if multipleNames then " (" + name + ")" else "") + let change = getOpenDeclaration sourceText ctx ns + + { + Name = CodeFix.AddOpen + Message = fixUnderscoresInMenuText displayText + Changes = [ change ] + } - let addSuggestionsAsCodeFixes + let getSuggestionsAsCodeFixes (context: CodeFixContext) (sourceText: SourceText) (candidates: (InsertionContextEntity * InsertionContext) list) = - do + seq { candidates |> Seq.choose (fun (entity, ctx) -> entity.Namespace |> Option.map (fun ns -> ns, entity.FullDisplayName, ctx)) |> Seq.groupBy (fun (ns, _, _) -> ns) @@ -50,128 +117,127 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr let multipleNames = names |> Array.length > 1 names |> Seq.map (fun (name, ctx) -> ns, name, ctx, multipleNames)) |> Seq.concat - |> Seq.iter (fun (ns, name, ctx, multipleNames) -> openNamespaceFix context ctx name ns multipleNames sourceText) + |> Seq.map (fun (ns, name, ctx, multipleNames) -> openNamespaceFix ctx name ns multipleNames sourceText) - do candidates |> Seq.filter (fun (entity, _) -> not (entity.LastIdent.StartsWith "op_")) // Don't include qualified operator names. The resultant codefix won't compile because it won't be an infix operator anymore. |> Seq.map (fun (entity, _) -> entity.FullRelativeName, entity.Qualifier) |> Seq.distinct |> Seq.sort - |> Seq.iter (qualifySymbolFix context) + |> Seq.map (qualifySymbolFix context) + + } + |> Seq.concat override _.FixableDiagnosticIds = ImmutableArray.Create("FS0039", "FS0043") - override _.RegisterCodeFixesAsync context : Task = - asyncMaybe { - let document = context.Document - - let! sourceText = document.GetTextAsync(context.CancellationToken) - - let! parseResults, checkResults = - document.GetFSharpParseAndCheckResultsAsync(nameof (AddOpenCodeFixProvider)) - |> CancellableTask.start context.CancellationToken - |> Async.AwaitTask - |> liftAsync - - let line = sourceText.Lines.GetLineFromPosition(context.Span.End) - let linePos = sourceText.Lines.GetLinePosition(context.Span.End) - - let! defines, langVersion, strictIndentation = - document.GetFsharpParsingOptionsAsync(nameof (AddOpenCodeFixProvider)) - |> liftAsync - - let! symbol = - maybe { - let! lexerSymbol = - Tokenizer.getSymbolAtPosition ( - document.Id, - sourceText, - context.Span.End, - document.FilePath, - defines, - SymbolLookupKind.Greedy, - false, - false, - Some langVersion, - strictIndentation, - context.CancellationToken - ) - - return - checkResults.GetSymbolUseAtLocation( - Line.fromZ linePos.Line, - lexerSymbol.Ident.idRange.EndColumn, - line.ToString(), - lexerSymbol.FullIsland - ) - } - - do! Option.guard symbol.IsNone - - let unresolvedIdentRange = - let startLinePos = sourceText.Lines.GetLinePosition context.Span.Start - let startPos = Position.fromZ startLinePos.Line startLinePos.Character - let endLinePos = sourceText.Lines.GetLinePosition context.Span.End - let endPos = Position.fromZ endLinePos.Line endLinePos.Character - Range.mkRange context.Document.FilePath startPos endPos - - let isAttribute = - ParsedInput.GetEntityKind(unresolvedIdentRange.Start, parseResults.ParseTree) = Some EntityKind.Attribute - - let entities = - assemblyContentProvider.GetAllEntitiesInProjectAndReferencedAssemblies checkResults - |> List.collect (fun s -> - [ - yield s.TopRequireQualifiedAccessParent, s.AutoOpenParent, s.Namespace, s.CleanedIdents - if isAttribute then - let lastIdent = s.CleanedIdents.[s.CleanedIdents.Length - 1] - - if - lastIdent.EndsWith "Attribute" - && s.Kind LookupType.Precise = EntityKind.Attribute - then - yield - s.TopRequireQualifiedAccessParent, - s.AutoOpenParent, - s.Namespace, - s.CleanedIdents - |> Array.replace (s.CleanedIdents.Length - 1) (lastIdent.Substring(0, lastIdent.Length - 9)) - ]) - - let longIdent = - ParsedInput.GetLongIdentAt parseResults.ParseTree unresolvedIdentRange.End - - let! maybeUnresolvedIdents = - longIdent - |> Option.map (fun longIdent -> - longIdent - |> List.map (fun ident -> - { - Ident = ident.idText - Resolved = not (ident.idRange = unresolvedIdentRange) - }) - |> List.toArray) - - let insertionPoint = - if document.Project.IsFSharpCodeFixesAlwaysPlaceOpensAtTopLevelEnabled then - OpenStatementInsertionPoint.TopLevel - else - OpenStatementInsertionPoint.Nearest - - let createEntity = - ParsedInput.TryFindInsertionContext - unresolvedIdentRange.StartLine - parseResults.ParseTree - maybeUnresolvedIdents - insertionPoint - - return - entities - |> Seq.map createEntity - |> Seq.concat - |> Seq.toList - |> addSuggestionsAsCodeFixes context sourceText - } - |> Async.Ignore - |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken) + override this.RegisterCodeFixesAsync context = context.RegisterFsharpFix this + + interface IFSharpCodeFixProvider with + member _.GetCodeFixIfAppliesAsync context = + cancellableTask { + let document = context.Document + + let! sourceText = context.GetSourceTextAsync() + + let! parseResults, checkResults = + document.GetFSharpParseAndCheckResultsAsync(nameof AddOpenCodeFixProvider) + + let line = sourceText.Lines.GetLineFromPosition(context.Span.End) + let linePos = sourceText.Lines.GetLinePosition(context.Span.End) + + let! defines, langVersion, strictIndentation = + document.GetFsharpParsingOptionsAsync(nameof AddOpenCodeFixProvider) + + return + Tokenizer.getSymbolAtPosition ( + document.Id, + sourceText, + context.Span.End, + document.FilePath, + defines, + SymbolLookupKind.Greedy, + false, + false, + Some langVersion, + strictIndentation, + context.CancellationToken + ) + |> Option.filter (fun lexerSymbol -> + let symbolOpt = + checkResults.GetSymbolUseAtLocation( + Line.fromZ linePos.Line, + lexerSymbol.Ident.idRange.EndColumn, + line.ToString(), + lexerSymbol.FullIsland + ) + + match symbolOpt with + | None -> true + // this is for operators for FS0043 + | Some symbol when PrettyNaming.IsLogicalOpName symbol.Symbol.DisplayName -> true + | _ -> false) + |> Option.bind (fun _ -> + let unresolvedIdentRange = + let startLinePos = sourceText.Lines.GetLinePosition context.Span.Start + let startPos = Position.fromZ startLinePos.Line startLinePos.Character + let endLinePos = sourceText.Lines.GetLinePosition context.Span.End + let endPos = Position.fromZ endLinePos.Line endLinePos.Character + Range.mkRange context.Document.FilePath startPos endPos + + let isAttribute = + ParsedInput.GetEntityKind(unresolvedIdentRange.Start, parseResults.ParseTree) = Some EntityKind.Attribute + + let entities = + assemblyContentProvider.GetAllEntitiesInProjectAndReferencedAssemblies checkResults + |> List.collect (fun s -> + [ + yield s.TopRequireQualifiedAccessParent, s.AutoOpenParent, s.Namespace, s.CleanedIdents + if isAttribute then + let lastIdent = s.CleanedIdents.[s.CleanedIdents.Length - 1] + + if + lastIdent.EndsWith "Attribute" + && s.Kind LookupType.Precise = EntityKind.Attribute + then + yield + s.TopRequireQualifiedAccessParent, + s.AutoOpenParent, + s.Namespace, + s.CleanedIdents + |> Array.replace (s.CleanedIdents.Length - 1) (lastIdent.Substring(0, lastIdent.Length - 9)) + ]) + + ParsedInput.GetLongIdentAt parseResults.ParseTree unresolvedIdentRange.End + |> Option.bind (fun longIdent -> + let maybeUnresolvedIdents = + longIdent + |> List.map (fun ident -> + { + Ident = ident.idText + Resolved = not (ident.idRange = unresolvedIdentRange) + }) + |> List.toArray + + let insertionPoint = + if document.Project.IsFSharpCodeFixesAlwaysPlaceOpensAtTopLevelEnabled then + OpenStatementInsertionPoint.TopLevel + else + OpenStatementInsertionPoint.Nearest + + let createEntity = + ParsedInput.TryFindInsertionContext + unresolvedIdentRange.StartLine + parseResults.ParseTree + maybeUnresolvedIdents + insertionPoint + + entities + |> Seq.map createEntity + |> Seq.concat + |> Seq.toList + |> getSuggestionsAsCodeFixes context sourceText + |> Seq.tryHead)) + + |> ValueOption.ofOption + } diff --git a/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs b/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs index 5c3fd525812..c98ad6a4a57 100644 --- a/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs +++ b/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs @@ -162,6 +162,8 @@ type EditorOptions() = [)>] member private _.SettingsStore = store + member _.With value = store.Register value + interface Microsoft.CodeAnalysis.Host.IWorkspaceService module internal OptionsUI = diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs new file mode 100644 index 00000000000..0999cbe50df --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -0,0 +1,406 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOffTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = + AddOpenCodeFixProvider(AssemblyContentProvider()) + +let mode = WithSettings { CodeFixesOptions.Default with AlwaysPlaceOpensAtTopLevel = false } + +[] +let ``Fixes FS0039 for missing opens - basic`` () = + let code = + """Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - first line is empty`` () = + let code = + """ +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - multiple first lines are empty`` () = + let code = + """ + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + + +[] +let ``Fixes FS0039 for missing opens - there is already an open directive`` () = + let code = + """open System.IO + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """open System.IO +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - top level module is explicit`` () = + let code = + """module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - nested module`` () = + let code = + """module Module1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 = + + open System + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - module has attributes`` () = + let code = + """ +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +// TODO: the open statement should actually be within the module +[] +let ``Fixes FS0039 for missing opens - nested module has attributes`` () = + let code = + """ +[] +module Module1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +open System + +[] +module Module1 = + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + + +[] +let ``Fixes FS0039 for missing opens - module has multiple attributes`` () = + let code = + """ +[] +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - attributes are mixed with empty lines`` () = + let code = + """ +[] + +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] + +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - multiple modules in one file`` () = + let code = + """ +module Module1 = + + let x = 42 + +module Module2 = + + Console.WriteLine(42) +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +module Module1 = + + let x = 42 + +module Module2 = + + open System + + Console.WriteLine(42) +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - explicit namespace`` () = + let code = + """ +namespace N1 + +module M1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +namespace N1 + +module M1 = + + open System + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Doesn't fix FS0039 for random undefined symbols`` () = + let code = + """ +let f = g +""" + + let expected = None + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0043 for missing opens`` () = + let code = + """ +module M = + let (++) x y = 10 * x + y + +module N = + let theAnswer = 4 ++ 2 +""" + + let expected = + Some + { + Message = "open M" + FixedCode = + """ +module M = + let (++) x y = 10 * x + y + +open M + +module N = + let theAnswer = 4 ++ 2 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Doesn't fix FS0043 for random unsupported values`` () = + let code = + """ +type RecordType = { X : int } + +let x : RecordType = null +""" + + let expected = None + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs new file mode 100644 index 00000000000..1b6f2ebe60b --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -0,0 +1,401 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOnTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = + AddOpenCodeFixProvider(AssemblyContentProvider()) + +[] +let ``Fixes FS0039 for missing opens - basic`` () = + let code = + """Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - first line is empty`` () = + let code = + """ +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - multiple first lines are empty`` () = + let code = + """ + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - there is already an open directive`` () = + let code = + """open System.IO + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """open System.IO +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - top level module is explicit`` () = + let code = + """module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - nested module`` () = + let code = + """module Module1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """open System + +module Module1 = + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - module has attributes`` () = + let code = + """ +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - nested module has attributes`` () = + let code = + """ +[] +module Module1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +open System + +[] +module Module1 = + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - module has multiple attributes`` () = + let code = + """ +[] +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - attributes are mixed with empty lines`` () = + let code = + """ +[] + +[] +module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +[] + +[] +module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - multiple modules in one file`` () = + let code = + """ +module Module1 = + + let x = 42 + +module Module2 = + + Console.WriteLine(42) +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +open System + +module Module1 = + + let x = 42 + +module Module2 = + + Console.WriteLine(42) +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - explicit namespace`` () = + let code = + """ +namespace N1 + +module M1 = + + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """ +namespace N1 + +open System + +module M1 = + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Doesn't fix FS0039 for random undefined symbols`` () = + let code = + """ +let f = g +""" + + let expected = None + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0043 for missing opens`` () = + let code = + """ +module M = + let (++) x y = 10 * x + y + +module N = + let theAnswer = 4 ++ 2 +""" + + let expected = + Some + { + Message = "open M" + FixedCode = + """ +module M = + let (++) x y = 10 * x + y + +open M + +module N = + let theAnswer = 4 ++ 2 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Doesn't fix FS0043 for random unsupported values`` () = + let code = + """ +type RecordType = { X : int } + +let x : RecordType = null +""" + + let expected = None + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs index 2098dc76958..d1986e6ded3 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs @@ -22,6 +22,7 @@ type Mode = | WithOption of CustomProjectOption: string | WithSignature of FsiCode: string | Manual of Squiggly: string * Diagnostic: string + | WithSettings of CodeFixesOptions let inline toOption o = match o with @@ -44,6 +45,7 @@ let getDocument code mode = | WithOption option -> RoslynTestHelpers.GetFsDocument(code, option) | WithSignature fsiCode -> RoslynTestHelpers.GetFsiAndFsDocuments fsiCode code |> Seq.last | Manual _ -> RoslynTestHelpers.GetFsDocument code + | WithSettings settings -> RoslynTestHelpers.GetFsDocument(code, customEditorOptions = settings) let getRelevantDiagnostics (document: Document) = cancellableTask { @@ -67,6 +69,7 @@ let createTestCodeFixContext (code: string) document (mode: Mode) diagnosticIds |> Array.filter (fun d -> diagnosticIds |> Seq.contains d.ErrorNumberText) | WithOption _ -> getRelevantDiagnostics document | WithSignature _ -> getRelevantDiagnostics document + | WithSettings _ -> getRelevantDiagnostics document | Manual (squiggly, diagnostic) -> let spanStart = code.IndexOf squiggly let span = TextSpan(spanStart, squiggly.Length) diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index 9972e915c22..1cdf8506dfb 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -58,6 +58,8 @@ + + diff --git a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs index ee069431c98..de16cf0a137 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs @@ -175,15 +175,6 @@ type TestHostWorkspaceServices(hostServices: HostServices, workspace: Workspace) let langServices = TestHostLanguageServices(this, LanguageNames.FSharp, exportProvider) - member this.SetEditorEptions(value) = - exportProvider - .GetExportedValue() - .SaveSettings(value) - - member this.WithEditorOptions(value) = - this.SetEditorEptions(value) - this - override _.Workspace = workspace override this.GetService<'T when 'T :> IWorkspaceService>() : 'T = @@ -281,7 +272,14 @@ type RoslynTestHelpers private () = options.OtherOptions |> ImmutableArray.CreateRange ) - static member CreateSolution(source, ?options: FSharpProjectOptions) = + static member SetEditorOptions (solution: Solution) options = + solution + .Workspace + .Services + .GetService() + .With(options) + + static member CreateSolution(source, ?options: FSharpProjectOptions, ?editorOptions) = let projId = ProjectId.CreateNewId() let docInfo = RoslynTestHelpers.CreateDocumentInfo projId "C:\\test.fs" source @@ -294,6 +292,9 @@ type RoslynTestHelpers private () = |> Option.defaultValue RoslynTestHelpers.DefaultProjectOptions |> RoslynTestHelpers.SetProjectOptions projId solution + if editorOptions.IsSome then + RoslynTestHelpers.SetEditorOptions solution editorOptions.Value + solution static member GetSingleDocument(solution: Solution) = @@ -338,7 +339,7 @@ type RoslynTestHelpers private () = solution, checker - static member GetFsDocument(code, ?customProjectOption: string) = + static member GetFsDocument(code, ?customProjectOption: string, ?customEditorOptions) = let customProjectOptions = customProjectOption |> Option.map (fun o -> [| o |]) @@ -354,7 +355,12 @@ type RoslynTestHelpers private () = |> Array.append customProjectOptions } - RoslynTestHelpers.CreateSolution(code, options = options) + let solution = + match customEditorOptions with + | Some o -> RoslynTestHelpers.CreateSolution(code, options, o) + | None -> RoslynTestHelpers.CreateSolution(code, options) + + solution |> RoslynTestHelpers.GetSingleDocument static member GetFsiAndFsDocuments (fsiCode: string) (fsCode: string) = From 3383b8c87ec5b6de8401e3fbefb3ea42ace5206f Mon Sep 17 00:00:00 2001 From: Petr Date: Fri, 1 Sep 2023 18:43:56 +0200 Subject: [PATCH 2/3] fantomas --- .../CodeFixes/AddOpenCodeFixProvider.fs | 58 ++++++++----------- .../CodeFixes/AddOpenOnTopOffTests.fs | 23 ++++---- .../CodeFixes/AddOpenOnTopOnTests.fs | 15 +++-- .../Helpers/RoslynHelpers.fs | 9 +-- 4 files changed, 46 insertions(+), 59 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index a1093b188dc..eb3fc04a0f0 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -16,7 +16,7 @@ open FSharp.Compiler.Text open CancellableTasks [] -type internal AddOpenCodeFixProvider [] (assemblyContentProvider: AssemblyContentProvider) = +type internal AddOpenCodeFixProvider [] (assemblyContentProvider: AssemblyContentProvider) = inherit CodeFixProvider() static let br = Environment.NewLine @@ -35,55 +35,49 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr // The things should be actually fixed in the InsertionContext, it's bugged. // But currently CompletionProvider also depends on InsertionContext and it's not tested enough. // So fixing InsertionContext or OpenDeclarationHelper might break completion which would be bad. - // The hack below is at least heavily tested. + // The hack below is at least heavily tested. // And at least it shows what should be fixed down the line. let getOpenDeclaration (sourceText: SourceText) (ctx: InsertionContext) (ns: string) = // insertion context counts from 2, make the world sane let insertionLineNumber = ctx.Pos.Line - 2 let margin = String(' ', ctx.Pos.Column) - let startLineNumber, openDeclaration = + let startLineNumber, openDeclaration = match ctx.ScopeKind with | ScopeKind.TopModule -> - match sourceText.Lines[insertionLineNumber].ToString().Trim() with - + match sourceText.Lines[ insertionLineNumber ].ToString().Trim() with + // explicit top level module - | line when line.StartsWith "module" && not (line.EndsWith "=") -> - insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" - + | line when line.StartsWith "module" && not (line.EndsWith "=") -> insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" + // nested module, shouldn't be here - | line when line.StartsWith "module" -> - insertionLineNumber, $"{margin}open {ns}{br}{br}" - + | line when line.StartsWith "module" -> insertionLineNumber, $"{margin}open {ns}{br}{br}" + // attribute, shouldn't be here | line when line.StartsWith "[<" && line.EndsWith ">]" -> - let moduleDeclLineNumber = + let moduleDeclLineNumber = sourceText.Lines |> Seq.skip insertionLineNumber |> Seq.findIndex (fun line -> line.ToString().Contains "module") |> (+) insertionLineNumber - let moduleDeclLineText = sourceText.Lines[moduleDeclLineNumber].ToString().Trim() - if moduleDeclLineText.EndsWith "=" then + let moduleDeclLineText = sourceText.Lines[ moduleDeclLineNumber ].ToString().Trim() + + if moduleDeclLineText.EndsWith "=" then insertionLineNumber, $"{margin}open {ns}{br}{br}" else moduleDeclLineNumber + 2, $"{margin}open {ns}{br}{br}" - + // something else, shot in the dark - | _ -> - insertionLineNumber, $"{margin}open {ns}{br}{br}" + | _ -> insertionLineNumber, $"{margin}open {ns}{br}{br}" - | ScopeKind.Namespace -> - insertionLineNumber + 3, $"{margin}open {ns}{br}{br}" - | ScopeKind.NestedModule -> - insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" - | ScopeKind.OpenDeclaration -> - insertionLineNumber + 1, $"{margin}open {ns}{br}" + | ScopeKind.Namespace -> insertionLineNumber + 3, $"{margin}open {ns}{br}{br}" + | ScopeKind.NestedModule -> insertionLineNumber + 2, $"{margin}open {ns}{br}{br}" + | ScopeKind.OpenDeclaration -> insertionLineNumber + 1, $"{margin}open {ns}{br}" // So far I don't know how to get here - | ScopeKind.HashDirective -> - insertionLineNumber + 1, $"open {ns}{br}{br}" - + | ScopeKind.HashDirective -> insertionLineNumber + 1, $"open {ns}{br}{br}" + let start = sourceText.Lines[startLineNumber].Start TextChange(TextSpan(start, 0), openDeclaration) @@ -140,14 +134,12 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr let! sourceText = context.GetSourceTextAsync() - let! parseResults, checkResults = - document.GetFSharpParseAndCheckResultsAsync(nameof AddOpenCodeFixProvider) + let! parseResults, checkResults = document.GetFSharpParseAndCheckResultsAsync(nameof AddOpenCodeFixProvider) let line = sourceText.Lines.GetLineFromPosition(context.Span.End) let linePos = sourceText.Lines.GetLinePosition(context.Span.End) - let! defines, langVersion, strictIndentation = - document.GetFsharpParsingOptionsAsync(nameof AddOpenCodeFixProvider) + let! defines, langVersion, strictIndentation = document.GetFsharpParsingOptionsAsync(nameof AddOpenCodeFixProvider) return Tokenizer.getSymbolAtPosition ( @@ -164,7 +156,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr context.CancellationToken ) |> Option.filter (fun lexerSymbol -> - let symbolOpt = + let symbolOpt = checkResults.GetSymbolUseAtLocation( Line.fromZ linePos.Line, lexerSymbol.Ident.idRange.EndColumn, @@ -177,7 +169,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr // this is for operators for FS0043 | Some symbol when PrettyNaming.IsLogicalOpName symbol.Symbol.DisplayName -> true | _ -> false) - |> Option.bind (fun _ -> + |> Option.bind (fun _ -> let unresolvedIdentRange = let startLinePos = sourceText.Lines.GetLinePosition context.Span.Start let startPos = Position.fromZ startLinePos.Line startLinePos.Character @@ -187,7 +179,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr let isAttribute = ParsedInput.GetEntityKind(unresolvedIdentRange.Start, parseResults.ParseTree) = Some EntityKind.Attribute - + let entities = assemblyContentProvider.GetAllEntitiesInProjectAndReferencedAssemblies checkResults |> List.collect (fun s -> diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index 0999cbe50df..297554f3784 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -7,10 +7,13 @@ open Xunit open CodeFixTestFramework -let private codeFix = - AddOpenCodeFixProvider(AssemblyContentProvider()) +let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) -let mode = WithSettings { CodeFixesOptions.Default with AlwaysPlaceOpensAtTopLevel = false } +let mode = + WithSettings + { CodeFixesOptions.Default with + AlwaysPlaceOpensAtTopLevel = false + } [] let ``Fixes FS0039 for missing opens - basic`` () = @@ -81,7 +84,6 @@ Console.WriteLine 42 Assert.Equal(expected, actual) - [] let ``Fixes FS0039 for missing opens - there is already an open directive`` () = let code = @@ -118,7 +120,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """module Module1 open System @@ -143,7 +145,7 @@ let ``Fixes FS0039 for missing opens - nested module`` () = Some { Message = "open System" - FixedCode = + FixedCode = """module Module1 = open System @@ -170,7 +172,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] module Module1 @@ -200,7 +202,7 @@ module Module1 = Some { Message = "open System" - FixedCode = + FixedCode = """ open System @@ -215,7 +217,6 @@ module Module1 = Assert.Equal(expected, actual) - [] let ``Fixes FS0039 for missing opens - module has multiple attributes`` () = let code = @@ -231,7 +232,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] [] @@ -263,7 +264,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 1b6f2ebe60b..ac95de440d4 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -7,8 +7,7 @@ open Xunit open CodeFixTestFramework -let private codeFix = - AddOpenCodeFixProvider(AssemblyContentProvider()) +let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) [] let ``Fixes FS0039 for missing opens - basic`` () = @@ -115,7 +114,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """module Module1 open System @@ -140,7 +139,7 @@ let ``Fixes FS0039 for missing opens - nested module`` () = Some { Message = "open System" - FixedCode = + FixedCode = """open System module Module1 = @@ -167,7 +166,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] module Module1 @@ -196,7 +195,7 @@ module Module1 = Some { Message = "open System" - FixedCode = + FixedCode = """ open System @@ -226,7 +225,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] [] @@ -258,7 +257,7 @@ Console.WriteLine 42 Some { Message = "open System" - FixedCode = + FixedCode = """ [] diff --git a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs index de16cf0a137..5b00301821b 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs @@ -273,11 +273,7 @@ type RoslynTestHelpers private () = ) static member SetEditorOptions (solution: Solution) options = - solution - .Workspace - .Services - .GetService() - .With(options) + solution.Workspace.Services.GetService().With(options) static member CreateSolution(source, ?options: FSharpProjectOptions, ?editorOptions) = let projId = ProjectId.CreateNewId() @@ -360,8 +356,7 @@ type RoslynTestHelpers private () = | Some o -> RoslynTestHelpers.CreateSolution(code, options, o) | None -> RoslynTestHelpers.CreateSolution(code, options) - solution - |> RoslynTestHelpers.GetSingleDocument + solution |> RoslynTestHelpers.GetSingleDocument static member GetFsiAndFsDocuments (fsiCode: string) (fsCode: string) = let projInfo = From 1ce80bd9067c0f40b57dd69fb8d0fcf3c3b6b207 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 5 Sep 2023 15:23:11 +0200 Subject: [PATCH 3/3] Update AddOpenCodeFixProvider.fs --- .../src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index eb3fc04a0f0..b6b13608cb1 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -59,7 +59,8 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr sourceText.Lines |> Seq.skip insertionLineNumber |> Seq.findIndex (fun line -> line.ToString().Contains "module") - |> (+) insertionLineNumber + // add back the skipped lines + |> fun i -> insertionLineNumber + i let moduleDeclLineText = sourceText.Lines[ moduleDeclLineNumber ].ToString().Trim()