Skip to content

Commit

Permalink
Parens: undentation in sequentials (#16977)
Browse files Browse the repository at this point in the history
* Handle some undentation oddities in sequentials

* Keep parens around nested sequentials

* This is technically only necessary if the outer sequential is itself
  nested inside of a list/array/sequence/computation expression where
  each node of the "sequential" expression is actually an implicit
  yield, but there is currently no easy, inexpensive way of knowing
  that.

* Update release notes

* Move release notes

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>
  • Loading branch information
brianrourkeboll and psfinaki authored Apr 15, 2024
1 parent e1343ce commit b4bf40c
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 0 deletions.
3 changes: 3 additions & 0 deletions docs/release-notes/.FSharp.Compiler.Service/8.0.400.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
### Fixed

Various parenthesization API fixes. ([PR #16977](https://github.com/dotnet/fsharp/pull/16977))
67 changes: 67 additions & 0 deletions src/Compiler/Service/SynExpr.fs
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,21 @@ module SynExpr =

loop ValueNone startLine range.StartColumn

/// Matches constructs that are sensitive to
/// certain kinds of undentation in sequential expressions.
[<return: Struct>]
let (|UndentationSensitive|_|) expr =
match expr with
| SynExpr.TryWith _
| SynExpr.TryFinally _
| SynExpr.For _
| SynExpr.ForEach _
| SynExpr.IfThenElse _
| SynExpr.Match _
| SynExpr.While _
| SynExpr.Do _ -> ValueSome UndentationSensitive
| _ -> ValueNone

let rec shouldBeParenthesizedInContext (getSourceLineStr: int -> string) path expr : bool =
let shouldBeParenthesizedInContext = shouldBeParenthesizedInContext getSourceLineStr
let containsSensitiveIndentation = containsSensitiveIndentation getSourceLineStr
Expand Down Expand Up @@ -732,6 +747,56 @@ module SynExpr =
->
true

// There are certain constructs whose indentation in
// a sequential expression is valid when parenthesized
// (and separated from the following expression by a semicolon),
// but where the parsing of the outer expression would change if
// the parentheses were removed in place, e.g.,
//
// [
// (if p then q else r); // Cannot remove in place because of the indentation of y below.
// y
// ]
//
// or
//
// [
// x;
// (if p then q else r); // Cannot remove in place because of the indentation of x above.
// z
// ]
//
// This analysis is imperfect in that it sometimes requires parens when they
// may not be required, but the only way to know for sure in such cases would be to walk up
// an unknown number of ancestral sequential expressions to check for problematic
// indentation (or to keep track of the offsides line throughout the AST traversal):
//
// [
// x; // This line's indentation means we cannot remove below.
// (…);
// (…);
// (* 𝑛 more such lines. *)
// (…);
// (…);
// (if p then q else r); // Can no longer remove here because of the indentation of x above.
// z
// ]
| UndentationSensitive,
SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr))) :: SyntaxNode.SynExpr(SynExpr.Sequential(
expr1 = SynExpr.Paren(expr = other) | other)) :: _
| UndentationSensitive,
SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.Sequential _) :: _
| UndentationSensitive,
SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ArrayOrListComputed _) :: _
| UndentationSensitive,
SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ArrayOrList _) :: _
| UndentationSensitive,
SyntaxNode.SynExpr(SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is expr); expr2 = other)) :: SyntaxNode.SynExpr(SynExpr.ComputationExpr _) :: _ when
expr.Range.StartLine <> other.Range.StartLine
&& expr.Range.StartColumn <= other.Range.StartColumn
->
true

// Check for nested matches, e.g.,
//
// match … with … -> (…, match … with … -> … | … -> …) | … -> …
Expand Down Expand Up @@ -990,6 +1055,8 @@ module SynExpr =
->
true

| SynExpr.Sequential _, Dangling.Problematic(SynExpr.Sequential _) -> true

| SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), Dangling.Problematic _ when
problematic inner.Range expr2.Range
->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,127 @@ in x
""" printfn "1"; (printfn "2") """, """ printfn "1"; printfn "2" """
"let x = 3; (5) in x", "let x = 3; 5 in x"

"""
[
()
(printfn "1"; ())
()
]
""",
"""
[
()
(printfn "1"; ())
()
]
"""

// Technically we could remove some parens in some of these,
// but additional exprs above or below can suddenly move the offsides line,
// and we don't want to do unbounded lookahead or lookbehind.

"
let _ =
[
(if p then q else r);
y
]
",
"
let _ =
[
(if p then q else r);
y
]
"

"
let _ =
[
(if p then q else r);
y
]
",
"
let _ =
[
if p then q else r;
y
]
"

"
let _ =
[
x;
(if p then q else r);
(if foo then bar else baz);
]
",
"
let _ =
[
x;
(if p then q else r);
if foo then bar else baz;
]
"

"
let _ =
[
(if p then q else r);
y;
(if foo then bar else baz);
]
",
"
let _ =
[
(if p then q else r);
y;
if foo then bar else baz;
]
"

"
let _ =
[
(if p then q else r);
(if foo then bar else baz);
z
]
",
"
let _ =
[
if p then q else r;
(if foo then bar else baz);
z
]
"

"
let _ =
[
x;
(if a then b else c);
(if p then q else r);
(if foo then bar else baz);
z
]
",
"
let _ =
[
x;
(if a then b else c);
(if p then q else r);
(if foo then bar else baz);
z
]
"

"
let _ =
let y = 100
Expand Down

0 comments on commit b4bf40c

Please sign in to comment.