From 37a9b11d03db1f63770436c1d3c4d8d97e377ed7 Mon Sep 17 00:00:00 2001 From: Mats Stijlaart Date: Thu, 3 Aug 2017 23:08:23 +0200 Subject: [PATCH] Issue #97: Add duplicate record field update check --- docs/Docs/MsgDoc.elm | 25 +++++++++ elm-package.json | 1 + .../Checks/DuplicateRecordFieldUpdate.elm | 51 +++++++++++++++++++ src/Analyser/Messages/Json.elm | 13 +++++ src/Analyser/Messages/Types.elm | 1 + src/Analyser/Messages/Util.elm | 13 +++++ .../DuplicateRecordFieldUpdateTests.elm | 49 ++++++++++++++++++ tests/Tests.elm | 2 + tests/elm-package.json | 1 + 9 files changed, 156 insertions(+) create mode 100644 src/Analyser/Checks/DuplicateRecordFieldUpdate.elm create mode 100644 tests/Analyser/Checks/DuplicateRecordFieldUpdateTests.elm diff --git a/docs/Docs/MsgDoc.elm b/docs/Docs/MsgDoc.elm index e095bd8c..06452b3f 100644 --- a/docs/Docs/MsgDoc.elm +++ b/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" diff --git a/elm-package.json b/elm-package.json index 0d9daaf7..e9d6af9d 100644 --- a/elm-package.json +++ b/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", diff --git a/src/Analyser/Checks/DuplicateRecordFieldUpdate.elm b/src/Analyser/Checks/DuplicateRecordFieldUpdate.elm new file mode 100644 index 00000000..6eae2489 --- /dev/null +++ b/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 diff --git a/src/Analyser/Messages/Json.elm b/src/Analyser/Messages/Json.elm index 63119ee1..2d8cdea6 100644 --- a/src/Analyser/Messages/Json.elm +++ b/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 ) + ] diff --git a/src/Analyser/Messages/Types.elm b/src/Analyser/Messages/Types.elm index bf886db6..e6028a82 100644 --- a/src/Analyser/Messages/Types.elm +++ b/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 = diff --git a/src/Analyser/Messages/Util.elm b/src/Analyser/Messages/Util.elm index 8de22e5e..c685e64c 100644 --- a/src/Analyser/Messages/Util.elm +++ b/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 + ) diff --git a/tests/Analyser/Checks/DuplicateRecordFieldUpdateTests.elm b/tests/Analyser/Checks/DuplicateRecordFieldUpdateTests.elm new file mode 100644 index 00000000..ceb9fe94 --- /dev/null +++ b/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 + ] diff --git a/tests/Tests.elm b/tests/Tests.elm index a2ba72a3..ce9ec0ab 100644 --- a/tests/Tests.elm +++ b/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 diff --git a/tests/elm-package.json b/tests/elm-package.json index dc413560..99728364 100644 --- a/tests/elm-package.json +++ b/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",