Skip to content

Commit

Permalink
Parens: Keep parens for non-identical infix operator pairs with same …
Browse files Browse the repository at this point in the history 
…precedence (#16372)

* Keep parens for non-ident. op pairs with same prec

* Keep parens in cases like `f <| (g << h)`, `x + (y ++ z)`, etc.

* 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.

* Add entry to FCS release notes
  • Loading branch information
@brianrourkeboll
brianrourkeboll authored Dec 15, 2023
1 parent b219831 commit fd642a9
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 185 deletions.
2 changes: 1 addition & 1 deletion docs/release-notes/.FSharp.Compiler.Service/8.0.200.md
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
### Fixed

* Miscellaneous fixes to parentheses analysis. ([PR #16262](https://github.com/dotnet/fsharp/pull/16262), [PR #16391](https://github.com/dotnet/fsharp/pull/16391), [PR #16370](https://github.com/dotnet/fsharp/pull/16370), [PR #16395](https://github.com/dotnet/fsharp/pull/16395))
* Miscellaneous fixes to parentheses analysis. ([PR #16262](https://github.com/dotnet/fsharp/pull/16262), [PR #16391](https://github.com/dotnet/fsharp/pull/16391), [PR #16370](https://github.com/dotnet/fsharp/pull/16370), [PR #16395](https://github.com/dotnet/fsharp/pull/16395), [PR #16372](https://github.com/dotnet/fsharp/pull/16372))
* Correctly handle assembly imports with public key token of 0 length. ([Issue #16359](https://github.com/dotnet/fsharp/issues/16359), [PR #16363](https://github.com/dotnet/fsharp/pull/16363))
* Range of [SynField](../reference/fsharp-compiler-syntax-synfield.html) ([PR #16357](https://github.com/dotnet/fsharp/pull/16357))
* Limit a type to 65K methods, introduce a compile-time error if any class has over approx 64K methods in generated IL. ([Issue #16398](https://github.com/dotnet/fsharp/issues/16398), [#PR 16427](https://github.com/dotnet/fsharp/pull/16427))
Expand Down
291 changes: 113 additions & 178 deletions src/Compiler/Service/ServiceAnalysis.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,14 +462,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.
[<CustomComparison; CustomEquality>]
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.
[<CustomComparison; CustomEquality>]
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.
[<CustomComparison; CustomEquality>]
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
Expand All @@ -487,67 +540,34 @@ 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

/// :?
| TypeTest

/// -…
| Sub

/// +…
| Add

/// %…
| Mod
/// +…, -…
| AddSub of AddSub * OriginalNotation

/// /…
| Div

/// *…
| Mul
/// *…, /…, %…
| MulDivMod of MulDivMod * OriginalNotation

/// **…
| Exp
Expand All @@ -564,85 +584,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.
Expand All @@ -661,26 +602,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
Expand Down Expand Up @@ -783,26 +713,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

[<return: Struct>]
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:
Expand All @@ -815,9 +749,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
Expand All @@ -843,8 +777,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

Expand Down Expand Up @@ -1228,11 +1162,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

Expand Down Expand Up @@ -1378,7 +1312,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, _
Expand All @@ -1387,11 +1321,12 @@ module UnnecessaryParentheses =
| Right, Right
| Left, Left -> false
| Right, Left ->
not (Precedence.sameKind outerPrecedence innerPrecedence)
|| match innerPrecedence with
| Div
| Mod
| Sub -> true
outerPrecedence <> innerPrecedence
|| match outerPrecedence, innerPrecedence with
| _, MulDivMod(Div, _)
| _, MulDivMod(Mod, _)
| _, AddSub(Sub, _) -> true
| Relational _, Relational _ -> true
| _ -> false

| c -> c > 0
Expand Down
Loading

0 comments on commit fd642a9

Please sign in to comment.