From da19ee52c702b685cbf222445d0a025dae5ca3f4 Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 29 Oct 2024 10:32:01 +0100 Subject: [PATCH 1/4] Don't put a space before a prefix operator, unless we have to. --- .../ControlStructureTests.fs | 2 +- .../Fantomas.Core.Tests.fsproj | 1 + .../InterpolatedStringTests.fs | 14 -- src/Fantomas.Core.Tests/LambdaTests.fs | 2 +- src/Fantomas.Core.Tests/LetBindingTests.fs | 2 +- .../MultiLineLambdaClosingNewlineTests.fs | 2 +- src/Fantomas.Core.Tests/OperatorTests.fs | 117 ------------ src/Fantomas.Core.Tests/PrefixTests.fs | 169 ++++++++++++++++++ .../SpaceBeforeLowercaseInvocationTests.fs | 6 +- .../SpaceBeforeUppercaseInvocationTests.fs | 6 +- src/Fantomas.Core/CodePrinter.fs | 30 ++-- 11 files changed, 195 insertions(+), 156 deletions(-) create mode 100644 src/Fantomas.Core.Tests/PrefixTests.fs diff --git a/src/Fantomas.Core.Tests/ControlStructureTests.fs b/src/Fantomas.Core.Tests/ControlStructureTests.fs index 8f55dc26f1..000b1ff609 100644 --- a/src/Fantomas.Core.Tests/ControlStructureTests.fs +++ b/src/Fantomas.Core.Tests/ControlStructureTests.fs @@ -644,7 +644,7 @@ let genPropertyWithGetSet astContext (b1, b2) rangeOfMember = genPreXmlDoc px +> genAttributes astContext ats +> genMemberFlags astContext mf1 - +> ifElse isInline (!- "inline ") sepNone + +> ifElse isInline (!-"inline ") sepNone +> opt sepSpace ao genAccess assert (ps1 |> Seq.map fst |> Seq.forall Option.isNone) diff --git a/src/Fantomas.Core.Tests/Fantomas.Core.Tests.fsproj b/src/Fantomas.Core.Tests/Fantomas.Core.Tests.fsproj index f764d1de94..6f606dc0ce 100644 --- a/src/Fantomas.Core.Tests/Fantomas.Core.Tests.fsproj +++ b/src/Fantomas.Core.Tests/Fantomas.Core.Tests.fsproj @@ -134,6 +134,7 @@ + diff --git a/src/Fantomas.Core.Tests/InterpolatedStringTests.fs b/src/Fantomas.Core.Tests/InterpolatedStringTests.fs index a960bb7d14..47665d4efe 100644 --- a/src/Fantomas.Core.Tests/InterpolatedStringTests.fs +++ b/src/Fantomas.Core.Tests/InterpolatedStringTests.fs @@ -165,20 +165,6 @@ $\"\"\"one: {1}< >two: {2}\"\"\" " -[] -let ``prefix application, 1414`` () = - formatSourceString - """ -!- $".{s}" -""" - config - |> prepend newline - |> should - equal - """ -!- $".{s}" -""" - [] let ``format in FillExpr, 1549`` () = formatSourceString diff --git a/src/Fantomas.Core.Tests/LambdaTests.fs b/src/Fantomas.Core.Tests/LambdaTests.fs index 5177324223..45d1011d2f 100644 --- a/src/Fantomas.Core.Tests/LambdaTests.fs +++ b/src/Fantomas.Core.Tests/LambdaTests.fs @@ -354,7 +354,7 @@ let genMemberFlagsForMemberBinding astContext (mf: MemberFlags) (rangeOfBindingA | Token { TokenInfo = { TokenName = "MEMBER" } } -> r.StartLine = rangeOfBindingAndRhs.StartLine | _ -> false) - |> Option.defaultValue (!- "override ") + |> Option.defaultValue (!-"override ") <| ctx) <| ctx """ diff --git a/src/Fantomas.Core.Tests/LetBindingTests.fs b/src/Fantomas.Core.Tests/LetBindingTests.fs index 046f6fb41b..60cb6dca2b 100644 --- a/src/Fantomas.Core.Tests/LetBindingTests.fs +++ b/src/Fantomas.Core.Tests/LetBindingTests.fs @@ -1342,7 +1342,7 @@ let internal sepSpace = then ctx else - (!- " ") ctx + (!-" ") ctx """ [] diff --git a/src/Fantomas.Core.Tests/MultiLineLambdaClosingNewlineTests.fs b/src/Fantomas.Core.Tests/MultiLineLambdaClosingNewlineTests.fs index 09397a552f..c0df14649c 100644 --- a/src/Fantomas.Core.Tests/MultiLineLambdaClosingNewlineTests.fs +++ b/src/Fantomas.Core.Tests/MultiLineLambdaClosingNewlineTests.fs @@ -268,7 +268,7 @@ let expr = es (fun e -> match e with - | Paren(_, Lambda _, _) -> !- "lambda" + | Paren(_, Lambda _, _) -> !-"lambda" | _ -> genExpr astContext e ) """ diff --git a/src/Fantomas.Core.Tests/OperatorTests.fs b/src/Fantomas.Core.Tests/OperatorTests.fs index 75d2c59ad7..ef01848955 100644 --- a/src/Fantomas.Core.Tests/OperatorTests.fs +++ b/src/Fantomas.Core.Tests/OperatorTests.fs @@ -5,41 +5,6 @@ open FsUnit open Fantomas.Core.Tests.TestHelpers open Fantomas.Core -[] -let ``should format prefix operators`` () = - formatSourceString - """let x = -y -let z = !!x - """ - config - |> should - equal - """let x = -y -let z = !!x -""" - -[] -let ``should keep triple ~~~ operator`` () = - formatSourceString - """x ~~~FileAttributes.ReadOnly - """ - config - |> should - equal - """x ~~~FileAttributes.ReadOnly -""" - -[] -let ``should keep single triple ~~~ operator`` () = - formatSourceString - """~~~FileAttributes.ReadOnly - """ - config - |> should - equal - """~~~FileAttributes.ReadOnly -""" - [] let ``should keep parens around ? operator definition`` () = formatSourceString @@ -62,17 +27,6 @@ let ``should keep parens around ?<- operator definition`` () = """let (?<-) f s = f s """ -[] -let ``should keep parens around !+ prefix operator definition`` () = - formatSourceString - """let (!+) x = Include x - """ - config - |> should - equal - """let (!+) x = Include x -""" - [] let ``should keep parens around ++ infix operator definition`` () = formatSourceString @@ -474,19 +428,6 @@ let result = (typ.GetInterface(typeof.FullName) = null) """ -[] -let ``operator before verbatim string add extra space, 736`` () = - formatSourceString - """Target M.Tools (fun _ -> !! @"Tools\Tools.sln" |> rebuild) -""" - config - |> prepend newline - |> should - equal - """ -Target M.Tools (fun _ -> !! @"Tools\Tools.sln" |> rebuild) -""" - [] let ``function call before pipe operator, 754`` () = formatSourceString @@ -1178,42 +1119,6 @@ module Foo = | false -> id) """ -let operator_application_literal_values = - [ "-86y" - "86uy" - "-86s" - "86us" - "-86" - "-86l" - "86u" - "86ul" - "-123n" - "0x00002D3Fun" - "-86L" - "86UL" - "-4.41F" - "-4.14" - "-12456I" - "-0.7833M" - "'a'" - "\"text\"" - "'a'B" - "\"text\"B" ] - -[] -let ``operators maintain spacing from literal values`` (literalValue: string) = - formatSourceString - $""" -let subtractTwo = + %s{literalValue} -""" - config - |> prepend newline - |> should - equal - $""" -let subtractTwo = + %s{literalValue} -""" - [] let ``qualified name to active pattern, 1937`` () = formatSourceString @@ -1515,25 +1420,3 @@ let allDecls = @+ iimplsLs @+ ctorLs """ - -[] -let ``adding space after prefix operator breaks code, 2796`` () = - formatSourceString - """ -let inline (~%%) id = int id - -let f a b = a + b - -let foo () = f %%"17" %%"42" -""" - config - |> prepend newline - |> should - equal - """ -let inline (~%%) id = int id - -let f a b = a + b - -let foo () = f %%"17" %%"42" -""" diff --git a/src/Fantomas.Core.Tests/PrefixTests.fs b/src/Fantomas.Core.Tests/PrefixTests.fs new file mode 100644 index 0000000000..61f67b45c1 --- /dev/null +++ b/src/Fantomas.Core.Tests/PrefixTests.fs @@ -0,0 +1,169 @@ +module Fantomas.Core.Tests.PrefixTests + +open NUnit.Framework +open FsUnit +open Fantomas.Core.Tests.TestHelpers +open Fantomas.Core + +[] +let ``should format prefix operators`` () = + formatSourceString + """let x = -y +let z = !!x + """ + config + |> should + equal + """let x = -y +let z = !!x +""" + +[] +let ``should keep triple ~~~ operator`` () = + formatSourceString + """x ~~~FileAttributes.ReadOnly + """ + config + |> should + equal + """x ~~~FileAttributes.ReadOnly +""" + +[] +let ``should keep single triple ~~~ operator`` () = + formatSourceString + """~~~FileAttributes.ReadOnly + """ + config + |> should + equal + """~~~FileAttributes.ReadOnly +""" + +[] +let ``operator before verbatim string add extra space, 736`` () = + formatSourceString + """Target M.Tools (fun _ -> !! @"Tools\Tools.sln" |> rebuild) +""" + config + |> prepend newline + |> should + equal + """ +Target M.Tools (fun _ -> !! @"Tools\Tools.sln" |> rebuild) +""" + +[] +let ``should keep parens around !+ prefix operator definition`` () = + formatSourceString + """let (!+) x = Include x + """ + config + |> should + equal + """let (!+) x = Include x +""" + +[] +let ``adding space after prefix operator breaks code, 2796`` () = + formatSourceString + """ +let inline (~%%) id = int id + +let f a b = a + b + +let foo () = f %%"17" %%"42" +""" + config + |> prepend newline + |> should + equal + """ +let inline (~%%) id = int id + +let f a b = a + b + +let foo () = f %%"17" %%"42" +""" + +[] +let ``tilde unary operator with literal and variable, 3131`` () = + formatSourceString + """ +let x = ~~1 +let y = ~~x +""" + config + |> prepend newline + |> should + equal + """ +let x = ~~1 +let y = ~~x +""" + +[] +let ``prefix application with interpolated string, 1414`` () = + formatSourceString + """ +!- $".{s}" +""" + config + |> prepend newline + |> should + equal + """ +!- $".{s}" +""" + +let operator_application_literal_values_with_sign = + [ "-86y" + "-86s" + "-86" + "-86l" + "-123n" + "-86L" + "-4.41F" + "-4.14" + "-12456I" + "-0.7833M" ] + +[] +let ``operators maintain spacing from literal values which start with + or -`` (literalValue: string) = + formatSourceString + $""" +let subtractTwo = + %s{literalValue} +""" + config + |> prepend newline + |> should + equal + $""" +let subtractTwo = + %s{literalValue} +""" + +let operator_application_literal_values_without_sign = + [ "86uy" + "86us" + "86u" + "86ul" + "0x00002D3Fun" + "86UL" + "'a'" + "\"text\"" + "'a'B" + "\"text\"B" ] + +[] +let ``operators maintain spacing from literal values which start without + or -`` (literalValue: string) = + formatSourceString + $""" +let subtractTwo = + %s{literalValue} +""" + config + |> prepend newline + |> should + equal + $""" +let subtractTwo = +%s{literalValue} +""" diff --git a/src/Fantomas.Core.Tests/SpaceBeforeLowercaseInvocationTests.fs b/src/Fantomas.Core.Tests/SpaceBeforeLowercaseInvocationTests.fs index 73df26b7d4..f14ede4906 100644 --- a/src/Fantomas.Core.Tests/SpaceBeforeLowercaseInvocationTests.fs +++ b/src/Fantomas.Core.Tests/SpaceBeforeLowercaseInvocationTests.fs @@ -217,9 +217,9 @@ let ``ignore setting when function call is the argument of prefix application`` |> should equal """ -!- String.Empty.padLeft(braceSize + spaceAround) -(!- System.String.Empty.padRight(delta)) ({ ctx with RecordBraceStart = rest }) -!- meh() +!-String.Empty.padLeft(braceSize + spaceAround) +(!-System.String.Empty.padRight(delta)) ({ ctx with RecordBraceStart = rest }) +!-meh() """ [] diff --git a/src/Fantomas.Core.Tests/SpaceBeforeUppercaseInvocationTests.fs b/src/Fantomas.Core.Tests/SpaceBeforeUppercaseInvocationTests.fs index 23843758a3..04983e94f0 100644 --- a/src/Fantomas.Core.Tests/SpaceBeforeUppercaseInvocationTests.fs +++ b/src/Fantomas.Core.Tests/SpaceBeforeUppercaseInvocationTests.fs @@ -251,9 +251,9 @@ let ``ignore setting when function call is the argument of prefix application, 1 |> should equal """ -!- String.Empty.PadLeft(braceSize + spaceAround) -(!- System.String.Empty.PadRight(delta)) ({ ctx with RecordBraceStart = rest }) -!- Meh() +!-String.Empty.PadLeft(braceSize + spaceAround) +(!-System.String.Empty.PadRight(delta)) ({ ctx with RecordBraceStart = rest }) +!-Meh() """ [] diff --git a/src/Fantomas.Core/CodePrinter.fs b/src/Fantomas.Core/CodePrinter.fs index 2f74ee753d..1cf3d21fe5 100644 --- a/src/Fantomas.Core/CodePrinter.fs +++ b/src/Fantomas.Core/CodePrinter.fs @@ -753,33 +753,33 @@ let genExpr (e: Expr) = |> genNode node | Expr.Dynamic node -> genExpr node.FuncExpr +> !- "?" +> genExpr node.ArgExpr |> genNode node | Expr.PrefixApp node -> - let fallback = genSingleTextNode node.Operator +> genExpr node.Expr + let genWithoutSpace = genSingleTextNode node.Operator +> genExpr node.Expr + let genWithSpace = genSingleTextNode node.Operator +> sepSpace +> genExpr node.Expr match node.Expr with - | Expr.Constant _ - | Expr.InterpolatedStringExpr _ when not (String.startsWithOrdinal "%" node.Operator.Text) -> - genSingleTextNode node.Operator +> sepSpace +> genExpr node.Expr + // E.g. !! @"foobar", because !!@ would be mistaken for an operator. + | Expr.Constant(Constant.FromText(stn)) -> + match stn.Text.[0] with + | '@' + | '-' + | '+' -> genWithSpace + | _ -> genWithoutSpace + // !- $"blah{s}" + | Expr.InterpolatedStringExpr _ -> genWithSpace + // We don't respect SpaceBeforeLowercaseInvocation here because it can alter the meaning of the code. + // E.g. !-meh() | Expr.AppSingleParenArg appNode -> genSingleTextNode node.Operator - +> sepSpace +> genExpr appNode.FunctionExpr +> genExpr appNode.ArgExpr + // E.g. !-Foo.Meh(a) | Expr.AppLongIdentAndSingleParenArg appNode -> let mOptVarNode = appNode.FunctionName.Range genSingleTextNode node.Operator - +> sepSpace +> genExpr (Expr.OptVar(ExprOptVarNode(false, appNode.FunctionName, mOptVarNode))) +> genExpr appNode.ArgExpr - | Expr.App appNode -> - match appNode.Arguments with - | [ Expr.Constant(Constant.Unit _) as argExpr ] -> - genSingleTextNode node.Operator - +> sepSpace - +> genExpr appNode.FunctionExpr - +> genExpr argExpr - | _ -> fallback - | _ -> fallback + | _ -> genWithoutSpace |> genNode node | Expr.SameInfixApps node -> let headIsSynExprLambdaOrIfThenElse = isLambdaOrIfThenElse node.LeadingExpr From ad7e654a0a20e3fef711f07de863f4b07ae0b57a Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 29 Oct 2024 15:40:08 +0100 Subject: [PATCH 2/4] Add feedback from code review --- src/Fantomas.Core.Tests/PrefixTests.fs | 46 ++++++++++++++++++++++++-- src/Fantomas.Core/CodePrinter.fs | 21 ++++++++---- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/Fantomas.Core.Tests/PrefixTests.fs b/src/Fantomas.Core.Tests/PrefixTests.fs index 61f67b45c1..a596bb4da0 100644 --- a/src/Fantomas.Core.Tests/PrefixTests.fs +++ b/src/Fantomas.Core.Tests/PrefixTests.fs @@ -126,7 +126,17 @@ let operator_application_literal_values_with_sign = "-4.41F" "-4.14" "-12456I" - "-0.7833M" ] + "-0.7833M" + "+46y" + "+46s" + "+46" + "+46l" + "+423n" + "+46L" + "+3.41F" + "+3.14" + "+32456I" + "+0.7833M" ] [] let ``operators maintain spacing from literal values which start with + or -`` (literalValue: string) = @@ -155,7 +165,9 @@ let operator_application_literal_values_without_sign = "\"text\"B" ] [] -let ``operators maintain spacing from literal values which start without + or -`` (literalValue: string) = +let ``no space added between prefix operators and literal values that do not start with a symbol`` + (literalValue: string) + = formatSourceString $""" let subtractTwo = + %s{literalValue} @@ -167,3 +179,33 @@ let subtractTwo = + %s{literalValue} $""" let subtractTwo = +%s{literalValue} """ + +[] +let ``add space between prefix and quotation`` () = + formatSourceString + """ +let _ = + <@ 1 @> +""" + config + |> prepend newline + |> should + equal + """ +let _ = + <@ 1 @> +""" + +[] +let ``add space between prefix and measure`` () = + formatSourceString + """ +let _ = - +1 +let _ = - -1 +""" + config + |> prepend newline + |> should + equal + """ +let _ = - +1 +let _ = - -1 +""" diff --git a/src/Fantomas.Core/CodePrinter.fs b/src/Fantomas.Core/CodePrinter.fs index 1cf3d21fe5..bb9f0d4738 100644 --- a/src/Fantomas.Core/CodePrinter.fs +++ b/src/Fantomas.Core/CodePrinter.fs @@ -757,13 +757,20 @@ let genExpr (e: Expr) = let genWithSpace = genSingleTextNode node.Operator +> sepSpace +> genExpr node.Expr match node.Expr with - // E.g. !! @"foobar", because !!@ would be mistaken for an operator. - | Expr.Constant(Constant.FromText(stn)) -> - match stn.Text.[0] with - | '@' - | '-' - | '+' -> genWithSpace - | _ -> genWithoutSpace + // E.g. + <@ 1 @> + | Expr.Quote _ -> genWithSpace + | Expr.Constant(constant) -> + // E.g. !! @"foobar", because !!@ would be mistaken for an operator. + match constant with + | Constant.FromText(stn) -> + match stn.Text.[0] with + | '@' + | '-' + | '+' -> genWithSpace + | _ -> genWithoutSpace + // E.g. - +1 + | Constant.Measure _ -> genWithSpace + | Constant.Unit _ -> genWithoutSpace // !- $"blah{s}" | Expr.InterpolatedStringExpr _ -> genWithSpace // We don't respect SpaceBeforeLowercaseInvocation here because it can alter the meaning of the code. From 5be6f99bcf0c64f125981706e16bd4ce6068f50c Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 29 Oct 2024 16:08:30 +0100 Subject: [PATCH 3/4] Only add space to measure when constant has sign. --- src/Fantomas.Core.Tests/PrefixTests.fs | 2 +- src/Fantomas.Core/CodePrinter.fs | 23 ++++++++++++----------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Fantomas.Core.Tests/PrefixTests.fs b/src/Fantomas.Core.Tests/PrefixTests.fs index a596bb4da0..1ec166006f 100644 --- a/src/Fantomas.Core.Tests/PrefixTests.fs +++ b/src/Fantomas.Core.Tests/PrefixTests.fs @@ -195,7 +195,7 @@ let _ = + <@ 1 @> """ [] -let ``add space between prefix and measure`` () = +let ``add space between prefix and measure literal that starts with a symbol`` () = formatSourceString """ let _ = - +1 diff --git a/src/Fantomas.Core/CodePrinter.fs b/src/Fantomas.Core/CodePrinter.fs index bb9f0d4738..b65149d473 100644 --- a/src/Fantomas.Core/CodePrinter.fs +++ b/src/Fantomas.Core/CodePrinter.fs @@ -756,21 +756,22 @@ let genExpr (e: Expr) = let genWithoutSpace = genSingleTextNode node.Operator +> genExpr node.Expr let genWithSpace = genSingleTextNode node.Operator +> sepSpace +> genExpr node.Expr - match node.Expr with - // E.g. + <@ 1 @> - | Expr.Quote _ -> genWithSpace - | Expr.Constant(constant) -> - // E.g. !! @"foobar", because !!@ would be mistaken for an operator. - match constant with + let rec (|ConstantWithSign|_|) = + function | Constant.FromText(stn) -> match stn.Text.[0] with | '@' | '-' - | '+' -> genWithSpace - | _ -> genWithoutSpace - // E.g. - +1 - | Constant.Measure _ -> genWithSpace - | Constant.Unit _ -> genWithoutSpace + | '+' -> Some() + | _ -> None + | Constant.Measure measure -> (|ConstantWithSign|_|) measure.Constant + | Constant.Unit _ -> None + + match node.Expr with + // E.g. + <@ 1 @> + | Expr.Quote _ -> genWithSpace + // E.g. !! @"foobar", because !!@ would be mistaken for an operator. + | Expr.Constant(ConstantWithSign) -> genWithSpace // !- $"blah{s}" | Expr.InterpolatedStringExpr _ -> genWithSpace // We don't respect SpaceBeforeLowercaseInvocation here because it can alter the meaning of the code. From a0b362cc384fadd9d45a85a5ec596974e9b7a2fb Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 29 Oct 2024 16:09:38 +0100 Subject: [PATCH 4/4] Remove unnecessary open --- src/Fantomas.Core.Tests/PrefixTests.fs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Fantomas.Core.Tests/PrefixTests.fs b/src/Fantomas.Core.Tests/PrefixTests.fs index 1ec166006f..df39c30322 100644 --- a/src/Fantomas.Core.Tests/PrefixTests.fs +++ b/src/Fantomas.Core.Tests/PrefixTests.fs @@ -3,7 +3,6 @@ open NUnit.Framework open FsUnit open Fantomas.Core.Tests.TestHelpers -open Fantomas.Core [] let ``should format prefix operators`` () =