Skip to content

Parens: more fixes #16901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Mar 21, 2024
Merged

Parens: more fixes #16901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5f669f2
Fix a few more paren corner cases
brianrourkeboll Mar 18, 2024
9cd2ab3
Handle typed pats in getters & setters
brianrourkeboll Mar 18, 2024
c3ff13b
Double parens oddities
brianrourkeboll Mar 19, 2024
4574d47
Keep parens for parenzed app preceded by prefix op
brianrourkeboll Mar 19, 2024
e77087d
Keep parens around tuple in interp string
brianrourkeboll Mar 19, 2024
3f27dab
More nested match fun
brianrourkeboll Mar 19, 2024
21ce52d
No space when expr would reparse as prefix op
brianrourkeboll Mar 19, 2024
b1084f2
No space when expr would reparse as prefix op
brianrourkeboll Mar 19, 2024
fe84e1c
No space when expr would reparse as prefix op
brianrourkeboll Mar 19, 2024
cc3be4d
Merge branch 'main' of https://github.com/dotnet/fsharp into parens-a…
brianrourkeboll Mar 19, 2024
39e5a22
Update release notes
brianrourkeboll Mar 19, 2024
c16e33d
Remove unfinished multiline comment stuff
brianrourkeboll Mar 19, 2024
8f003e6
Merge branch 'main' of https://github.com/dotnet/fsharp into parens-a…
brianrourkeboll Mar 19, 2024
225d530
Merge branch 'main' of https://github.com/dotnet/fsharp into parens-a…
brianrourkeboll Mar 20, 2024
365ffd1
Keep parens around dot-get that would be adjacent
brianrourkeboll Mar 20, 2024
9b08270
Fantomas
brianrourkeboll Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,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), [PR #16743](https://github.com/dotnet/fsharp/pull/16743))
* 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))
* Various parenthesization API fixes. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16901](https://github.com/dotnet/fsharp/pull/16901))
* 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))
* Fix16572 - Fixed the preview feature enabling Is properties for union case did not work correctly with let .rec and .fsi files ([PR #16657](https://github.com/dotnet/fsharp/pull/16657))
Expand Down
2 changes: 1 addition & 1 deletion docs/release-notes/.VisualStudio/17.10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +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), [PR #16789](https://github.com/dotnet/fsharp/pull/16789))
* Various unneeded parentheses code fix improvements. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578), [PR #16666](https://github.com/dotnet/fsharp/pull/16666), [PR #16789](https://github.com/dotnet/fsharp/pull/16789), [PR #16901](https://github.com/dotnet/fsharp/pull/16901))

### Changed

Expand Down
77 changes: 66 additions & 11 deletions src/Compiler/Service/SynExpr.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ module SynExpr =
| InfixApp(prec, side) -> ValueSome(prec, side)
| SynExpr.App(argExpr = SynExpr.ComputationExpr _) -> ValueSome(UnaryPrefix, Left)
| SynExpr.App(funcExpr = SynExpr.Paren(expr = SynExpr.App _)) -> ValueSome(Apply, Left)
| SynExpr.App(flag = ExprAtomicFlag.Atomic) -> ValueSome(Dot, Non)
| SynExpr.App _ -> ValueSome(Apply, Non)
| SynExpr.DotSet(targetExpr = SynExpr.Paren(expr = Is inner)) -> ValueSome(Dot, Left)
| SynExpr.DotSet(rhsExpr = SynExpr.Paren(expr = Is inner)) -> ValueSome(Set, Right)
Expand Down Expand Up @@ -437,6 +438,14 @@ module SynExpr =
| SynExpr.IfThenElse _ as expr -> Some expr
| _ -> None)

/// Matches a dangling let or use construct.
[<return: Struct>]
let (|LetOrUse|_|) =
dangling (function
| SynExpr.LetOrUse _
| SynExpr.LetOrUseBang _ as expr -> Some expr
| _ -> None)

/// Matches a dangling sequential expression.
[<return: Struct>]
let (|Sequential|_|) =
Expand Down Expand Up @@ -610,13 +619,26 @@ module SynExpr =
//
// o.M((x = y))
// o.N((x = y), z)
//
// Likewise, double parens must stay around a tuple, since we don't know whether
// the method being invoked might have a signature like
//
// val TryGetValue : 'Key * outref<'Value> -> bool
//
// where 'Key is 'a * 'b, in which case the double parens are required.
| SynExpr.Paren(expr = InfixApp(Relational(OriginalNotation "="), _)),
SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _
SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
| InfixApp(Relational(OriginalNotation "="), _),
SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _
SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
| InfixApp(Relational(OriginalNotation "="), _),
SyntaxNode.SynExpr(SynExpr.Tuple(isStruct = false)) :: SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
funcExpr = SynExpr.LongIdent _)) :: _ -> true
funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
| SynExpr.Paren(expr = SynExpr.Tuple(isStruct = false)),
SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _
| SynExpr.Tuple(isStruct = false),
SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(
funcExpr = SynExpr.LongIdent _ | SynExpr.DotGet _ | SynExpr.Ident _)) :: _ -> true

// Already parenthesized.
| _, SyntaxNode.SynExpr(SynExpr.Paren _) :: _ -> false
Expand Down Expand Up @@ -676,6 +698,15 @@ module SynExpr =
SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(HighPrecedenceApp | SynExpr.Assert _ | SynExpr.InferredUpcast _ | SynExpr.InferredDowncast _) :: _ ->
true

// Parens must be kept in a scenario like
//
// !x.M(y)
// ~~~x.M(y)
//
// since prefix ! or ~~~ (with no space) have higher
// precedence than regular function application.
| _, SyntaxNode.SynExpr(SynExpr.App _) :: SyntaxNode.SynExpr(PrefixApp High) :: _ -> true

// Parens are never required around suffixed or infixed numeric literals, e.g.,
//
// (1l).ToString()
Expand Down Expand Up @@ -794,13 +825,17 @@ module SynExpr =
match outer, inner with
| ConfusableWithTypeApp, _ -> true

| SynExpr.IfThenElse _, Dangling.Sequential _ -> true
| SynExpr.IfThenElse(trivia = trivia), Dangling.LetOrUse letOrUse ->
Position.posLt letOrUse.Range.Start trivia.ThenKeyword.Start

| SynExpr.IfThenElse(trivia = trivia), Dangling.IfThen ifThenElse when
problematic ifThenElse.Range trivia.ThenKeyword
|| trivia.ElseKeyword |> Option.exists (problematic ifThenElse.Range)
->
true
| SynExpr.IfThenElse(trivia = trivia), Dangling.IfThen dangling
| SynExpr.IfThenElse(trivia = trivia), Dangling.Match dangling ->
problematic dangling.Range trivia.ThenKeyword
|| trivia.ElseKeyword |> Option.exists (problematic dangling.Range)

| SynExpr.IfThenElse(ifExpr = expr), Dangling.Sequential dangling
| SynExpr.While(whileExpr = expr), Dangling.Problematic dangling
| SynExpr.ForEach(enumExpr = expr), Dangling.Problematic dangling -> Range.rangeContainsRange expr.Range dangling.Range

| SynExpr.TryFinally(trivia = trivia), Dangling.Try tryExpr when problematic tryExpr.Range trivia.FinallyKeyword -> true

Expand All @@ -824,6 +859,25 @@ module SynExpr =
->
true

// A match-like construct could be problematically nested like this:
//
// match () with
// | _ when
// p &&
// let x = f ()
// (let y = z
// match x with
// | 3 | _ -> y) -> ()
// | _ -> ()
| _, Dangling.Match matchOrTry when
let line = getSourceLineStr matchOrTry.Range.EndLine
let endCol = matchOrTry.Range.EndColumn

line.Length > endCol + 1
&& line.AsSpan(endCol + 1).TrimStart(' ').StartsWith("->".AsSpan())
->
true

| SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), Dangling.Problematic _ when
problematic inner.Range expr2.Range
->
Expand All @@ -842,9 +896,10 @@ module SynExpr =
| SynExpr.Sequential(expr1 = SynExpr.Paren(expr = Is inner); expr2 = expr2), _ when innerBindingsWouldShadowOuter inner expr2 ->
true

| SynExpr.InterpolatedString _, SynExpr.Sequential _ -> true
| SynExpr.InterpolatedString _, SynExpr.Sequential _
| SynExpr.InterpolatedString _, SynExpr.Tuple(isStruct = false) -> true

| SynExpr.InterpolatedString(contents = contents), (SynExpr.Tuple(isStruct = false) | Dangling.Problematic _) ->
| SynExpr.InterpolatedString(contents = contents), Dangling.Problematic _ ->
contents
|> List.exists (function
| SynInterpolatedStringPart.FillExpr(qualifiers = Some _) -> true
Expand Down
72 changes: 65 additions & 7 deletions src/Compiler/Service/SynPat.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,24 @@ module SynPat =
| SynPat.Tuple(isStruct = false; elementPats = Last(Rightmost pat)) -> pat
| pat -> pat

/// Matches a nested as pattern.
[<return: Struct>]
let rec (|DanglingAs|_|) pat =
let (|AnyDanglingAs|_|) =
List.tryPick (function
| DanglingAs -> Some()
| _ -> None)

match pat with
| SynPat.As _ -> ValueSome()
| SynPat.Or(lhsPat = DanglingAs)
| SynPat.Or(rhsPat = DanglingAs)
| SynPat.ListCons(lhsPat = DanglingAs)
| SynPat.ListCons(rhsPat = DanglingAs)
| SynPat.Ands(pats = AnyDanglingAs)
| SynPat.Tuple(isStruct = false; elementPats = AnyDanglingAs) -> ValueSome()
| _ -> ValueNone

/// Matches if the given pattern is atomic.
[<return: Struct>]
let (|Atomic|_|) pat =
Expand Down Expand Up @@ -88,6 +106,7 @@ module SynPat =
// fun (x, y, …) -> …
// fun (x: …) -> …
// fun (Pattern …) -> …
// set (x: …, y: …) = …
| SynPat.Typed _, SyntaxNode.SynPat(Rightmost(SynPat.Paren(Is pat, _))) :: SyntaxNode.SynMatchClause _ :: _
| Rightmost(SynPat.Typed _), SyntaxNode.SynMatchClause _ :: _
| SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.LetOrUseBang _) :: _
Expand All @@ -98,23 +117,62 @@ module SynPat =
| SynPat.LongIdent(argPats = SynArgPats.Pats(_ :: _)), SyntaxNode.SynBinding _ :: _
| SynPat.LongIdent(argPats = SynArgPats.Pats(_ :: _)), SyntaxNode.SynExpr(SynExpr.Lambda _) :: _
| SynPat.Tuple(isStruct = false), SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _
| SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _ -> true
| SynPat.Typed _, SyntaxNode.SynExpr(SynExpr.Lambda(parsedData = Some _)) :: _
| SynPat.Typed _,
SyntaxNode.SynPat(SynPat.Tuple(isStruct = false)) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.GetSetMember _) :: _
| SynPat.Typed _,
SyntaxNode.SynPat(SynPat.Tuple(isStruct = false)) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding(SynBinding(
valData = SynValData(
memberFlags = Some {
MemberKind = SynMemberKind.PropertyGetSet | SynMemberKind.PropertyGet | SynMemberKind.PropertySet
}))) :: _ -> true

// () is parsed as this.
| SynPat.Const(SynConst.Unit, _), _ -> true

// (()) is required when overriding a generic member
// where unit is the generic type argument:
// where unit or a tuple type is the generic type argument:
//
// type C<'T> = abstract M : 'T -> unit
// let _ = { new C<unit> with override _.M (()) = () }
| SynPat.Paren(SynPat.Const(SynConst.Unit, _), _),
// let _ = { new C<int * int> with override _.M ((x, y)) = () }
| SynPat.Paren((SynPat.Const(SynConst.Unit, _) | SynPat.Tuple(isStruct = false)), _),
SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynExpr(SynExpr.ObjExpr(
objType = SynType.App(typeArgs = _ :: _) | SynType.LongIdentApp(typeArgs = _ :: _))) :: _
| SynPat.Paren(SynPat.Const(SynConst.Unit, _), _),
| SynPat.Tuple(isStruct = false),
SyntaxNode.SynPat(SynPat.Paren _) :: SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynExpr(SynExpr.ObjExpr(
objType = SynType.App(typeArgs = _ :: _) | SynType.LongIdentApp(typeArgs = _ :: _))) :: _
| SynPat.Paren((SynPat.Const(SynConst.Unit, _) | SynPat.Tuple(isStruct = false)), _),
SyntaxNode.SynPat(SynPat.LongIdent _) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(
typeRepr = SynTypeDefnRepr.ObjectModel(members = AnyGenericInheritOrInterfaceImpl))) :: _ -> true

// Not required:
//
// let (a,
// b,
// c) = …
//
// Required:
//
// let (a,
// b,
// c) =
// …
| SynPat.Tuple(isStruct = false; range = innerRange), SyntaxNode.SynBinding(SynBinding(expr = body)) :: _ ->
innerRange.StartLine <> innerRange.EndLine
&& innerRange.StartLine < body.Range.StartLine
&& body.Range.StartColumn <= innerRange.StartColumn

// The parens could be required by a signature file like this:
//
// type SemanticClassificationItem =
// new: (range * SemanticClassificationType) -> SemanticClassificationItem
| SynPat.Paren(SynPat.Tuple(isStruct = false), _),
SyntaxNode.SynPat(SynPat.LongIdent(longDotId = SynLongIdent(id = [ Ident "new" ]))) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn _ :: _
| SynPat.Tuple(isStruct = false),
SyntaxNode.SynPat(SynPat.Paren _) :: SyntaxNode.SynPat(SynPat.LongIdent(longDotId = SynLongIdent(id = [ Ident "new" ]))) :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn _ :: _ ->
true

// Parens are required around the atomic argument of
// any additional `new` constructor that is not the last.
//
Expand Down Expand Up @@ -204,9 +262,9 @@ module SynPat =
// A | (B as C)
// A & (B as C)
// A, (B as C)
| SynPat.Or _, SynPat.As _
| SynPat.Ands _, SynPat.As _
| SynPat.Tuple _, SynPat.As _
| SynPat.Or _, (SynPat.As _ | DanglingAs)
| SynPat.Ands _, (SynPat.As _ | DanglingAs)
| SynPat.Tuple _, (SynPat.As _ | DanglingAs)

// x, (y, z)
// x & (y, z)
Expand Down
Loading