Parens: ${(…; …)} & multiline expr handling (#16666)

* Parens needed for sequential exprs in interp strs

* Handle more multiline parenthesization scenarios

* Fmt

* Expose `SyntaxNodes` API for internal use

* Keep parens when removal would result in shadowing

* Update release notes

* Use `when`

* Use better range

* Consider spacing _after_ trimming

---------

Co-authored-by: Adam Boniecki <20281641+abonie@users.noreply.github.com>
Co-authored-by: Petr <psfinaki@users.noreply.github.com>

Author: 3 people

docs/release-notes/.FSharp.Compiler.Service/8.0.300.md

@@ -4,6 +4,7 @@
 * Code generated files with > 64K methods and generated symbols crash when loaded. Use infered sequence points for debugging. ([Issue #16399](https://github.com/dotnet/fsharp/issues/16399), [#PR 16514](https://github.com/dotnet/fsharp/pull/16514))
 * `nameof Module` expressions and patterns are processed to link files in `--test:GraphBasedChecking`. ([PR #16550](https://github.com/dotnet/fsharp/pull/16550))
 * Graph Based Checking doesn't throw on invalid parsed input so it can be used for IDE scenarios ([PR #16575](https://github.com/dotnet/fsharp/pull/16575), [PR #16588](https://github.com/dotnet/fsharp/pull/16588), [PR #16643](https://github.com/dotnet/fsharp/pull/16643))
+* Various parenthesization API fixes. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666))
 * Keep parens for problematic exprs (`if`, `match`, etc.) in `$"{(…):N0}"`, `$"{(…),-3}"`, etc. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578))
 * Fix crash in DOTNET_SYSTEM_GLOBALIZATION_INVARIANT mode ([PR #16471](https://github.com/dotnet/fsharp/pull/16471))
 * `[<CliEvent>]` member should not produce property symbol. ([Issue #16640](https://github.com/dotnet/fsharp/issues/16640), [PR #16658](https://github.com/dotnet/fsharp/pull/16658))

docs/release-notes/.VisualStudio/17.10.md

@@ -1,6 +1,7 @@
 ### Fixed
 
 * Show signature help mid-pipeline in more scenarios. ([PR #16462](https://github.com/dotnet/fsharp/pull/16462))
+* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666))
 
 ### Changed
 

src/Compiler/Service/ServiceParseTreeWalk.fs

@@ -1163,8 +1163,8 @@ module SyntaxNode =
 
 [<RequireQualifiedAccess>]
 [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
-module ParsedInput =
-    let fold folder state (parsedInput: ParsedInput) =
+module internal SyntaxNodes =
+    let fold folder state (ast: SyntaxNode list) =
         let mutable state = state
 
         let visitor =
@@ -1270,7 +1270,6 @@ module ParsedInput =
 
             loop diveResults
 
-        let ast = parsedInput.Contents
         let m = (range0, ast) ||> List.fold (fun acc node -> unionRanges acc node.Range)
         ignore<unit option> (SyntaxTraversal.traverseUntil pickAll m.End visitor ast)
         state
@@ -1454,7 +1453,7 @@ module ParsedInput =
         ignore<unit option> (SyntaxTraversal.traverseUntil pick pos visitor ast)
         state
 
-    let foldWhile folder state (parsedInput: ParsedInput) =
+    let foldWhile folder state (ast: SyntaxNode list) =
         let pickAll _ _ _ diveResults =
             let rec loop diveResults =
                 match diveResults with
@@ -1465,11 +1464,10 @@ module ParsedInput =
 
             loop diveResults
 
-        let ast = parsedInput.Contents
         let m = (range0, ast) ||> List.fold (fun acc node -> unionRanges acc node.Range)
         foldWhileImpl pickAll m.End folder state ast
 
-    let tryPick chooser position (parsedInput: ParsedInput) =
+    let tryPick chooser position (ast: SyntaxNode list) =
         let visitor =
             { new SyntaxVisitorBase<'T>() with
                 member _.VisitExpr(path, _, defaultTraverse, expr) =
@@ -1545,25 +1543,46 @@ module ParsedInput =
                     | _ -> None
             }
 
-        SyntaxTraversal.traverseUntil SyntaxTraversal.pick position visitor parsedInput.Contents
+        SyntaxTraversal.traverseUntil SyntaxTraversal.pick position visitor ast
 
-    let tryPickLast chooser position (parsedInput: ParsedInput) =
-        (None, parsedInput.Contents)
+    let tryPickLast chooser position (ast: SyntaxNode list) =
+        (None, ast)
         ||> foldWhileImpl SyntaxTraversal.pick position (fun prev path node ->
             match chooser path node with
             | Some _ as next -> Some next
             | None -> Some prev)
 
-    let tryNode position (parsedInput: ParsedInput) =
+    let tryNode position (ast: SyntaxNode list) =
         let Matching = Some
 
-        (None, parsedInput.Contents)
+        (None, ast)
         ||> foldWhileImpl SyntaxTraversal.pick position (fun _prev path node ->
             if rangeContainsPos node.Range position then
                 Some(Matching(node, path))
             else
                 None)
 
-    let exists predicate position parsedInput =
-        tryPick (fun path node -> if predicate path node then Some() else None) position parsedInput
+    let exists predicate position ast =
+        tryPick (fun path node -> if predicate path node then Some() else None) position ast
         |> Option.isSome
+
+[<RequireQualifiedAccess>]
+[<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
+module ParsedInput =
+    let fold folder state (parsedInput: ParsedInput) =
+        SyntaxNodes.fold folder state parsedInput.Contents
+
+    let foldWhile folder state (parsedInput: ParsedInput) =
+        SyntaxNodes.foldWhile folder state parsedInput.Contents
+
+    let tryPick chooser position (parsedInput: ParsedInput) =
+        SyntaxNodes.tryPick chooser position parsedInput.Contents
+
+    let tryPickLast chooser position (parsedInput: ParsedInput) =
+        SyntaxNodes.tryPickLast chooser position parsedInput.Contents
+
+    let tryNode position (parsedInput: ParsedInput) =
+        SyntaxNodes.tryNode position parsedInput.Contents
+
+    let exists predicate position (parsedInput: ParsedInput) =
+        SyntaxNodes.exists predicate position parsedInput.Contents

src/Compiler/Service/ServiceParseTreeWalk.fsi

@@ -219,6 +219,135 @@ module SyntaxNode =
     /// </summary>
     val (|Attributes|): node: SyntaxNode -> SynAttributes
 
+/// <summary>
+/// Holds operations for working with the untyped abstract syntax tree.
+/// </summary>
+[<RequireQualifiedAccess>]
+[<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
+module internal SyntaxNodes =
+    /// <summary>
+    /// Applies the given predicate to each node of the AST and its context (path)
+    /// down to a given position, returning true if a matching node is found, otherwise false.
+    /// Traversal is short-circuited if no matching node is found through the given position.
+    /// </summary>
+    /// <param name="predicate">The predicate to match each node against.</param>
+    /// <param name="position">The position in the input file down to which to apply the function.</param>
+    /// <param name="ast">The AST to search.</param>
+    /// <returns>True if a matching node is found, or false if no matching node is found.</returns>
+    /// <example>
+    /// <code lang="fsharp">
+    /// let isInTypeDefn =
+    ///     (pos, ast)
+    ///     ||> SyntaxNodes.exists (fun _path node ->
+    ///         match node with
+    ///         | SyntaxNode.SynTypeDefn _ -> true
+    ///         | _ -> false)
+    /// </code>
+    /// </example>
+    val exists: predicate: (SyntaxVisitorPath -> SyntaxNode -> bool) -> position: pos -> ast: SyntaxNode list -> bool
+
+    /// <summary>
+    /// Applies a function to each node of the AST and its context (path),
+    /// threading an accumulator through the computation.
+    /// </summary>
+    /// <param name="folder">The function to use to update the state given each node and its context.</param>
+    /// <param name="state">The initial state.</param>
+    /// <param name="ast">The AST to fold over.</param>
+    /// <returns>The final state.</returns>
+    /// <example>
+    /// <code lang="fsharp">
+    /// let unnecessaryParentheses =
+    ///    (HashSet Range.comparer, ast) ||> SyntaxNodes.fold (fun acc path node ->
+    ///        match node with
+    ///        | SyntaxNode.SynExpr (SynExpr.Paren (expr = inner; rightParenRange = Some _; range = range)) when
+    ///            not (SynExpr.shouldBeParenthesizedInContext getLineString path inner)
+    ///            ->
+    ///            ignore (acc.Add range)
+    ///            acc
+    ///
+    ///        | SyntaxNode.SynPat (SynPat.Paren (inner, range)) when
+    ///            not (SynPat.shouldBeParenthesizedInContext path inner)
+    ///            ->
+    ///            ignore (acc.Add range)
+    ///            acc
+    ///
+    ///        | _ -> acc)
+    /// </code>
+    /// </example>
+    val fold:
+        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State) -> state: 'State -> ast: SyntaxNode list -> 'State
+
+    /// <summary>
+    /// Applies a function to each node of the AST and its context (path)
+    /// until the folder returns <c>None</c>, threading an accumulator through the computation.
+    /// </summary>
+    /// <param name="folder">The function to use to update the state given each node and its context, or to stop traversal by returning <c>None</c>.</param>
+    /// <param name="state">The initial state.</param>
+    /// <param name="ast">The AST to fold over.</param>
+    /// <returns>The final state.</returns>
+    val foldWhile:
+        folder: ('State -> SyntaxVisitorPath -> SyntaxNode -> 'State option) ->
+        state: 'State ->
+        ast: SyntaxNode list ->
+            'State
+
+    /// <summary>
+    /// Dives to the deepest node that contains the given position,
+    /// returning the node and its path if found, or <c>None</c> if no
+    /// node contains the position.
+    /// </summary>
+    /// <param name="position">The position in the input file down to which to dive.</param>
+    /// <param name="ast">The AST to search.</param>
+    /// <returns>The deepest node containing the given position, along with the path taken through the node's ancestors to find it.</returns>
+    val tryNode: position: pos -> ast: SyntaxNode list -> (SyntaxNode * SyntaxVisitorPath) option
+
+    /// <summary>
+    /// Applies the given function to each node of the AST and its context (path)
+    /// down to a given position, returning <c>Some x</c> for the first node
+    /// for which the function returns <c>Some x</c> for some value <c>x</c>, otherwise <c>None</c>.
+    /// Traversal is short-circuited if no matching node is found through the given position.
+    /// </summary>
+    /// <param name="chooser">The function to apply to each node and its context to derive an optional value.</param>
+    /// <param name="position">The position in the input file down to which to apply the function.</param>
+    /// <param name="ast">The AST to search.</param>
+    /// <returns>The first value for which the function returns <c>Some</c>, or <c>None</c> if no matching node is found.</returns>
+    /// <example>
+    /// <code lang="fsharp">
+    /// let range =
+    ///     (pos, ast) ||> SyntaxNodes.tryPick (fun _path node ->
+    ///       match node with
+    ///       | SyntaxNode.SynExpr (SynExpr.InterpolatedString (range = range)) when
+    ///           rangeContainsPos range pos
+    ///           -> Some range
+    ///       | _ -> None)
+    /// </code>
+    /// </example>
+    val tryPick:
+        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) -> position: pos -> ast: SyntaxNode list -> 'T option
+
+    /// <summary>
+    /// Applies the given function to each node of the AST and its context (path)
+    /// down to a given position, returning <c>Some x</c> for the last (deepest) node
+    /// for which the function returns <c>Some x</c> for some value <c>x</c>, otherwise <c>None</c>.
+    /// Traversal is short-circuited if no matching node is found through the given position.
+    /// </summary>
+    /// <param name="chooser">The function to apply to each node and its context to derive an optional value.</param>
+    /// <param name="position">The position in the input file down to which to apply the function.</param>
+    /// <param name="ast">The AST to search.</param>
+    /// <returns>The last (deepest) value for which the function returns <c>Some</c>, or <c>None</c> if no matching node is found.</returns>
+    /// <example>
+    /// <code lang="fsharp">
+    /// let range =
+    ///     (pos, ast)
+    ///     ||> SyntaxNodes.tryPickLast (fun path node ->
+    ///         match node, path with
+    ///         | FuncIdent range -> Some range
+    ///         | _ -> None)
+    /// </code>
+    /// </example>
+    val tryPickLast:
+        chooser: (SyntaxVisitorPath -> SyntaxNode -> 'T option) -> position: pos -> ast: SyntaxNode list -> 'T option
+
 /// <summary>
 /// Holds operations for working with the
 /// untyped abstract syntax tree (<see cref="T:FSharp.Compiler.Syntax.ParsedInput"/>).

src/Compiler/Service/SynExpr.fs

@@ -774,6 +774,23 @@ module SynExpr =
 
                 loop clauses
 
+            let innerBindingsWouldShadowOuter expr1 (expr2: SynExpr) =
+                let identsBoundInInner =
+                    (Set.empty, [ SyntaxNode.SynExpr expr1 ])
+                    ||> SyntaxNodes.fold (fun idents _path node ->
+                        match node with
+                        | SyntaxNode.SynPat(SynPat.Named(ident = SynIdent(ident = ident))) -> idents.Add ident.idText
+                        | _ -> idents)
+
+                if identsBoundInInner.IsEmpty then
+                    false
+                else
+                    (expr2.Range.End, [ SyntaxNode.SynExpr expr2 ])
+                    ||> SyntaxNodes.exists (fun _path node ->
+                        match node with
+                        | SyntaxNode.SynExpr(SynExpr.Ident ident) -> identsBoundInInner.Contains ident.idText
+                        | _ -> false)
+
             match outer, inner with
             | ConfusableWithTypeApp, _ -> true
 
@@ -812,6 +829,21 @@ module SynExpr =
                 ->
                 true
 
+            // Keep parens if a name in the outer scope is rebound
+            // in the inner scope and would shadow the outer binding
+            // if the parens were removed, e.g.:
+            //
+            //     let x = 3
+            //     (
+            //         let x = 4
+            //         printfn $"{x}"
+            //     )
+            //     x
+            | SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), _ when innerBindingsWouldShadowOuter inner expr2 ->
+                true
+
+            | SynExpr.InterpolatedString _, SynExpr.Sequential _ -> true
+
             | SynExpr.InterpolatedString(contents = contents), (SynExpr.Tuple(isStruct = false) | Dangling.Problematic _) ->
                 contents
                 |> List.exists (function