From 38840f3929c51b3d21420376a2b91fc6b7658066 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Sun, 3 Dec 2023 12:54:34 -0500 Subject: [PATCH 1/3] Keep parens for non-ident. op pairs with same prec * Keep parens in cases like `f <| (g << h)`, `x + (y ++ z)`, etc. --- src/Compiler/Service/ServiceAnalysis.fs | 288 +++++++----------- .../RemoveUnnecessaryParenthesesTests.fs | 21 +- 2 files changed, 127 insertions(+), 182 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 09dc977ec81..2c7b148afee 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -458,14 +458,67 @@ module UnnecessaryParentheses = let (|Ident|) (ident: Ident) = ident.idText - /// Represents an expression's precedence, or, - /// for a few few types of expression whose exact - /// kind can be significant, the expression's exact kind. + /// Represents a symbolic infix operator with the precedence of *, /, or %. + /// All instances of this type are considered equal. + [] + type MulDivMod = + | Mul + | Div + | Mod + + member _.CompareTo(_other: MulDivMod) = 0 + override this.Equals obj = this.CompareTo(unbox obj) = 0 + override _.GetHashCode() = 0 + + interface IComparable with + member this.CompareTo obj = this.CompareTo(unbox obj) + + /// Represents a symbolic infix operator with the precedence of + or -. + /// All instances of this type are considered equal. + [] + type AddSub = + | Add + | Sub + + member _.CompareTo(_other: AddSub) = 0 + override this.Equals obj = this.CompareTo(unbox obj) = 0 + override _.GetHashCode() = 0 + + interface IComparable with + member this.CompareTo obj = this.CompareTo(unbox obj) + + /// Holds a symbolic operator's original notation. + /// Equality is based on the contents of the string. + /// Comparison always returns 0. + [] + type OriginalNotation = + | OriginalNotation of string + + member _.CompareTo(_other: OriginalNotation) = 0 + + override this.Equals obj = + match this, obj with + | OriginalNotation this, (:? OriginalNotation as OriginalNotation other) -> String.Equals(this, other, StringComparison.Ordinal) + | _ -> false + + override this.GetHashCode() = + match this with + | OriginalNotation notation -> notation.GetHashCode() + + interface IComparable with + member this.CompareTo obj = this.CompareTo(unbox obj) + + /// Represents an expression's precedence. + /// Comparison is based only on the precedence case. + /// Equality considers the embedded original notation, if any. + /// + /// For example: + /// + /// compare (AddSub (Add, OriginalNotation "+")) (AddSub (Add, OriginalNotation "++")) = 0 /// - /// Use Precedence.sameKind to determine whether two expressions - /// have the same kind. Use Precedence.compare to compare two - /// expressions' precedence. Avoid using relational operators or the - /// built-in compare function on this type. + /// but + /// + /// AddSub (Add, OriginalNotation "+") <> AddSub (Add, OriginalNotation "++") type Precedence = /// yield, yield!, return, return! | Low @@ -483,46 +536,22 @@ module UnnecessaryParentheses = /// /// Refers to the exact operators or and ||. /// Instances with leading dots or question marks or trailing characters are parsed as Bar instead. - | BarBar + | BarBar of OriginalNotation /// &, && /// /// Refers to the exact operators & and &&. /// Instances with leading dots or question marks or trailing characters are parsed as Amp instead. - | AmpAmp - - /// :?> - | Downcast - - /// :> - | Upcast + | AmpAmp of OriginalNotation - /// =… - | Eq + /// :>, :?> + | UpcastDowncast - /// |… - | Bar + /// =…, |…, &…, $…, >…, <…, !=…, + | Relational of OriginalNotation - /// &… - | Amp - - /// $… - | Dollar - - /// >… - | Greater - - /// <… - | Less - - /// !=… - | BangEq - - /// ^… - | Hat - - /// @… - | At + /// ^…, @… + | HatAt /// :: | Cons @@ -530,20 +559,11 @@ module UnnecessaryParentheses = /// :? | TypeTest - /// -… - | Sub - - /// +… - | Add - - /// %… - | Mod + /// +…, -… + | AddSub of AddSub * OriginalNotation - /// /… - | Div - - /// *… - | Mul + /// *…, /…, %… + | MulDivMod of MulDivMod * OriginalNotation /// **… | Exp @@ -560,85 +580,6 @@ module UnnecessaryParentheses = // x.y | Dot - module Precedence = - /// Returns true only if the two expressions are of the - /// exact same kind. E.g., Add = Add and Sub = Sub, - /// but Add <> Sub, even though their precedence compares equally. - let sameKind prec1 prec2 = prec1 = prec2 - - /// Compares two expressions' precedence. - let compare prec1 prec2 = - match prec1, prec2 with - | Dot, Dot -> 0 - | Dot, _ -> 1 - | _, Dot -> -1 - - | High, High -> 0 - | High, _ -> 1 - | _, High -> -1 - - | Apply, Apply -> 0 - | Apply, _ -> 1 - | _, Apply -> -1 - - | UnaryPrefix, UnaryPrefix -> 0 - | UnaryPrefix, _ -> 1 - | _, UnaryPrefix -> -1 - - | Exp, Exp -> 0 - | Exp, _ -> 1 - | _, Exp -> -1 - - | (Mod | Div | Mul), (Mod | Div | Mul) -> 0 - | (Mod | Div | Mul), _ -> 1 - | _, (Mod | Div | Mul) -> -1 - - | (Sub | Add), (Sub | Add) -> 0 - | (Sub | Add), _ -> 1 - | _, (Sub | Add) -> -1 - - | TypeTest, TypeTest -> 0 - | TypeTest, _ -> 1 - | _, TypeTest -> -1 - - | Cons, Cons -> 0 - | Cons, _ -> 1 - | _, Cons -> -1 - - | (Hat | At), (Hat | At) -> 0 - | (Hat | At), _ -> 1 - | _, (Hat | At) -> -1 - - | (Eq | Bar | Amp | Dollar | Greater | Less | BangEq), (Eq | Bar | Amp | Dollar | Greater | Less | BangEq) -> 0 - | (Eq | Bar | Amp | Dollar | Greater | Less | BangEq), _ -> 1 - | _, (Eq | Bar | Amp | Dollar | Greater | Less | BangEq) -> -1 - - | (Downcast | Upcast), (Downcast | Upcast) -> 0 - | (Downcast | Upcast), _ -> 1 - | _, (Downcast | Upcast) -> -1 - - | AmpAmp, AmpAmp -> 0 - | AmpAmp, _ -> 1 - | _, AmpAmp -> -1 - - | BarBar, BarBar -> 0 - | BarBar, _ -> 1 - | _, BarBar -> -1 - - | Comma, Comma -> 0 - | Comma, _ -> 1 - | _, Comma -> -1 - - | ColonEquals, ColonEquals -> 0 - | ColonEquals, _ -> 1 - | _, ColonEquals -> -1 - - | Set, Set -> 0 - | Set, _ -> 1 - | _, Set -> -1 - - | Low, Low -> 0 - /// Associativity/association. type Assoc = /// Non-associative or no association. @@ -657,26 +598,15 @@ module UnnecessaryParentheses = | Set -> Non | ColonEquals -> Right | Comma -> Non - | BarBar -> Left - | AmpAmp -> Left - | Upcast - | Downcast -> Right - | Eq - | Bar - | Amp - | Dollar - | Greater - | Less - | BangEq -> Left - | At - | Hat -> Right + | BarBar _ -> Left + | AmpAmp _ -> Left + | UpcastDowncast -> Right + | Relational _ -> Left + | HatAt -> Right | Cons -> Right | TypeTest -> Non - | Add - | Sub -> Left - | Mul - | Div - | Mod -> Left + | AddSub _ -> Left + | MulDivMod _ -> Left | Exp -> Right | UnaryPrefix -> Left | Apply -> Left @@ -779,26 +709,30 @@ module UnnecessaryParentheses = match trimmed[0], originalNotation with | _, ":=" -> ValueSome ColonEquals - | _, ("||" | "or") -> ValueSome BarBar - | _, ("&" | "&&") -> ValueSome AmpAmp - | '|', _ -> ValueSome Bar - | '&', _ -> ValueSome Amp - | '<', _ -> ValueSome Less - | '>', _ -> ValueSome Greater - | '=', _ -> ValueSome Eq - | '$', _ -> ValueSome Dollar - | '!', _ when trimmed.Length > 1 && trimmed[1] = '=' -> ValueSome BangEq - | '^', _ -> ValueSome Hat - | '@', _ -> ValueSome At + | _, ("||" | "or") -> ValueSome(BarBar(OriginalNotation originalNotation)) + | _, ("&" | "&&") -> ValueSome(AmpAmp(OriginalNotation originalNotation)) + | '|', _ + | '&', _ + | '<', _ + | '>', _ + | '=', _ + | '$', _ -> ValueSome(Relational(OriginalNotation originalNotation)) + | '!', _ when trimmed.Length > 1 && trimmed[1] = '=' -> ValueSome(Relational(OriginalNotation originalNotation)) + | '^', _ + | '@', _ -> ValueSome HatAt | _, "::" -> ValueSome Cons - | '+', _ -> ValueSome Add - | '-', _ -> ValueSome Sub - | '/', _ -> ValueSome Div - | '%', _ -> ValueSome Mod + | '+', _ -> ValueSome(AddSub(Add, OriginalNotation originalNotation)) + | '-', _ -> ValueSome(AddSub(Sub, OriginalNotation originalNotation)) + | '/', _ -> ValueSome(MulDivMod(Div, OriginalNotation originalNotation)) + | '%', _ -> ValueSome(MulDivMod(Mod, OriginalNotation originalNotation)) | '*', _ when trimmed.Length > 1 && trimmed[1] = '*' -> ValueSome Exp - | '*', _ -> ValueSome Mul + | '*', _ -> ValueSome(MulDivMod(Mul, OriginalNotation originalNotation)) | _ -> ValueNone + [] + let (|Contains|_|) (c: char) (s: string) = + if s.IndexOf c >= 0 then ValueSome Contains else ValueNone + /// Any expressions in which the removal of parens would /// lead to something like the following that would be /// confused by the parser with a type parameter application: @@ -811,9 +745,9 @@ module UnnecessaryParentheses = match synExpr with | SynExpr.Paren(expr = ConfusableWithTypeApp) | SynExpr.App(funcExpr = ConfusableWithTypeApp) - | SynExpr.App(isInfix = true; funcExpr = FuncExpr.SymbolicOperator(SymbolPrec Greater); argExpr = ConfusableWithTypeApp) -> + | SynExpr.App(isInfix = true; funcExpr = FuncExpr.SymbolicOperator(Contains '>'); argExpr = ConfusableWithTypeApp) -> ValueSome ConfusableWithTypeApp - | SynExpr.App(isInfix = true; funcExpr = funcExpr & FuncExpr.SymbolicOperator(SymbolPrec Less); argExpr = argExpr) when + | SynExpr.App(isInfix = true; funcExpr = funcExpr & FuncExpr.SymbolicOperator(Contains '<'); argExpr = argExpr) when argExpr.Range.IsAdjacentTo funcExpr.Range -> ValueSome ConfusableWithTypeApp @@ -839,8 +773,8 @@ module UnnecessaryParentheses = | SynExpr.App(funcExpr = SynExpr.App(isInfix = true; funcExpr = FuncExpr.SymbolicOperator(SymbolPrec prec))) -> ValueSome(prec, Right) | SynExpr.App(isInfix = true; funcExpr = FuncExpr.SymbolicOperator(SymbolPrec prec)) -> ValueSome(prec, Left) - | SynExpr.Upcast _ -> ValueSome(Upcast, Left) - | SynExpr.Downcast _ -> ValueSome(Downcast, Left) + | SynExpr.Upcast _ + | SynExpr.Downcast _ -> ValueSome(UpcastDowncast, Left) | SynExpr.TypeTest _ -> ValueSome(TypeTest, Left) | _ -> ValueNone @@ -1222,11 +1156,11 @@ module UnnecessaryParentheses = // // o.M((x = y)) // o.N((x = y), z) - | SynExpr.Paren(expr = SynExpr.Paren(expr = InfixApp(Eq, _))), + | SynExpr.Paren(expr = SynExpr.Paren(expr = InfixApp(Relational(OriginalNotation "="), _))), SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _ - | SynExpr.Paren(expr = InfixApp(Eq, _)), + | SynExpr.Paren(expr = InfixApp(Relational(OriginalNotation "="), _)), SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App(funcExpr = SynExpr.LongIdent _)) :: _ - | SynExpr.Paren(expr = InfixApp(Eq, _)), + | SynExpr.Paren(expr = InfixApp(Relational(OriginalNotation "="), _)), SyntaxNode.SynExpr(SynExpr.Tuple(isStruct = false)) :: SyntaxNode.SynExpr(SynExpr.Paren _) :: SyntaxNode.SynExpr(SynExpr.App( funcExpr = SynExpr.LongIdent _)) :: _ -> ValueNone @@ -1342,7 +1276,7 @@ module UnnecessaryParentheses = | OuterBinaryExpr inner (outerPrecedence, side), InnerBinaryExpr innerPrecedence -> let ambiguous = - match Precedence.compare outerPrecedence innerPrecedence with + match compare outerPrecedence innerPrecedence with | 0 -> match side, Assoc.ofPrecedence innerPrecedence with | Non, _ @@ -1351,11 +1285,11 @@ module UnnecessaryParentheses = | Right, Right | Left, Left -> false | Right, Left -> - not (Precedence.sameKind outerPrecedence innerPrecedence) + outerPrecedence <> innerPrecedence || match innerPrecedence with - | Div - | Mod - | Sub -> true + | MulDivMod(Div, _) + | MulDivMod(Mod, _) + | AddSub(Sub, _) -> true | _ -> false | c -> c > 0 diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 27bce12bb2d..5bea96c12bd 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -1338,6 +1338,12 @@ let _ = (2 + 2) { return 5 } /// (x λ y) ρ z | OuterRight of l: string * r: string + /// Indicates whether both operators are the same exact symbolic operator. + member this.Identical = + match this with + | OuterLeft(l, r) + | OuterRight(l, r) -> l = r + override this.ToString() = match this with | OuterLeft(l, r) -> $"x {l} (y {r} z)" @@ -1386,11 +1392,11 @@ let _ = (2 + 2) { return 5 } | OuterLeft((":?" | ":>" | ":?>"), _) -> invalidPairing | OuterLeft(_, "**op") -> fixable pair | OuterLeft("**op", _) -> unfixable pair - | OuterLeft("*op", "*op") -> fixable pair + | OuterLeft("*op", "*op") -> if pair.Identical then fixable pair else unfixable pair | OuterLeft(("%op" | "/op" | "*op"), ("%op" | "/op" | "*op")) -> unfixable pair | OuterLeft(_, ("%op" | "/op" | "*op")) -> fixable pair | OuterLeft(("%op" | "/op" | "*op"), _) -> unfixable pair - | OuterLeft("+op", "+op") -> fixable pair + | OuterLeft("+op", "+op") -> if pair.Identical then fixable pair else unfixable pair | OuterLeft(("-op" | "+op"), ("-op" | "+op")) -> unfixable pair | OuterLeft(_, ("-op" | "+op")) -> fixable pair | OuterLeft(("-op" | "+op"), _) -> unfixable pair @@ -1399,14 +1405,16 @@ let _ = (2 + 2) { return 5 } | OuterLeft("::", _) -> unfixable pair | OuterLeft(_, ("^op" | "@op")) -> fixable pair | OuterLeft(("^op" | "@op"), _) -> unfixable pair - | OuterLeft(l & ("=op" | "|op" | "&op" | "$" | ">op" | "op" | " - if l = r then fixable pair else unfixable pair + | OuterLeft(("=op" | "|op" | "&op" | "$" | ">op" | "op" | " + if pair.Identical then fixable pair else unfixable pair | OuterLeft(_, ("=op" | "|op" | "&op" | "$" | ">op" | " fixable pair | OuterLeft(("=op" | "|op" | "&op" | "$" | ">op" | " unfixable pair | OuterLeft(_, (":>" | ":?>")) -> fixable pair + | OuterLeft(("&" | "&&"), ("&" | "&&")) -> if pair.Identical then fixable pair else unfixable pair | OuterLeft(_, ("&" | "&&")) -> fixable pair | OuterLeft(("&" | "&&"), _) -> unfixable pair + | OuterLeft(("||" | "or"), ("||" | "or")) -> if pair.Identical then fixable pair else unfixable pair | OuterLeft(_, ("||" | "or")) -> fixable pair | OuterLeft(("||" | "or"), _) -> unfixable pair | OuterLeft(":=", ":=") -> fixable pair @@ -1439,11 +1447,14 @@ let _ = (2 + 2) { return 5 } let operators = [ "**" + "***" "*" + "*." "/" "%" "-" "+" + "++" ":?" "::" "^^^" From 84ead68a59e76b2f1cd61ef0cdb0e26a0e3e2399 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Sun, 3 Dec 2023 16:10:36 -0500 Subject: [PATCH 2/3] Keep parens around "relational" ops even when same * The order of operations in `x > (y < z)` or `x <> (y <> z)` is not necessarily immediately obvious, and there are built-in operators in this class like `<|` for which the associativity of the underlying operation depends on the types of the operands. --- src/Compiler/Service/ServiceAnalysis.fs | 11 ++++++----- .../CodeFixes/RemoveUnnecessaryParenthesesTests.fs | 9 ++++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 2c7b148afee..074058a7dcc 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -547,7 +547,7 @@ module UnnecessaryParentheses = /// :>, :?> | UpcastDowncast - /// =…, |…, &…, $…, >…, <…, !=…, + /// =…, |…, &…, $…, >…, <…, !=… | Relational of OriginalNotation /// ^…, @… @@ -1286,10 +1286,11 @@ module UnnecessaryParentheses = | Left, Left -> false | Right, Left -> outerPrecedence <> innerPrecedence - || match innerPrecedence with - | MulDivMod(Div, _) - | MulDivMod(Mod, _) - | AddSub(Sub, _) -> true + || match outerPrecedence, innerPrecedence with + | _, MulDivMod(Div, _) + | _, MulDivMod(Mod, _) + | _, AddSub(Sub, _) -> true + | Relational _, Relational _ -> true | _ -> false | c -> c > 0 diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 5bea96c12bd..0da638ec19f 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -993,7 +993,7 @@ in x |> id " - "x |> (id |> fun x -> x)", "x |> id |> fun x -> x" + "x |> (id |> fun x -> x)", "x |> (id |> fun x -> x)" "x |> (id <| fun x -> x)", "x |> (id <| fun x -> x)" "id <| (fun x -> x)", "id <| fun x -> x" "id <| (fun x -> x) |> id", "id <| (fun x -> x) |> id" @@ -1035,6 +1035,10 @@ in x "id (-(-x))", "id -(-x)" "id -(-x)", "id -(-x)" + "f <| (g << h)", "f <| (g << h)" + "x <> (y <> z)", "x <> (y <> z)" + "x > (y > z)", "x > (y > z)" + " let f x y = 0 f ((+) x y) z @@ -1406,8 +1410,7 @@ let _ = (2 + 2) { return 5 } | OuterLeft(_, ("^op" | "@op")) -> fixable pair | OuterLeft(("^op" | "@op"), _) -> unfixable pair | OuterLeft(("=op" | "|op" | "&op" | "$" | ">op" | "op" | " - if pair.Identical then fixable pair else unfixable pair + ("=op" | "|op" | "&op" | "$" | ">op" | " unfixable pair | OuterLeft(_, ("=op" | "|op" | "&op" | "$" | ">op" | " fixable pair | OuterLeft(("=op" | "|op" | "&op" | "$" | ">op" | " unfixable pair | OuterLeft(_, (":>" | ":?>")) -> fixable pair From 89cb11d6a9df57aeddef22eeaaf68b44e9b810dc Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Sun, 3 Dec 2023 16:15:44 -0500 Subject: [PATCH 3/3] Add entry to FCS release notes --- docs/release-notes/FSharp.Compiler.Service/8.0.200.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md index c0d199a7864..29d414ff6d2 100644 --- a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md +++ b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md @@ -1,2 +1,3 @@ - Miscellaneous fixes to parens analysis - https://github.com/dotnet/fsharp/pull/16262 -- Fixes #16359 - correctly handle imports with 0 length public key tokens - https://github.com/dotnet/fsharp/pull/16363 \ No newline at end of file +- Parens analysis: keep parens for non-identical infix operator pairs with same precedence - https://github.com/dotnet/fsharp/pull/16372 +- Fixes #16359 - correctly handle imports with 0 length public key tokens - https://github.com/dotnet/fsharp/pull/16363