From 4c6ed372ac779ffdd2ca2f9c726eea94ec6cfc98 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Mon, 6 Jan 2025 08:52:39 -0500 Subject: [PATCH] Wrap arg in parens when needed when adding `new` keyword (#18179) --- docs/release-notes/.VisualStudio/17.13.md | 1 + ...eywordToDisposableConstructorInvocation.fs | 103 ++++++++++++++-- ...dToDisposableConstructorInvocationTests.fs | 113 ++++++++++++++++++ 3 files changed, 209 insertions(+), 8 deletions(-) diff --git a/docs/release-notes/.VisualStudio/17.13.md b/docs/release-notes/.VisualStudio/17.13.md index dae07600e1b..3d773a92b2f 100644 --- a/docs/release-notes/.VisualStudio/17.13.md +++ b/docs/release-notes/.VisualStudio/17.13.md @@ -1,4 +1,5 @@ ### Fixed +* Wrap arg in parens when needed when adding `new` keyword. ([PR #18179](https://github.com/dotnet/fsharp/pull/18179)) ### Added * Code fix for adding missing `seq`. ([PR #17772](https://github.com/dotnet/fsharp/pull/17772)) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fs index 188645d2d62..af0309b1810 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fs @@ -5,6 +5,9 @@ namespace Microsoft.VisualStudio.FSharp.Editor open System.Composition open System.Collections.Immutable +open FSharp.Compiler.Syntax +open FSharp.Compiler.Text + open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.CodeFixes @@ -24,11 +27,95 @@ type internal AddNewKeywordCodeFixProvider() = interface IFSharpCodeFixProvider with member _.GetCodeFixIfAppliesAsync context = - CancellableTask.singleton ( - ValueSome - { - Name = CodeFix.AddNewKeyword - Message = title - Changes = [ TextChange(TextSpan(context.Span.Start, 0), "new ") ] - } - ) + cancellableTask { + let! sourceText = context.GetSourceTextAsync() + let! parseFileResults = context.Document.GetFSharpParseResultsAsync(nameof AddNewKeywordCodeFixProvider) + + let getSourceLineStr line = + sourceText.Lines[Line.toZ line].ToString() + + let range = + RoslynHelpers.TextSpanToFSharpRange(context.Document.FilePath, context.Span, sourceText) + + // Constructor arg + // Qualified.Constructor arg + // Constructor arg + // Qualified.Constructor arg + let matchingApp path node = + let (|TargetTy|_|) expr = + match expr with + | SynExpr.Ident id -> Some(SynType.LongIdent(SynLongIdent([ id ], [], []))) + | SynExpr.LongIdent(longDotId = longDotId) -> Some(SynType.LongIdent longDotId) + | SynExpr.TypeApp(SynExpr.Ident id, lessRange, typeArgs, commaRanges, greaterRange, _, range) -> + Some( + SynType.App( + SynType.LongIdent(SynLongIdent([ id ], [], [])), + Some lessRange, + typeArgs, + commaRanges, + greaterRange, + false, + range + ) + ) + | SynExpr.TypeApp(SynExpr.LongIdent(longDotId = longDotId), lessRange, typeArgs, commaRanges, greaterRange, _, range) -> + Some( + SynType.App(SynType.LongIdent longDotId, Some lessRange, typeArgs, commaRanges, greaterRange, false, range) + ) + | _ -> None + + match node with + | SyntaxNode.SynExpr(SynExpr.App(funcExpr = TargetTy targetTy; argExpr = argExpr; range = m)) when + m |> Range.equals range + -> + Some(targetTy, argExpr, path) + | _ -> None + + match (range.Start, parseFileResults.ParseTree) ||> ParsedInput.tryPick matchingApp with + | None -> return ValueNone + | Some(targetTy, argExpr, path) -> + // Adding `new` may require additional parentheses: https://github.com/dotnet/fsharp/issues/15622 + let needsParens = + let newExpr = SynExpr.New(false, targetTy, argExpr, range) + + argExpr + |> SynExpr.shouldBeParenthesizedInContext getSourceLineStr (SyntaxNode.SynExpr newExpr :: path) + + let newText = + let targetTyText = + sourceText.ToString(RoslynHelpers.FSharpRangeToTextSpan(sourceText, targetTy.Range)) + + // Constructor namedArg → new Constructor(namedArg) + // Constructor "literal" → new Constructor "literal" + // Constructor () → new Constructor () + // Constructor() → new Constructor() + // Constructor → new Constructor + // ····indentedArg ····(indentedArg) + let textBetween = + let range = + Range.mkRange context.Document.FilePath targetTy.Range.End argExpr.Range.Start + + if needsParens && range.StartLine = range.EndLine then + "" + else + sourceText.ToString(RoslynHelpers.FSharpRangeToTextSpan(sourceText, range)) + + let argExprText = + let originalArgText = + sourceText.ToString(RoslynHelpers.FSharpRangeToTextSpan(sourceText, argExpr.Range)) + + if needsParens then + $"(%s{originalArgText})" + else + originalArgText + + $"new %s{targetTyText}%s{textBetween}%s{argExprText}" + + return + ValueSome + { + Name = CodeFix.AddNewKeyword + Message = title + Changes = [ TextChange(context.Span, newText) ] + } + } diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddNewKeywordToDisposableConstructorInvocationTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddNewKeywordToDisposableConstructorInvocationTests.fs index 2709d139387..0fee7c9527b 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddNewKeywordToDisposableConstructorInvocationTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddNewKeywordToDisposableConstructorInvocationTests.fs @@ -29,3 +29,116 @@ let sr = new System.IO.StreamReader "test.txt" let actual = codeFix |> tryFix code Auto Assert.Equal(expected, actual) + +[] +let ``Fixes FS0760 — type app`` () = + let code = + """ +let _ = System.Threading.Tasks.Task(fun _ -> 3) +""" + + let expected = + Some + { + Message = "Add 'new' keyword" + FixedCode = + """ +let _ = new System.Threading.Tasks.Task(fun _ -> 3) +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0760 — keeps space`` () = + let code = + """ +let stream = System.IO.MemoryStream () +""" + + let expected = + Some + { + Message = "Add 'new' keyword" + FixedCode = + """ +let stream = new System.IO.MemoryStream () +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0760 — does not add space`` () = + let code = + """ +let stream = System.IO.MemoryStream() +""" + + let expected = + Some + { + Message = "Add 'new' keyword" + FixedCode = + """ +let stream = new System.IO.MemoryStream() +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0760 — adds parentheses when needed`` () = + let code = + """ +let path = "test.txt" +let sr = System.IO.StreamReader path +""" + + let expected = + Some + { + Message = "Add 'new' keyword" + FixedCode = + """ +let path = "test.txt" +let sr = new System.IO.StreamReader(path) +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0760 — adds parentheses when needed and keeps indentation`` () = + let code = + """ +let path = "test.txt" +let sr = + System.IO.StreamReader + path +""" + + let expected = + Some + { + Message = "Add 'new' keyword" + FixedCode = + """ +let path = "test.txt" +let sr = + new System.IO.StreamReader + (path) +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual)