From 1928d4ef034bf9a3bdadc7492f45cb73efb277e7 Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Tue, 13 Feb 2024 12:32:04 +0330 Subject: [PATCH 1/7] Tests(AvoidSinglePipeOperator): add a failure test --- .../Rules/Conventions/AvoidSinglePipeOperator.fs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs index c976f6c41..5f334e49a 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -140,3 +140,12 @@ let someFunc someParam = """ Assert.IsTrue this.NoErrorsExist + + [] + member this.``Use pipe operator once inside of an array``() = + this.Parse """ +let someFunc () = + [| "Foo" |> String.replicate 2 |] +""" + + Assert.IsTrue this.ErrorsExist From 23f5a7fb5c9c62908f70e99b70e84a506d48910e Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Tue, 13 Feb 2024 12:34:49 +0330 Subject: [PATCH 2/7] AvoidSinglePipeOperator: apply fix --- .../Rules/Conventions/AvoidSinglePipeOperator.fs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs index 135a63f92..38ed43a26 100644 --- a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -45,6 +45,8 @@ let runner (args: AstNodeRuleParams) = checkExpr funcExpr | SynExpr.IfThenElse(_, SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range), _, _, _, _, _) -> checkExpr funcExpr + | SynExpr.ArrayOrListComputed(_, SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range), _) -> + checkExpr funcExpr | _ -> Array.empty | _ -> From 9f3c47e8e5b82cd08d4c099fc10e59a4a892ddaf Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Tue, 13 Feb 2024 12:54:41 +0330 Subject: [PATCH 3/7] Tests(AvoidSinglePipeOperator): add a failure test --- .../Conventions/AvoidSinglePipeOperator.fs | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs index 5f334e49a..0964874fd 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -106,7 +106,7 @@ let foo param = Assert.True this.NoErrorsExist [] - member this.``Use pipe operator once on record``() = + member this.``Use pipe operator once on record within a if statement``() = this.Parse """ type Person = { @@ -123,7 +123,7 @@ let someFunc someParam = Assert.IsTrue this.ErrorsExist [] - member this.``Use pipe operator twice on record``() = + member this.``Use pipe operator twice on record within a if statement``() = this.Parse """ type Person = { @@ -149,3 +149,23 @@ let someFunc () = """ Assert.IsTrue this.ErrorsExist + + [] + member this.``Use pipe operator once on record within a nested if statement``() = + this.Parse """ +type Person = + { + FirstName: string + } + +let someFunc someParam barParam = + if someParam then + if barParam then + { FirstName = "Bar" } |> someOtherFunc + else + Array.empty + else + Array.empty +""" + + Assert.IsTrue this.ErrorsExist From 9de0a2410ffab9f8ff059447500efd234cc42b27 Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Thu, 15 Feb 2024 12:17:29 +0330 Subject: [PATCH 4/7] Tests(AvoidSinglePipeOperator): add 2 more tests --- .../Conventions/AvoidSinglePipeOperator.fs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs index 0964874fd..98e630d74 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -169,3 +169,23 @@ let someFunc someParam barParam = """ Assert.IsTrue this.ErrorsExist + + [] + member this.``Use pipe operator once without binding``() = + this.Parse """ +module Foo + +-1.0 |> printf "%d" +""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Use pipe operator twice without binding``() = + this.Parse """ +module Foo + +-1.0 |> printf "%d" |> ignore +""" + + Assert.IsTrue this.NoErrorsExist From ee5470293545a42e21d92ef1bcc4a50c2a6be3f6 Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Tue, 13 Feb 2024 12:56:38 +0330 Subject: [PATCH 5/7] AvoidSinglePipeOperator: fix for all inner exprs --- .../Conventions/AvoidSinglePipeOperator.fs | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs index 38ed43a26..e961fadc5 100644 --- a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -17,7 +17,13 @@ let runner (args: AstNodeRuleParams) = TypeChecks = List.Empty } |> Array.singleton - let checkExpr (expr: SynExpr) = + let rec checkExpr (expr: SynExpr) (parentList: AstNode list): WarningDetails array = + let checkParentPiped (expr: AstNode) = + match expr with + | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range)) -> + (checkExpr funcExpr []).Length = 0 + | _ -> false + match expr with | SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, argExpr, _range) -> match funcExpr with @@ -29,7 +35,14 @@ let runner (args: AstNodeRuleParams) = | SynExpr.IfThenElse _ -> Array.empty | _ -> - errors ident.idRange + let isParentPiped = + match parentList with + | head::_ -> checkParentPiped head + | [] -> false + if isParentPiped then + Array.empty + else + errors ident.idRange else Array.empty | _ -> @@ -39,16 +52,8 @@ let runner (args: AstNodeRuleParams) = let error = match args.AstNode with - | AstNode.Binding (SynBinding(_synAcc, _synBinding, _mustInline, _isMut, _synAttribs, _preXmlDoc, _synValData, _headPat, _synBindingRet, synExpr, _range, _debugPointAtBinding, _)) -> - match synExpr with - | SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range) -> - checkExpr funcExpr - | SynExpr.IfThenElse(_, SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range), _, _, _, _, _) -> - checkExpr funcExpr - | SynExpr.ArrayOrListComputed(_, SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range), _) -> - checkExpr funcExpr - | _ -> - Array.empty + | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range)) -> + checkExpr funcExpr (args.GetParents args.NodeIndex) | _ -> Array.empty From 34e2d7dade8e6a26c75ba14520445799a2e5f8f8 Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Thu, 15 Feb 2024 12:13:56 +0330 Subject: [PATCH 6/7] AvoidSinglePipeOperator: refactoring Switched to using `Seq.isEmpty` instead of comparing array length. --- .../Rules/Conventions/AvoidSinglePipeOperator.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs index e961fadc5..ffe4aa0d4 100644 --- a/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/src/FSharpLint.Core/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -21,7 +21,7 @@ let runner (args: AstNodeRuleParams) = let checkParentPiped (expr: AstNode) = match expr with | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range)) -> - (checkExpr funcExpr []).Length = 0 + checkExpr funcExpr [] |> Seq.isEmpty | _ -> false match expr with From 8a93684a1ebc795519c24b833c4eccf9586654df Mon Sep 17 00:00:00 2001 From: Mehrshad Date: Thu, 15 Feb 2024 13:07:56 +0330 Subject: [PATCH 7/7] Core.Tests: add a regression test This test was incorporated following the suggestion of @webwarrior-ws to include a test case that utilizes three pipe operators [1]. [1] https://github.com/fsprojects/FSharpLint/pull/697#pullrequestreview-1882186417 --- .../Rules/Conventions/AvoidSinglePipeOperator.fs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs index 98e630d74..d49aabd54 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidSinglePipeOperator.fs @@ -186,6 +186,16 @@ module Foo module Foo -1.0 |> printf "%d" |> ignore +""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``Use pipe operator thrice without binding``() = + this.Parse """ +module Foo + +-1.0 |> printf "%d" |> ignore |> someOtherFunc """ Assert.IsTrue this.NoErrorsExist