Add rule NoExtraBooleanComparison

LintConfig.elm

@@ -2,10 +2,12 @@ module LintConfig exposing (config)
 
 import Lint.Rule exposing (Rule, Severity(..))
 import Lint.Rule.DefaultPatternPosition
+import Lint.Rule.ElmTest.NoDuplicateTestBodies
 import Lint.Rule.NoConstantCondition
 import Lint.Rule.NoDebug
 import Lint.Rule.NoDuplicateImports
 import Lint.Rule.NoExposingEverything
+import Lint.Rule.NoExtraBooleanComparison
 import Lint.Rule.NoImportingEverything
 import Lint.Rule.NoNestedLet
 import Lint.Rule.NoUnannotatedFunction
@@ -15,12 +17,12 @@ import Lint.Rule.NoUselessPatternMatching
 import Lint.Rule.NoWarningComments
 import Lint.Rule.SimplifyPiping
 import Lint.Rule.SimplifyPropertyAccess
-import Lint.Rule.ElmTest.NoDuplicateTestBodies
 
 
 config : List ( Severity, Rule )
 config =
     [ ( Critical, Lint.Rule.DefaultPatternPosition.rule { position = Lint.Rule.DefaultPatternPosition.Last } )
+    , ( Critical, Lint.Rule.NoExtraBooleanComparison.rule )
     , ( Critical, Lint.Rule.NoConstantCondition.rule )
     , ( Critical, Lint.Rule.NoDebug.rule )
     , ( Critical, Lint.Rule.NoDuplicateImports.rule )

README.md

@@ -26,6 +26,7 @@ These are the rules that are built-in and available for you to choose from.
 - **NoConstantCondition** - Forbid the use of expressions in an If condition whose value are always the same.
 - **NoDebug** - Forbid the use of `Debug` before it goes into production.
 - **NoDuplicateImports** - Forbid importing the same module several times in a file.
+- **NoExtraBooleanComparison** - Forbid extraneous comparisons with booleans, like `(isAdmin user) == True`.
 - **NoExposingEverything** - Forbid exporting everything in your modules `module Main exposing (..)`, to make your module explicit in what it exposes.
 - **NoImportingEverything** - Forbid importing everything from your module. This can especially be confusing to newcomers when the exposed functions and types are unknown to them.
 - **NoNestedLet** - Forbid nesting let expressions directly.

elm.json

@@ -7,10 +7,11 @@
     "exposed-modules": [
         "Lint",
         "Lint.Rule",
+        "Lint.Rule.DefaultPatternPosition",
         "Lint.Rule.NoDebug",
-        "Lint.Rule.NoUnusedVariables",
+        "Lint.Rule.NoExtraBooleanComparison",
         "Lint.Rule.NoImportingEverything",
-        "Lint.Rule.DefaultPatternPosition"
+        "Lint.Rule.NoUnusedVariables"
     ],
     "elm-version": "0.19.0 <= v < 0.20.0",
     "dependencies": {

example/Main.elm

@@ -9,6 +9,7 @@ import Html.Events exposing (onInput)
 import Lint exposing (Rule, Severity(..), lintSource)
 import Lint.Rule.DefaultPatternPosition
 import Lint.Rule.NoDebug
+import Lint.Rule.NoExtraBooleanComparison
 import Lint.Rule.NoImportingEverything
 import Lint.Rule.NoUnusedVariables
 import Lint.RuleError exposing (RuleError)
@@ -25,6 +26,7 @@ config =
     , ( Critical, Lint.Rule.NoUnusedVariables.rule )
     , ( Critical, Lint.Rule.NoImportingEverything.rule { exceptions = [ "Html" ] } )
     , ( Critical, Lint.Rule.DefaultPatternPosition.rule { position = Lint.Rule.DefaultPatternPosition.Last } )
+    , ( Critical, Lint.Rule.NoExtraBooleanComparison.rule )
 
     -- , ( Critical, Lint.Rule.NoConstantCondition.rule )
     -- , ( Critical, Lint.Rule.NoDuplicateImports.rule )

src/Lint/Rule/NoExtraBooleanComparison.elm

@@ -0,0 +1,101 @@
+module Lint.Rule.NoExtraBooleanComparison exposing (rule)
+
+{-|
+
+@docs rule
+
+
+# Fail
+
+    if Debug.log "condition" condition then
+        a
+
+    else
+        b
+
+    if condition then
+        Debug.crash "Nooo!"
+
+    else
+        value
+
+
+# Success
+
+    if condition then
+        a
+
+    else
+        b
+
+-}
+
+import Elm.Syntax.Expression exposing (Expression(..))
+import Elm.Syntax.Node as Node exposing (Node)
+import Lint exposing (Rule, lint)
+import Lint.Error as Error exposing (Error)
+import Lint.Rule as Rule
+
+
+type alias Context =
+    ()
+
+
+{-| Forbid the use of `Debug` before it goes into production.
+
+    rules =
+        [ NoExtraBooleanComparison.rule
+        ]
+
+-}
+rule : Rule
+rule =
+    Lint.createRule
+        "NoExtraBooleanComparison"
+        (lint implementation)
+
+
+implementation : Rule.Implementation Context
+implementation =
+    Rule.create ()
+        |> Rule.withExpressionVisitor expressionVisitor
+
+
+error : String -> Node a -> Error
+error comparedValue node =
+    Error.create
+        ("Unnecessary comparison with `" ++ comparedValue ++ "`")
+        (Node.range node)
+
+
+expressionVisitor : Context -> Rule.Direction -> Node Expression -> ( List Error, Context )
+expressionVisitor context direction node =
+    case ( direction, Node.value node ) of
+        ( Rule.Enter, Elm.Syntax.Expression.OperatorApplication operator _ left right ) ->
+            if isEqualityOperator operator then
+                ( List.filterMap isTrueOrFalse [ left, right ], context )
+
+            else
+                ( [], context )
+
+        _ ->
+            ( [], context )
+
+
+isEqualityOperator : String -> Bool
+isEqualityOperator operator =
+    operator == "==" || operator == "/="
+
+
+isTrueOrFalse : Node Expression -> Maybe Error
+isTrueOrFalse node =
+    case Node.value node of
+        FunctionOrValue [] functionOrValue ->
+            if functionOrValue == "True" || functionOrValue == "False" then
+                Just <| error functionOrValue node
+
+            else
+                Nothing
+
+        _ ->
+            Nothing

tests/NoExtraBooleanComparisonTest.elm

@@ -0,0 +1,83 @@
+module NoExtraBooleanComparisonTest exposing (all)
+
+import Elm.Syntax.Range exposing (Location, Range)
+import Lint exposing (Rule)
+import Lint.Error as Error exposing (Error)
+import Lint.Rule exposing (LintResult)
+import Lint.Rule.NoExtraBooleanComparison exposing (rule)
+import Test exposing (Test, describe, test)
+import TestUtil
+
+
+testRule : String -> LintResult
+testRule string =
+    "module A exposing (..)\n\n"
+        ++ string
+        |> TestUtil.ruleTester rule
+
+
+tests : List Test
+tests =
+    [ test "should not report condition without an operator" <|
+        \() ->
+            testRule "a = if n then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange []
+    , test "should not report condition with integer operators" <|
+        \() ->
+            testRule """
+a = if n < 1 then 1 else 2
+b = if n <= 1 then 1 else 2
+c = if n > 1 then 1 else 2
+d = if n >= 1 then 1 else 2
+"""
+                |> TestUtil.expectErrorsWithoutRange []
+    , test "should not report condition using `not`" <|
+        \() ->
+            testRule "a = if not n then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange []
+    , test "should report condition with `expr == True`" <|
+        \() ->
+            testRule "a = if b == True then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `True`" ]
+    , test "should report condition with `True == expr`" <|
+        \() ->
+            testRule "a = if True == b then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `True`" ]
+    , test "should report condition with `expr == False`" <|
+        \() ->
+            testRule "a = if b == False then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `False`" ]
+    , test "should report condition with `False == expr`" <|
+        \() ->
+            testRule "a = if False == b then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `False`" ]
+    , test "should report condition with `expr /= True`" <|
+        \() ->
+            testRule "a = if b /= True then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `True`" ]
+    , test "should report condition with `True /= expr`" <|
+        \() ->
+            testRule "a = if True /= b then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `True`" ]
+    , test "should report condition with `expr /= False`" <|
+        \() ->
+            testRule "a = if b /= False then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `False`" ]
+    , test "should report condition with `False /= expr`" <|
+        \() ->
+            testRule "a = if False /= b then 1 else 2"
+                |> TestUtil.expectErrorsWithoutRange
+                    [ TestUtil.errorWithoutRange "Unnecessary comparison with `False`" ]
+    ]
+
+
+all : Test
+all =
+    describe "NoExtraBooleanComparison" tests