From 932c788b9f506b5983523f0f4f517ac8c63c677b Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Wed, 3 Jun 2020 18:23:19 +0200 Subject: [PATCH] Backport work from review packages --- tests/NoExposingEverythingTest.elm | 14 +- tests/NoMissingSubscriptionsCall.elm | 205 ++++ tests/NoMissingSubscriptionsCallTest.elm | 102 ++ tests/NoMissingTypeAnnotationInLetIn.elm | 3 + tests/NoRecursiveUpdate.elm | 105 ++ tests/NoRecursiveUpdateTest.elm | 67 ++ tests/NoUnused/CustomTypeConstructors.elm | 2 +- tests/NoUnused/CustomTypeConstructorsTest.elm | 10 +- tests/NoUnused/Dependencies.elm | 7 +- tests/NoUnused/DependenciesTest.elm | 20 +- tests/NoUnused/Exports.elm | 159 ++- tests/NoUnused/ExportsTest.elm | 81 ++ tests/NoUnused/NonemptyList.elm | 2 +- tests/NoUnused/Parameters.elm | 520 ++++++++++ tests/NoUnused/ParametersTests.elm | 889 ++++++++++++++++ tests/NoUnused/Patterns.elm | 478 +++++++++ tests/NoUnused/Patterns/NameVisitor.elm | 215 ++++ tests/NoUnused/PatternsTests.elm | 960 ++++++++++++++++++ tests/NoUnused/Variables.elm | 53 +- tests/NoUnused/VariablesTest.elm | 8 +- tests/NoUselessSubscriptions.elm | 97 ++ tests/NoUselessSubscriptionsTest.elm | 75 ++ tests/Scope.elm | 13 +- 23 files changed, 4003 insertions(+), 82 deletions(-) create mode 100644 tests/NoMissingSubscriptionsCall.elm create mode 100644 tests/NoMissingSubscriptionsCallTest.elm create mode 100644 tests/NoRecursiveUpdate.elm create mode 100644 tests/NoRecursiveUpdateTest.elm create mode 100644 tests/NoUnused/Parameters.elm create mode 100644 tests/NoUnused/ParametersTests.elm create mode 100644 tests/NoUnused/Patterns.elm create mode 100644 tests/NoUnused/Patterns/NameVisitor.elm create mode 100644 tests/NoUnused/PatternsTests.elm create mode 100644 tests/NoUselessSubscriptions.elm create mode 100644 tests/NoUselessSubscriptionsTest.elm diff --git a/tests/NoExposingEverythingTest.elm b/tests/NoExposingEverythingTest.elm index f46cf2c0..c9fe909f 100644 --- a/tests/NoExposingEverythingTest.elm +++ b/tests/NoExposingEverythingTest.elm @@ -8,14 +8,21 @@ import Test exposing (Test, describe, test) all : Test all = describe "NoExposingEverything" - [ test "should not when a module exposes a limited set of things" <| + [ test "should not report anything when a module exposes a limited set of things" <| \_ -> - "module A exposing (B(..), C, d)" + """ +module A exposing (B(..), C, d) +type B = B +d = 1 +""" |> Review.Test.run rule |> Review.Test.expectNoErrors , test "should report when a module exposes everything" <| \_ -> - "module A exposing (..)" + """ +module A exposing (..) +import B exposing (..) +""" |> Review.Test.run rule |> Review.Test.expectErrors [ Review.Test.error @@ -25,5 +32,6 @@ all = ] , under = "(..)" } + |> Review.Test.atExactly { start = { row = 2, column = 19 }, end = { row = 2, column = 23 } } ] ] diff --git a/tests/NoMissingSubscriptionsCall.elm b/tests/NoMissingSubscriptionsCall.elm new file mode 100644 index 00000000..14797c2a --- /dev/null +++ b/tests/NoMissingSubscriptionsCall.elm @@ -0,0 +1,205 @@ +module NoMissingSubscriptionsCall exposing (rule) + +{-| + +@docs rule + +-} + +import Dict exposing (Dict) +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.ModuleName exposing (ModuleName) +import Elm.Syntax.Node as Node exposing (Node) +import Elm.Syntax.Range exposing (Range) +import Review.Rule as Rule exposing (Error, Rule) +import Scope +import Set exposing (Set) + + +{-| Reports likely missing calls to a `subscriptions` function. + + config = + [ NoMissingSubscriptionsCall.rule + ] + + +## Fail + + import SomeModule + + update msg model = + case msg of + UsedMsg subMsg -> + SomeModule.update subMsg model.used + + subscriptions model = + -- We used `SomeModule.update` but not `SomeModule.subscriptions` + Sub.none + +This won't fail if `SomeModule` does not define a `subscriptions` function. + + +## Success + + import SomeModule + + update msg model = + case msg of + UsedMsg subMsg -> + SomeModule.update subMsg model.used + + subscriptions model = + SomeModule.subscriptions + +-} +rule : Rule +rule = + Rule.newProjectRuleSchema "NoMissingSubscriptionsCall" initialProjectContext + |> Scope.addProjectVisitors + |> Rule.withModuleVisitor moduleVisitor + |> Rule.withModuleContext + { fromProjectToModule = fromProjectToModule + , fromModuleToProject = fromModuleToProject + , foldProjectContexts = foldProjectContexts + } + |> Rule.withContextFromImportedModules + |> Rule.fromProjectRuleSchema + + +moduleVisitor : Rule.ModuleRuleSchema schemaState ModuleContext -> Rule.ModuleRuleSchema { schemaState | hasAtLeastOneVisitor : () } ModuleContext +moduleVisitor schema = + schema + |> Rule.withDeclarationVisitor declarationVisitor + |> Rule.withExpressionVisitor expressionVisitor + |> Rule.withFinalModuleEvaluation finalEvaluation + + +type alias ProjectContext = + { scope : Scope.ProjectContext + , modulesThatExposeSubscriptionsAndUpdate : Set ModuleName + } + + +type alias ModuleContext = + { scope : Scope.ModuleContext + , modulesThatExposeSubscriptionsAndUpdate : Set ModuleName + + --, usesUpdate : Bool + --, usesSubscription : Bool + , definesUpdate : Bool + , definesSubscriptions : Bool + , usesUpdateOfModule : Dict ModuleName Range + , usesSubscriptionsOfModule : Set ModuleName + } + + +initialProjectContext : ProjectContext +initialProjectContext = + { scope = Scope.initialProjectContext + , modulesThatExposeSubscriptionsAndUpdate = Set.empty + } + + +fromProjectToModule : Rule.ModuleKey -> Node ModuleName -> ProjectContext -> ModuleContext +fromProjectToModule _ _ projectContext = + { scope = Scope.fromProjectToModule projectContext.scope + , modulesThatExposeSubscriptionsAndUpdate = projectContext.modulesThatExposeSubscriptionsAndUpdate + , definesUpdate = False + , definesSubscriptions = False + , usesUpdateOfModule = Dict.empty + , usesSubscriptionsOfModule = Set.empty + } + + +fromModuleToProject : Rule.ModuleKey -> Node ModuleName -> ModuleContext -> ProjectContext +fromModuleToProject _ moduleName moduleContext = + { scope = Scope.fromModuleToProject moduleName moduleContext.scope + , modulesThatExposeSubscriptionsAndUpdate = + if moduleContext.definesSubscriptions && moduleContext.definesUpdate then + Set.singleton (Node.value moduleName) + + else + Set.empty + } + + +foldProjectContexts : ProjectContext -> ProjectContext -> ProjectContext +foldProjectContexts newContext previousContext = + { scope = Scope.foldProjectContexts newContext.scope previousContext.scope + , modulesThatExposeSubscriptionsAndUpdate = + Set.union + newContext.modulesThatExposeSubscriptionsAndUpdate + previousContext.modulesThatExposeSubscriptionsAndUpdate + } + + +expressionVisitor : Node Expression -> Rule.Direction -> ModuleContext -> ( List (Error {}), ModuleContext ) +expressionVisitor node direction moduleContext = + case ( direction, Node.value node ) of + ( Rule.OnEnter, Expression.FunctionOrValue moduleName "update" ) -> + let + realModuleName : List String + realModuleName = + Scope.moduleNameForValue moduleContext.scope "update" moduleName + in + if Set.member realModuleName moduleContext.modulesThatExposeSubscriptionsAndUpdate then + ( [], { moduleContext | usesUpdateOfModule = Dict.insert realModuleName (Node.range node) moduleContext.usesUpdateOfModule } ) + + else + ( [], moduleContext ) + + ( Rule.OnEnter, Expression.FunctionOrValue moduleName "subscriptions" ) -> + let + realModuleName : List String + realModuleName = + Scope.moduleNameForValue moduleContext.scope "subscriptions" moduleName + in + if Set.member realModuleName moduleContext.modulesThatExposeSubscriptionsAndUpdate then + ( [], { moduleContext | usesSubscriptionsOfModule = Set.insert realModuleName moduleContext.usesSubscriptionsOfModule } ) + + else + ( [], moduleContext ) + + _ -> + ( [], moduleContext ) + + +declarationVisitor : Node Declaration -> Rule.Direction -> ModuleContext -> ( List (Error nothing), ModuleContext ) +declarationVisitor declaration direction moduleContext = + case ( direction, Node.value declaration ) of + ( Rule.OnEnter, Declaration.FunctionDeclaration function ) -> + case + function.declaration + |> Node.value + |> .name + |> Node.value + of + "update" -> + ( [], { moduleContext | definesUpdate = True } ) + + "subscriptions" -> + ( [], { moduleContext | definesSubscriptions = True } ) + + _ -> + ( [], moduleContext ) + + _ -> + ( [], moduleContext ) + + +finalEvaluation : ModuleContext -> List (Error {}) +finalEvaluation moduleContext = + moduleContext.usesUpdateOfModule + |> Dict.filter (\moduleName _ -> not <| Set.member moduleName moduleContext.usesSubscriptionsOfModule) + |> Dict.toList + |> List.map + (\( moduleName, range ) -> + Rule.error + { message = "Missing subscriptions call to " ++ String.join "." moduleName ++ ".subscriptions" + , details = + [ "The " ++ String.join "." moduleName ++ " module defines a `subscriptions` function, which you are not using even though you are using its `update` function. This makes me think that you are not subscribing to all the things you should." + ] + } + range + ) diff --git a/tests/NoMissingSubscriptionsCallTest.elm b/tests/NoMissingSubscriptionsCallTest.elm new file mode 100644 index 00000000..a1151e78 --- /dev/null +++ b/tests/NoMissingSubscriptionsCallTest.elm @@ -0,0 +1,102 @@ +module NoMissingSubscriptionsCallTest exposing (all) + +import NoMissingSubscriptionsCall exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "NoMissingSubscriptionsCall" + [ test "should report when a module defines when a module exposes a limited set of things" <| + \_ -> + [ """ +module Main exposing (main) +import Imported + +main = Browser.element + { init = init + , update = update + , subscriptions = subscriptions + , view = view + } + +update msg model = + case msg of + UsedMsg subMsg -> + Imported.update subMsg model.used + +subscriptions model = + Sub.none +""", """ +module Imported exposing (update, subscriptions) +update = Debug.todo "" +subscriptions = Debug.todo "" +""" ] + |> Review.Test.runOnModules rule + |> Review.Test.expectErrorsForModules + [ ( "Main" + , [ Review.Test.error + { message = "Missing subscriptions call to Imported.subscriptions" + , details = + [ "The Imported module defines a `subscriptions` function, which you are not using even though you are using its `update` function. This makes me think that you are not subscribing to all the things you should." + ] + , under = "Imported.update" + } + ] + ) + ] + , test "should not report anything when the imported module's subscriptions function is used" <| + \_ -> + [ """ +module Main exposing (main) +import Imported + +main = Browser.element + { init = init + , update = update + , subscriptions = subscriptions + , view = view + } + +update msg model = + case msg of + UsedMsg subMsg -> + Imported.update subMsg model.used + +subscriptions model = + Imported.subscriptions +""", """ +module Imported exposing (update, subscriptions) +update = Debug.todo "" +subscriptions = Debug.todo "" +""" ] + |> Review.Test.runOnModules rule + |> Review.Test.expectNoErrors + , test "should not report anything when the imported module does not define a subscriptions function" <| + \_ -> + [ """ +module Main exposing (main) +import Imported + +main = Browser.element + { init = init + , update = update + , subscriptions = subscriptions + , view = view + } + +update msg model = + case msg of + UsedMsg subMsg -> + Imported.update subMsg model.used + +subscriptions model = + Sub.none +""", """ +module Imported exposing (update) +update = Debug.todo "" +""" ] + |> Review.Test.runOnModules rule + |> Review.Test.expectNoErrors + ] diff --git a/tests/NoMissingTypeAnnotationInLetIn.elm b/tests/NoMissingTypeAnnotationInLetIn.elm index c870e58c..57abdb38 100644 --- a/tests/NoMissingTypeAnnotationInLetIn.elm +++ b/tests/NoMissingTypeAnnotationInLetIn.elm @@ -28,6 +28,7 @@ For that, enable [`NoMissingTypeAnnotation`](./NoMissingTypeAnnotation). a : number a = let + -- Missing annotation b = 2 in @@ -36,6 +37,8 @@ For that, enable [`NoMissingTypeAnnotation`](./NoMissingTypeAnnotation). ## Success + -- Top-level annotation is not necessary, but good to have! + a : number a = let b : number diff --git a/tests/NoRecursiveUpdate.elm b/tests/NoRecursiveUpdate.elm new file mode 100644 index 00000000..f446107b --- /dev/null +++ b/tests/NoRecursiveUpdate.elm @@ -0,0 +1,105 @@ +module NoRecursiveUpdate exposing (rule) + +{-| + +@docs rule + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Node as Node exposing (Node) +import Review.Rule as Rule exposing (Error, Rule) + + +{-| Reports when the `update` function calls itself. + +This is often done in order to have one message (A) trigger (all or some of) the same +model updates and commands as another message (B). + + update msg model = + case msg of + Foo -> + { model | foo = True } + + Bar -> + update Foo { model | bar = True } + +This is advised against, because if the way that message B is handled changes, +that will implicitly change how message A is handled in ways that may not have +been foreseen. + +A better solution is to move the common handling into a different function and +have it called in the handling of both messages. + + update msg model = + case msg of + Foo -> + commonOperationOnModel model + + Bar -> + commonOperationOnModel { model | bar = True } + + commonOperationOnModel model = + { model | foo = True } + +Calls to other modules' `update` function are allowed. + +To add the rule to your configuration: + + config = + [ NoRecursiveUpdate.rule + ] + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoRecursiveUpdate" { isInUpdateFunction = False } + |> Rule.withDeclarationVisitor declarationVisitor + |> Rule.withExpressionVisitor expressionVisitor + |> Rule.fromModuleRuleSchema + + +type alias Context = + { isInUpdateFunction : Bool + } + + +declarationVisitor : Node Declaration -> Rule.Direction -> Context -> ( List nothing, Context ) +declarationVisitor declaration direction _ = + case ( direction, Node.value declaration ) of + ( Rule.OnEnter, Declaration.FunctionDeclaration function ) -> + ( [] + , { isInUpdateFunction = + (function.declaration + |> Node.value + |> .name + |> Node.value + ) + == "update" + } + ) + + _ -> + ( [], { isInUpdateFunction = False } ) + + +expressionVisitor : Node Expression -> Rule.Direction -> Context -> ( List (Error {}), Context ) +expressionVisitor node direction context = + if context.isInUpdateFunction then + case ( direction, Node.value node ) of + ( Rule.OnEnter, Expression.FunctionOrValue [] "update" ) -> + ( [ Rule.error + { message = "`update` shouldn't call itself" + , details = [ "If you wish to have the same behavior for different messages, move that behavior into a new function and call have it called in the handling of both messages." ] + } + (Node.range node) + ] + , context + ) + + _ -> + ( [], context ) + + else + ( [], context ) diff --git a/tests/NoRecursiveUpdateTest.elm b/tests/NoRecursiveUpdateTest.elm new file mode 100644 index 00000000..40c46439 --- /dev/null +++ b/tests/NoRecursiveUpdateTest.elm @@ -0,0 +1,67 @@ +module NoRecursiveUpdateTest exposing (all) + +import NoRecursiveUpdate exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "NoRecursiveUpdate" + [ test "should report when an update function calls itself" <| + \_ -> + """ +module A exposing (..) +update msg model = + case msg of + Foo -> + { model | foo = True } + Bar -> + update Foo { model | bar = True } +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "`update` shouldn't call itself" + , details = [ "If you wish to have the same behavior for different messages, move that behavior into a new function and call have it called in the handling of both messages." ] + , under = "update" + } + |> Review.Test.atExactly { start = { row = 8, column = 7 }, end = { row = 8, column = 13 } } + ] + , test "should not report other recursive functions" <| + \_ -> + """ +module A exposing (..) +recursive list = + case list of + [] -> Nothing + _ :: xs -> recursive xs +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report calls to update functions from other modules" <| + \_ -> + """ +module A exposing (..) +update msg model = + case msg of + Bar subMsg -> + Bar.update subMsg model.bar +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report references to update functions outside the update functions" <| + \_ -> + """ +module A exposing (..) + +main = Browser.sandbox { init = init, update = update, view = view } + +update msg model = + case msg of + Bar -> + 1 +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] diff --git a/tests/NoUnused/CustomTypeConstructors.elm b/tests/NoUnused/CustomTypeConstructors.elm index 1ddc5f08..81182a79 100644 --- a/tests/NoUnused/CustomTypeConstructors.elm +++ b/tests/NoUnused/CustomTypeConstructors.elm @@ -319,7 +319,7 @@ moduleDefinitionVisitor moduleNode context = List.filterMap (\node -> case Node.value node of - Exposing.TypeExpose { name, open } -> + Exposing.TypeExpose { name } -> Just name _ -> diff --git a/tests/NoUnused/CustomTypeConstructorsTest.elm b/tests/NoUnused/CustomTypeConstructorsTest.elm index 083b804e..bea063ea 100644 --- a/tests/NoUnused/CustomTypeConstructorsTest.elm +++ b/tests/NoUnused/CustomTypeConstructorsTest.elm @@ -142,15 +142,15 @@ type Foo = Bar | Baz""" |> Review.Test.runWithProjectData project (rule []) |> Review.Test.expectErrors [ Review.Test.error - { message = "Type constructor `Baz` is not used." - , details = details - , under = "Baz" - } - , Review.Test.error { message = "Type constructor `Bar` is not used." , details = details , under = "Bar" } + , Review.Test.error + { message = "Type constructor `Baz` is not used." + , details = details + , under = "Baz" + } ] ] diff --git a/tests/NoUnused/Dependencies.elm b/tests/NoUnused/Dependencies.elm index ad39357d..81e509e8 100644 --- a/tests/NoUnused/Dependencies.elm +++ b/tests/NoUnused/Dependencies.elm @@ -98,7 +98,7 @@ fromProjectToModule _ _ projectContext = fromModuleToProject : Rule.ModuleKey -> Node ModuleName -> ModuleContext -> ProjectContext -fromModuleToProject moduleKey moduleName importedModuleNames = +fromModuleToProject _ _ importedModuleNames = { moduleNameToDependency = Dict.empty , directProjectDependencies = Set.empty , importedModuleNames = importedModuleNames @@ -194,7 +194,10 @@ error elmJsonKey packageName = Rule.errorForElmJson elmJsonKey (\elmJson -> { message = "Unused dependency `" ++ packageName ++ "`" - , details = [ "To remove it, I recommend installing `elm-json` and running `elm-json uninstall " ++ packageName ++ "`" ] + , details = + [ "To remove it, I recommend running the following command:" + , " elm-json uninstall " ++ packageName + ] , range = findPackageNameInElmJson packageName elmJson } ) diff --git a/tests/NoUnused/DependenciesTest.elm b/tests/NoUnused/DependenciesTest.elm index ccf8c115..a50080d2 100644 --- a/tests/NoUnused/DependenciesTest.elm +++ b/tests/NoUnused/DependenciesTest.elm @@ -176,12 +176,18 @@ a = 1 |> Review.Test.expectErrorsForElmJson [ Review.Test.error { message = "Unused dependency `author/package-with-bar`" - , details = [ "To remove it, I recommend installing `elm-json` and running `elm-json uninstall author/package-with-bar`" ] + , details = + [ "To remove it, I recommend running the following command:" + , " elm-json uninstall author/package-with-bar" + ] , under = "author/package-with-bar" } , Review.Test.error { message = "Unused dependency `author/package-with-foo`" - , details = [ "To remove it, I recommend installing `elm-json` and running `elm-json uninstall author/package-with-foo`" ] + , details = + [ "To remove it, I recommend running the following command:" + , " elm-json uninstall author/package-with-foo" + ] , under = "author/package-with-foo" } ] @@ -205,12 +211,18 @@ a = 1 |> Review.Test.expectErrorsForElmJson [ Review.Test.error { message = "Unused dependency `author/package-with-bar`" - , details = [ "To remove it, I recommend installing `elm-json` and running `elm-json uninstall author/package-with-bar`" ] + , details = + [ "To remove it, I recommend running the following command:" + , " elm-json uninstall author/package-with-bar" + ] , under = "author/package-with-bar" } , Review.Test.error { message = "Unused dependency `author/package-with-foo`" - , details = [ "To remove it, I recommend installing `elm-json` and running `elm-json uninstall author/package-with-foo`" ] + , details = + [ "To remove it, I recommend running the following command:" + , " elm-json uninstall author/package-with-foo" + ] , under = "author/package-with-foo" } ] diff --git a/tests/NoUnused/Exports.elm b/tests/NoUnused/Exports.elm index 08692d18..8795a7bd 100644 --- a/tests/NoUnused/Exports.elm +++ b/tests/NoUnused/Exports.elm @@ -18,11 +18,13 @@ import Elm.Project import Elm.Syntax.Declaration as Declaration exposing (Declaration) import Elm.Syntax.Exposing as Exposing import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Import exposing (Import) import Elm.Syntax.Module as Module exposing (Module) import Elm.Syntax.ModuleName exposing (ModuleName) import Elm.Syntax.Node as Node exposing (Node(..)) import Elm.Syntax.Range exposing (Range) import Elm.Syntax.TypeAnnotation as TypeAnnotation exposing (TypeAnnotation) +import Review.Fix as Fix exposing (Fix) import Review.Rule as Rule exposing (Error, Rule) import Scope import Set exposing (Set) @@ -59,6 +61,7 @@ moduleVisitor : Rule.ModuleRuleSchema {} ModuleContext -> Rule.ModuleRuleSchema moduleVisitor schema = schema |> Rule.withModuleDefinitionVisitor moduleDefinitionVisitor + |> Rule.withImportVisitor importVisitor |> Rule.withDeclarationListVisitor declarationListVisitor |> Rule.withExpressionVisitor expressionVisitor @@ -73,18 +76,25 @@ type alias ProjectContext = , modules : Dict ModuleName { moduleKey : Rule.ModuleKey - , exposed : Dict String { range : Range, exposedElement : ExposedElement } + , exposed : Dict String ExposedElement } , used : Set ( ModuleName, String ) } +type alias ExposedElement = + { range : Range + , rangeToRemove : Maybe Range + , elementType : ExposedElementType + } + + type ProjectType = IsApplication | IsPackage (Set (List String)) -type ExposedElement +type ExposedElementType = Function | TypeOrTypeAlias | ExposedType @@ -93,7 +103,7 @@ type ExposedElement type alias ModuleContext = { scope : Scope.ModuleContext , exposesEverything : Bool - , exposed : Dict String { range : Range, exposedElement : ExposedElement } + , exposed : Dict String ExposedElement , used : Set ( ModuleName, String ) , elementsNotToReport : Set String } @@ -109,7 +119,7 @@ initialProjectContext = fromProjectToModule : Rule.ModuleKey -> Node ModuleName -> ProjectContext -> ModuleContext -fromProjectToModule moduleKey moduleName projectContext = +fromProjectToModule _ _ projectContext = { scope = Scope.fromProjectToModule projectContext.scope , exposesEverything = False , exposed = Dict.empty @@ -198,16 +208,16 @@ finalEvaluationForProject projectContext = |> List.concatMap (\( moduleName, { moduleKey, exposed } ) -> exposed - |> removeApplicationExceptions projectContext moduleName + |> removeApplicationExceptions projectContext |> removeReviewConfig moduleName |> Dict.filter (\name _ -> not <| Set.member ( moduleName, name ) projectContext.used) |> Dict.toList |> List.concatMap - (\( name, { range, exposedElement } ) -> + (\( name, element ) -> let what : String what = - case exposedElement of + case element.elementType of Function -> "Exposed function or value" @@ -216,12 +226,22 @@ finalEvaluationForProject projectContext = ExposedType -> "Exposed type" + + fixes : List Fix + fixes = + case element.rangeToRemove of + Just rangeToRemove -> + [ Fix.removeRange rangeToRemove ] + + Nothing -> + [] in - [ Rule.errorForModule moduleKey + [ Rule.errorForModuleWithFix moduleKey { message = what ++ " `" ++ name ++ "` is never used outside this module." , details = [ "This exposed element is never used. You may want to remove it to keep your project clean, and maybe detect some unused code in your project." ] } - range + element.range + fixes ] ) ) @@ -237,8 +257,8 @@ removeExposedPackages projectContext dict = Dict.filter (\name _ -> not <| Set.member name exposedModuleNames) dict -removeApplicationExceptions : ProjectContext -> ModuleName -> Dict String a -> Dict String a -removeApplicationExceptions projectContext moduleName dict = +removeApplicationExceptions : ProjectContext -> Dict String a -> Dict String a +removeApplicationExceptions projectContext dict = case projectContext.projectType of IsApplication -> Dict.remove "main" dict @@ -267,27 +287,81 @@ moduleDefinitionVisitor moduleNode moduleContext = ( [], { moduleContext | exposesEverything = True } ) Exposing.Explicit list -> - ( [], { moduleContext | exposed = exposedElements list } ) + ( [], { moduleContext | exposed = collectExposedElements list } ) -exposedElements : List (Node Exposing.TopLevelExpose) -> Dict String { range : Range, exposedElement : ExposedElement } -exposedElements nodes = +collectExposedElements : List (Node Exposing.TopLevelExpose) -> Dict String ExposedElement +collectExposedElements nodes = + let + listWithPreviousRange : List (Maybe Range) + listWithPreviousRange = + Nothing + :: (nodes + |> List.map (Node.range >> Just) + |> List.take (List.length nodes - 1) + ) + + listWithNextRange : List Range + listWithNextRange = + (nodes + |> List.map Node.range + |> List.drop 1 + ) + ++ [ { start = { row = 0, column = 0 }, end = { row = 0, column = 0 } } ] + in nodes - |> List.filterMap - (\node -> - case Node.value node of + |> List.map3 (\prev next current -> ( prev, current, next )) listWithPreviousRange listWithNextRange + |> List.indexedMap + (\index ( maybePreviousRange, Node range value, nextRange ) -> + let + rangeToRemove : Maybe Range + rangeToRemove = + if List.length nodes == 1 then + Nothing + + else if index == 0 then + Just { range | end = nextRange.start } + + else + case maybePreviousRange of + Nothing -> + Just range + + Just previousRange -> + Just { range | start = previousRange.end } + in + case value of Exposing.FunctionExpose name -> - Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = Function } ) + Just + ( name + , { range = untilEndOfVariable name range + , rangeToRemove = rangeToRemove + , elementType = Function + } + ) Exposing.TypeOrAliasExpose name -> - Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = TypeOrTypeAlias } ) + Just + ( name + , { range = untilEndOfVariable name range + , rangeToRemove = rangeToRemove + , elementType = TypeOrTypeAlias + } + ) Exposing.TypeExpose { name } -> - Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = ExposedType } ) + Just + ( name + , { range = untilEndOfVariable name range + , rangeToRemove = Nothing + , elementType = ExposedType + } + ) Exposing.InfixExpose _ -> Nothing ) + |> List.filterMap identity |> Dict.fromList @@ -301,6 +375,49 @@ untilEndOfVariable name range = +-- IMPORT VISITOR + + +importVisitor : Node Import -> ModuleContext -> ( List nothing, ModuleContext ) +importVisitor node moduleContext = + case (Node.value node).exposingList |> Maybe.map Node.value of + Just (Exposing.Explicit list) -> + let + moduleName : ModuleName + moduleName = + Node.value (Node.value node).moduleName + + usedElements : List ( ModuleName, String ) + usedElements = + list + |> List.filterMap + (Node.value + >> (\element -> + case element of + Exposing.FunctionExpose name -> + Just ( moduleName, name ) + + Exposing.TypeOrAliasExpose name -> + Just ( moduleName, name ) + + Exposing.TypeExpose { name } -> + Just ( moduleName, name ) + + Exposing.InfixExpose _ -> + Nothing + ) + ) + in + ( [], registerMultipleAsUsed usedElements moduleContext ) + + Just (Exposing.All _) -> + ( [], moduleContext ) + + Nothing -> + ( [], moduleContext ) + + + -- DECLARATION LIST VISITOR @@ -436,7 +553,7 @@ typesUsedInDeclaration moduleContext declaration = |> List.concatMap (Node.value >> .arguments) |> List.concatMap (collectTypesFromTypeAnnotation moduleContext.scope) , not <| - case Dict.get (Node.value type_.name) moduleContext.exposed |> Maybe.map .exposedElement of + case Dict.get (Node.value type_.name) moduleContext.exposed |> Maybe.map .elementType of Just ExposedType -> True diff --git a/tests/NoUnused/ExportsTest.elm b/tests/NoUnused/ExportsTest.elm index 3a9385f5..bf37f456 100644 --- a/tests/NoUnused/ExportsTest.elm +++ b/tests/NoUnused/ExportsTest.elm @@ -96,6 +96,7 @@ all = , typesTests , typeAliasesTests , duplicateModuleNameTests + , importsTests -- TODO Add tests that report exposing the type's variants if they are never used. ] @@ -183,6 +184,38 @@ main = exposed } |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 27 } } ] + , test "should propose a fix for unused exports if there are others exposed elements" <| + \() -> + """ +module A exposing (exposed1, exposed2) +exposed1 = 1 +exposed2 = 2 +""" + |> Review.Test.runWithProjectData package_ rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Exposed function or value `exposed1` is never used outside this module." + , details = details + , under = "exposed1" + } + |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 28 } } + |> Review.Test.whenFixed """ +module A exposing (exposed2) +exposed1 = 1 +exposed2 = 2 +""" + , Review.Test.error + { message = "Exposed function or value `exposed2` is never used outside this module." + , details = details + , under = "exposed2" + } + |> Review.Test.atExactly { start = { row = 2, column = 30 }, end = { row = 2, column = 38 } } + |> Review.Test.whenFixed """ +module A exposing (exposed1) +exposed1 = 1 +exposed2 = 2 +""" + ] , test "should not report anything for modules that expose everything`" <| \() -> """ @@ -391,6 +424,11 @@ type alias B = A.OtherType , under = "MyType" } |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 26 } } + |> Review.Test.whenFixed """ +module A exposing (OtherType) +type MyType = VariantA | VariantB +type OtherType = Thing MyType +""" ] ) ] @@ -427,6 +465,11 @@ type alias B = A.OtherType , under = "MyType" } |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 26 } } + |> Review.Test.whenFixed """ +module A exposing (OtherType) +type MyType = VariantA | VariantB +type OtherType = OtherThing | SomeThing ((), List MyType) +""" ] ) ] @@ -566,6 +609,11 @@ type alias B = A.OtherType , under = "MyType" } |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 26 } } + |> Review.Test.whenFixed """ +module A exposing (OtherType) +type alias MyType = {} +type OtherType = OtherType MyType +""" ] ) ] @@ -602,6 +650,11 @@ type alias B = A.OtherType , under = "MyType" } |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 26 } } + |> Review.Test.whenFixed """ +module A exposing (OtherType) +type alias MyType = {} +type OtherType = OtherThing | SomeThing ((), List MyType) +""" ] ) ] @@ -630,6 +683,15 @@ a = A.Card A.init A.toElement , under = "Link" } |> Review.Test.atExactly { start = { row = 3, column = 7 }, end = { row = 3, column = 11 } } + |> Review.Test.whenFixed """module A + exposing ( Card + , init + , toElement + ) +type Card = Card +type Link = Link +init = 1 +""" ] ) ] @@ -653,3 +715,22 @@ b = 2 |> Review.Test.runOnModulesWithProjectData application rule |> Review.Test.expectNoErrors ] + + +importsTests : Test +importsTests = + describe "Imports" + [ test "should not report an export if it is imported by name" <| + \() -> + [ """module Main exposing (main) +import B exposing (Type1, Type2(..), TypeAlias, b) +main = 1 +""", """module B exposing (Type1, Type2(..), TypeAlias, b) +type Type1 = Type1 +type Type2 = Type2 +type alias TypeAlias = {} +b = 2 +""" ] + |> Review.Test.runOnModulesWithProjectData application rule + |> Review.Test.expectNoErrors + ] diff --git a/tests/NoUnused/NonemptyList.elm b/tests/NoUnused/NonemptyList.elm index 69525412..249ea93f 100644 --- a/tests/NoUnused/NonemptyList.elm +++ b/tests/NoUnused/NonemptyList.elm @@ -92,7 +92,7 @@ fromElement x = {-| Return the head of the list. -} head : Nonempty a -> a -head (Nonempty x xs) = +head (Nonempty x _) = x diff --git a/tests/NoUnused/Parameters.elm b/tests/NoUnused/Parameters.elm new file mode 100644 index 00000000..7ba8a27c --- /dev/null +++ b/tests/NoUnused/Parameters.elm @@ -0,0 +1,520 @@ +module NoUnused.Parameters exposing (rule) + +{-| Report parameters that are not used. + + +# Rule + +@docs rule + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.ModuleName exposing (ModuleName) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Pattern as Pattern exposing (Pattern) +import Elm.Syntax.Range as Range exposing (Range) +import Elm.Writer as Writer +import NoUnused.Patterns.NameVisitor as NameVisitor +import Review.Fix as Fix +import Review.Rule as Rule exposing (Rule) +import Set exposing (Set) + + +{-| Report parameters that are not used. + + config = + [ NoUnused.Parameters.rule + ] + +This rule looks within function arguments, let functions and lambdas to find any values that are unused. It will report any parameters that are not used. + + +## Fixes for lambdas + +We're only offering fixes for lambdas here because we believe unused parameters in functions are a code smell that should be refactored. + + +## Fail + +Value `something` is not used: + + add1 number = + 1 + + +## Success + + add1 number = + number + 1 + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoUnused.Parameters" initialContext + |> Rule.withDeclarationVisitor declarationVisitor + |> Rule.withExpressionVisitor expressionVisitor + |> NameVisitor.withValueVisitor valueVisitor + |> Rule.fromModuleRuleSchema + + +declarationVisitor : Node Declaration -> Rule.Direction -> Context -> ( List (Rule.Error {}), Context ) +declarationVisitor node direction context = + case ( direction, Node.value node ) of + ( Rule.OnEnter, Declaration.FunctionDeclaration { declaration } ) -> + ( [], rememberFunctionImplementation declaration context ) + + ( Rule.OnExit, Declaration.FunctionDeclaration { declaration } ) -> + errorsForFunctionImplementation declaration context + + _ -> + ( [], context ) + + +expressionVisitor : Node Expression -> Rule.Direction -> Context -> ( List (Rule.Error {}), Context ) +expressionVisitor (Node _ expression) direction context = + case ( direction, expression ) of + ( Rule.OnEnter, Expression.LambdaExpression { args } ) -> + ( [], rememberPatternList args context ) + + ( Rule.OnExit, Expression.LambdaExpression { args } ) -> + errorsForPatternList Lambda args context + + ( Rule.OnEnter, Expression.LetExpression { declarations } ) -> + ( [], rememberLetDeclarationList declarations context ) + + ( Rule.OnExit, Expression.LetExpression { declarations } ) -> + errorsForLetDeclarationList declarations context + + _ -> + ( [], context ) + + +valueVisitor : Node ( ModuleName, String ) -> Context -> ( List (Rule.Error {}), Context ) +valueVisitor (Node _ ( moduleName, value )) context = + case moduleName of + [] -> + ( [], useValue value context ) + + _ -> + ( [], context ) + + + +--- ON ENTER + + +rememberFunctionImplementation : Node Expression.FunctionImplementation -> Context -> Context +rememberFunctionImplementation (Node _ { arguments }) context = + rememberPatternList arguments context + + +rememberLetDeclarationList : List (Node Expression.LetDeclaration) -> Context -> Context +rememberLetDeclarationList list context = + List.foldl rememberLetDeclaration context list + + +rememberLetDeclaration : Node Expression.LetDeclaration -> Context -> Context +rememberLetDeclaration (Node _ letDeclaration) context = + case letDeclaration of + Expression.LetFunction { declaration } -> + rememberLetFunctionImplementation declaration context + + Expression.LetDestructuring _ _ -> + context + + +rememberLetFunctionImplementation : Node Expression.FunctionImplementation -> Context -> Context +rememberLetFunctionImplementation (Node _ { arguments }) context = + rememberPatternList arguments context + + +rememberPatternList : List (Node Pattern) -> Context -> Context +rememberPatternList list context = + List.foldl rememberPattern context list + + +rememberPattern : Node Pattern -> Context -> Context +rememberPattern (Node _ pattern) context = + case pattern of + Pattern.AllPattern -> + context + + Pattern.VarPattern value -> + rememberValue value context + + Pattern.TuplePattern patterns -> + rememberPatternList patterns context + + Pattern.RecordPattern values -> + rememberValueList values context + + Pattern.UnConsPattern first second -> + context + |> rememberPattern first + |> rememberPattern second + + Pattern.ListPattern patterns -> + rememberPatternList patterns context + + Pattern.NamedPattern _ patterns -> + rememberPatternList patterns context + + Pattern.AsPattern inner name -> + context + |> rememberPattern inner + |> rememberValue (Node.value name) + + Pattern.ParenthesizedPattern inner -> + rememberPattern inner context + + _ -> + context + + +rememberValueList : List (Node String) -> Context -> Context +rememberValueList list context = + List.foldl (Node.value >> rememberValue) context list + + + +--- ON EXIT + + +singularDetails : List String +singularDetails = + [ "You should either use this parameter somewhere, or remove it at the location I pointed at." ] + + +pluralDetails : List String +pluralDetails = + [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + + +removeDetails : List String +removeDetails = + [ "You should remove it at the location I pointed at." ] + + +andThen : + (Context -> ( List (Rule.Error {}), Context )) + -> ( List (Rule.Error {}), Context ) + -> ( List (Rule.Error {}), Context ) +andThen function ( errors, context ) = + let + ( newErrors, newContext ) = + function context + in + ( newErrors ++ errors, newContext ) + + +errorsForFunctionImplementation : Node Expression.FunctionImplementation -> Context -> ( List (Rule.Error {}), Context ) +errorsForFunctionImplementation (Node _ { arguments }) context = + errorsForPatternList Function arguments context + + +errorsForLetDeclarationList : List (Node Expression.LetDeclaration) -> Context -> ( List (Rule.Error {}), Context ) +errorsForLetDeclarationList list context = + case list of + [] -> + ( [], context ) + + first :: rest -> + context + |> errorsForLetDeclaration first + |> andThen (errorsForLetDeclarationList rest) + + +errorsForLetDeclaration : Node Expression.LetDeclaration -> Context -> ( List (Rule.Error {}), Context ) +errorsForLetDeclaration (Node _ letDeclaration) context = + case letDeclaration of + Expression.LetFunction { declaration } -> + errorsForLetFunctionImplementation declaration context + + Expression.LetDestructuring _ _ -> + ( [], context ) + + +errorsForLetFunctionImplementation : Node Expression.FunctionImplementation -> Context -> ( List (Rule.Error {}), Context ) +errorsForLetFunctionImplementation (Node _ { arguments }) context = + errorsForPatternList Function arguments context + + +type PatternUse + = Lambda + | Function + + +errorsForPatternList : PatternUse -> List (Node Pattern) -> Context -> ( List (Rule.Error {}), Context ) +errorsForPatternList use list context = + case list of + [] -> + ( [], context ) + + first :: rest -> + context + |> errorsForPattern use first + |> andThen (errorsForPatternList use rest) + + +errorsForPattern : PatternUse -> Node Pattern -> Context -> ( List (Rule.Error {}), Context ) +errorsForPattern use (Node range pattern) context = + case pattern of + Pattern.AllPattern -> + ( [], context ) + + Pattern.VarPattern value -> + errorsForValue use value range context + + Pattern.RecordPattern values -> + errorsForRecordValueList use range values context + + Pattern.TuplePattern [ Node _ Pattern.AllPattern, Node _ Pattern.AllPattern ] -> + errorsForUselessTuple use range context + + Pattern.TuplePattern [ Node _ Pattern.AllPattern, Node _ Pattern.AllPattern, Node _ Pattern.AllPattern ] -> + errorsForUselessTuple use range context + + Pattern.TuplePattern patterns -> + errorsForPatternList use patterns context + + Pattern.UnConsPattern first second -> + errorsForPatternList use [ first, second ] context + + Pattern.ListPattern patterns -> + errorsForPatternList use patterns context + + Pattern.NamedPattern _ patterns -> + if List.all isAllPattern patterns then + errorsForUselessNamePattern use range context + + else + errorsForPatternList use patterns context + + Pattern.AsPattern inner name -> + context + |> errorsForAsPattern use range inner name + |> andThen (errorsForPattern use inner) + + Pattern.ParenthesizedPattern inner -> + errorsForPattern use inner context + + _ -> + ( [], context ) + + +errorsForUselessNamePattern : PatternUse -> Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForUselessNamePattern use range context = + let + fix = + case use of + Lambda -> + [ Fix.replaceRangeBy range "_" ] + + Function -> + [] + in + ( [ Rule.errorWithFix + { message = "Named pattern is not needed." + , details = removeDetails + } + range + fix + ] + , context + ) + + +errorsForUselessTuple : PatternUse -> Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForUselessTuple use range context = + let + fix = + case use of + Lambda -> + [ Fix.replaceRangeBy range "_" ] + + Function -> + [] + in + ( [ Rule.errorWithFix + { message = "Tuple pattern is not needed." + , details = removeDetails + } + range + fix + ] + , context + ) + + +errorsForRecordValueList : PatternUse -> Range -> List (Node String) -> Context -> ( List (Rule.Error {}), Context ) +errorsForRecordValueList use recordRange list context = + let + ( unused, used ) = + List.partition (\(Node _ value) -> Set.member value context) list + in + case unused of + [] -> + ( [], context ) + + firstNode :: restNodes -> + let + first = + firstNode |> Node.value + + rest = + List.map Node.value restNodes + + errorRange = + Range.combine (List.map Node.range unused) + + fix = + case ( use, used ) of + ( Lambda, [] ) -> + [ Fix.replaceRangeBy recordRange "_" ] + + ( Lambda, _ ) -> + [ Node Range.emptyRange (Pattern.RecordPattern used) + |> Writer.writePattern + |> Writer.write + |> Fix.replaceRangeBy recordRange + ] + + ( Function, _ ) -> + [] + in + ( [ Rule.errorWithFix + { message = listToMessage first rest + , details = listToDetails first rest + } + errorRange + fix + ] + , List.foldl forgetNode context unused + ) + + +listToMessage : String -> List String -> String +listToMessage first rest = + case List.reverse rest of + [] -> + "Parameter `" ++ first ++ "` is not used." + + last :: middle -> + "Parameters `" ++ String.join "`, `" (first :: middle) ++ "` and `" ++ last ++ "` are not used." + + +listToDetails : String -> List String -> List String +listToDetails _ rest = + case rest of + [] -> + singularDetails + + _ -> + pluralDetails + + +errorsForAsPattern : PatternUse -> Range -> Node Pattern -> Node String -> Context -> ( List (Rule.Error {}), Context ) +errorsForAsPattern use patternRange inner (Node range name) context = + if Set.member name context then + let + fix = + case use of + Lambda -> + [ inner + |> Writer.writePattern + |> Writer.write + |> Fix.replaceRangeBy patternRange + ] + + Function -> + [] + in + ( [ Rule.errorWithFix + { message = "Pattern alias `" ++ name ++ "` is not used." + , details = singularDetails + } + range + fix + ] + , Set.remove name context + ) + + else if isAllPattern inner then + ( [ Rule.errorWithFix + { message = "Pattern `_` is not needed." + , details = removeDetails + } + (Node.range inner) + [ Fix.replaceRangeBy patternRange name ] + ] + , Set.remove name context + ) + + else + ( [], context ) + + +isAllPattern : Node Pattern -> Bool +isAllPattern (Node _ pattern) = + case pattern of + Pattern.AllPattern -> + True + + _ -> + False + + +forgetNode : Node String -> Context -> Context +forgetNode (Node _ value) context = + Set.remove value context + + + +--- CONTEXT + + +type alias Context = + Set String + + +initialContext : Context +initialContext = + Set.empty + + +errorsForValue : PatternUse -> String -> Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForValue use value range context = + if Set.member value context then + let + fix = + case use of + Lambda -> + [ Fix.replaceRangeBy range "_" ] + + Function -> + [] + in + ( [ Rule.errorWithFix + { message = "Parameter `" ++ value ++ "` is not used." + , details = singularDetails + } + range + fix + ] + , Set.remove value context + ) + + else + ( [], context ) + + +rememberValue : String -> Context -> Context +rememberValue value context = + Set.insert value context + + +useValue : String -> Context -> Context +useValue value context = + Set.remove value context diff --git a/tests/NoUnused/ParametersTests.elm b/tests/NoUnused/ParametersTests.elm new file mode 100644 index 00000000..172180e9 --- /dev/null +++ b/tests/NoUnused/ParametersTests.elm @@ -0,0 +1,889 @@ +module NoUnused.ParametersTests exposing (all) + +import NoUnused.Parameters exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +details : List String +details = + [ "You should either use this parameter somewhere, or remove it at the location I pointed at." ] + + +all : Test +all = + describe "NoUnused.Patterns" + [ describe "in Function arguments" functionArgumentTests + , describe "in Lambda arguments" lambdaArgumentTests + , describe "in Let Functions" letFunctionTests + + --- un-tests + , describe "in Case branches" caseTests + , describe "in Let destructuring" letDestructuringTests + + --- in lambda + , describe "with as pattern in lambdas" lambdaAsPatternTests + , describe "with named pattern in lambdas" lambdaNamedPatternTests + , describe "with record pattern in lambdas" lambdaRecordPatternTests + , describe "with tuple pattern in lambdas" lambdaTuplePatternTests + + --- in function + , describe "with as pattern in functions" functionAsPatternTests + , describe "with named pattern in functions" functionNamedPatternTests + , describe "with record pattern in functions" functionRecordPatternTests + , describe "with tuple pattern in functions" functionTuplePatternTests + ] + + +caseTests : List Test +caseTests = + [ test "should not report unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + bish -> + Nothing + bash -> + Nothing +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report list of unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [one] -> 1 + [first, two] -> 2 + more -> 3 +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report uncons of unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + one :: [] -> 1 + first :: two :: [] -> 2 + _ :: _ :: more -> 3 +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + +functionArgumentTests : List Test +functionArgumentTests = + [ test "should report unused arguments" <| + \() -> + """ +module A exposing (..) +foo : Int -> String -> String -> String +foo one two three = + three +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `one` is not used." + , details = details + , under = "one" + } + , Review.Test.error + { message = "Parameter `two` is not used." + , details = details + , under = "two" + } + ] + , test "should not consider values from other modules" <| + \() -> + """ +module A exposing (..) +foo one = + Bar.one +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `one` is not used." + , details = details + , under = "one" + } + |> Review.Test.atExactly { start = { row = 3, column = 5 }, end = { row = 3, column = 8 } } + ] + ] + + +lambdaArgumentTests : List Test +lambdaArgumentTests = + [ test "should report unused arguments" <| + \() -> + """ +module A exposing (..) +foo = + List.map (\\value -> Nothing) list +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `value` is not used." + , details = details + , under = "value" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + List.map (\\_ -> Nothing) list +""" + ] + ] + + +letDestructuringTests : List Test +letDestructuringTests = + [ test "should not report unused values" <| + \() -> + """ +module A exposing (..) +foo = + let + ( left, right ) = + tupleValue + in + left +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report unused patterns that are aliased" <| + \() -> + """ +module A exposing (..) +foo = + let + (_ as bar) = 1 + in + bar +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + +letFunctionTests : List Test +letFunctionTests = + [ test "should report unused arguments" <| + \() -> + """ +module A exposing (..) +foo = + let + one oneValue = + 1 + two twoValue = + 2 + in + one two 3 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `oneValue` is not used." + , details = details + , under = "oneValue" + } + , Review.Test.error + { message = "Parameter `twoValue` is not used." + , details = details + , under = "twoValue" + } + ] + , test "should not report unused let functions" <| + \() -> + """ +module A exposing (..) +foo = + let + value = + something 5 + in + bar +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + + +--- LAMBDA PATTERN TESTS ------------------------ + + +lambdaAsPatternTests : List Test +lambdaAsPatternTests = + [ test "should report unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo = + \\({ bish, bash } as bosh) -> + ( bish, bash ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\({bish, bash}) -> + ( bish, bash ) +""" + ] + , test "should report unused patterns in an as pattern" <| + \() -> + """ +module A exposing (..) +foo = + \\({ bish, bash } as bosh) -> + ( bish, bosh ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bash` is not used." + , details = details + , under = "bash" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\({bish} as bosh) -> + ( bish, bosh ) +""" + ] + , test "should report unused patterns and unused aliases" <| + \() -> + """ +module A exposing (..) +foo = + \\({ bish, bash } as bosh) -> + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bash` is not used." + , details = details + , under = "bash" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\({bish} as bosh) -> + bish +""" + , Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\({bish, bash}) -> + bish +""" + ] + , test "should report unused patterns that are aliased" <| + \() -> + """ +module A exposing (..) +foo = + \\(_ as bar) -> bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern `_` is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "_" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\(bar) -> bar +""" + ] + , test "should report nested unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo = + \\(Named ( _, ( Named bash ) as bish )) -> + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\(Named ( _, ( Named bash ) )) -> + bash +""" + ] + ] + + +lambdaNamedPatternTests : List Test +lambdaNamedPatternTests = + [ test "should report unused named patterns" <| + \() -> + """ +module A exposing (..) +foo = + \\Named bish -> + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\Named _ -> + bash +""" + ] + , test "should report unused nested named patterns" <| + \() -> + """ +module A exposing (..) +foo = + \\Named (Bish bish) -> + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\Named (Bish _) -> + bash +""" + ] + , test "should report unused named patterns with multiple segments" <| + \() -> + """ +module A exposing (..) +foo = + \\(Pair _ _) -> bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\(_) -> bash +""" + ] + , test "should report unused named patterns in tuples" <| + \() -> + """ +module A exposing (..) +foo = + \\(Singular _, Pair _ _) -> bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Singular _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\(_, Pair _ _) -> bish +""" + , Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\(Singular _, _) -> bish +""" + ] + ] + + +lambdaRecordPatternTests : List Test +lambdaRecordPatternTests = + [ test "should replace unused record with `_`" <| + \() -> + """ +module A exposing (..) +foo = + \\{ bish, bash, bosh } -> + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bish`, `bash` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bish, bash, bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\_ -> + bar +""" + ] + , test "should report unused record values" <| + \() -> + """ +module A exposing (..) +foo = + \\{ bish, bash, bosh } -> + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bish` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bish, bash, bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\{bash} -> + bash +""" + ] + , test "should report highlight the least amount of values possible" <| + \() -> + """ +module A exposing (..) +foo = + \\{ bish, bash, bosh } -> + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bash` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bash, bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\{bish} -> + bish +""" + ] + ] + + +lambdaTuplePatternTests : List Test +lambdaTuplePatternTests = + [ test "should report unused tuple values" <| + \() -> + """ +module A exposing (..) +foo = + \\( bish, bash, bosh ) -> + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\( _, bash, bosh ) -> + bash +""" + , Review.Test.error + { message = "Parameter `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\( bish, bash, _ ) -> + bash +""" + ] + , test "should replace unused tuple with `_`" <| + \() -> + """ +module A exposing (..) +foo = + \\( _, _ ) -> + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _ )" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\_ -> + bar +""" + ] + , test "should replace unused threeple with `_`" <| + \() -> + """ +module A exposing (..) +foo = + \\( _, _, _ ) -> + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _, _ )" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + \\_ -> + bar +""" + ] + ] + + + +--- FUNCTION PATTERN TESTS + + +functionAsPatternTests : List Test +functionAsPatternTests = + [ test "should report unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo ({ bish, bash } as bosh) = + ( bish, bash ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + ] + , test "should report unused patterns in an as pattern" <| + \() -> + """ +module A exposing (..) +foo ({ bish, bash } as bosh) = + ( bish, bosh ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bash` is not used." + , details = details + , under = "bash" + } + ] + , test "should report unused patterns and unused aliases" <| + \() -> + """ +module A exposing (..) +foo ({ bish, bash } as bosh) = + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bash` is not used." + , details = details + , under = "bash" + } + , Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + ] + , test "should report unused patterns that are aliased" <| + \() -> + """ +module A exposing (..) +foo (_ as bar) = + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern `_` is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "_" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo (bar) = + bar +""" + ] + , test "should report nested unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo (Named ( _, ( Just bash ) as bish )) = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bish` is not used." + , details = details + , under = "bish" + } + ] + ] + + +functionNamedPatternTests : List Test +functionNamedPatternTests = + [ test "should report unused named patterns" <| + \() -> + """ +module A exposing (..) +foo (Named bish) = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + ] + , test "should report unused nested named patterns" <| + \() -> + """ +module A exposing (..) +foo (Named (Bish bish)) = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + ] + , test "should report unused named patterns with multiple segments" <| + \() -> + """ +module A exposing (..) +foo (Pair _ _) = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + ] + , test "should report unused named patterns in tuples" <| + \() -> + """ +module A exposing (..) +foo (Singular _, Pair _ _) = + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Singular _" + } + , Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + ] + ] + + +functionRecordPatternTests : List Test +functionRecordPatternTests = + [ test "should replace unused record with `_`" <| + \() -> + """ +module A exposing (..) +foo { bish, bash, bosh } = + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bish`, `bash` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bish, bash, bosh" + } + ] + , test "should report unused record values" <| + \() -> + """ +module A exposing (..) +foo { bish, bash, bosh } = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bish` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bish, bash, bosh" + } + ] + , test "should report highlight the least amount of values possible" <| + \() -> + """ +module A exposing (..) +foo { bish, bash, bosh } = + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameters `bash` and `bosh` are not used." + , details = [ "You should either use these parameters somewhere, or remove them at the location I pointed at." ] + , under = "bash, bosh" + } + ] + ] + + +functionTuplePatternTests : List Test +functionTuplePatternTests = + [ test "should report unused tuple values" <| + \() -> + """ +module A exposing (..) +foo ( bish, bash, bosh ) = + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Parameter `bish` is not used." + , details = details + , under = "bish" + } + , Review.Test.error + { message = "Parameter `bosh` is not used." + , details = details + , under = "bosh" + } + ] + , test "should report unused tuple" <| + \() -> + """ +module A exposing (..) +foo ( _, _ ) = + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _ )" + } + ] + , test "should report unused threeple" <| + \() -> + """ +module A exposing (..) +foo ( _, _, _ ) = + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _, _ )" + } + ] + ] diff --git a/tests/NoUnused/Patterns.elm b/tests/NoUnused/Patterns.elm new file mode 100644 index 00000000..14dbac30 --- /dev/null +++ b/tests/NoUnused/Patterns.elm @@ -0,0 +1,478 @@ +module NoUnused.Patterns exposing (rule) + +{-| Report useless patterns and pattern values that are not used. + + +# Rule + +@docs rule + +-} + +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.ModuleName exposing (ModuleName) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Pattern as Pattern exposing (Pattern) +import Elm.Syntax.Range as Range exposing (Range) +import Elm.Writer as Writer +import NoUnused.Patterns.NameVisitor as NameVisitor +import Review.Fix as Fix +import Review.Rule as Rule exposing (Rule) +import Set exposing (Set) + + +{-| Report useless patterns and pattern values that are not used. + + config = + [ NoUnused.Patterns.rule + ] + +This rule looks within let..in blocks and case branches to find any patterns that are unused. It will report any useless patterns as well as any pattern values that are not used. + + +## Fail + +Value `something` is not used: + + case maybe of + Just something -> + True + + Nothing -> + False + + +## Success + + case maybe of + Just _ -> + True + + Nothing -> + False + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoUnused.Patterns" initialContext + |> Rule.withExpressionVisitor expressionVisitor + |> NameVisitor.withValueVisitor valueVisitor + |> Rule.fromModuleRuleSchema + + +expressionVisitor : Node Expression -> Rule.Direction -> Context -> ( List (Rule.Error {}), Context ) +expressionVisitor (Node _ expression) direction context = + case ( direction, expression ) of + ( Rule.OnEnter, Expression.LetExpression { declarations } ) -> + ( [], rememberLetDeclarationList declarations context ) + + ( Rule.OnExit, Expression.LetExpression { declarations } ) -> + errorsForLetDeclarationList declarations context + + ( Rule.OnEnter, Expression.CaseExpression { cases } ) -> + ( [], rememberCaseList cases context ) + + ( Rule.OnExit, Expression.CaseExpression { cases } ) -> + errorsForCaseList cases context + + _ -> + ( [], context ) + + +valueVisitor : Node ( ModuleName, String ) -> Context -> ( List (Rule.Error {}), Context ) +valueVisitor (Node _ ( moduleName, value )) context = + case moduleName of + [] -> + ( [], useValue value context ) + + _ -> + ( [], context ) + + + +--- ON ENTER + + +rememberCaseList : List Expression.Case -> Context -> Context +rememberCaseList list context = + List.foldl rememberCase context list + + +rememberCase : Expression.Case -> Context -> Context +rememberCase ( pattern, _ ) context = + rememberPattern pattern context + + +rememberLetDeclarationList : List (Node Expression.LetDeclaration) -> Context -> Context +rememberLetDeclarationList list context = + List.foldl rememberLetDeclaration context list + + +rememberLetDeclaration : Node Expression.LetDeclaration -> Context -> Context +rememberLetDeclaration (Node _ letDeclaration) context = + case letDeclaration of + Expression.LetFunction _ -> + context + + Expression.LetDestructuring pattern _ -> + rememberPattern pattern context + + +rememberPatternList : List (Node Pattern) -> Context -> Context +rememberPatternList list context = + List.foldl rememberPattern context list + + +rememberPattern : Node Pattern -> Context -> Context +rememberPattern (Node _ pattern) context = + case pattern of + Pattern.AllPattern -> + context + + Pattern.VarPattern value -> + rememberValue value context + + Pattern.TuplePattern patterns -> + rememberPatternList patterns context + + Pattern.RecordPattern values -> + rememberValueList values context + + Pattern.UnConsPattern first second -> + context + |> rememberPattern first + |> rememberPattern second + + Pattern.ListPattern patterns -> + rememberPatternList patterns context + + Pattern.NamedPattern _ patterns -> + rememberPatternList patterns context + + Pattern.AsPattern inner name -> + context + |> rememberPattern inner + |> rememberValue (Node.value name) + + Pattern.ParenthesizedPattern inner -> + rememberPattern inner context + + _ -> + context + + +rememberValueList : List (Node String) -> Context -> Context +rememberValueList list context = + List.foldl (Node.value >> rememberValue) context list + + + +--- ON EXIT + + +singularDetails : List String +singularDetails = + [ "You should either use this value somewhere, or remove it at the location I pointed at." ] + + +pluralDetails : List String +pluralDetails = + [ "You should either use these values somewhere, or remove them at the location I pointed at." ] + + +removeDetails : List String +removeDetails = + [ "You should remove it at the location I pointed at." ] + + +andThen : + (Context -> ( List (Rule.Error {}), Context )) + -> ( List (Rule.Error {}), Context ) + -> ( List (Rule.Error {}), Context ) +andThen function ( errors, context ) = + let + ( newErrors, newContext ) = + function context + in + ( newErrors ++ errors, newContext ) + + +errorsForCaseList : List Expression.Case -> Context -> ( List (Rule.Error {}), Context ) +errorsForCaseList list context = + case list of + [] -> + ( [], context ) + + first :: rest -> + context + |> errorsForCase first + |> andThen (errorsForCaseList rest) + + +errorsForCase : Expression.Case -> Context -> ( List (Rule.Error {}), Context ) +errorsForCase ( pattern, _ ) context = + errorsForPattern Matching pattern context + + +errorsForLetDeclarationList : List (Node Expression.LetDeclaration) -> Context -> ( List (Rule.Error {}), Context ) +errorsForLetDeclarationList list context = + case list of + [] -> + ( [], context ) + + first :: rest -> + context + |> errorsForLetDeclaration first + |> andThen (errorsForLetDeclarationList rest) + + +errorsForLetDeclaration : Node Expression.LetDeclaration -> Context -> ( List (Rule.Error {}), Context ) +errorsForLetDeclaration (Node _ letDeclaration) context = + case letDeclaration of + Expression.LetFunction _ -> + ( [], context ) + + Expression.LetDestructuring pattern _ -> + errorsForPattern Destructuring pattern context + + +type PatternUse + = Destructuring + | Matching + + +errorsForPatternList : PatternUse -> List (Node Pattern) -> Context -> ( List (Rule.Error {}), Context ) +errorsForPatternList use list context = + case list of + [] -> + ( [], context ) + + first :: rest -> + context + |> errorsForPattern use first + |> andThen (errorsForPatternList use rest) + + +errorsForPattern : PatternUse -> Node Pattern -> Context -> ( List (Rule.Error {}), Context ) +errorsForPattern use (Node range pattern) context = + case pattern of + Pattern.AllPattern -> + ( [], context ) + + Pattern.VarPattern value -> + errorsForValue value range context + + Pattern.RecordPattern values -> + errorsForRecordValueList range values context + + Pattern.TuplePattern [ Node _ Pattern.AllPattern, Node _ Pattern.AllPattern ] -> + errorsForUselessTuple range context + + Pattern.TuplePattern [ Node _ Pattern.AllPattern, Node _ Pattern.AllPattern, Node _ Pattern.AllPattern ] -> + errorsForUselessTuple range context + + Pattern.TuplePattern patterns -> + errorsForPatternList use patterns context + + Pattern.UnConsPattern first second -> + errorsForPatternList use [ first, second ] context + + Pattern.ListPattern patterns -> + errorsForPatternList use patterns context + + Pattern.NamedPattern _ patterns -> + if use == Destructuring && List.all isAllPattern patterns then + errorsForUselessNamePattern range context + + else + errorsForPatternList use patterns context + + Pattern.AsPattern inner name -> + context + |> errorsForAsPattern range inner name + |> andThen (errorsForPattern use inner) + + Pattern.ParenthesizedPattern inner -> + errorsForPattern use inner context + + _ -> + ( [], context ) + + +errorsForUselessNamePattern : Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForUselessNamePattern range context = + ( [ Rule.errorWithFix + { message = "Named pattern is not needed." + , details = removeDetails + } + range + [ Fix.replaceRangeBy range "_" ] + ] + , context + ) + + +errorsForUselessTuple : Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForUselessTuple range context = + ( [ Rule.errorWithFix + { message = "Tuple pattern is not needed." + , details = removeDetails + } + range + [ Fix.replaceRangeBy range "_" ] + ] + , context + ) + + +errorsForRecordValueList : Range -> List (Node String) -> Context -> ( List (Rule.Error {}), Context ) +errorsForRecordValueList recordRange list context = + let + ( unused, used ) = + List.partition (\(Node _ value) -> Set.member value context) list + in + case unused of + [] -> + ( [], context ) + + firstNode :: restNodes -> + let + first = + firstNode |> Node.value + + rest = + List.map Node.value restNodes + + ( errorRange, fix ) = + case used of + [] -> + ( recordRange, Fix.replaceRangeBy recordRange "_" ) + + _ -> + ( Range.combine (List.map Node.range unused) + , Node Range.emptyRange (Pattern.RecordPattern used) + |> Writer.writePattern + |> Writer.write + |> Fix.replaceRangeBy recordRange + ) + in + ( [ Rule.errorWithFix + { message = listToMessage first rest + , details = listToDetails first rest + } + errorRange + [ fix ] + ] + , List.foldl forgetNode context unused + ) + + +listToMessage : String -> List String -> String +listToMessage first rest = + case List.reverse rest of + [] -> + "Value `" ++ first ++ "` is not used." + + last :: middle -> + "Values `" ++ String.join "`, `" (first :: middle) ++ "` and `" ++ last ++ "` are not used." + + +listToDetails : String -> List String -> List String +listToDetails _ rest = + case rest of + [] -> + singularDetails + + _ -> + pluralDetails + + +errorsForAsPattern : Range -> Node Pattern -> Node String -> Context -> ( List (Rule.Error {}), Context ) +errorsForAsPattern patternRange inner (Node range name) context = + if Set.member name context then + let + fix = + [ inner + |> Writer.writePattern + |> Writer.write + |> Fix.replaceRangeBy patternRange + ] + in + ( [ Rule.errorWithFix + { message = "Pattern alias `" ++ name ++ "` is not used." + , details = singularDetails + } + range + fix + ] + , Set.remove name context + ) + + else if isAllPattern inner then + ( [ Rule.errorWithFix + { message = "Pattern `_` is not needed." + , details = removeDetails + } + (Node.range inner) + [ Fix.replaceRangeBy patternRange name ] + ] + , Set.remove name context + ) + + else + ( [], context ) + + +isAllPattern : Node Pattern -> Bool +isAllPattern (Node _ pattern) = + case pattern of + Pattern.AllPattern -> + True + + _ -> + False + + +forgetNode : Node String -> Context -> Context +forgetNode (Node _ value) context = + Set.remove value context + + + +--- CONTEXT + + +type alias Context = + Set String + + +initialContext : Context +initialContext = + Set.empty + + +errorsForValue : String -> Range -> Context -> ( List (Rule.Error {}), Context ) +errorsForValue value range context = + if Set.member value context then + ( [ Rule.errorWithFix + { message = "Value `" ++ value ++ "` is not used." + , details = singularDetails + } + range + [ Fix.replaceRangeBy range "_" ] + ] + , Set.remove value context + ) + + else + ( [], context ) + + +rememberValue : String -> Context -> Context +rememberValue value context = + Set.insert value context + + +useValue : String -> Context -> Context +useValue value context = + Set.remove value context diff --git a/tests/NoUnused/Patterns/NameVisitor.elm b/tests/NoUnused/Patterns/NameVisitor.elm new file mode 100644 index 00000000..6185f460 --- /dev/null +++ b/tests/NoUnused/Patterns/NameVisitor.elm @@ -0,0 +1,215 @@ +module NoUnused.Patterns.NameVisitor exposing (withValueVisitor) + +{-| Visit each name in the module. + +A "name" is a `Node ( ModuleName, String )` and represents a value or type reference. Here are some value examples: + + - `Html.Attributes.class` -> `( [ "Html", "Attributes" ], "class" )` + - `view` -> `( [], "view" )` + +These can appear in many places throughout declarations and expressions, and picking them out each time is a lot of work. Instead of writing 1000 lines of code and tests each time, you can write one `nameVisitor` and plug it straight into your module schema, or separate `valueVisitor` and `typeVisitor`s. + +@docs withValueVisitor + + +## Scope + +This makes no attempt to resolve module names from imports, it just returns what's written in the code. It would be trivial to connect [elm-review-scope] with the name visitor if you want to do this. + +[elm-review-scope]: http://github.com/jfmengels/elm-review-scope/ + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.ModuleName exposing (ModuleName) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Pattern as Pattern exposing (Pattern) +import Review.Rule as Rule exposing (Error) + + +type alias VisitorFunction context = + Node ( ModuleName, String ) -> context -> ( List (Error {}), context ) + + +type Name + = Value (Node ( ModuleName, String )) + + +withValueVisitor : + (Node ( ModuleName, String ) -> context -> ( List (Error {}), context )) + -> Rule.ModuleRuleSchema { schemaState | canCollectProjectData : () } context + -> Rule.ModuleRuleSchema { schemaState | canCollectProjectData : (), hasAtLeastOneVisitor : () } context +withValueVisitor valueVisitor rule = + rule + |> Rule.withDeclarationListVisitor (declarationListVisitor valueVisitor) + |> Rule.withExpressionVisitor (expressionVisitor valueVisitor) + + + +--- VISITORS + + +declarationListVisitor : + VisitorFunction context + -> (List (Node Declaration) -> context -> ( List (Error {}), context )) +declarationListVisitor visitor list context = + visitDeclarationList list + |> folder visitor context + + +expressionVisitor : + VisitorFunction context + -> (Node Expression -> Rule.Direction -> context -> ( List (Error {}), context )) +expressionVisitor visitor node direction context = + case direction of + Rule.OnEnter -> + visitExpression node + |> folder visitor context + + Rule.OnExit -> + ( [], context ) + + + +--- FOLDER + + +folder : + VisitorFunction context + -> (context -> List Name -> ( List (Error {}), context )) +folder visitor context list = + case list of + [] -> + ( [], context ) + + head :: rest -> + let + ( headErrors, headContext ) = + applyVisitor visitor head context + + ( restErrors, restContext ) = + folder visitor headContext rest + in + ( headErrors ++ restErrors, restContext ) + + +applyVisitor : VisitorFunction context -> Name -> context -> ( List (Error {}), context ) +applyVisitor visitor (Value node) context = + visitor node context + + + +--- PRIVATE + + +visitDeclarationList : List (Node Declaration) -> List Name +visitDeclarationList nodes = + List.concatMap visitDeclaration nodes + + +visitDeclaration : Node Declaration -> List Name +visitDeclaration node = + case Node.value node of + Declaration.FunctionDeclaration { declaration } -> + visitFunctionImplementation declaration + + _ -> + [] + + +visitFunctionImplementation : Node Expression.FunctionImplementation -> List Name +visitFunctionImplementation node = + visitPatternList (node |> Node.value |> .arguments) + + +visitExpression : Node Expression -> List Name +visitExpression (Node range expression) = + case expression of + Expression.FunctionOrValue moduleName function -> + visitValue (Node range ( moduleName, function )) + + Expression.LetExpression { declarations } -> + visitLetDeclarationList declarations + + Expression.CaseExpression { cases } -> + visitCaseList cases + + Expression.LambdaExpression { args } -> + visitPatternList args + + Expression.RecordUpdateExpression name _ -> + visitValue (Node.map (\function -> ( [], function )) name) + + _ -> + [] + + +visitLetDeclarationList : List (Node Expression.LetDeclaration) -> List Name +visitLetDeclarationList list = + List.concatMap visitLetDeclaration list + + +visitLetDeclaration : Node Expression.LetDeclaration -> List Name +visitLetDeclaration node = + case Node.value node of + Expression.LetFunction { declaration } -> + visitFunctionImplementation declaration + + Expression.LetDestructuring pattern _ -> + visitPattern pattern + + +visitCaseList : List Expression.Case -> List Name +visitCaseList list = + List.concatMap visitCase list + + +visitCase : Expression.Case -> List Name +visitCase ( pattern, _ ) = + visitPattern pattern + + +visitPatternList : List (Node Pattern) -> List Name +visitPatternList list = + List.concatMap visitPattern list + + +visitPattern : Node Pattern -> List Name +visitPattern node = + case Node.value node of + Pattern.TuplePattern patterns -> + visitPatternList patterns + + Pattern.UnConsPattern head rest -> + visitPattern head ++ visitPattern rest + + Pattern.ListPattern list -> + visitPatternList list + + Pattern.NamedPattern { moduleName, name } _ -> + let + { start } = + Node.range node + + newEnd = + { start | column = start.column + (name :: moduleName |> String.join "." |> String.length) } + + range = + { start = start, end = newEnd } + in + visitValue (Node range ( moduleName, name )) + + Pattern.AsPattern pattern _ -> + visitPattern pattern + + Pattern.ParenthesizedPattern pattern -> + visitPattern pattern + + _ -> + [] + + +visitValue : Node ( ModuleName, String ) -> List Name +visitValue node = + [ Value node ] diff --git a/tests/NoUnused/PatternsTests.elm b/tests/NoUnused/PatternsTests.elm new file mode 100644 index 00000000..aef12ff6 --- /dev/null +++ b/tests/NoUnused/PatternsTests.elm @@ -0,0 +1,960 @@ +module NoUnused.PatternsTests exposing (all) + +import NoUnused.Patterns exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +details : List String +details = + [ "You should either use this value somewhere, or remove it at the location I pointed at." + ] + + +all : Test +all = + describe "NoUnused.Patterns" + [ describe "in Case branches" caseTests + , describe "in Let destructuring" letDestructuringTests + + --- un-tests + , describe "in Function arguments" functionArgumentTests + , describe "in Lambda arguments" lambdaArgumentTests + , describe "in Let Functions" letFunctionTests + + --- patterns + , describe "with as pattern" asPatternTests + , describe "with list pattern" listPatternTests + , describe "with named pattern" namedPatternTests + , describe "with record pattern" recordPatternTests + , describe "with tuple pattern" tuplePatternTests + , describe "with uncons pattern" unconsPatternTests + ] + + +caseTests : List Test +caseTests = + [ test "reports unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + bish -> + Nothing + bash -> + Nothing +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + _ -> + Nothing + bash -> + Nothing +""" + , Review.Test.error + { message = "Value `bash` is not used." + , details = details + , under = "bash" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + bish -> + Nothing + _ -> + Nothing +""" + ] + , test "should not remove all list of unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [one] -> 1 + [first, two] -> 2 + more -> 3 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `one` is not used." + , details = details + , under = "one" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [_] -> 1 + [first, two] -> 2 + more -> 3 +""" + , Review.Test.error + { message = "Value `first` is not used." + , details = details + , under = "first" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [one] -> 1 + [_, two] -> 2 + more -> 3 +""" + , Review.Test.error + { message = "Value `two` is not used." + , details = details + , under = "two" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [one] -> 1 + [first, _] -> 2 + more -> 3 +""" + , Review.Test.error + { message = "Value `more` is not used." + , details = details + , under = "more" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + [one] -> 1 + [first, two] -> 2 + _ -> 3 +""" + ] + , test "should not remove all uncons of unused values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + one :: [] -> 1 + first :: two :: [] -> 2 + _ :: _ :: more -> 3 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `one` is not used." + , details = details + , under = "one" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + _ :: [] -> 1 + first :: two :: [] -> 2 + _ :: _ :: more -> 3 +""" + , Review.Test.error + { message = "Value `first` is not used." + , details = details + , under = "first" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + one :: [] -> 1 + _ :: two :: [] -> 2 + _ :: _ :: more -> 3 +""" + , Review.Test.error + { message = "Value `two` is not used." + , details = details + , under = "two" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + one :: [] -> 1 + first :: _ :: [] -> 2 + _ :: _ :: more -> 3 +""" + , Review.Test.error + { message = "Value `more` is not used." + , details = details + , under = "more" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [] -> 0 + one :: [] -> 1 + first :: two :: [] -> 2 + _ :: _ :: _ -> 3 +""" + ] + ] + + +functionArgumentTests : List Test +functionArgumentTests = + [ test "should not report unused arguments" <| + \() -> + """ +module A exposing (..) +foo : Int -> String -> String -> String +foo one two three = + three +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + +lambdaArgumentTests : List Test +lambdaArgumentTests = + [ test "should not report unused arguments" <| + \() -> + """ +module A exposing (..) +foo = + List.map (\\value -> Nothing) list +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + +letDestructuringTests : List Test +letDestructuringTests = + [ test "should report unused values" <| + \() -> + """ +module A exposing (..) +foo = + let + ( left, right ) = + tupleValue + in + left +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `right` is not used." + , details = details + , under = "right" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + ( left, _ ) = + tupleValue + in + left +""" + ] + , test "should report unused patterns that are aliased" <| + \() -> + """ +module A exposing (..) +foo = + let + (_ as bar) = 1 + in + bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern `_` is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "_" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + (bar) = 1 + in + bar +""" + ] + ] + + +letFunctionTests : List Test +letFunctionTests = + [ test "should not report unused arguments" <| + \() -> + """ +module A exposing (..) +foo = + let + one oneValue = + 1 + two twoValue = + 2 + in + one two 3 +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report unused let functions" <| + \() -> + """ +module A exposing (..) +foo = + let + value foo = + something foo + in + bar +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + + +--- PATTERN TESTS ------------------------ + + +asPatternTests : List Test +asPatternTests = + [ test "should report unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + ({ bish, bash } as bosh) -> + ( bish, bash ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + ({bish, bash}) -> + ( bish, bash ) +""" + ] + , test "should report unused patterns in an as pattern" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + ({ bish, bash } as bosh) -> + ( bish, bosh ) +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bash` is not used." + , details = details + , under = "bash" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + ({bish} as bosh) -> + ( bish, bosh ) +""" + ] + , test "should report unused patterns and unused aliases" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + ({ bish, bash } as bosh) -> + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bash` is not used." + , details = details + , under = "bash" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + ({bish} as bosh) -> + bish +""" + , Review.Test.error + { message = "Pattern alias `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + ({bish, bash}) -> + bish +""" + ] + , test "should report unused patterns that are aliased" <| + \() -> + """ +module A exposing (..) +foo = + case 1 of + (_ as bar) -> bar +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern `_` is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "_" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case 1 of + (bar) -> bar +""" + ] + , test "should report nested unused pattern aliases" <| + \() -> + """ +module A exposing (..) +foo = + case maybeTupleMaybe of + Just ( _, ( Just _ ) as bish ) -> + bash + _ -> + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Pattern alias `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case maybeTupleMaybe of + Just ( _, ( Just _ ) ) -> + bash + _ -> + bosh +""" + ] + ] + + +listPatternTests : List Test +listPatternTests = + [ test "should report unused list values" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + [ first, second ] -> + another +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `first` is not used." + , details = details + , under = "first" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [ _, second ] -> + another +""" + , Review.Test.error + { message = "Value `second` is not used." + , details = details + , under = "second" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + [ first, _ ] -> + another +""" + ] + ] + + +namedPatternTests : List Test +namedPatternTests = + [ test "should report unused named patterns" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + Just bish -> + bash + Nothing -> + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + Just _ -> + bash + Nothing -> + bosh +""" + ] + , test "should report unused parenthesized patterns" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + Just (Bish bish) -> + bash + Nothing -> + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case bar of + Just (Bish _) -> + bash + Nothing -> + bosh +""" + ] + , test "should not report unused named patterns in case" <| + \() -> + """ +module A exposing (..) +foo = + case maybeTupleMaybe of + Just ( Just _, (Just _) as bish ) -> + bish + Just _ -> + bash + _ -> + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should report unused named patterns in destructuring" <| + \() -> + """ +module A exposing (..) +foo = + let + (Singular _) = bish + (Pair _ _) = bash + in + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Singular _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + (_) = bish + (Pair _ _) = bash + in + bosh +""" + , Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + (Singular _) = bish + (_) = bash + in + bosh +""" + ] + , test "should report unused named patterns in destructuring tuples" <| + \() -> + """ +module A exposing (..) +foo = + let + (Singular _, Pair _ _) = bish + in + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Singular _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + (_, Pair _ _) = bish + in + bosh +""" + , Review.Test.error + { message = "Named pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "Pair _ _" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + (Singular _, _) = bish + in + bosh +""" + ] + ] + + +recordPatternTests : List Test +recordPatternTests = + [ test "should replace unused record with `_`" <| + \() -> + """ +module A exposing (..) +foo = + let + { bish, bash } = + bar + in + bosh +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Values `bish` and `bash` are not used." + , details = [ "You should either use these values somewhere, or remove them at the location I pointed at." ] + , under = "{ bish, bash }" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + _ = + bar + in + bosh +""" + ] + , test "should report unused record values" <| + \() -> + """ +module A exposing (..) +foo = + let + { bish, bash, bosh } = + bar + in + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Values `bish` and `bosh` are not used." + , details = [ "You should either use these values somewhere, or remove them at the location I pointed at." ] + , under = "bish, bash, bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + {bash} = + bar + in + bash +""" + ] + , test "should report highlight the least amount of values possible" <| + \() -> + """ +module A exposing (..) +foo = + let + { bish, bash, bosh } = + bar + in + bish +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Values `bash` and `bosh` are not used." + , details = [ "You should either use these values somewhere, or remove them at the location I pointed at." ] + , under = "bash, bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + {bish} = + bar + in + bish +""" + ] + ] + + +tuplePatternTests : List Test +tuplePatternTests = + [ test "should report unused tuple values" <| + \() -> + """ +module A exposing (..) +foo = + let + ( bish, bash, bosh ) = + bar + in + bash +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `bish` is not used." + , details = details + , under = "bish" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + ( _, bash, bosh ) = + bar + in + bash +""" + , Review.Test.error + { message = "Value `bosh` is not used." + , details = details + , under = "bosh" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + ( bish, bash, _ ) = + bar + in + bash +""" + ] + , test "should replace unused tuple with `_`" <| + \() -> + """ +module A exposing (..) +foo = + let + ( _, _ ) = + bar + in + 1 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _ )" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + _ = + bar + in + 1 +""" + ] + , test "should replace unused threeple with `_`" <| + \() -> + """ +module A exposing (..) +foo = + let + ( _, _, _ ) = + bar + in + 1 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Tuple pattern is not needed." + , details = [ "You should remove it at the location I pointed at." ] + , under = "( _, _, _ )" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + let + _ = + bar + in + 1 +""" + ] + , test "should not report a tuple containing an empty list" <| + \() -> + """ +module A exposing (..) +foo = + case bar of + ( [], _ ) -> + 1 +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ] + + +unconsPatternTests : List Test +unconsPatternTests = + [ test "should report unused uncons values" <| + \() -> + """ +module A exposing (..) +foo = + case list of + first :: rest -> + list +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Value `first` is not used." + , details = details + , under = "first" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case list of + _ :: rest -> + list +""" + , Review.Test.error + { message = "Value `rest` is not used." + , details = details + , under = "rest" + } + |> Review.Test.whenFixed + """ +module A exposing (..) +foo = + case list of + first :: _ -> + list +""" + ] + ] diff --git a/tests/NoUnused/Variables.elm b/tests/NoUnused/Variables.elm index eddf20f8..a4ee07dd 100644 --- a/tests/NoUnused/Variables.elm +++ b/tests/NoUnused/Variables.elm @@ -56,8 +56,8 @@ rule = Rule.newModuleRuleSchema "NoUnused.Variables" initialContext |> Rule.withModuleDefinitionVisitor moduleDefinitionVisitor |> Rule.withImportVisitor importVisitor - |> Rule.withExpressionVisitor expressionVisitor |> Rule.withDeclarationVisitor declarationVisitor + |> Rule.withExpressionVisitor expressionVisitor |> Rule.withFinalModuleEvaluation finalEvaluation |> Rule.fromModuleRuleSchema @@ -216,7 +216,7 @@ fix declaredModules { variableType, rangeToRemove } = True Port -> - True + False in if shouldOfferFix then [ Fix.removeRange rangeToRemove ] @@ -264,7 +264,7 @@ importVisitor ((Node _ import_) as node) context = Just moduleAlias -> if Node.value moduleAlias == Node.value import_.moduleName then [ Rule.errorWithFix - { message = "Module `Html` is aliased as `Html`" + { message = "Module `" ++ String.join "." (Node.value moduleAlias) ++ "` is aliased as itself" , details = [ "The alias is the same as the module name, and brings no useful value" ] } (Node.range moduleAlias) @@ -291,7 +291,7 @@ importVisitor ((Node _ import_) as node) context = registerModuleNameOrAlias : Node Import -> Context -> Context -registerModuleNameOrAlias ((Node range { exposingList, moduleAlias, moduleName }) as node) context = +registerModuleNameOrAlias ((Node range { moduleAlias, moduleName }) as node) context = case moduleAlias of Just _ -> registerModuleAlias node context @@ -381,7 +381,7 @@ expressionVisitor (Node range value) direction context = |> registerFunction letBlockContext function |> markUsedTypesAndModules namesUsedInArgumentPatterns - Expression.LetDestructuring pattern _ -> + Expression.LetDestructuring _ _ -> context_ ) { context | scopes = NonemptyList.cons emptyScope context.scopes } @@ -408,7 +408,7 @@ expressionVisitor (Node range value) direction context = usedVariables = cases |> List.map - (\( patternNode, expressionNode ) -> + (\( patternNode, _ ) -> getUsedVariablesFromPattern patternNode ) |> foldUsedTypesAndModules @@ -480,19 +480,14 @@ getUsedTypesFromPattern patternNode = [] Pattern.NamedPattern qualifiedNameRef patterns -> - let - usedVariable : List String - usedVariable = - case qualifiedNameRef.moduleName of - [] -> - [ qualifiedNameRef.name ] + case qualifiedNameRef.moduleName of + [] -> + qualifiedNameRef.name :: List.concatMap getUsedTypesFromPattern patterns - moduleName -> - [] - in - usedVariable ++ List.concatMap getUsedTypesFromPattern patterns + _ -> + List.concatMap getUsedTypesFromPattern patterns - Pattern.AsPattern pattern alias_ -> + Pattern.AsPattern pattern _ -> getUsedTypesFromPattern pattern Pattern.ParenthesizedPattern pattern -> @@ -539,19 +534,14 @@ getUsedModulesFromPattern patternNode = [] Pattern.NamedPattern qualifiedNameRef patterns -> - let - usedVariable : List String - usedVariable = - case qualifiedNameRef.moduleName of - [] -> - [] + case qualifiedNameRef.moduleName of + [] -> + List.concatMap getUsedModulesFromPattern patterns - moduleName -> - [ getModuleName moduleName ] - in - usedVariable ++ List.concatMap getUsedModulesFromPattern patterns + moduleName -> + getModuleName moduleName :: List.concatMap getUsedModulesFromPattern patterns - Pattern.AsPattern pattern alias_ -> + Pattern.AsPattern pattern _ -> getUsedModulesFromPattern pattern Pattern.ParenthesizedPattern pattern -> @@ -842,6 +832,7 @@ collectFromExposing exposingNode = ) Exposing.TypeOrAliasExpose name -> + -- TODO Detect whether it is a custom type or type alias Just ( name , { variableType = ImportedItem ImportedType @@ -852,7 +843,7 @@ collectFromExposing exposingNode = Exposing.TypeExpose { name, open } -> case open of - Just openRange -> + Just _ -> -- TODO Change this behavior once we know the contents of the open range, using dependencies or the interfaces of the other modules Nothing @@ -898,7 +889,7 @@ collectTypesFromTypeAnnotation node = ( [], str ) -> [ str ] - ( moduleName, _ ) -> + _ -> [] in name ++ List.concatMap collectTypesFromTypeAnnotation params @@ -935,7 +926,7 @@ collectModuleNamesFromTypeAnnotation node = name : List String name = case Node.value nameNode of - ( [], str ) -> + ( [], _ ) -> [] ( moduleName, _ ) -> diff --git a/tests/NoUnused/VariablesTest.elm b/tests/NoUnused/VariablesTest.elm index deee05e3..17215ed0 100644 --- a/tests/NoUnused/VariablesTest.elm +++ b/tests/NoUnused/VariablesTest.elm @@ -781,7 +781,7 @@ a = Html.div""" |> Review.Test.run rule |> Review.Test.expectErrors [ Review.Test.error - { message = "Module `Html` is aliased as `Html`" + { message = "Module `Html` is aliased as itself" , details = [ "The alias is the same as the module name, and brings no useful value" ] , under = "Html" } @@ -1396,9 +1396,6 @@ port input : (() -> msg) -> Sub msg""" , details = details , under = "input" } - |> Review.Test.whenFixed """port module SomeModule exposing (a) -a = 1 -""" ] , test "should report unused ports (outgoing)" <| \() -> @@ -1412,8 +1409,5 @@ port output : String -> Cmd msg""" , details = details , under = "output" } - |> Review.Test.whenFixed """port module SomeModule exposing (a) -a = 1 -""" ] ] diff --git a/tests/NoUselessSubscriptions.elm b/tests/NoUselessSubscriptions.elm new file mode 100644 index 00000000..f1c772dd --- /dev/null +++ b/tests/NoUselessSubscriptions.elm @@ -0,0 +1,97 @@ +module NoUselessSubscriptions exposing (rule) + +{-| + +@docs rule + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression as Expression +import Elm.Syntax.Node as Node exposing (Node(..)) +import Review.Rule as Rule exposing (Error, Rule) + + +{-| Reports `subscriptions` functions that never return a subscription. + +In my opinion, this is often a sign of premature architectural work, where you +set up an `init`, `view`, `update` and `subscriptions` functions. I think +it is better to define them as they are needed, to avoid adding upfront complexity +that turn out to be unnecessary later. + + config = + [ NoUselessSubscriptions.rule + ] + + +## Fail + + subscriptions : Model -> Sub Msg + subscriptions model = + Sub.none + + +## Success + + main = + Browser.element + { init = init + , update = update + , subscriptions = \_ -> Sub.none + , view = view + } + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoUselessSubscriptions" () + |> Rule.withSimpleDeclarationVisitor declarationVisitor + |> Rule.fromModuleRuleSchema + + +declarationVisitor : Node Declaration -> List (Error {}) +declarationVisitor declaration = + case Node.value declaration of + Declaration.FunctionDeclaration function -> + if + (function.declaration + |> Node.value + |> .name + |> Node.value + ) + == "subscriptions" + then + case Node.value function.declaration |> .expression |> Node.value of + Expression.FunctionOrValue [ "Sub" ] "none" -> + [ error function + ] + + Expression.Application [ Node _ (Expression.FunctionOrValue [] "always"), Node _ (Expression.FunctionOrValue [ "Sub" ] "none") ] -> + [ error function + ] + + Expression.Application [ Node _ (Expression.FunctionOrValue [ "Basics" ] "always"), Node _ (Expression.FunctionOrValue [ "Sub" ] "none") ] -> + [ error function + ] + + Expression.Application [ Node _ (Expression.FunctionOrValue [ "Sub" ] "batch"), Node _ (Expression.ListExpr []) ] -> + [ error function + ] + + _ -> + [] + + else + [] + + _ -> + [] + + +error : Expression.Function -> Error {} +error function = + Rule.error + { message = "The `subscription` function never returns any subscriptions" + , details = [ "The `subscription` function never returns any subscriptions. You might as well remove it." ] + } + (Node.value function.declaration |> .expression |> Node.range) diff --git a/tests/NoUselessSubscriptionsTest.elm b/tests/NoUselessSubscriptionsTest.elm new file mode 100644 index 00000000..417963b7 --- /dev/null +++ b/tests/NoUselessSubscriptionsTest.elm @@ -0,0 +1,75 @@ +module NoUselessSubscriptionsTest exposing (all) + +import NoUselessSubscriptions exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "NoUselessSubscriptions" + [ test "should report an error when subscriptions returns Sub.none" <| + \_ -> + """ +module Main exposing (..) +subscriptions _ = Sub.none +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The `subscription` function never returns any subscriptions" + , details = [ "The `subscription` function never returns any subscriptions. You might as well remove it." ] + , under = "Sub.none" + } + ] + , test "should not report an error if it returns anything else" <| + \_ -> + """ +module Main exposing (..) +subscriptions _ = Time.every 5000 SomeMsg +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should report an error when subscriptions returns `Sub.batch []`" <| + \_ -> + """ +module Main exposing (..) +subscriptions _ = Sub.batch [] +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The `subscription` function never returns any subscriptions" + , details = [ "The `subscription` function never returns any subscriptions. You might as well remove it." ] + , under = "Sub.batch []" + } + ] + , test "should report an error when subscriptions returns `always Sub.none`" <| + \_ -> + """ +module Main exposing (..) +subscriptions = always Sub.none +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The `subscription` function never returns any subscriptions" + , details = [ "The `subscription` function never returns any subscriptions. You might as well remove it." ] + , under = "always Sub.none" + } + ] + , test "should report an error when subscriptions returns `Basics.always Sub.none`" <| + \_ -> + """ +module Main exposing (..) +subscriptions = Basics.always Sub.none +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The `subscription` function never returns any subscriptions" + , details = [ "The `subscription` function never returns any subscriptions. You might as well remove it." ] + , under = "Basics.always Sub.none" + } + ] + ] diff --git a/tests/Scope.elm b/tests/Scope.elm index 8c036590..86df2a98 100644 --- a/tests/Scope.elm +++ b/tests/Scope.elm @@ -27,7 +27,7 @@ module Scope exposing {- Copied over from https://github.com/jfmengels/elm-review-scope - Version: 0.2.0 + Version: 0.2.1 Copyright (c) 2020, Jeroen Engels All rights reserved. @@ -225,9 +225,9 @@ fromProjectToModule (ProjectContext projectContext) = fromModuleToProject : Node ModuleName -> ModuleContext -> ProjectContext fromModuleToProject moduleName (ModuleContext moduleContext) = ProjectContext - { dependenciesModules = moduleContext.dependenciesModules + { dependenciesModules = Dict.empty , modules = - Dict.insert (Node.value moduleName) + Dict.singleton (Node.value moduleName) { name = String.join "." (Node.value moduleName) , comment = "" , unions = moduleContext.exposedUnions @@ -235,7 +235,6 @@ fromModuleToProject moduleName (ModuleContext moduleContext) = , values = moduleContext.exposedValues , binops = moduleContext.exposedBinops } - moduleContext.modules } @@ -250,10 +249,10 @@ fromModuleToProject moduleName (ModuleContext moduleContext) = -} foldProjectContexts : ProjectContext -> ProjectContext -> ProjectContext -foldProjectContexts (ProjectContext a) (ProjectContext b) = +foldProjectContexts (ProjectContext newContext) (ProjectContext previousContext) = ProjectContext - { dependenciesModules = Dict.union b.dependenciesModules a.dependenciesModules - , modules = Dict.union b.modules a.modules + { dependenciesModules = previousContext.dependenciesModules + , modules = Dict.union previousContext.modules newContext.modules }