Wrap arg in parens when needed when adding new keyword (#1335)

* Add parens when needed when adding `new` keyword

* Update comment

Author: brianrourkeboll

src/FsAutoComplete/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fs

@@ -6,19 +6,114 @@ open FsAutoComplete.CodeFix.Types
 open Ionide.LanguageServerProtocol.Types
 open FsAutoComplete
 open FsAutoComplete.LspHelpers
+open FSharp.Compiler.Syntax
+open FSharp.Compiler.Text
 
 let title = "Add 'new'"
 
 /// a codefix that suggests using the 'new' keyword on IDisposables
-let fix =
+let fix (getParseResultsForFile: GetParseResultsForFile) =
   Run.ifDiagnosticByCode (Set.ofList [ "760" ]) (fun diagnostic codeActionParams ->
-    AsyncResult.retn
-      [ { SourceDiagnostic = Some diagnostic
-          File = codeActionParams.TextDocument
-          Title = title
-          Edits =
-            [| { Range =
-                   { Start = diagnostic.Range.Start
-                     End = diagnostic.Range.Start }
-                 NewText = $"new " } |]
-          Kind = FixKind.Refactor } ])
+    asyncResult {
+      let fileName = codeActionParams.TextDocument.GetFilePath() |> normalizePath
+      let fcsRange = protocolRangeToRange (string fileName) diagnostic.Range
+      let fcsPos = protocolPosToPos diagnostic.Range.Start
+      let! parseResults, _, sourceText = getParseResultsForFile fileName fcsPos
+
+      // Constructor arg
+      // Qualified.Constructor arg
+      // Constructor<TypeArg> arg
+      // Qualified.Constructor<TypeArg> 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 fcsRange
+          ->
+          Some(targetTy, argExpr, path)
+        | _ -> None
+
+      return
+        (fcsRange.Start, parseResults.GetAST)
+        ||> ParsedInput.tryPick matchingApp
+        |> Option.toList
+        |> List.map (fun (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, fcsRange)
+
+            argExpr
+            |> SynExpr.shouldBeParenthesizedInContext sourceText.GetLineString (SyntaxNode.SynExpr newExpr :: path)
+
+          let newText =
+            let targetTyText = sourceText.GetSubTextFromRange 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 (string fileName) targetTy.Range.End argExpr.Range.Start
+
+              if needsParens && range.StartLine = range.EndLine then
+                ""
+              else
+                sourceText.GetSubTextFromRange range
+
+            let argExprText =
+              let originalArgText = sourceText.GetSubTextFromRange argExpr.Range
+
+              if needsParens then
+                $"(%s{originalArgText})"
+              else
+                originalArgText
+
+            $"new %s{targetTyText}%s{textBetween}%s{argExprText}"
+
+          { SourceDiagnostic = Some diagnostic
+            File = codeActionParams.TextDocument
+            Title = title
+            Edits =
+              [| { Range =
+                     { Start = diagnostic.Range.Start
+                       End = diagnostic.Range.End }
+                   NewText = newText } |]
+            Kind = FixKind.Refactor })
+    })

src/FsAutoComplete/CodeFixes/AddNewKeywordToDisposableConstructorInvocation.fsi

@@ -4,5 +4,6 @@ open FsAutoComplete.CodeFix.Types
 open Ionide.LanguageServerProtocol.Types
 
 val title: string
+
 /// a codefix that suggests using the 'new' keyword on IDisposables
-val fix: (CodeActionParams -> Async<Result<Fix list, string>>)
+val fix: getParseResultsForFile: GetParseResultsForFile -> (CodeActionParams -> Async<Result<Fix list, string>>)

src/FsAutoComplete/LspServers/AdaptiveServerState.fs

@@ -2130,7 +2130,7 @@ type AdaptiveState
          Run.ifEnabled
            (fun _ -> config.UnusedDeclarationsAnalyzer)
            (RenameUnusedValue.fix tryGetParseAndCheckResultsForFile)
-         AddNewKeywordToDisposableConstructorInvocation.fix
+         AddNewKeywordToDisposableConstructorInvocation.fix tryGetParseAndCheckResultsForFile
          Run.ifEnabled
            (fun _ -> config.UnionCaseStubGeneration)
            (GenerateUnionCases.fix

test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs

@@ -122,7 +122,63 @@ let private addNewKeywordToDisposableConstructorInvocationTests state =
         let _ = System.$0String('.', 3)
         """
         Diagnostics.acceptAll
-        selectCodeFix ])
+        selectCodeFix
+
+      testCaseAsync "adds parentheses when needed"
+      <| CodeFix.check
+        server
+        """
+        let path = "test.txt"
+        let _ = System.IO.StreamReader path$0
+        """
+        (Diagnostics.expectCode "760")
+        selectCodeFix
+        """
+        let path = "test.txt"
+        let _ = new System.IO.StreamReader(path)
+        """
+
+      testCaseAsync "keeps space"
+      <| CodeFix.check
+        server
+        """
+        let stream = System.IO.MemoryStream ()$0
+        """
+        (Diagnostics.expectCode "760")
+        selectCodeFix
+        """
+        let stream = new System.IO.MemoryStream ()
+        """
+
+      testCaseAsync "does not add space"
+      <| CodeFix.check
+        server
+        """
+        let stream = System.IO.MemoryStream()$0
+        """
+        (Diagnostics.expectCode "760")
+        selectCodeFix
+        """
+        let stream = new System.IO.MemoryStream()
+        """
+
+      testCaseAsync "adds parentheses when needed and keeps indentation"
+      <| CodeFix.check
+        server
+        """
+        let path = "test.txt"
+        let sr =
+            System.IO.StreamReader
+                path$0
+        """
+        (Diagnostics.expectCode "760")
+        selectCodeFix
+        """
+        let path = "test.txt"
+        let sr =
+            new System.IO.StreamReader
+                (path)
+        """ ])
 
 let private addTypeToIndeterminateValueTests state =
   serverTestList (nameof AddTypeToIndeterminateValue) state defaultConfigDto None (fun server ->
@@ -3550,8 +3606,8 @@ let private removeUnnecessaryParenthesesTests state =
       ])
 
 let tests textFactory state =
-  testSequenced <|
-  testList
+  testSequenced
+  <| testList
     "CodeFix-tests"
     [ HelpersTests.tests textFactory
       AddExplicitTypeAnnotationTests.tests state