Issue #97: Add duplicate record field update check

docs/Docs/MsgDoc.elm

@@ -20,6 +20,7 @@ import Analyser.Checks.UnnecessaryParens
 import Analyser.Checks.NoDebug
 import Analyser.Checks.DuplicateImport
 import Analyser.Checks.DuplicateImportedVariable
+import Analyser.Checks.DuplicateRecordFieldUpdate
 import Analyser.Checks.UnusedTypeAlias
 import Analyser.Checks.OverriddenVariables
 import Analyser.Checks.NoUncurriedPrefix
@@ -104,6 +105,7 @@ allMessages =
     , importAll
     , singleFieldRecord
     , lineLengthExceeded
+    , duplicateRecordFieldUpdate
     ]
 
 
@@ -114,6 +116,29 @@ forKey x =
         |> List.head
 
 
+duplicateRecordFieldUpdate : MsgDoc
+duplicateRecordFieldUpdate =
+    { name = "Duplicate Record Field Update"
+    , shortDescription = "You only want to update a field once in the record update syntax. Doing twice may only cause bugs."
+    , key = "DuplicateRecordFieldUpdate"
+    , arguments =
+        [ ( "file", FileName )
+        , ( "fieldName", VariableName )
+        , ( "ranges", RangeList )
+        ]
+    , example = Dynamic Analyser.Checks.DuplicateRecordFieldUpdate.checker
+    , input = """
+module Person exposing (Person, changeName)
+
+type alias Person = { name : String }
+
+changeName : Person -> Person
+changeName person =
+    { person | name = "John", name = "Jane" }
+"""
+    }
+
+
 lineLengthExceeded : MsgDoc
 lineLengthExceeded =
     { name = "Line Length Exceeded"

elm-package.json

@@ -11,6 +11,7 @@
     "dependencies": {
         "Fresheyeball/elm-tuple-extra": "3.0.0 <= v < 4.0.0",
         "danielnarey/elm-css-frameworks": "4.0.0 <= v < 5.0.0",
+        "elm-community/dict-extra": "2.2.0 <= v < 3.0.0",
         "elm-community/json-extra": "2.1.0 <= v < 3.0.0",
         "elm-community/list-extra": "6.0.0 <= v < 7.0.0",
         "elm-community/maybe-extra": "3.1.0 <= v < 4.0.0",

src/Analyser/Checks/DuplicateRecordFieldUpdate.elm

@@ -0,0 +1,51 @@
+module Analyser.Checks.DuplicateRecordFieldUpdate exposing (checker)
+
+import Elm.Syntax.Range as Syntax
+import Elm.Syntax.Expression exposing (RecordUpdate, Expression)
+import Analyser.Messages.Range as Range exposing (RangeContext)
+import Analyser.FileContext exposing (FileContext)
+import Analyser.Messages.Types exposing (Message, MessageData(DuplicateRecordFieldUpdate), newMessage)
+import ASTUtil.Inspector as Inspector exposing (Order(Post), defaultConfig)
+import Dict
+import Analyser.Configuration exposing (Configuration)
+import Analyser.Checks.Base exposing (Checker, keyBasedChecker)
+import Dict.Extra as Dict
+
+
+checker : Checker
+checker =
+    { check = scan
+    , shouldCheck = keyBasedChecker [ "DuplicateRecordFieldUpdate" ]
+    }
+
+
+type alias Context =
+    List ( String, List Syntax.Range )
+
+
+scan : RangeContext -> FileContext -> Configuration -> List Message
+scan rangeContext fileContext _ =
+    Inspector.inspect
+        { defaultConfig
+            | onRecordUpdate = Post onRecordUpdate
+        }
+        fileContext.ast
+        []
+        |> List.map (Tuple.mapSecond (List.map (Range.build rangeContext)))
+        |> List.map (uncurry (DuplicateRecordFieldUpdate fileContext.path))
+        |> List.map (newMessage [ ( fileContext.sha1, fileContext.path ) ])
+
+
+onRecordUpdate : RecordUpdate -> Context -> Context
+onRecordUpdate { updates } context =
+    updates
+        |> Dict.groupBy Tuple.first
+        |> Dict.filter (\_ v -> List.length v > 1)
+        |> Dict.map (\_ v -> List.map (Tuple.second >> expressionRange) v)
+        |> Dict.toList
+        |> (++) context
+
+
+expressionRange : Expression -> Syntax.Range
+expressionRange ( r, _ ) =
+    r

src/Analyser/Messages/Json.elm

@@ -141,6 +141,12 @@ decodeMessageData =
         , ( "CoreArrayUsage", decodeFileAndRange CoreArrayUsage )
         , ( "FunctionInLet", decodeFileAndRange FunctionInLet )
         , ( "SingleFieldRecord", decodeFileAndRange SingleFieldRecord )
+        , ( "DuplicateRecordFieldUpdate"
+          , JD.succeed DuplicateRecordFieldUpdate
+                |: fileField
+                |: JD.field "fieldName" JD.string
+                |: JD.field "ranges" (JD.list Range.decode)
+          )
         ]
 
 
@@ -414,3 +420,10 @@ encodeMessageData m =
                     [ ( "file", JE.string fileName )
                     , ( "range", Range.encode range )
                     ]
+
+        DuplicateRecordFieldUpdate fileName fieldName ranges ->
+            JE.object
+                [ ( "file", JE.string fileName )
+                , ( "fieldName", JE.string fieldName )
+                , ( "ranges", JE.list <| List.map Range.encode ranges )
+                ]

src/Analyser/Messages/Types.elm

@@ -73,6 +73,7 @@ type MessageData
     | CoreArrayUsage FileName Range
     | FunctionInLet FileName Range
     | SingleFieldRecord FileName Range
+    | DuplicateRecordFieldUpdate FileName String (List Range)
 
 
 type alias GetFiles =

src/Analyser/Messages/Util.elm

@@ -500,3 +500,16 @@ getMessageInfo m =
             , [ range ]
             , False
             )
+
+        Analyser.Messages.Types.DuplicateRecordFieldUpdate fileName fieldName ranges ->
+            ( String.concat
+                [ "The '"
+                , fieldName
+                , "' field for a record is updated multiple times in one expression in file "
+                , fileName
+                , "."
+                ]
+            , always [ fileName ]
+            , ranges
+            , False
+            )

tests/Analyser/Checks/DuplicateRecordFieldUpdateTests.elm

@@ -0,0 +1,49 @@
+module Analyser.Checks.DuplicateRecordFieldUpdateTests exposing (..)
+
+import Analyser.Checks.DuplicateRecordFieldUpdate as DuplicateRecordFieldUpdate
+import Analyser.Checks.CheckTestUtil as CTU
+import Analyser.Messages.Types exposing (..)
+import Analyser.Messages.Range as Range
+import Test exposing (..)
+
+
+duplicateUpdate : ( String, String, List MessageData )
+duplicateUpdate =
+    ( "duplicateUpdate"
+    , """module Foo exposing (..)
+
+
+foo = { bar | a = x, a = y}
+"""
+    , [ DuplicateRecordFieldUpdate "./foo.elm"
+            "a"
+            [ Range.manual
+                { start = { row = 3, column = 18 }, end = { row = 3, column = 19 } }
+                { start = { row = 3, column = 17 }, end = { row = 3, column = 18 } }
+            , Range.manual
+                { start = { row = 3, column = 25 }, end = { row = 3, column = 26 } }
+                { start = { row = 3, column = 24 }, end = { row = 3, column = 25 } }
+            ]
+      ]
+    )
+
+
+nonDuplicateUpdate : ( String, String, List MessageData )
+nonDuplicateUpdate =
+    ( "nonDuplicateUpdate"
+    , """module Foo exposing (..)
+
+
+foo = { bar | a = x }
+"""
+    , []
+    )
+
+
+all : Test
+all =
+    CTU.build "Analyser.Checks.ImportAllTests"
+        DuplicateRecordFieldUpdate.checker
+        [ duplicateUpdate
+        , nonDuplicateUpdate
+        ]

tests/Tests.elm

@@ -28,6 +28,7 @@ import Analyser.Checks.NonStaticRegexTests
 import Analyser.Checks.FunctionInLetTests
 import Analyser.Checks.DuplicateImportedVariableTests
 import Analyser.Checks.SingleFieldRecordTests
+import Analyser.Checks.DuplicateRecordFieldUpdateTests
 
 
 all : Test
@@ -53,6 +54,7 @@ all =
         , Analyser.Checks.ImportAllTests.all
         , Analyser.Checks.ExposeAllTests.all
         , Analyser.Checks.UnnecessaryListConcatTests.all
+        , Analyser.Checks.DuplicateRecordFieldUpdateTests.all
         , InterfaceTest.all
         , ASTUtil.PatternOptimizerTests.all
         , Analyser.Fixes.FileContentTests.all

tests/elm-package.json

@@ -10,6 +10,7 @@
     "exposed-modules": [],
     "dependencies": {
         "Fresheyeball/elm-tuple-extra": "3.0.0 <= v < 4.0.0",
+        "elm-community/dict-extra": "2.2.0 <= v < 3.0.0",
         "elm-community/elm-test": "3.0.0 <= v < 4.0.0",
         "elm-community/json-extra": "2.0.0 <= v < 3.0.0",
         "elm-community/list-extra": "6.0.0 <= v < 7.0.0",