From c7fcbf638052efbed3e00eec1682b50b0d3bab98 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Thu, 21 Mar 2024 12:40:44 -0400 Subject: [PATCH] Parens: more fixes (#16901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix a few more paren corner cases * Match-like exprs in `if` exprs, `while` exprs, and `for` exprs. Also `let` exprs. * Nested, dangling `as` patterns. * Outlaw `match` exprs (where the first `|` is leftward of the `m` in `match) * Single-line comments (`//`, `///`). Multiline comments (`(*…*)`) would be… rather more difficult to handle. * Double-parenthesized tuples in method applications, since we can't tell purely syntactically whether the tuple might be the first parameter of a method whose next parameter is an implied outref parameter: `x.TryGetValue ((y, z))` i.e., `x.TryGetValue ((y, z), &value)` * Multiline tuple patterns in `let`-bindings. These need parens if the bound expression starts on the same column. * Handle typed pats in getters & setters * Double parens oddities * Sometimes we can't tell just by looking at the untyped AST whether we need parens, since their necessity may be dictated by type information located elsewhere. Compare, e.g., https://github.com/dotnet/fsharp/issues/16254, which probably has a similar underlying cause. * Keep parens for parenzed app preceded by prefix op * Keep parens around tuple in interp string * More nested match fun * No space when expr would reparse as prefix op * No space when expr would reparse as prefix op * No space when expr would reparse as prefix op * Update release notes * Remove unfinished multiline comment stuff * Keep parens around dot-get that would be adjacent * E.g., removing parens in place from ```fsharp Debug.Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel) ``` would result in the the argument to `Assert` becoming `(xT.DeclaringType :?> ProvidedTypeDefinition)`. The `.BelongsToTargetModel` would then be parsed as a get on the return value of _that_ call. * Fantomas --- .../.FSharp.Compiler.Service/8.0.300.md | 2 +- docs/release-notes/.VisualStudio/17.10.md | 2 +- src/Compiler/Service/SynExpr.fs | 77 ++++- src/Compiler/Service/SynPat.fs | 72 ++++- .../CodeFixes/RemoveUnnecessaryParentheses.fs | 101 +++++-- .../RemoveUnnecessaryParenthesesTests.fs | 278 ++++++++++++++++++ 6 files changed, 486 insertions(+), 46 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index 1ec3b5add20..d055d7c988b 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -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)) diff --git a/docs/release-notes/.VisualStudio/17.10.md b/docs/release-notes/.VisualStudio/17.10.md index 6c27a8fcfd1..3059b1b8868 100644 --- a/docs/release-notes/.VisualStudio/17.10.md +++ b/docs/release-notes/.VisualStudio/17.10.md @@ -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 diff --git a/src/Compiler/Service/SynExpr.fs b/src/Compiler/Service/SynExpr.fs index 07321ce1646..db284ac36f7 100644 --- a/src/Compiler/Service/SynExpr.fs +++ b/src/Compiler/Service/SynExpr.fs @@ -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) @@ -437,6 +438,14 @@ module SynExpr = | SynExpr.IfThenElse _ as expr -> Some expr | _ -> None) + /// Matches a dangling let or use construct. + [] + let (|LetOrUse|_|) = + dangling (function + | SynExpr.LetOrUse _ + | SynExpr.LetOrUseBang _ as expr -> Some expr + | _ -> None) + /// Matches a dangling sequential expression. [] let (|Sequential|_|) = @@ -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 @@ -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() @@ -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 @@ -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 -> @@ -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 diff --git a/src/Compiler/Service/SynPat.fs b/src/Compiler/Service/SynPat.fs index 53212cf17dc..0016acb26d0 100644 --- a/src/Compiler/Service/SynPat.fs +++ b/src/Compiler/Service/SynPat.fs @@ -55,6 +55,24 @@ module SynPat = | SynPat.Tuple(isStruct = false; elementPats = Last(Rightmost pat)) -> pat | pat -> pat + /// Matches a nested as pattern. + [] + 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. [] let (|Atomic|_|) pat = @@ -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 _) :: _ @@ -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 with override _.M (()) = () } - | SynPat.Paren(SynPat.Const(SynConst.Unit, _), _), + // let _ = { new C 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. // @@ -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) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs index b55fbb26139..d072d2154b3 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs @@ -15,6 +15,27 @@ open CancellableTasks module private Patterns = let inline toPat f x = if f x then ValueSome() else ValueNone + /// Starts with //. + [] + let (|StartsWithSingleLineComment|_|) (s: string) = + if s.AsSpan().TrimStart(' ').StartsWith("//".AsSpan()) then + ValueSome StartsWithSingleLineComment + else + ValueNone + + /// Starts with match, e.g., + /// + /// (match … with + /// | … -> …) + [] + let (|StartsWithMatch|_|) (s: string) = + let s = s.AsSpan().TrimStart ' ' + + if s.StartsWith("match".AsSpan()) && (s.Length = 5 || s[5] = ' ') then + ValueSome StartsWithMatch + else + ValueNone + [] module Char = [] @@ -137,11 +158,17 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ loop innerOffsides (lineNo + 1) 0 | i -> - match innerOffsides with - | NoneYet -> loop (FirstLine(i + startCol)) (lineNo + 1) 0 - | FirstLine innerOffsides -> loop (FollowingLine(innerOffsides, i + startCol)) (lineNo + 1) 0 - | FollowingLine(firstLine, innerOffsides) -> - loop (FollowingLine(firstLine, min innerOffsides (i + startCol))) (lineNo + 1) 0 + match line[i + startCol ..] with + | StartsWithMatch + | StartsWithSingleLineComment -> loop innerOffsides (lineNo + 1) 0 + | _ -> + match innerOffsides with + | NoneYet -> loop (FirstLine(i + startCol)) (lineNo + 1) 0 + + | FirstLine inner -> loop (FollowingLine(inner, i + startCol)) (lineNo + 1) 0 + + | FollowingLine(firstLine, innerOffsides) -> + loop (FollowingLine(firstLine, min innerOffsides (i + startCol))) (lineNo + 1) 0 else innerOffsides @@ -165,21 +192,24 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [ None - | '[', '|', (Punctuation | LetterOrDigit) -> None - | _, '[', '<' -> Some ShouldPutSpaceBefore - | _, ('(' | '[' | '{'), _ -> None - | _, '>', _ -> Some ShouldPutSpaceBefore - | ' ', '=', _ -> Some ShouldPutSpaceBefore - | _, '=', ('(' | '[' | '{') -> None - | _, '=', (Punctuation | Symbol) -> Some ShouldPutSpaceBefore - | _, LetterOrDigit, '(' -> None - | _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore - | _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore - | _ -> None + match s with + | StartsWithMatch -> None + | _ -> + // ……(……) + // ↑↑ ↑ + match sourceText[max (context.Span.Start - 2) 0], sourceText[max (context.Span.Start - 1) 0], s[0] with + | _, _, ('\n' | '\r') -> None + | '[', '|', (Punctuation | LetterOrDigit) -> None + | _, '[', '<' -> Some ShouldPutSpaceBefore + | _, ('(' | '[' | '{'), _ -> None + | _, '>', _ -> Some ShouldPutSpaceBefore + | ' ', '=', _ -> Some ShouldPutSpaceBefore + | _, '=', ('(' | '[' | '{') -> None + | _, '=', (Punctuation | Symbol) -> Some ShouldPutSpaceBefore + | _, LetterOrDigit, '(' -> None + | _, (LetterOrDigit | '`'), _ -> Some ShouldPutSpaceBefore + | _, (Punctuation | Symbol), (Punctuation | Symbol) -> Some ShouldPutSpaceBefore + | _ -> None let (|ShouldPutSpaceAfter|_|) (s: string) = // (……)… @@ -187,23 +217,42 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [', ('|' | ']') -> Some ShouldPutSpaceAfter | _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None + | _, ('+' | '-' | '%' | '&' | '!' | '~') -> None | (Punctuation | Symbol), (Punctuation | Symbol | LetterOrDigit) -> Some ShouldPutSpaceAfter | LetterOrDigit, LetterOrDigit -> Some ShouldPutSpaceAfter | _ -> None + let (|WouldTurnInfixIntoPrefix|_|) (s: string) = + // (……)… + // ↑ ↑ + match s[s.Length - 1], sourceText[min context.Span.End (sourceText.Length - 1)] with + | (Punctuation | Symbol), ('+' | '-' | '%' | '&' | '!' | '~') -> + let linePos = sourceText.Lines.GetLinePosition context.Span.End + let line = sourceText.Lines[linePos.Line].ToString() + + // (……)+… + // ↑ + match line.AsSpan(linePos.Character).IndexOfAnyExcept("*/%-+:^@><=!|$.?".AsSpan()) with + | -1 -> None + | i when line[linePos.Character + i] <> ' ' -> Some WouldTurnInfixIntoPrefix + | _ -> None + | _ -> None + match adjusted with - | ShouldPutSpaceBefore & ShouldPutSpaceAfter -> " " + adjusted + " " - | ShouldPutSpaceBefore -> " " + adjusted - | ShouldPutSpaceAfter -> adjusted + " " - | adjusted -> adjusted + | WouldTurnInfixIntoPrefix -> ValueNone + | ShouldPutSpaceBefore & ShouldPutSpaceAfter -> ValueSome(" " + adjusted + " ") + | ShouldPutSpaceBefore -> ValueSome(" " + adjusted) + | ShouldPutSpaceAfter -> ValueSome(adjusted + " ") + | adjusted -> ValueSome adjusted return - ValueSome + newText + |> ValueOption.map (fun newText -> { Name = CodeFix.RemoveUnnecessaryParentheses Message = title Changes = [ TextChange(context.Span, newText) ] - } + }) | notParens -> System.Diagnostics.Debug.Fail $"%A{notParens} <> ('(', ')')" diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs index 8808871f497..13227d8eb42 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/RemoveUnnecessaryParenthesesTests.fs @@ -151,6 +151,42 @@ let _ = 1 " + " + let (a, + b, + c, + d, + e, + f) = g + ", + " + let a, + b, + c, + d, + e, + f = g + " + + " + let (a, + b, + c, + d, + e, + f) = + g + ", + " + let (a, + b, + c, + d, + e, + f) = + g + " + // AnonymousRecord "{| A = (1) |}", "{| A = 1 |}" "{| A = (1); B = 2 |}", "{| A = 1; B = 2 |}" @@ -188,6 +224,7 @@ let _ = // While "while (true) do ()", "while true do ()" "while true do (ignore 3)", "while true do ignore 3" + "while (match () with _ -> true) do ()", "while (match () with _ -> true) do ()" // For "for x = (0) to 1 do ()", "for x = 0 to 1 do ()" @@ -200,6 +237,7 @@ let _ = "for (x) in [] do ()", "for x in [] do ()" "for x in ([]) do ()", "for x in [] do ()" "for x in [] do (ignore 3)", "for x in [] do ignore 3" + "for x in (try [] with _ -> []) do ()", "for x in (try [] with _ -> []) do ()" // ArrayOrListComputed "[1; 2; (if x then 3 else 4); 5]", "[1; 2; (if x then 3 else 4); 5]" @@ -293,6 +331,63 @@ let _ = | _ -> 3 " + " + 3 > ( match x with + | 1 + | _ -> 3) + ", + " + 3 > match x with + | 1 + | _ -> 3 + " + + " + 3 > (match x with + | 1 + | _ -> 3) + ", + " + 3 > match x with + | 1 + | _ -> 3 + " + + " + 3 > (match x with + // Lol. + | 1 + | _ -> 3) + ", + " + 3 > match x with + // Lol. + | 1 + | _ -> 3 + " + + " + 3 >(match x with + | 1 + | _ -> 3) + ", + " + 3 >match x with + | 1 + | _ -> 3 + " + + " + f(match x with + | 1 + | _ -> 3) + ", + " + f(match x with + | 1 + | _ -> 3) + " + // Do "do (ignore 3)", "do ignore 3" @@ -903,6 +998,11 @@ in x () """ + "if (match () with _ -> true) then ()", "if (match () with _ -> true) then ()" + + "if (match () with _ -> true) && (match () with _ -> true) then ()", + "if (match () with _ -> true) && (match () with _ -> true) then ()" + // LongIdent "(|Failure|_|) null", "(|Failure|_|) null" @@ -913,6 +1013,15 @@ in x "([]).Length", "[].Length" "([] : int list).Length", "([] : int list).Length" + "Debug.Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel)", + "Debug.Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel)" + + "Debug.Assert ((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel)", + "Debug.Assert (xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel" + + "Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel)", + "Assert((xT.DeclaringType :?> ProvidedTypeDefinition).BelongsToTargetModel)" + // DotLambda """_.ToString("x")""", """_.ToString("x")""" """_.ToString(("x"))""", """_.ToString("x")""" @@ -1097,6 +1206,7 @@ in x "$\"{-(3)}\"", "$\"{-3}\"" "$\"{(id 3)}\"", "$\"{id 3}\"" "$\"{(x)}\"", "$\"{x}\"" + "$\"{(x, y)}\"", "$\"{(x, y)}\"" "$\"{(if true then 1 else 0)}\"", "$\"{if true then 1 else 0}\"" "$\"{(if true then 1 else 0):N0}\"", "$\"{(if true then 1 else 0):N0}\"" @@ -1238,6 +1348,20 @@ in x "id (struct (x, y))", "id struct (x, y)" "id (x, y)", "id (x, y)" + // We can't tell syntactically whether the method might have the signature + // + // val TryGetValue : 'Key * outref<'Value> -> bool + // + // where 'Key is 'a * 'b, in which case the double parens are required. + // We could look this up in the typed tree, but we don't currently. + "x.TryGetValue((y, z))", "x.TryGetValue((y, z))" + + "valInfosForFslib.Force(g).TryGetValue((vref, vref.Deref.GetLinkageFullKey()))", + "valInfosForFslib.Force(g).TryGetValue((vref, vref.Deref.GetLinkageFullKey()))" + + "SemanticClassificationItem((m, SemanticClassificationType.Printf))", + "SemanticClassificationItem((m, SemanticClassificationType.Printf))" + // AnonRecd "id ({||})", "id {||}" "{| A = (fun () -> ()) |}", "{| A = fun () -> () |}" @@ -1417,6 +1541,65 @@ in x "(id <| match x with _ -> x) |> id", "(id <| match x with _ -> x) |> id" "id <| (match x with _ -> x) |> id", "id <| (match x with _ -> x) |> id" + " + match () with + | _ when + (true && + let x = 3 + match x with + | 3 | _ -> true) -> () + | _ -> () + ", + " + match () with + | _ when + (true && + let x = 3 + match x with + | 3 | _ -> true) -> () + | _ -> () + " + + " + match () with + | _ when + true && + let x = 3 + (match x with + | 3 | _ -> true) -> () + | _ -> () + ", + " + match () with + | _ when + true && + let x = 3 + (match x with + | 3 | _ -> true) -> () + | _ -> () + " + + " + match () with + | _ when + true && + let x = 3 + (let y = false + match x with + | 3 | _ -> y) -> () + | _ -> () + ", + " + match () with + | _ when + true && + let x = 3 + (let y = false + match x with + | 3 | _ -> y) -> () + | _ -> () + " + // Do "id (do ())", "id (do ())" @@ -1513,6 +1696,7 @@ in x """(id("x")).Length""", """(id "x").Length""" """(id "x").Length""", """(id "x").Length""" """(3L.ToString("x")).Length""", """(3L.ToString "x").Length""" + "~~TypedResults.Ok(maybe.Value)", "~~TypedResults.Ok(maybe.Value)" // DotLambda "[{| A = x |}] |> List.map (_.A)", "[{| A = x |}] |> List.map _.A" @@ -1611,6 +1795,22 @@ in x // Miscellaneous "System.Threading.Tasks.Task.CompletedTask.ConfigureAwait((x = x))", "System.Threading.Tasks.Task.CompletedTask.ConfigureAwait((x = x))" + + "x.M(y).N((z = z))", "x.M(y).N((z = z))" + + """ + dprintn ("The local method '"+(String.concat "." (tenc@[tname]))+"'::'"+mdkey.Name+"' was referenced but not declared") + """, + """ + dprintn ("The local method '"+(String.concat "." (tenc@[tname]))+"'::'"+mdkey.Name+"' was referenced but not declared") + """ + + """ + ""+(Unchecked.defaultof)+"" + """, + """ + ""+(Unchecked.defaultof)+"" + """ } [] @@ -2552,6 +2752,84 @@ module Patterns = new (x, y, z) = T (x, y) member _.Z = x + y " + + // The parens could be required by a signature file like this: + // + // type SemanticClassificationItem = + // val Range: range + // val Type: SemanticClassificationType + // new: (range * SemanticClassificationType) -> SemanticClassificationItem + " + type SemanticClassificationItem = + val Range: range + val Type: SemanticClassificationType + new((range, ty)) = { Range = range; Type = ty } + ", + " + type SemanticClassificationItem = + val Range: range + val Type: SemanticClassificationType + new((range, ty)) = { Range = range; Type = ty } + " + + " + match 1, 2 with + | _, (1 | 2 as x) -> () + | _ -> () + ", + " + match 1, 2 with + | _, (1 | 2 as x) -> () + | _ -> () + " + + " + match 1, [2] with + | _, (1 | 2 as x :: _) -> () + | _ -> () + ", + " + match 1, [2] with + | _, (1 | 2 as x :: _) -> () + | _ -> () + " + + " + match 1, [2] with + | _, (1 as x :: _ :: _) -> () + | _ -> () + ", + " + match 1, [2] with + | _, (1 as x :: _ :: _) -> () + | _ -> () + " + + " + type T () = + member this.Item + with get (y : int) = 3 + and set (x : int) (y : int) = ignore (x, y) + ", + " + type T () = + member this.Item + with get (y : int) = 3 + and set (x : int) (y : int) = ignore (x, y) + " + + " + let _ = + { new IEquatable with + member this.GetHashCode ((x, y, z)) = x + y + z + member this.Equals ((x, y, z), (x', y', z')) = false } + ", + " + let _ = + { new IEquatable with + member this.GetHashCode ((x, y, z)) = x + y + z + member this.Equals ((x, y, z), (x', y', z')) = false } + " } []