diff --git a/tests/NoUnused/Exports.elm b/tests/NoUnused/Exports.elm index abbc7bfe..9e5758bc 100644 --- a/tests/NoUnused/Exports.elm +++ b/tests/NoUnused/Exports.elm @@ -1,9 +1,89 @@ -module NoUnused.Exports exposing (rule) +module NoUnused.Exports exposing + ( rule + , Configuration, defaults, toRule + , reportUnusedProductionExports + , Exception, annotatedBy, suffixedBy, prefixedBy, definedInModule + ) -{-| Forbid the use of exposed elements that are never used in your project. +{-| Forbid the use of exposed elements (functions, values or types) that are never used in your project. + +🔧 Running with `--fix` will automatically remove all the reported errors, +except for the ones reported when using [`reportUnusedProductionExports`](#reportUnusedProductionExports). +It won't automatically remove unused modules though. + +If the project is a package and the module that declared the element is exposed, +then nothing will be reported. @docs rule + +## Going one step further + +This rule can be configured to report more unused elements than the default configuration. + +@docs Configuration, defaults, toRule + +By default, this rule only reports exposed elements that are never imported in other modules. +It is however pretty common to have elements imported and used in non-production parts of the codebase, +such as in tests or in a styleguide. + +For instance, let's say there is a module `A` that exposes a function `someFunction`: + + module A exposing (someFunction) + + someFunction input = + doSomethingComplexWith input + +And there is this test module to test `A.someFunction`: + + module ATest exposing (tests) + + import A + import Test exposing (Test, describe, test) + + tests : Test + tests = + describe "A.someFunction" + [ test "does something complex" <| + \() -> + A.someFunction someInput + |> Expect.equal someExpectedOutput + ] + +Let's say this is the only use of `A.someFunction` in the entire project. +Because `A.someFunction` is technically used in the project, this rule won't report it. + +But since the function is not used in production code, it is a good practice to remove it, as that will remove the +amount of code that needs to be maintained unnecessarily. We can detect that using [`reportUnusedProductionExports`](#reportUnusedProductionExports). + +@docs reportUnusedProductionExports + +@docs Exception, annotatedBy, suffixedBy, prefixedBy, definedInModule + + +## Try it out + +You can try this rule out by running the following commands: + +Using the default configuration: + +```bash +elm-review --template jfmengels/elm-review-unused/example --rules NoUnused.Exports +``` + +Using `reportUnusedProductionExports` with the following configuration: + + NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = \{ moduleName, filePath, isInSourceDirectories } -> isInSourceDirectories + , exceptionsAre = [ annotatedBy "@test-helper", suffixedBy "_FOR_TESTS" ] + } + |> NoUnused.Exports.toRule + +```bash +elm-review --template jfmengels/elm-review-unused/example-ignore-tests --rules NoUnused.Exports +``` + -} -- TODO Don't report type or type aliases (still `A(..)` though) if they are @@ -33,46 +113,335 @@ import Set exposing (Set) {-| Report functions and types that are exposed from a module but that are never used in other modules. Also reports when a module is entirely unused. -🔧 Running with `--fix` will automatically remove all the reported errors. -It won't automatically remove unused modules though. - -If the project is a package and the module that declared the element is exposed, -then nothing will be reported. - config = [ NoUnused.Exports.rule ] - -## Try it out - -You can try this rule out by running the following command: - -```bash -elm-review --template jfmengels/elm-review-unused/example --rules NoUnused.Exports -``` +This is equivalent to `NoUnused.Exports.toRule NoUnused.Exports.defaults`. -} rule : Rule rule = + toRule defaults + + +{-| Configuration for the rule. Use [`defaults`](#defaults) to get a default configuration and use [`toRule`](#toRule) to turn it into a rule. +You can change the configuration using [`reportUnusedProductionExports`](#reportUnusedProductionExports). +-} +type Configuration + = Configuration Config + + +type alias Config = + { isProductionFile : { moduleName : ModuleName, filePath : String, isInSourceDirectories : Bool } -> Bool + , exceptionModules : List ({ moduleName : ModuleName, filePath : String } -> Bool) + , exceptionTags : List String + , exceptionByName : Maybe (String -> Bool) + , exceptionExplanation : Maybe String + } + + +{-| Default configuration. This will only report exported elements that are never used in other modules. +-} +defaults : Configuration +defaults = + Configuration + { isProductionFile = always True + , exceptionModules = [] + , exceptionTags = [] + , exceptionByName = Nothing + , exceptionExplanation = Nothing + } + + +{-| Configures the rule to report elements defined in production code but only used in non-production files. + + import NoUnused.Exports exposing (annotatedBy) + + config = + [ NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = + \{ moduleName, filePath, isInSourceDirectories } -> + isInSourceDirectories + && not (String.endsWith "/Example.elm" filePath) + , exceptionsAre = [ annotatedBy "@test-helper" ] + } + |> NoUnused.Exports.toRule + ] + +Elements reported using this configuration won't be automatically fixed as they require removing the code +that uses the element. + +This function needs to know two things: + +1. Which files are considered to be production files, which is determined by a function that you provide. + Generally, production files are in the `"source-directories"`, which is indicated by + `isInSourceDirectories` (given as an argument to the function) being `True`. If you want to exclude + more files, you can use the `filePath` or `moduleName` of the Elm module, whichever is more practical for you to use. + `filePath` is relative to the folder containing the `elm.json` file and is written in a UNIX format (`/`, no `\`). + +2. How to identify exceptions. See [`Exception`](#Exception) for more information. + +-} +reportUnusedProductionExports : + { isProductionFile : { moduleName : ModuleName, filePath : String, isInSourceDirectories : Bool } -> Bool + , exceptionsAre : List Exception + } + -> Configuration + -> Configuration +reportUnusedProductionExports { isProductionFile, exceptionsAre } _ = + let + affixMatches : List (String -> Bool) + affixMatches = + List.filterMap + (\helper -> + case helper of + AnnotatedBy _ -> + Nothing + + SuffixedBy suffix -> + Just (\name -> String.endsWith suffix name) + + PrefixedBy prefix -> + Just (\name -> String.startsWith prefix name) + + DefinedInModule _ -> + Nothing + ) + exceptionsAre + + exceptionModules : List ({ moduleName : ModuleName, filePath : String } -> Bool) + exceptionModules = + List.filterMap + (\helper -> + case helper of + AnnotatedBy _ -> + Nothing + + SuffixedBy _ -> + Nothing + + PrefixedBy _ -> + Nothing + + DefinedInModule predicate -> + Just predicate + ) + exceptionsAre + + exceptionTags : List String + exceptionTags = + List.filterMap + (\helper -> + case helper of + AnnotatedBy tag -> + Just tag + + SuffixedBy _ -> + Nothing + + PrefixedBy _ -> + Nothing + + DefinedInModule _ -> + Nothing + ) + exceptionsAre + + exceptionByName : Maybe (String -> Bool) + exceptionByName = + if List.isEmpty affixMatches then + Nothing + + else + Just (\name -> List.any (\predicate -> predicate name) affixMatches) + in + Configuration + { isProductionFile = isProductionFile + , exceptionModules = exceptionModules + , exceptionTags = exceptionTags + , exceptionByName = exceptionByName + , exceptionExplanation = createExceptionsExplanation exceptionsAre + } + + +createExceptionsExplanation : List Exception -> Maybe String +createExceptionsExplanation exceptions = + if List.isEmpty exceptions then + Nothing + + else + let + options : List String + options = + List.map + (\helper -> + case helper of + AnnotatedBy tag -> + "Include " ++ tag ++ " in the documentation of the element" + + SuffixedBy suffix -> + "Rename the element to end with " ++ suffix + + PrefixedBy prefix -> + "Rename the element to start with " ++ prefix + + DefinedInModule _ -> + "Adapt your configuration to mark the whole module to as an exception" + ) + exceptions + in + Just ("- " ++ String.join "\n- " options) + + +{-| Predicate to identify exceptions (that shouldn't be reported) for elements defined in production code that are only used in non-production code. + +A problem with reporting these elements is that it's going to produce false positives, as there are legitimate use-cases +for exporting these elements, hence the need for the rule to be able to identify them. + +For instance, while it's generally discouraged, you might want to test the internals of an API (to make sure some properties hold +given very specific situations). In this case, your module then needs to expose a way to gain insight to the internals. + +Another example is giving the means for tests to create opaque types that are impossible or very hard to create in a test +environment. This can be the case for types that can only be created through the decoding of an HTTP request. + +Note that another common way to handle these use-cases is to move the internals to another module that exposes everything +while making sure only specific production modules import it. + +-} +type Exception + = AnnotatedBy String + | SuffixedBy String + | PrefixedBy String + | DefinedInModule ({ moduleName : ModuleName, filePath : String } -> Bool) + + +{-| Prevents reporting usages of elements that contain a specific tag in their documentation. + +Given the following configuration + + NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = isProductionFile + , exceptionsAre = [ annotatedBy "@test-helper" ] + } + |> NoUnused.Exports.toRule + +any element that has `@test-helper` in its documentation will not be reported as unused (as long as its used at least once in the project): + + {-| @test-helper + -} + someFunction input = + doSomethingComplexWith input + +A recommended practice is to have annotations start with `@`. + +You can use this function several times to define multiple annotations. + +-} +annotatedBy : String -> Exception +annotatedBy = + AnnotatedBy + + +{-| Prevents reporting usages of elements whose name end with a specific string. + +Given the following configuration + + NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = isProductionFile + , exceptionsAre = [ suffixedBy "_FOR_TESTS" ] + } + |> NoUnused.Exports.toRule + +any element that ends with `"_FOR_TESTS"` will not be reported as unused (as long as its used at least once in the project): + + someFunction_FOR_TESTS input = + doSomethingComplexWith input + +You can use this function several times to define multiple suffixes. + +-} +suffixedBy : String -> Exception +suffixedBy = + SuffixedBy + + +{-| Prevents reporting usages of elements whose name start with a specific string. + +Given the following configuration + + NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = isProductionFile + , exceptionsAre = [ prefixedBy "test_" ] + } + |> NoUnused.Exports.toRule + +any element that starts with `"test_"` will not be reported as unused (as long as its used at least once in the project): + + test_someFunction input = + doSomethingComplexWith input + +You can use this function several times to define multiple prefixes. + +-} +prefixedBy : String -> Exception +prefixedBy = + PrefixedBy + + +{-| Prevents reporting usages of elements in some modules. + +Given the following configuration + + NoUnused.Exports.defaults + |> NoUnused.Exports.reportUnusedProductionExports + { isProductionFile = isProductionFile + , exceptionsAre = + [ definedInModule + (\{ moduleName, filePath } -> + List.member "Util" moduleName + || String.startsWith "src/test-helpers/" filePath + ) + ] + } + |> NoUnused.Exports.toRule + +no elements from modules named `*.Util.*` or modules inside `src/test-helpers/` will be reported. + +The provided `filePath` is relative to the project's `elm.json` and is in a UNIX style (`/`, no `\`). + +-} +definedInModule : ({ moduleName : ModuleName, filePath : String } -> Bool) -> Exception +definedInModule = + DefinedInModule + + +{-| Creates a rule that reports unused exports using a [`Configuration`](#Configuration). +-} +toRule : Configuration -> Rule +toRule (Configuration config) = Rule.newProjectRuleSchema "NoUnused.Exports" initialProjectContext - |> Rule.withModuleVisitor moduleVisitor + |> Rule.withModuleVisitor (moduleVisitor config) |> Rule.withModuleContextUsingContextCreator { fromProjectToModule = fromProjectToModule - , fromModuleToProject = fromModuleToProject + , fromModuleToProject = fromModuleToProject config , foldProjectContexts = foldProjectContexts } |> Rule.withElmJsonProjectVisitor (\elmJson context -> ( [], elmJsonVisitor elmJson context )) - |> Rule.withFinalProjectEvaluation finalEvaluationForProject + |> Rule.withFinalProjectEvaluation (finalEvaluationForProject config.exceptionExplanation) |> Rule.providesFixesForProjectRule |> Rule.fromProjectRuleSchema -moduleVisitor : Rule.ModuleRuleSchema {} ModuleContext -> Rule.ModuleRuleSchema { hasAtLeastOneVisitor : () } ModuleContext -moduleVisitor schema = +moduleVisitor : Config -> Rule.ModuleRuleSchema {} ModuleContext -> Rule.ModuleRuleSchema { hasAtLeastOneVisitor : () } ModuleContext +moduleVisitor config schema = schema |> Rule.withImportVisitor (\node context -> ( [], importVisitor node context )) - |> Rule.withDeclarationEnterVisitor (\node context -> ( [], declarationVisitor node context )) + |> Rule.withDeclarationEnterVisitor (\node context -> ( [], declarationVisitor config node context )) |> Rule.withExpressionEnterVisitor (\node context -> ( [], expressionVisitor node context )) @@ -84,14 +453,18 @@ type alias ProjectContext = { projectType : ProjectType , modules : Dict - ModuleName + ModuleNameStr { moduleKey : Rule.ModuleKey , exposed : Dict String ExposedElement , moduleNameLocation : Range + , isProductionFile : Bool + , isProductionFileNotToReport : Bool + , ignoredElementsNotToReport : Set String } - , usedModules : Set ModuleName - , used : Set ( ModuleName, String ) - , constructors : Dict ( ModuleName, String ) String + , usedModules : Set ModuleNameStr + , used : Set ( ModuleNameStr, String ) + , usedInIgnoredModules : Set ( ModuleNameStr, String ) + , constructors : Dict ( ModuleNameStr, String ) String } @@ -104,7 +477,7 @@ type alias ExposedElement = type ProjectType = IsApplication ElmApplicationType - | IsPackage (Set (List String)) + | IsPackage (Set ModuleNameStr) type ElmApplicationType @@ -118,12 +491,17 @@ type ExposedElementType | ExposedType (List String) +type alias ModuleNameStr = + String + + type alias ModuleContext = { lookupTable : ModuleNameLookupTable , exposed : Dict String ExposedElement - , used : Set ( ModuleName, String ) + , used : Set ( ModuleNameStr, String ) , elementsNotToReport : Set String - , importedModules : Set ModuleName + , ignoredElementsNotToReport : Set String + , importedModules : Set ModuleNameStr , containsMainFunction : Bool , projectType : ProjectType } @@ -133,8 +511,9 @@ initialProjectContext : ProjectContext initialProjectContext = { projectType = IsApplication ElmApplication , modules = Dict.empty - , usedModules = Set.singleton [ "ReviewConfig" ] + , usedModules = Set.singleton "ReviewConfig" , used = Set.empty + , usedInIgnoredModules = Set.empty , constructors = Dict.empty } @@ -157,6 +536,7 @@ fromProjectToModule = , exposed = exposed , used = Set.empty , elementsNotToReport = Set.empty + , ignoredElementsNotToReport = Set.empty , importedModules = Set.empty , containsMainFunction = False , projectType = projectContext.projectType @@ -167,26 +547,52 @@ fromProjectToModule = |> Rule.withModuleDocumentation -fromModuleToProject : Rule.ContextCreator ModuleContext ProjectContext -fromModuleToProject = +fromModuleToProject : Config -> Rule.ContextCreator ModuleContext ProjectContext +fromModuleToProject config = Rule.initContextCreator - (\moduleKey (Node moduleNameRange moduleName) moduleContext -> + (\moduleKey (Node moduleNameRange moduleName) filePath isInSourceDirectories moduleContext -> + let + moduleNameStr : ModuleNameStr + moduleNameStr = + String.join "." moduleName + + used : Set ( ModuleNameStr, String ) + used = + Set.foldl + (\element acc -> Set.insert ( moduleNameStr, element ) acc) + moduleContext.used + moduleContext.elementsNotToReport + + isProductionFile : Bool + isProductionFile = + config.isProductionFile { moduleName = moduleName, filePath = filePath, isInSourceDirectories = isInSourceDirectories } + in { projectType = IsApplication ElmApplication , modules = Dict.singleton - moduleName + moduleNameStr { moduleKey = moduleKey , exposed = moduleContext.exposed , moduleNameLocation = moduleNameRange + , isProductionFile = isProductionFile + , isProductionFileNotToReport = any config.exceptionModules { moduleName = moduleName, filePath = filePath } + , ignoredElementsNotToReport = moduleContext.ignoredElementsNotToReport } , used = - Set.foldl - (\element acc -> Set.insert ( moduleName, element ) acc) - moduleContext.used - moduleContext.elementsNotToReport + if isProductionFile then + used + + else + Set.empty + , usedInIgnoredModules = + if isProductionFile then + Set.empty + + else + used , usedModules = - if Set.member [ "Test" ] moduleContext.importedModules || moduleContext.containsMainFunction then - Set.insert moduleName moduleContext.importedModules + if Set.member "Test" moduleContext.importedModules || moduleContext.containsMainFunction then + Set.insert moduleNameStr moduleContext.importedModules else moduleContext.importedModules @@ -196,7 +602,7 @@ fromModuleToProject = case element.elementType of ExposedType constructorNames -> List.foldl - (\constructorName listAcc -> Dict.insert ( moduleName, constructorName ) name listAcc) + (\constructorName listAcc -> Dict.insert ( moduleNameStr, constructorName ) name listAcc) acc constructorNames @@ -209,6 +615,22 @@ fromModuleToProject = ) |> Rule.withModuleKey |> Rule.withModuleNameNode + |> Rule.withFilePath + |> Rule.withIsInSourceDirectories + + +any : List (a -> Bool) -> a -> Bool +any list a = + case list of + [] -> + False + + head :: tail -> + if head a then + True + + else + any tail a foldProjectContexts : ProjectContext -> ProjectContext -> ProjectContext @@ -217,13 +639,14 @@ foldProjectContexts newContext previousContext = , modules = Dict.union newContext.modules previousContext.modules , usedModules = Set.union newContext.usedModules previousContext.usedModules , used = Set.union newContext.used previousContext.used + , usedInIgnoredModules = Set.union newContext.usedInIgnoredModules previousContext.usedInIgnoredModules , constructors = Dict.union newContext.constructors previousContext.constructors } -registerAsUsed : ( ModuleName, String ) -> ModuleContext -> ModuleContext -registerAsUsed ( moduleName, name ) moduleContext = - { moduleContext | used = Set.insert ( moduleName, name ) moduleContext.used } +registerAsUsed : ( ModuleNameStr, String ) -> ModuleContext -> ModuleContext +registerAsUsed key moduleContext = + { moduleContext | used = Set.insert key moduleContext.used } @@ -249,7 +672,7 @@ elmJsonVisitor maybeProject projectContext = exposedModuleNames |> List.foldr (\moduleName acc -> - Set.insert (Elm.Module.toString moduleName |> String.split ".") acc + Set.insert (Elm.Module.toString moduleName) acc ) Set.empty |> IsPackage @@ -275,10 +698,10 @@ elmJsonVisitor maybeProject projectContext = -- PROJECT EVALUATION -finalEvaluationForProject : ProjectContext -> List (Error { useErrorForModule : () }) -finalEvaluationForProject projectContext = +finalEvaluationForProject : Maybe String -> ProjectContext -> List (Error { useErrorForModule : () }) +finalEvaluationForProject exceptionExplanation projectContext = let - used : Set ( ModuleName, String ) + used : Set ( ModuleNameStr, String ) used = Set.foldl (\(( moduleName, _ ) as key) acc -> @@ -292,7 +715,21 @@ finalEvaluationForProject projectContext = projectContext.used projectContext.used - filterExposedPackage_ : ModuleName -> Bool + usedInIgnoredModules : Set ( ModuleNameStr, String ) + usedInIgnoredModules = + Set.foldl + (\(( moduleName, _ ) as key) acc -> + case Dict.get key projectContext.constructors of + Just typeName -> + Set.insert ( moduleName, typeName ) acc + + Nothing -> + acc + ) + projectContext.usedInIgnoredModules + projectContext.usedInIgnoredModules + + filterExposedPackage_ : ModuleNameStr -> Bool filterExposedPackage_ = filterExposedPackage projectContext in @@ -302,7 +739,7 @@ finalEvaluationForProject projectContext = acc else if Set.member moduleName projectContext.usedModules then - errorsForModule projectContext used moduleName module_ acc + errorsForModule exceptionExplanation projectContext { used = used, usedInIgnoredModules = usedInIgnoredModules } moduleName module_ acc else unusedModuleError moduleName module_ :: acc @@ -311,38 +748,62 @@ finalEvaluationForProject projectContext = projectContext.modules -unusedModuleError : ModuleName -> { a | moduleKey : Rule.ModuleKey, moduleNameLocation : Range } -> Error scope +unusedModuleError : ModuleNameStr -> { a | moduleKey : Rule.ModuleKey, moduleNameLocation : Range } -> Error scope unusedModuleError moduleName { moduleKey, moduleNameLocation } = Rule.errorForModule moduleKey - { message = "Module `" ++ String.join "." moduleName ++ "` is never used." + { message = "Module `" ++ moduleName ++ "` is never used." , details = [ "This module is never used. You may want to remove it to keep your project clean, and maybe detect some unused code in your project." ] } moduleNameLocation -errorsForModule : ProjectContext -> Set ( ModuleName, String ) -> ModuleName -> { a | moduleKey : Rule.ModuleKey, exposed : Dict String ExposedElement } -> List (Error scope) -> List (Error scope) -errorsForModule projectContext used moduleName { moduleKey, exposed } acc = +errorsForModule : + Maybe String + -> ProjectContext + -> { used : Set ( ModuleNameStr, String ), usedInIgnoredModules : Set ( ModuleNameStr, String ) } + -> ModuleNameStr + -> + { a + | moduleKey : Rule.ModuleKey + , exposed : Dict String ExposedElement + , isProductionFile : Bool + , isProductionFileNotToReport : Bool + , ignoredElementsNotToReport : Set String + } + -> List (Error scope) + -> List (Error scope) +errorsForModule exceptionExplanation projectContext { used, usedInIgnoredModules } moduleName { moduleKey, exposed, isProductionFile, isProductionFileNotToReport, ignoredElementsNotToReport } acc = Dict.foldl (\name element subAcc -> if isUsedOrException projectContext used moduleName name then subAcc + else if Set.member ( moduleName, name ) usedInIgnoredModules then + if not isProductionFile || isProductionFileNotToReport || Set.member name ignoredElementsNotToReport then + subAcc + + else + Rule.errorForModule moduleKey + { message = what element.elementType ++ " `" ++ name ++ "` is never used in production code." + , details = + "This exposed element is only used in files you have marked as non-production code (e.g. the tests folder), and should therefore be removed along with the places it's used in. This will help reduce the amount of code you will need to maintain." + :: (case exceptionExplanation of + Nothing -> + [ "It is possible that this element is meant to enable work in your ignored folder (test helpers for instance), in which case you should keep it. To avoid this problem being reported again, please read the documentation on how to configure the rule." + ] + + Just explanation -> + [ "It is possible that this element is meant to enable work in your ignored folder (test helpers for instance), in which case you should keep it. To avoid this problem being reported again, you can:" + , explanation + ] + ) + } + element.range + :: subAcc + else - let - what : String - what = - case element.elementType of - Function -> - "Exposed function or value" - - TypeOrTypeAlias -> - "Exposed type or type alias" - - ExposedType _ -> - "Exposed type" - in Rule.errorForModuleWithFix moduleKey - { message = what ++ " `" ++ name ++ "` is never used outside this module." + { message = what element.elementType ++ " `" ++ 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." ] } element.range @@ -353,7 +814,20 @@ errorsForModule projectContext used moduleName { moduleKey, exposed } acc = exposed -filterExposedPackage : ProjectContext -> ModuleName -> Bool +what : ExposedElementType -> String +what elementType = + case elementType of + Function -> + "Exposed function or value" + + TypeOrTypeAlias -> + "Exposed type or type alias" + + ExposedType _ -> + "Exposed type" + + +filterExposedPackage : ProjectContext -> ModuleNameStr -> Bool filterExposedPackage projectContext = case projectContext.projectType of IsApplication _ -> @@ -363,11 +837,11 @@ filterExposedPackage projectContext = \moduleName -> not <| Set.member moduleName exposedModuleNames -isUsedOrException : ProjectContext -> Set ( ModuleName, String ) -> List String -> String -> Bool +isUsedOrException : ProjectContext -> Set ( ModuleNameStr, String ) -> ModuleNameStr -> String -> Bool isUsedOrException projectContext used moduleName name = Set.member ( moduleName, name ) used || isApplicationException projectContext name - || (moduleName == [ "ReviewConfig" ]) + || (moduleName == "ReviewConfig") isApplicationException : ProjectContext -> String -> Bool @@ -481,9 +955,10 @@ untilEndOfVariable name range = importVisitor : Node Import -> ModuleContext -> ModuleContext importVisitor (Node _ import_) moduleContext = let - moduleName : ModuleName + moduleName : ModuleNameStr moduleName = Node.value import_.moduleName + |> String.join "." in { moduleContext | used = collectUsedFromImport moduleName import_.exposingList moduleContext.used @@ -491,7 +966,7 @@ importVisitor (Node _ import_) moduleContext = } -collectUsedFromImport : ModuleName -> Maybe (Node Exposing) -> Set ( ModuleName, String ) -> Set ( ModuleName, String ) +collectUsedFromImport : ModuleNameStr -> Maybe (Node Exposing) -> Set ( ModuleNameStr, String ) -> Set ( ModuleNameStr, String ) collectUsedFromImport moduleName exposingList used = case Maybe.map Node.value exposingList of Just (Exposing.Explicit list) -> @@ -641,8 +1116,8 @@ collectExposedElementsHelp docsReferences declarations declaredNames canRemoveEx newAcc -declarationVisitor : Node Declaration -> ModuleContext -> ModuleContext -declarationVisitor node moduleContext = +declarationVisitor : Config -> Node Declaration -> ModuleContext -> ModuleContext +declarationVisitor config node moduleContext = let ( allUsedTypes, comesFromCustomTypeWithHiddenConstructors ) = typesUsedInDeclaration moduleContext node @@ -657,12 +1132,22 @@ declarationVisitor node moduleContext = ) |> maybeSetInsert (testFunctionName moduleContext node) - used : Set ( ModuleName, String ) + ignoredElementsNotToReport : Set String + ignoredElementsNotToReport = + case isException config node of + Just name -> + Set.insert name moduleContext.ignoredElementsNotToReport + + Nothing -> + moduleContext.ignoredElementsNotToReport + + used : Set ( ModuleNameStr, String ) used = List.foldl Set.insert moduleContext.used allUsedTypes in { moduleContext | elementsNotToReport = elementsNotToReport + , ignoredElementsNotToReport = ignoredElementsNotToReport , used = used , containsMainFunction = moduleContext.containsMainFunction @@ -670,6 +1155,101 @@ declarationVisitor node moduleContext = } +isException : Config -> Node Declaration -> Maybe String +isException config node = + if config.exceptionByName == Nothing && List.isEmpty config.exceptionTags then + Nothing + + else + case getDeclarationName node of + Just name -> + case config.exceptionByName of + Just exceptionByName -> + if exceptionByName name then + Just name + + else + isExceptionByAnnotation config name node + + Nothing -> + isExceptionByAnnotation config name node + + Nothing -> + Nothing + + +isExceptionByAnnotation : Config -> b -> Node Declaration -> Maybe b +isExceptionByAnnotation config name node = + if List.isEmpty config.exceptionTags then + Nothing + + else + case getDeclarationDocumentation node of + Just documentation -> + if List.any (\exceptionTag -> String.contains exceptionTag documentation) config.exceptionTags then + Just name + + else + Nothing + + Nothing -> + Nothing + + +getDeclarationName : Node Declaration -> Maybe String +getDeclarationName node = + case Node.value node of + Declaration.FunctionDeclaration { declaration } -> + Just (declaration |> Node.value |> .name |> Node.value) + + Declaration.AliasDeclaration { name } -> + Just (Node.value name) + + Declaration.CustomTypeDeclaration { name } -> + Just (Node.value name) + + Declaration.PortDeclaration { name } -> + Just (Node.value name) + + _ -> + Nothing + + +getDeclarationDocumentation : Node Declaration -> Maybe String +getDeclarationDocumentation node = + case Node.value node of + Declaration.FunctionDeclaration { documentation } -> + case documentation of + Just doc -> + Just (Node.value doc) + + Nothing -> + Nothing + + Declaration.AliasDeclaration { documentation } -> + case documentation of + Just doc -> + Just (Node.value doc) + + Nothing -> + Nothing + + Declaration.CustomTypeDeclaration { documentation } -> + case documentation of + Just doc -> + Just (Node.value doc) + + Nothing -> + Nothing + + Declaration.PortDeclaration _ -> + -- TODO When we have documentation syntax for ports + Nothing + + _ -> + Nothing + + doesModuleContainMainFunction : ProjectType -> Node Declaration -> Bool doesModuleContainMainFunction projectType declaration = case projectType of @@ -777,7 +1357,7 @@ testFunctionName moduleContext node = Nothing -typesUsedInDeclaration : ModuleContext -> Node Declaration -> ( List ( ModuleName, String ), Bool ) +typesUsedInDeclaration : ModuleContext -> Node Declaration -> ( List ( ModuleNameStr, String ), Bool ) typesUsedInDeclaration moduleContext declaration = case Node.value declaration of Declaration.FunctionDeclaration function -> @@ -794,7 +1374,7 @@ typesUsedInDeclaration moduleContext declaration = Declaration.CustomTypeDeclaration type_ -> let - typesUsedInArguments : List ( ModuleName, String ) + typesUsedInArguments : List ( ModuleNameStr, String ) typesUsedInArguments = List.foldl (\constructor acc -> collectTypesFromTypeAnnotation moduleContext (Node.value constructor).arguments acc) @@ -823,7 +1403,7 @@ typesUsedInDeclaration moduleContext declaration = ( [], False ) -collectTypesFromTypeAnnotation : ModuleContext -> List (Node TypeAnnotation) -> List ( ModuleName, String ) -> List ( ModuleName, String ) +collectTypesFromTypeAnnotation : ModuleContext -> List (Node TypeAnnotation) -> List ( ModuleNameStr, String ) -> List ( ModuleNameStr, String ) collectTypesFromTypeAnnotation moduleContext nodes acc = case nodes of [] -> @@ -837,7 +1417,7 @@ collectTypesFromTypeAnnotation moduleContext nodes acc = TypeAnnotation.Typed (Node range ( _, name )) params -> case ModuleNameLookupTable.moduleNameAt moduleContext.lookupTable range of Just moduleName -> - collectTypesFromTypeAnnotation moduleContext (params ++ restOfNodes) (( moduleName, name ) :: acc) + collectTypesFromTypeAnnotation moduleContext (params ++ restOfNodes) (( String.join "." moduleName, name ) :: acc) Nothing -> collectTypesFromTypeAnnotation moduleContext (params ++ restOfNodes) acc @@ -873,28 +1453,14 @@ expressionVisitor : Node Expression -> ModuleContext -> ModuleContext expressionVisitor node moduleContext = case Node.value node of Expression.FunctionOrValue _ name -> - case ModuleNameLookupTable.moduleNameFor moduleContext.lookupTable node of - Just moduleName -> - registerAsUsed - ( moduleName, name ) - moduleContext - - Nothing -> - moduleContext + registerLocalValue (Node.range node) name moduleContext Expression.RecordUpdateExpression (Node range name) _ -> - case ModuleNameLookupTable.moduleNameAt moduleContext.lookupTable range of - Just moduleName -> - registerAsUsed - ( moduleName, name ) - moduleContext - - Nothing -> - moduleContext + registerLocalValue range name moduleContext Expression.LetExpression { declarations } -> let - used : List ( ModuleName, String ) + used : List ( ModuleNameStr, String ) used = List.foldl (\declaration acc -> @@ -919,7 +1485,7 @@ expressionVisitor node moduleContext = Expression.CaseExpression { cases } -> let - usedConstructors : List ( ModuleName, String ) + usedConstructors : List ( ModuleNameStr, String ) usedConstructors = findUsedConstructors moduleContext.lookupTable @@ -932,7 +1498,24 @@ expressionVisitor node moduleContext = moduleContext -findUsedConstructors : ModuleNameLookupTable -> List (Node Pattern) -> List ( ModuleName, String ) -> List ( ModuleName, String ) +registerLocalValue : Range -> String -> ModuleContext -> ModuleContext +registerLocalValue range name moduleContext = + case ModuleNameLookupTable.moduleNameAt moduleContext.lookupTable range of + Just [] -> + if Dict.member name moduleContext.exposed then + { moduleContext | ignoredElementsNotToReport = Set.insert name moduleContext.ignoredElementsNotToReport } + + else + moduleContext + + Just moduleName -> + registerAsUsed ( String.join "." moduleName, name ) moduleContext + + Nothing -> + moduleContext + + +findUsedConstructors : ModuleNameLookupTable -> List (Node Pattern) -> List ( ModuleNameStr, String ) -> List ( ModuleNameStr, String ) findUsedConstructors lookupTable patterns acc = case patterns of [] -> @@ -942,11 +1525,11 @@ findUsedConstructors lookupTable patterns acc = case Node.value pattern of Pattern.NamedPattern qualifiedNameRef newPatterns -> let - newAcc : List ( ModuleName, String ) + newAcc : List ( ModuleNameStr, String ) newAcc = case ModuleNameLookupTable.moduleNameFor lookupTable pattern of Just moduleName -> - ( moduleName, qualifiedNameRef.name ) :: acc + ( String.join "." moduleName, qualifiedNameRef.name ) :: acc Nothing -> acc diff --git a/tests/NoUnused/ExportsTest.elm b/tests/NoUnused/ExportsTest.elm index 530870dc..ad1d6479 100644 --- a/tests/NoUnused/ExportsTest.elm +++ b/tests/NoUnused/ExportsTest.elm @@ -1,6 +1,6 @@ module NoUnused.ExportsTest exposing (all) -import NoUnused.Exports exposing (rule) +import NoUnused.Exports exposing (annotatedBy, defaults, definedInModule, prefixedBy, reportUnusedProductionExports, rule, suffixedBy, toRule) import Review.Test import Test exposing (Test, describe, test) import TestProject exposing (application, lamderaApplication, package) @@ -28,6 +28,7 @@ all = , importsTests , lamderaTests , unusedModuleTests + , reportUnusedProductionExportsTest -- TODO Add tests that report exposing the type's variants if they are never used. ] @@ -1219,3 +1220,332 @@ main = text "" |> Review.Test.runWithProjectData lamderaApplication rule |> Review.Test.expectNoErrors ] + + +reportUnusedProductionExportsTest : Test +reportUnusedProductionExportsTest = + describe "reportUnusedProductionExports" + [ test "should report functions that are only used in ignored files (no helpers defined)" <| + \() -> + [ """ +module Main exposing (main) +import A +main = A.used +""", """ +module A exposing (used, unusedInProductionCode) +used = 1 +unusedInProductionCode = 2 +""", """ +module ATest exposing (..) +import A +import Test +a = A.unusedInProductionCode +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [] + } + |> toRule + ) + |> Review.Test.expectErrorsForModules + [ ( "A" + , [ Review.Test.error + { message = "Exposed function or value `unusedInProductionCode` is never used in production code." + , details = + [ "This exposed element is only used in files you have marked as non-production code (e.g. the tests folder), and should therefore be removed along with the places it's used in. This will help reduce the amount of code you will need to maintain." + , "It is possible that this element is meant to enable work in your ignored folder (test helpers for instance), in which case you should keep it. To avoid this problem being reported again, please read the documentation on how to configure the rule." + ] + , under = "unusedInProductionCode" + } + |> Review.Test.atExactly { start = { row = 2, column = 26 }, end = { row = 2, column = 48 } } + ] + ) + ] + , test "should report functions that are only used in ignored files (helpers defined)" <| + \() -> + [ """ +module Main exposing (main) +import A +main = A.used +""", """ +module A exposing (used, unusedInProductionCode) +used = 1 +unusedInProductionCode = 2 +""", """ +module ATest exposing (..) +import A +import Test +a = A.unusedInProductionCode +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = + [ annotatedBy "@helper" + , annotatedBy "@test-helper" + , suffixedBy "_FOR_TESTS" + , prefixedBy "test_" + ] + } + |> toRule + ) + |> Review.Test.expectErrorsForModules + [ ( "A" + , [ Review.Test.error + { message = "Exposed function or value `unusedInProductionCode` is never used in production code." + , details = + [ "This exposed element is only used in files you have marked as non-production code (e.g. the tests folder), and should therefore be removed along with the places it's used in. This will help reduce the amount of code you will need to maintain." + , "It is possible that this element is meant to enable work in your ignored folder (test helpers for instance), in which case you should keep it. To avoid this problem being reported again, you can:" + , """- Include @helper in the documentation of the element +- Include @test-helper in the documentation of the element +- Rename the element to end with _FOR_TESTS +- Rename the element to start with test_""" + ] + , under = "unusedInProductionCode" + } + |> Review.Test.atExactly { start = { row = 2, column = 26 }, end = { row = 2, column = 48 } } + ] + ) + ] + , test "should not report exposed tests even if they're in an ignored module" <| + \() -> + [ """ +module Main exposing (main) +main = 1 +""", """ +module ATest exposing (tests) +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" [] +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should not report elements from ignored modules used in other ignored modules exposed tests even if they're in an ignored module" <| + \() -> + [ """ +module ATest exposing (tests) +import BTest +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" BTest.helper +""", """ +module BTest exposing (helper) +helper = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should not report elements only used in ignored modules if they're annotated with a tag" <| + \() -> + [ """ +module ATest exposing (tests) +import B +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" B.helper +""", """ +module B exposing (helper) +{-| @ignore-helper -} +helper = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [ annotatedBy "@ignore-helper" ] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should not report elements from ignored modules if they're imported only in tests but also used locally in the module" <| + \() -> + [ """ +module Main exposing (main) +import B +main = B.exposed +""", """ +module ATest exposing (tests) +import B +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" B.usedLocally +""", """ +module B exposing (exposed, usedLocally) +exposed = usedLocally + 1 +usedLocally = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should report elements never used anywhere even if they're annotated with a tag" <| + \() -> + [ """ +module ATest exposing (tests) +import Test exposing (Test) +import B +tests : Test +tests = Test.describe "thing" [] +""", """ +module B exposing (helper) +{-| @ignore-helper -} +helper = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [] + } + |> toRule + ) + |> Review.Test.expectErrorsForModules + [ ( "B" + , [ Review.Test.error + { message = "Exposed function or value `helper` is never used outside this module." + , details = unusedExposedElementDetails + , under = "helper" + } + |> Review.Test.atExactly { start = { row = 2, column = 20 }, end = { row = 2, column = 26 } } + ] + ) + ] + , test "should report elements never used anywhere even if their name ends with the configured suffix" <| + \() -> + [ """ +module Main exposing (main) +import B +main = B.b +""", """ +module ATest exposing (tests) +import B +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" B.helperTEST +""", """ +module B exposing (b, helperTEST) +b = 1 +helperTEST = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [ suffixedBy "TEST" ] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should report elements never used anywhere even if their name starts with the configured suffix" <| + \() -> + [ """ +module Main exposing (main) +import B +main = B.b +""", """ +module ATest exposing (tests) +import B +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" B.test_helper +""", """ +module B exposing (b, test_helper) +b = 1 +test_helper = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [ prefixedBy "test_" ] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should report elements never used anywhere even if they're defined in a module marked as an exception" <| + \() -> + [ """ +module Main exposing (main) +import Project.Utils.B as B +main = B.b +""", """ +module ATest exposing (tests) +import Project.Utils.B as B +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" B.helper +""", """ +module Project.Utils.B exposing (b, helper) +b = 1 +helper = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [ definedInModule (\{ moduleName } -> List.member "Utils" moduleName) ] + } + |> toRule + ) + |> Review.Test.expectNoErrors + , test "should report unused exports in ignored files as regular errors" <| + \() -> + [ """ +module Main exposing (main) +import B +main = B.b +""", """ +module ATest exposing (tests, unused) +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" [] +unused = 1 +""" ] + |> Review.Test.runOnModules + (defaults + |> reportUnusedProductionExports + { isProductionFile = \{ moduleName } -> String.join "." moduleName |> String.endsWith "Test" |> not + , exceptionsAre = [ prefixedBy "test_" ] + } + |> toRule + ) + |> Review.Test.expectErrorsForModules + [ ( "ATest" + , [ Review.Test.error + { message = "Exposed function or value `unused` is never used outside this module." + , details = unusedExposedElementDetails + , under = "unused" + } + |> Review.Test.atExactly { start = { row = 2, column = 31 }, end = { row = 2, column = 37 } } + |> Review.Test.whenFixed """ +module ATest exposing (tests) +import Test exposing (Test) +tests : Test +tests = Test.describe "thing" [] +unused = 1 +""" + ] + ) + ] + ]