From 4f2500c9c8f44c655b0902c506bb0d379bebce40 Mon Sep 17 00:00:00 2001
From: Jeroen Engels <jfm.engels@gmail.com>
Date: Sun, 5 Apr 2020 09:57:34 +0200
Subject: [PATCH] Backport work from review-simplification

---
 tests/NoBooleanCaseOf.elm                  | 103 ++++++++++++++
 tests/NoBooleanCaseOfTest.elm              | 111 +++++++++++++++
 tests/NoFullyAppliedPrefixOperator.elm     |  63 +++++++++
 tests/NoFullyAppliedPrefixOperatorTest.elm |  96 +++++++++++++
 tests/NoListLiteralsConcat.elm             | 116 ++++++++++++++++
 tests/NoListLiteralsConcatTest.elm         | 154 +++++++++++++++++++++
 6 files changed, 643 insertions(+)
 create mode 100644 tests/NoBooleanCaseOf.elm
 create mode 100644 tests/NoBooleanCaseOfTest.elm
 create mode 100644 tests/NoFullyAppliedPrefixOperator.elm
 create mode 100644 tests/NoFullyAppliedPrefixOperatorTest.elm
 create mode 100644 tests/NoListLiteralsConcat.elm
 create mode 100644 tests/NoListLiteralsConcatTest.elm

diff --git a/tests/NoBooleanCaseOf.elm b/tests/NoBooleanCaseOf.elm
new file mode 100644
index 00000000..5d4ac17c
--- /dev/null
+++ b/tests/NoBooleanCaseOf.elm
@@ -0,0 +1,103 @@
+module NoBooleanCaseOf exposing (rule)
+
+{-|
+
+@docs rule
+
+-}
+
+import Elm.Syntax.Expression as Expression exposing (Expression)
+import Elm.Syntax.Node as Node exposing (Node)
+import Elm.Syntax.Pattern as Pattern exposing (Pattern)
+import Review.Rule as Rule exposing (Error, Rule)
+
+
+{-| Reports when pattern matching is used for a boolean value.
+
+The idiomatic way to check for a condition is to use an `if` expression.
+Read more about it at: <https://guide.elm-lang.org/core_language.html#if-expressions>
+
+    config =
+        [ NoBooleanCaseOf.rule
+        ]
+
+This won't report pattern matching when a boolean is part of the evaluated value.
+
+
+## Fail
+
+    _ =
+        case bool of
+            True ->
+                expression
+
+            False ->
+                otherExpression
+
+
+## Success
+
+    _ =
+        if bool then
+            expression
+
+        else
+            otherExpression
+
+    _ =
+        case ( bool, somethingElse ) of
+            ( True, SomeThingElse ) ->
+                expression
+
+            _ ->
+                otherExpression
+
+
+# When (not) to use this rule
+
+You should not use this rule if you do not care about how your boolean values are
+evaluated.
+
+-}
+rule : Rule
+rule =
+    Rule.newModuleRuleSchema "NoBooleanCaseOf" ()
+        |> Rule.withSimpleExpressionVisitor expressionVisitor
+        |> Rule.fromModuleRuleSchema
+
+
+error : Node a -> Error {}
+error node =
+    Rule.error
+        { message = "Replace `case..of` by an `if` condition"
+        , details =
+            [ "The idiomatic way to check for a condition is to use an `if` expression."
+            , "Read more about it at: https://guide.elm-lang.org/core_language.html#if-expressions"
+            ]
+        }
+        (Node.range node)
+
+
+expressionVisitor : Node Expression -> List (Error {})
+expressionVisitor node =
+    case Node.value node of
+        Expression.CaseExpression { expression, cases } ->
+            if List.any (Tuple.first >> isBoolConstructor) cases then
+                [ error expression ]
+
+            else
+                []
+
+        _ ->
+            []
+
+
+isBoolConstructor : Node Pattern -> Bool
+isBoolConstructor node =
+    case Node.value node of
+        Pattern.NamedPattern { moduleName, name } _ ->
+            (name == "True" || name == "False")
+                && (moduleName == [] || moduleName == [ "Basics" ])
+
+        _ ->
+            False
diff --git a/tests/NoBooleanCaseOfTest.elm b/tests/NoBooleanCaseOfTest.elm
new file mode 100644
index 00000000..bb72e029
--- /dev/null
+++ b/tests/NoBooleanCaseOfTest.elm
@@ -0,0 +1,111 @@
+module NoBooleanCaseOfTest exposing (all)
+
+import NoBooleanCaseOf exposing (rule)
+import Review.Test exposing (ReviewResult)
+import Test exposing (Test, describe, test)
+
+
+testRule : String -> ReviewResult
+testRule string =
+    "module A exposing (..)\n\n"
+        ++ string
+        |> Review.Test.run rule
+
+
+message : String
+message =
+    "Replace `case..of` by an `if` condition"
+
+
+details : List String
+details =
+    [ "The idiomatic way to check for a condition is to use an `if` expression."
+    , "Read more about it at: https://guide.elm-lang.org/core_language.html#if-expressions"
+    ]
+
+
+tests : List Test
+tests =
+    [ test "should not report pattern matches for non-boolean values" <|
+        \() ->
+            testRule """
+a = case thing of
+      Thing -> 1"""
+                |> Review.Test.expectNoErrors
+    , test "should not report pattern matches when the evaluated expression is a tuple of with a boolean" <|
+        \() ->
+            testRule """
+a = case ( bool1, bool2 ) of
+      ( True, True ) -> 1
+      _ -> 2"""
+                |> Review.Test.expectNoErrors
+    , test "should report pattern matches when one of the patterns is a bool constructor" <|
+        \() ->
+            testRule """
+a = case boolTrue of
+      True -> 1
+      _ -> 2
+
+b = case boolFalse of
+      False -> 1
+      _ -> 2
+
+c = case boolAll of
+      False -> 1
+      True -> 2"""
+                |> Review.Test.expectErrors
+                    [ Review.Test.error
+                        { message = message
+                        , details = details
+                        , under = "boolTrue"
+                        }
+                    , Review.Test.error
+                        { message = message
+                        , details = details
+                        , under = "boolFalse"
+                        }
+                    , Review.Test.error
+                        { message = message
+                        , details = details
+                        , under = "boolAll"
+                        }
+                    ]
+    , test "should report pattern matches for booleans even when one of the patterns starts with `Basics.`" <|
+        \() ->
+            testRule """
+a = case boolTrue of
+      Basics.True -> 1
+      _ -> 2
+
+b = case boolFalse of
+      Basics.False -> 1
+      _ -> 2"""
+                |> Review.Test.expectErrors
+                    [ Review.Test.error
+                        { message = message
+                        , details = details
+                        , under = "boolTrue"
+                        }
+                    , Review.Test.error
+                        { message = message
+                        , details = details
+                        , under = "boolFalse"
+                        }
+                    ]
+    , test "should report pattern matches for booleans even when the constructor seems to be for booleans but comes from an unknown module" <|
+        \() ->
+            testRule """
+a = case boolTrue of
+      OtherModule.True -> 1
+      _ -> 2
+
+b = case boolFalse of
+      OtherModule.False -> 1
+      _ -> 2"""
+                |> Review.Test.expectNoErrors
+    ]
+
+
+all : Test
+all =
+    describe "NoBooleanCaseOf" tests
diff --git a/tests/NoFullyAppliedPrefixOperator.elm b/tests/NoFullyAppliedPrefixOperator.elm
new file mode 100644
index 00000000..932a8fec
--- /dev/null
+++ b/tests/NoFullyAppliedPrefixOperator.elm
@@ -0,0 +1,63 @@
+module NoFullyAppliedPrefixOperator exposing (rule)
+
+{-|
+
+@docs rule
+
+-}
+
+import Elm.Syntax.Expression as Expression exposing (Expression)
+import Elm.Syntax.Node as Node exposing (Node)
+import Elm.Syntax.Range exposing (Range)
+import Review.Rule as Rule exposing (Error, Rule)
+
+
+{-| Reports when an operator is used as a prefix operator and all the operands are already given.
+
+    config =
+        [ NoFullyAppliedPrefixOperator.rule
+        ]
+
+
+## Fail
+
+    _ =
+        (+) 1 2
+
+
+## Success
+
+    _ =
+        1 + 2
+
+    _ =
+        (+) 1
+
+    _ =
+        (+)
+
+-}
+rule : Rule
+rule =
+    Rule.newModuleRuleSchema "NoFullyAppliedPrefixOperator" ()
+        |> Rule.withSimpleExpressionVisitor expressionVisitor
+        |> Rule.fromModuleRuleSchema
+
+
+error : Range -> Error {}
+error range =
+    Rule.error
+        { message = "Prefer using the infix form (`a + b`) over the prefix form (`(+) a b`) when possible"
+        , details = [ "The prefix form is generally harder to read over the infix form." ]
+        }
+        range
+
+
+expressionVisitor : Node Expression -> List (Error {})
+expressionVisitor node =
+    case Node.value node of
+        Expression.Application [ Node.Node range (Expression.PrefixOperator _), _, _ ] ->
+            [ error range ]
+
+        _ ->
+            []
diff --git a/tests/NoFullyAppliedPrefixOperatorTest.elm b/tests/NoFullyAppliedPrefixOperatorTest.elm
new file mode 100644
index 00000000..e473b798
--- /dev/null
+++ b/tests/NoFullyAppliedPrefixOperatorTest.elm
@@ -0,0 +1,96 @@
+module NoFullyAppliedPrefixOperatorTest exposing (all)
+
+import NoFullyAppliedPrefixOperator exposing (rule)
+import Review.Test exposing (ReviewResult)
+import Test exposing (Test, describe, test)
+
+
+message : String
+message =
+    "Prefer using the infix form (`a + b`) over the prefix form (`(+) a b`) when possible"
+
+
+details : List String
+details =
+    [ "The prefix form is generally harder to read over the infix form."
+    ]
+
+
+all : Test
+all =
+    describe "NoFullyAppliedPrefixOperator"
+        [ test "should not report a lonely operator" <|
+            \() ->
+                """
+module A exposing (..)
+a = (++)
+b = (::)
+c = (//)
+d = (+)
+e = (/)
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectNoErrors
+        , test "should not report an operator used in infix position" <|
+            \() ->
+                """
+module A exposing (..)
+a = y ++ z
+b = y :: z
+c = y // z
+d = y + z
+e = y / z
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectNoErrors
+        , test "should not report an operator used in prefix position with one argument" <|
+            \() ->
+                """
+module A exposing (..)
+a = (++) z
+b = (::) z
+c = (//) z
+d = (+) z
+e = (/) z
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectNoErrors
+        , test "should report an operator used in prefix position with both arguments" <|
+            \() ->
+                """
+module A exposing (..)
+a = (++) y z
+b = (::) y z
+c = (//) y z
+d = (+) y z
+e = (/) y z
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "(++)"
+                            }
+                        , Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "(::)"
+                            }
+                        , Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "(//)"
+                            }
+                        , Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "(+)"
+                            }
+                        , Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "(/)"
+                            }
+                        ]
+        ]
diff --git a/tests/NoListLiteralsConcat.elm b/tests/NoListLiteralsConcat.elm
new file mode 100644
index 00000000..f56080f4
--- /dev/null
+++ b/tests/NoListLiteralsConcat.elm
@@ -0,0 +1,116 @@
+module NoListLiteralsConcat exposing (rule)
+
+{-|
+
+@docs rule
+
+-}
+
+import Elm.Syntax.Expression as Expression exposing (Expression)
+import Elm.Syntax.Node as Node exposing (Node)
+import Elm.Syntax.Range exposing (Range)
+import Review.Rule as Rule exposing (Error, Rule)
+
+
+{-| Reports when an operation on lists could be simplified to a single literal list.
+
+    config =
+        [ NoListLiteralsConcat.rule
+        ]
+
+
+## Fail
+
+    _ =
+        [ 1, 2, 3 ] ++ [ 4, mysteryNumber, 6 ]
+
+    _ =
+        List.concat
+            [ [ 1, 2, 3 ]
+            , [ 4, mysteryNumber, 6 ]
+            ]
+
+    _ =
+        List.concat
+            [ [ 1, 2, 3 ]
+            ]
+
+    _ =
+        1 :: [ 2, 3 ]
+
+    _ =
+        [] ++ list
+
+    _ =
+        list ++ []
+
+
+## Success
+
+    _ =
+        [ 1, 2, 3, 4, mysteryNumber, 6 ]
+
+    _ =
+        [ 1, 2, 3 ] ++ list ++ [ 4, mysteryNumber, 6 ]
+
+    _ =
+        List.concat
+            [ [ 1, 2, 3 ]
+            , list
+            , [ 4, mysteryNumber, 6 ]
+            ]
+
+-}
+rule : Rule
+rule =
+    Rule.newModuleRuleSchema "NoListLiteralsConcat" ()
+        |> Rule.withSimpleExpressionVisitor expressionVisitor
+        |> Rule.fromModuleRuleSchema
+
+
+error : Range -> Error {}
+error range =
+    Rule.error
+        { message = "Expression could be simplified to be a single List"
+        , details = [ "Try moving all the elements into a single list." ]
+        }
+        range
+
+
+expressionVisitor : Node Expression -> List (Error {})
+expressionVisitor node =
+    case Node.value node of
+        Expression.OperatorApplication "++" _ (Node.Node _ (Expression.ListExpr _)) (Node.Node _ (Expression.ListExpr _)) ->
+            [ error (Node.range node) ]
+
+        Expression.OperatorApplication "++" _ (Node.Node range (Expression.ListExpr [])) _ ->
+            [ error range ]
+
+        Expression.OperatorApplication "++" _ _ (Node.Node range (Expression.ListExpr [])) ->
+            [ error range ]
+
+        Expression.OperatorApplication "::" _ _ (Node.Node _ (Expression.ListExpr _)) ->
+            [ error (Node.range node) ]
+
+        Expression.Application [ Node.Node _ (Expression.FunctionOrValue [ "List" ] "concat"), Node.Node _ (Expression.ListExpr list) ] ->
+            if List.length list < 2 then
+                [ error (Node.range node) ]
+
+            else if List.all isListLiteral list then
+                [ error (Node.range node) ]
+
+            else
+                []
+
+        _ ->
+            []
+
+
+isListLiteral : Node Expression -> Bool
+isListLiteral node =
+    case Node.value node of
+        Expression.ListExpr _ ->
+            True
+
+        _ ->
+            False
diff --git a/tests/NoListLiteralsConcatTest.elm b/tests/NoListLiteralsConcatTest.elm
new file mode 100644
index 00000000..872a23d9
--- /dev/null
+++ b/tests/NoListLiteralsConcatTest.elm
@@ -0,0 +1,154 @@
+module NoListLiteralsConcatTest exposing (all)
+
+import NoListLiteralsConcat exposing (rule)
+import Review.Test exposing (ReviewResult)
+import Test exposing (Test, describe, test)
+
+
+testRule : String -> ReviewResult
+testRule string =
+    "module A exposing (..)\n\n"
+        ++ string
+        |> Review.Test.run rule
+
+
+all : Test
+all =
+    describe "NoListLiteralsConcat"
+        [ usingPlusPlusTests
+        ]
+
+
+message : String
+message =
+    "Expression could be simplified to be a single List"
+
+
+details : List String
+details =
+    [ "Try moving all the elements into a single list." ]
+
+
+usingPlusPlusTests : Test
+usingPlusPlusTests =
+    describe "Using (++)"
+        [ test "should not report a single list literal" <|
+            \() ->
+                testRule """
+a = []
+b = [1]
+c = [ "string", "foo", "bar" ]
+"""
+                    |> Review.Test.expectNoErrors
+        , test "should report concatenating two list literals" <|
+            \() ->
+                testRule """
+a = [ 1 ] ++ [ 2, 3 ]
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "[ 1 ] ++ [ 2, 3 ]"
+                            }
+                        ]
+        , test "should report concatenating two list literals, even they contain variables" <|
+            \() ->
+                testRule """
+a = [ a, 1 ] ++ [ b, 2 ]
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "[ a, 1 ] ++ [ b, 2 ]"
+                            }
+                        ]
+        , test "should report concatenating an empty list and something" <|
+            \() ->
+                testRule """
+a = [] ++ something
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "[]"
+                            }
+                        ]
+        , test "should report concatenating something and an empty list" <|
+            \() ->
+                testRule """
+a = something ++ []
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "[]"
+                            }
+                        ]
+        , test "should not report using :: to a variable or expression" <|
+            \() ->
+                testRule """
+a = 1 :: list
+b = 1 :: foo bar
+"""
+                    |> Review.Test.expectNoErrors
+        , test "should report using :: to a list literal" <|
+            \() ->
+                testRule """
+a = 1 :: [ 2, 3]
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "1 :: [ 2, 3]"
+                            }
+                        ]
+        , test "should not report List.concat that contains a variable or expression" <|
+            \() ->
+                testRule """
+a = List.concat [ foo, bar ]
+b = List.concat [ [ 1 ], foo ]
+c = List.concat [ foo, [ 1 ] ]
+"""
+                    |> Review.Test.expectNoErrors
+        , test "should report List.concat with no items" <|
+            \() ->
+                testRule """
+a = List.concat []
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "List.concat []"
+                            }
+                        ]
+        , test "should report List.concat with a single item" <|
+            \() ->
+                testRule """
+a = List.concat [ a ]
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "List.concat [ a ]"
+                            }
+                        ]
+        , test "should report List.concat that only contains list literals" <|
+            \() ->
+                testRule """
+a = List.concat [ [ 1, 2, 3 ], [ 4, 5, 6] ]
+"""
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = message
+                            , details = details
+                            , under = "List.concat [ [ 1, 2, 3 ], [ 4, 5, 6] ]"
+                            }
+                        ]
+        ]