From f9e1d8aef8b56b2423a42c543e6a5778d5ef81e8 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Thu, 1 Sep 2022 16:15:28 +0200 Subject: [PATCH] Backport rules from Simplify --- elm.json | 3 +- tests/Simplify.elm | 1423 +++++++++++----------- tests/Simplify/AstHelpers.elm | 25 + tests/Simplify/Evaluate.elm | 113 ++ tests/Simplify/Infer.elm | 532 ++++++++ tests/Simplify/InferTest.elm | 535 +++++++++ tests/Simplify/Normalize.elm | 421 ++++--- tests/Simplify/NormalizeTest.elm | 849 +++++++++++++ tests/Simplify/RangeDict.elm | 39 + tests/SimplifyTest.elm | 1931 ++++++++++++++++++++++++++---- 10 files changed, 4772 insertions(+), 1099 deletions(-) create mode 100644 tests/Simplify/AstHelpers.elm create mode 100644 tests/Simplify/Evaluate.elm create mode 100644 tests/Simplify/Infer.elm create mode 100644 tests/Simplify/InferTest.elm create mode 100644 tests/Simplify/NormalizeTest.elm create mode 100644 tests/Simplify/RangeDict.elm diff --git a/elm.json b/elm.json index c917b2fc..93caa950 100644 --- a/elm.json +++ b/elm.json @@ -23,6 +23,7 @@ }, "test-dependencies": { "elm/parser": "1.1.0 <= v < 2.0.0", - "elm/regex": "1.0.0 <= v < 2.0.0" + "elm/regex": "1.0.0 <= v < 2.0.0", + "pzp1997/assoc-list": "1.0.0 <= v < 2.0.0" } } diff --git a/tests/Simplify.elm b/tests/Simplify.elm index 23439409..4c092d99 100644 --- a/tests/Simplify.elm +++ b/tests/Simplify.elm @@ -46,6 +46,15 @@ Below is the list of all kinds of simplifications this rule applies. not True --> False + not (not x) + --> x + + not >> not + --> identity + + +### Comparisons + x == True --> x @@ -64,12 +73,6 @@ Below is the list of all kinds of simplifications this rule applies. { r | a = 1 } == { r | a = 2 } --> False - not (not x) - --> x - - not >> not - --> identity - ### If expressions @@ -89,6 +92,17 @@ Below is the list of all kinds of simplifications this rule applies. --> not condition + a = + if condition then + if not condition then + 1 + else + 2 + else + 3 + --> if condition then 2 else 3 + + ### Case expressions case condition of @@ -101,9 +115,11 @@ Below is the list of all kinds of simplifications this rule applies. True -> x --> if not condition then x else y + -- only when no variables are introduced in the pattern + -- and no custom types defined in the project are referenced case value of - A _ -> x - B -> x + Just _ -> x + Nothing -> x --> x @@ -116,6 +132,21 @@ Below is the list of all kinds of simplifications this rule applies. --> { a | c = 1 } +### Field access + + { a = b }.a + --> b + + { a | b = c }.b + --> c + + { a | b = c }.d + --> a.d + + (let a = b in c).d + --> let a = b in c.d + + ### Basics functions identity x @@ -446,8 +477,8 @@ Below is the list of all kinds of simplifications this rule applies. Set.map fn Set.empty -- same for Set.filter, Set.remove... --> Set.empty - Set.map identity list - --> list + Set.map identity set + --> set Set.map identity --> identity @@ -486,7 +517,7 @@ Below is the list of all kinds of simplifications this rule applies. --> ( Set.empty, Set.empty ) Set.partition (always True) set - --> ( list, Set.empty ) + --> ( set, Set.empty ) ### Dict @@ -503,6 +534,9 @@ Below is the list of all kinds of simplifications this rule applies. Dict.size Dict.empty --> 0 + Dict.member x Dict.empty + --> False + ### Cmd / Sub @@ -538,75 +572,46 @@ All of these also apply for `Sub`. -} import Dict exposing (Dict) -import Elm.Syntax.Declaration as Declaration exposing (Declaration) -import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Docs +import Elm.Syntax.Expression as Expression exposing (Expression, RecordSetter) 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 (Location, Range) -import Elm.Type -import Json.Decode as Decode import Review.Fix as Fix exposing (Fix) import Review.ModuleNameLookupTable as ModuleNameLookupTable exposing (ModuleNameLookupTable) import Review.Project.Dependency as Dependency exposing (Dependency) import Review.Rule as Rule exposing (Error, Rule) import Set exposing (Set) +import Simplify.AstHelpers as AstHelpers +import Simplify.Evaluate as Evaluate +import Simplify.Infer as Infer import Simplify.Match as Match exposing (Match(..)) import Simplify.Normalize as Normalize +import Simplify.RangeDict as RangeDict exposing (RangeDict) {-| Rule to simplify Elm code. -} rule : Configuration -> Rule rule (Configuration config) = - case parseTypeNames config.ignoreConstructors of - Ok [] -> - Rule.newModuleRuleSchemaUsingContextCreator "Simplify" initialModuleContext - |> moduleVisitor Set.empty - |> Rule.fromModuleRuleSchema - - Ok typeNamesList -> - let - typeNames : Set ( ModuleName, String ) - typeNames = - Set.fromList typeNamesList - in - Rule.newProjectRuleSchema "Simplify" initialContext - |> Rule.withDirectDependenciesProjectVisitor (dependenciesVisitor typeNames) - |> Rule.withModuleVisitor (moduleVisitor typeNames) - |> Rule.withModuleContextUsingContextCreator - { fromProjectToModule = fromProjectToModule - , fromModuleToProject = fromModuleToProject - , foldProjectContexts = foldProjectContexts - } - |> Rule.withContextFromImportedModules - |> Rule.withFinalProjectEvaluation (finalEvaluation config.ignoreConstructors) - |> Rule.fromProjectRuleSchema - - Err invalidTypes -> - Rule.configurationError "Simplify" - { message = "Invalid type names: " ++ (invalidTypes |> List.map wrapInBackticks |> String.join ", ") - , details = - [ "I expect valid type names to be passed to Simplify.ignoreCaseOfForTypes, that include the module name, like `Module.Name.TypeName`." - ] - } + Rule.newProjectRuleSchema "Simplify" initialContext + |> Rule.withDirectDependenciesProjectVisitor (dependenciesVisitor (Set.fromList config.ignoreConstructors)) + |> Rule.withModuleVisitor moduleVisitor + |> Rule.withModuleContextUsingContextCreator + { fromProjectToModule = fromProjectToModule + , fromModuleToProject = fromModuleToProject + , foldProjectContexts = \_ previous -> previous + } + |> Rule.fromProjectRuleSchema -moduleVisitor : Set ( ModuleName, String ) -> Rule.ModuleRuleSchema schemaState ModuleContext -> Rule.ModuleRuleSchema { schemaState | hasAtLeastOneVisitor : () } ModuleContext -moduleVisitor typeNames schema = +moduleVisitor : Rule.ModuleRuleSchema schemaState ModuleContext -> Rule.ModuleRuleSchema { schemaState | hasAtLeastOneVisitor : () } ModuleContext +moduleVisitor schema = schema - |> Rule.withDeclarationEnterVisitor declarationVisitor + |> Rule.withDeclarationEnterVisitor (\node context -> ( [], declarationVisitor node context )) |> Rule.withExpressionEnterVisitor expressionVisitor - |> addDeclarationListVisitor typeNames - - -addDeclarationListVisitor : Set ( ModuleName, String ) -> Rule.ModuleRuleSchema { schemaState | hasAtLeastOneVisitor : () } ModuleContext -> Rule.ModuleRuleSchema { schemaState | hasAtLeastOneVisitor : () } ModuleContext -addDeclarationListVisitor typeNames schema = - if Set.isEmpty typeNames then - schema - - else - Rule.withDeclarationListVisitor (declarationListVisitor typeNames) schema + |> Rule.withExpressionExitVisitor (\node context -> ( [], expressionExitVisitor node context )) @@ -635,24 +640,21 @@ defaults = Configuration { ignoreConstructors = [] } -{-| Ignore some reports about types used in case expressions. +{-| Ignore some reports about types from dependencies used in case expressions. This rule simplifies the following construct: module Module.Name exposing (..) - type Type = A | B - case value of - A -> x - B -> x + Just _ -> x + Nothing -> x --> x -In some cases, you may want to disable this simplification because you expect to change or add constructors to this custom type. -Keeping the case expression as it is will make the compiler remind you to update this code when you add new variants, which can be valuable. +(Since `v2.0.19`) it will not try to simplify the case expression when some of the patterns references custom types constructors +defined in the project. It will only do so for custom types that are defined in dependencies (including `elm/core`). -Using the following configuration, case of expressions — where all variants of the `Type` custom type -from the `Module.Name` module appear — will not be simplified. +If you do happen to want to disable this simplification for a type `Module.Name.Type`, you can configure the rule like this: config = [ Simplify.defaults @@ -660,20 +662,10 @@ from the `Module.Name` module appear — will not be simplified. |> Simplify.rule ] -Note that if you use a wildcard, you will still get the simplification, since in this case the compiler will -not remind you anyway. - - case value of - A -> x - _ -> x - --> x - I personally don't recommend to use this function too much, because this could be a sign of premature abstraction, and because I think that often [You Aren't Gonna Need this code](https://jfmengels.net/safe-dead-code-removal/#yagni-you-arent-gonna-need-it). -Only use it for custom types that you think will change soon. When using it, I recommend not keeping it there too long. -Come back after a while to see if this exception is still worth having. Maybe add a comment with the date and an -explanation next to each exception? +Please let me know by opening an issue if you do use this function, I am very curious to know; -} ignoreCaseOfForTypes : List String -> Configuration -> Configuration @@ -681,56 +673,12 @@ ignoreCaseOfForTypes ignoreConstructors (Configuration config) = Configuration { config | ignoreConstructors = ignoreConstructors ++ config.ignoreConstructors } -parseTypeNames : List String -> Result (List String) (List ( ModuleName, String )) -parseTypeNames strings = - let - parsedTypeNames : List (Result String ( ModuleName, String )) - parsedTypeNames = - List.map isValidType strings - - invalidTypeNames : List String - invalidTypeNames = - List.filterMap - (\result -> - case result of - Err typeName -> - Just typeName - - Ok _ -> - Nothing - ) - parsedTypeNames - in - if List.isEmpty invalidTypeNames then - parsedTypeNames - |> List.filterMap Result.toMaybe - |> Ok - - else - Err invalidTypeNames - - -isValidType : String -> Result String ( ModuleName, String ) -isValidType typeAsString = - case Decode.decodeString Elm.Type.decoder ("\"" ++ typeAsString ++ "\"") of - Ok (Elm.Type.Type name _) -> - case List.reverse <| String.split "." name of - functionName :: moduleName :: restOfModuleName -> - Ok ( List.reverse (moduleName :: restOfModuleName), functionName ) - - _ -> - Err typeAsString - - _ -> - Err typeAsString - - -- CONTEXT type alias ProjectContext = - { ignoredCustomTypes : List Constructor + { customTypesToReportInCases : Set ( ModuleName, ConstructorName ) } @@ -739,12 +687,18 @@ type alias ModuleContext = , moduleName : ModuleName , rangesToIgnore : List Range , rightSidesOfPlusPlus : List Range - , ignoredCustomTypes : List Constructor + , customTypesToReportInCases : Set ( ModuleName, ConstructorName ) , localIgnoredCustomTypes : List Constructor , constructorsToIgnore : Set ( ModuleName, String ) + , inferredConstantsDict : RangeDict Infer.Inferred + , inferredConstants : ( Infer.Inferred, List Infer.Inferred ) } +type alias ConstructorName = + String + + type alias Constructor = { moduleName : ModuleName , name : String @@ -754,77 +708,60 @@ type alias Constructor = initialContext : ProjectContext initialContext = - { ignoredCustomTypes = [] + { customTypesToReportInCases = Set.empty } fromModuleToProject : Rule.ContextCreator ModuleContext ProjectContext fromModuleToProject = - Rule.initContextCreator - (\moduleContext -> - { ignoredCustomTypes = moduleContext.localIgnoredCustomTypes - } - ) - - -initialModuleContext : Rule.ContextCreator () ModuleContext -initialModuleContext = - Rule.initContextCreator - (\lookupTable moduleName () -> - { lookupTable = lookupTable - , moduleName = moduleName - , rangesToIgnore = [] - , rightSidesOfPlusPlus = [] - , localIgnoredCustomTypes = [] - , ignoredCustomTypes = [] - , constructorsToIgnore = Set.empty - } - ) - |> Rule.withModuleNameLookupTable - |> Rule.withModuleName + Rule.initContextCreator (\_ -> initialContext) fromProjectToModule : Rule.ContextCreator ProjectContext ModuleContext fromProjectToModule = Rule.initContextCreator - (\lookupTable moduleName projectContext -> + (\lookupTable metadata projectContext -> { lookupTable = lookupTable - , moduleName = moduleName + , moduleName = Rule.moduleNameFromMetadata metadata , rangesToIgnore = [] , rightSidesOfPlusPlus = [] , localIgnoredCustomTypes = [] - , ignoredCustomTypes = projectContext.ignoredCustomTypes - , constructorsToIgnore = buildConstructorsToIgnore projectContext.ignoredCustomTypes + , customTypesToReportInCases = projectContext.customTypesToReportInCases + , constructorsToIgnore = Set.empty + , inferredConstantsDict = RangeDict.empty + , inferredConstants = ( Infer.empty, [] ) } ) |> Rule.withModuleNameLookupTable - |> Rule.withModuleName - - -buildConstructorsToIgnore : List Constructor -> Set ( ModuleName, String ) -buildConstructorsToIgnore constructors = - constructors - |> List.concatMap (\c -> List.map (Tuple.pair c.moduleName) c.constructors) - |> Set.fromList - - -foldProjectContexts : ProjectContext -> ProjectContext -> ProjectContext -foldProjectContexts newContext previousContext = - { ignoredCustomTypes = newContext.ignoredCustomTypes ++ previousContext.ignoredCustomTypes - } + |> Rule.withMetadata -- DEPENDENCIES VISITOR -dependenciesVisitor : Set ( ModuleName, String ) -> Dict String Dependency -> ProjectContext -> ( List nothing, ProjectContext ) -dependenciesVisitor typeNames dict _ = - ( [] - , { ignoredCustomTypes = +dependenciesVisitor : Set String -> Dict String Dependency -> ProjectContext -> ( List (Error scope), ProjectContext ) +dependenciesVisitor typeNamesAsStrings dict _ = + let + modules : List Elm.Docs.Module + modules = dict |> Dict.values |> List.concatMap Dependency.modules + + unions : Set String + unions = + List.concatMap (\module_ -> List.map (\union -> module_.name ++ "." ++ union.name) module_.unions) modules + |> Set.fromList + + unknownTypesToIgnore : List String + unknownTypesToIgnore = + Set.diff typeNamesAsStrings unions + |> Set.toList + + customTypesToReportInCases : Set ( ModuleName, String ) + customTypesToReportInCases = + modules |> List.concatMap (\mod -> let @@ -833,48 +770,33 @@ dependenciesVisitor typeNames dict _ = String.split "." mod.name in mod.unions - |> List.filter (\{ name } -> Set.member ( moduleName, name ) typeNames) - |> List.map - (\union -> - { moduleName = moduleName - , name = union.name - , constructors = List.map Tuple.first union.tags - } - ) + |> List.filter (\union -> not (Set.member (mod.name ++ "." ++ union.name) typeNamesAsStrings)) + |> List.concatMap (\union -> union.tags) + |> List.map (\( tagName, _ ) -> ( moduleName, tagName )) ) - } + |> Set.fromList + in + ( if List.isEmpty unknownTypesToIgnore then + [] + + else + [ errorForUnknownIgnoredConstructor unknownTypesToIgnore ] + , { customTypesToReportInCases = customTypesToReportInCases } ) - --- FINAL EVALUATION - - -finalEvaluation : List String -> ProjectContext -> List (Error { useErrorForModule : () }) -finalEvaluation ignoreConstructors projectContext = - let - list : List String - list = - projectContext.ignoredCustomTypes - |> List.map (\type_ -> String.join "." type_.moduleName ++ "." ++ type_.name) - |> Set.fromList - |> Set.diff (Set.fromList ignoreConstructors) - |> Set.toList - in - if List.isEmpty list then - [] - - else - [ Rule.globalError - { message = "Could not find type names: " ++ (String.join ", " <| List.map wrapInBackticks list) - , details = - [ "I expected to find these custom types in the code or dependencies, but I could not find them." - , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." - , "If you find that these types have been moved or renamed, please update your configuration." - , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." - ] - } - ] +errorForUnknownIgnoredConstructor : List String -> Error scope +errorForUnknownIgnoredConstructor list = + Rule.globalError + { message = "Could not find type names: " ++ (String.join ", " <| List.map wrapInBackticks list) + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } wrapInBackticks : String -> String @@ -883,50 +805,16 @@ wrapInBackticks s = --- DECLARATION LIST VISITOR - - -declarationListVisitor : Set ( ModuleName, String ) -> List (Node Declaration) -> ModuleContext -> ( List nothing, ModuleContext ) -declarationListVisitor constructorsToIgnore declarations context = - let - localIgnoredCustomTypes : List Constructor - localIgnoredCustomTypes = - List.filterMap (findCustomTypes constructorsToIgnore context) declarations - in - ( [] - , { context - | localIgnoredCustomTypes = localIgnoredCustomTypes - , ignoredCustomTypes = localIgnoredCustomTypes ++ context.ignoredCustomTypes - , constructorsToIgnore = Set.union (buildConstructorsToIgnore localIgnoredCustomTypes) context.constructorsToIgnore - } - ) - - -findCustomTypes : Set ( ModuleName, String ) -> ModuleContext -> Node Declaration -> Maybe Constructor -findCustomTypes constructorsToIgnore context node = - case Node.value node of - Declaration.CustomTypeDeclaration { name, constructors } -> - if Set.member ( context.moduleName, Node.value name ) constructorsToIgnore then - Just - { moduleName = context.moduleName - , name = Node.value name - , constructors = List.map (\constructor -> constructor |> Node.value |> .name |> Node.value) constructors - } - - else - Nothing - - _ -> - Nothing - - - -- DECLARATION VISITOR -declarationVisitor : Node a -> ModuleContext -> ( List nothing, ModuleContext ) +declarationVisitor : Node a -> ModuleContext -> ModuleContext declarationVisitor _ context = - ( [], { context | rangesToIgnore = [], rightSidesOfPlusPlus = [] } ) + { context + | rangesToIgnore = [] + , rightSidesOfPlusPlus = [] + , inferredConstantsDict = RangeDict.empty + } @@ -935,40 +823,71 @@ declarationVisitor _ context = expressionVisitor : Node Expression -> ModuleContext -> ( List (Error {}), ModuleContext ) expressionVisitor node context = - if List.member (Node.range node) context.rangesToIgnore then - ( [], context ) + let + newContext : ModuleContext + newContext = + case RangeDict.get (Node.range node) context.inferredConstantsDict of + Just inferredConstants -> + let + ( previous, previousStack ) = + context.inferredConstants + in + { context | inferredConstants = ( inferredConstants, previous :: previousStack ) } + + Nothing -> + context + in + if List.member (Node.range node) newContext.rangesToIgnore then + ( [], newContext ) else let - { errors, rangesToIgnore, rightSidesOfPlusPlus } = - expressionVisitorHelp node context + { errors, rangesToIgnore, rightSidesOfPlusPlus, inferredConstants } = + expressionVisitorHelp node newContext in ( errors - , { context - | rangesToIgnore = rangesToIgnore ++ context.rangesToIgnore - , rightSidesOfPlusPlus = - rightSidesOfPlusPlus ++ context.rightSidesOfPlusPlus + , { newContext + | rangesToIgnore = rangesToIgnore ++ newContext.rangesToIgnore + , rightSidesOfPlusPlus = rightSidesOfPlusPlus ++ newContext.rightSidesOfPlusPlus + , inferredConstantsDict = List.foldl (\( range, constants ) acc -> RangeDict.insert range constants acc) newContext.inferredConstantsDict inferredConstants } ) -errorsAndRangesToIgnore : List (Error {}) -> List Range -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range } +expressionExitVisitor : Node Expression -> ModuleContext -> ModuleContext +expressionExitVisitor node context = + if RangeDict.member (Node.range node) context.inferredConstantsDict then + case Tuple.second context.inferredConstants of + topOfStack :: restOfStack -> + { context | inferredConstants = ( topOfStack, restOfStack ) } + + [] -> + -- should never be empty + context + + else + context + + +errorsAndRangesToIgnore : List (Error {}) -> List Range -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range, inferredConstants : List ( Range, Infer.Inferred ) } errorsAndRangesToIgnore errors rangesToIgnore = { errors = errors , rangesToIgnore = rangesToIgnore , rightSidesOfPlusPlus = [] + , inferredConstants = [] } -onlyErrors : List (Error {}) -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range } +onlyErrors : List (Error {}) -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range, inferredConstants : List ( Range, Infer.Inferred ) } onlyErrors errors = { errors = errors , rangesToIgnore = [] , rightSidesOfPlusPlus = [] + , inferredConstants = [] } -expressionVisitorHelp : Node Expression -> ModuleContext -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range } +expressionVisitorHelp : Node Expression -> ModuleContext -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range, inferredConstants : List ( Range, Infer.Inferred ) } expressionVisitorHelp node context = case Node.value node of -------------------- @@ -983,6 +902,7 @@ expressionVisitorHelp node context = onlyErrors (checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , fnRange = fnRange , firstArg = firstArg @@ -998,105 +918,14 @@ expressionVisitorHelp node context = ------------------- -- IF EXPRESSION -- ------------------- - Expression.IfBlock cond trueBranch falseBranch -> - case getBoolean context.lookupTable cond of - Determined True -> - onlyErrors - [ Rule.errorWithFix - { message = "The condition will always evaluate to True" - , details = [ "The expression can be replaced by what is inside the 'then' branch." ] - } - (targetIf node) - [ Fix.removeRange - { start = (Node.range node).start - , end = (Node.range trueBranch).start - } - , Fix.removeRange - { start = (Node.range trueBranch).end - , end = (Node.range node).end - } - ] - ] - - Determined False -> - onlyErrors - [ Rule.errorWithFix - { message = "The condition will always evaluate to False" - , details = [ "The expression can be replaced by what is inside the 'else' branch." ] - } - (targetIf node) - [ Fix.removeRange - { start = (Node.range node).start - , end = (Node.range falseBranch).start - } - ] - ] - - Undetermined -> - case ( getBoolean context.lookupTable trueBranch, getBoolean context.lookupTable falseBranch ) of - ( Determined True, Determined False ) -> - onlyErrors - [ Rule.errorWithFix - { message = "The if expression's value is the same as the condition" - , details = [ "The expression can be replaced by the condition." ] - } - (targetIf node) - [ Fix.removeRange - { start = (Node.range node).start - , end = (Node.range cond).start - } - , Fix.removeRange - { start = (Node.range cond).end - , end = (Node.range node).end - } - ] - ] - - ( Determined False, Determined True ) -> - onlyErrors - [ Rule.errorWithFix - { message = "The if expression's value is the inverse of the condition" - , details = [ "The expression can be replaced by the condition wrapped by `not`." ] - } - (targetIf node) - [ Fix.replaceRangeBy - { start = (Node.range node).start - , end = (Node.range cond).start - } - "not (" - , Fix.replaceRangeBy - { start = (Node.range cond).end - , end = (Node.range node).end - } - ")" - ] - ] - - _ -> - case Normalize.compare context.lookupTable trueBranch falseBranch of - Normalize.ConfirmedEquality -> - onlyErrors - [ Rule.errorWithFix - { message = "The values in both branches is the same." - , details = [ "The expression can be replaced by the contents of either branch." ] - } - (targetIf node) - [ Fix.removeRange - { start = (Node.range node).start - , end = (Node.range trueBranch).start - } - , Fix.removeRange - { start = (Node.range trueBranch).end - , end = (Node.range node).end - } - ] - ] - - Normalize.ConfirmedInequality -> - onlyErrors [] - - Normalize.Unconfirmed -> - onlyErrors [] + Expression.IfBlock condition trueBranch falseBranch -> + ifChecks + context + (Node.range node) + { condition = condition + , trueBranch = trueBranch + , falseBranch = falseBranch + } ------------------------------- -- APPLIED LAMBDA FUNCTIONS -- @@ -1191,6 +1020,7 @@ expressionVisitorHelp node context = onlyErrors (checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , fnRange = fnRange , firstArg = firstArg @@ -1212,6 +1042,7 @@ expressionVisitorHelp node context = errorsAndRangesToIgnore (checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , fnRange = fnRange , firstArg = firstArg @@ -1237,6 +1068,7 @@ expressionVisitorHelp node context = onlyErrors (checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , fnRange = fnRange , firstArg = firstArg @@ -1258,6 +1090,7 @@ expressionVisitorHelp node context = errorsAndRangesToIgnore (checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , fnRange = fnRange , firstArg = firstArg @@ -1329,6 +1162,7 @@ expressionVisitorHelp node context = { errors = checkFn { lookupTable = context.lookupTable + , inferredConstants = context.inferredConstants , parentRange = Node.range node , operator = operator , left = left @@ -1340,18 +1174,19 @@ expressionVisitorHelp node context = , rangesToIgnore = [] , rightSidesOfPlusPlus = if operator == "++" then - [ Node.range <| removeParens right ] + [ Node.range <| AstHelpers.removeParens right ] else [] + , inferredConstants = [] } Nothing -> onlyErrors [] Expression.Negation baseExpr -> - case removeParens baseExpr of - Node range (Expression.Negation _) -> + case AstHelpers.removeParens baseExpr of + Node range (Expression.Negation negatedValue) -> let doubleNegationRange : Range doubleNegationRange = @@ -1365,18 +1200,146 @@ expressionVisitorHelp node context = , details = [ "Negating a number twice is the same as the number itself." ] } doubleNegationRange - [ Fix.replaceRangeBy doubleNegationRange "(" ] + (replaceBySubExpressionFix (Node.range node) negatedValue) ] _ -> onlyErrors [] + Expression.RecordAccess record field -> + case Node.value (AstHelpers.removeParens record) of + Expression.RecordExpr setters -> + onlyErrors (recordAccessChecks (Node.range node) Nothing (Node.value field) setters) + + Expression.RecordUpdateExpression (Node recordNameRange _) setters -> + onlyErrors (recordAccessChecks (Node.range node) (Just recordNameRange) (Node.value field) setters) + + Expression.LetExpression { expression } -> + onlyErrors (recordAccessLetInChecks (Node.range node) field expression) + + _ -> + onlyErrors [] + _ -> onlyErrors [] +recordAccessChecks : Range -> Maybe Range -> String -> List (Node RecordSetter) -> List (Error {}) +recordAccessChecks nodeRange recordNameRange fieldName setters = + case + findMap + (\(Node _ ( setterField, setterValue )) -> + if Node.value setterField == fieldName then + Just setterValue + + else + Nothing + ) + setters + of + Just setter -> + [ Rule.errorWithFix + { message = "Field access can be simplified" + , details = [ "Accessing the field of a record or record update can be simplified to just that field's value" ] + } + nodeRange + (replaceBySubExpressionFix nodeRange setter) + ] + + Nothing -> + case recordNameRange of + Just rnr -> + [ Rule.errorWithFix + { message = "Field access can be simplified" + , details = [ "Accessing the field of an unrelated record update can be simplified to just the original field's value" ] + } + nodeRange + [ Fix.replaceRangeBy { start = nodeRange.start, end = rnr.start } "" + , Fix.replaceRangeBy { start = rnr.end, end = nodeRange.end } ("." ++ fieldName) + ] + ] + + Nothing -> + [] + + +replaceBySubExpressionFix : Range -> Node Expression -> List Fix +replaceBySubExpressionFix outerRange (Node exprRange exprValue) = + if needsParens exprValue then + [ Fix.replaceRangeBy { start = outerRange.start, end = exprRange.start } "(" + , Fix.replaceRangeBy { start = exprRange.end, end = outerRange.end } ")" + ] + + else + [ Fix.removeRange { start = outerRange.start, end = exprRange.start } + , Fix.removeRange { start = exprRange.end, end = outerRange.end } + ] + + +needsParens : Expression -> Bool +needsParens expr = + case expr of + Expression.Application _ -> + True + + Expression.OperatorApplication _ _ _ _ -> + True + + Expression.IfBlock _ _ _ -> + True + + Expression.Negation _ -> + True + + Expression.LetExpression _ -> + True + + Expression.CaseExpression _ -> + True + + Expression.LambdaExpression _ -> + True + + _ -> + False + + +recordAccessLetInChecks : Range -> Node String -> Node Expression -> List (Error {}) +recordAccessLetInChecks nodeRange (Node fieldRange fieldName) expr = + let + fieldRangeStart : Location + fieldRangeStart = + fieldRange.start + + fieldRemovalFix : Fix + fieldRemovalFix = + Fix.removeRange + { start = { row = fieldRangeStart.row, column = fieldRangeStart.column - 1 } + , end = fieldRange.end + } + in + [ Rule.errorWithFix + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] + } + nodeRange + (if needsParens (Node.value expr) then + [ Fix.insertAt (Node.range expr).start "(" + , Fix.insertAt (Node.range expr).end (")." ++ fieldName) + , fieldRemovalFix + ] + + else + [ Fix.insertAt (Node.range expr).end ("." ++ fieldName) + , fieldRemovalFix + ] + ) + ] + + type alias CheckInfo = { lookupTable : ModuleNameLookupTable + , inferredConstants : ( Infer.Inferred, List Infer.Inferred ) , parentRange : Range , fnRange : Range , firstArg : Node Expression @@ -1405,6 +1368,7 @@ functionCallChecks = , reportEmptyListFirstArgument ( ( [ "List" ], "concat" ), listConcatChecks ) , reportEmptyListSecondArgument ( ( [ "List" ], "concatMap" ), listConcatMapChecks ) , reportEmptyListSecondArgument ( ( [ "List" ], "indexedMap" ), listIndexedMapChecks ) + , reportEmptyListSecondArgument ( ( [ "List" ], "intersperse" ), listIndexedMapChecks ) , ( ( [ "List" ], "all" ), listAllChecks ) , ( ( [ "List" ], "any" ), listAnyChecks ) , ( ( [ "List" ], "range" ), listRangeChecks ) @@ -1415,6 +1379,7 @@ functionCallChecks = , ( ( [ "List" ], "reverse" ), listReverseChecks ) , ( ( [ "List" ], "take" ), listTakeChecks ) , ( ( [ "List" ], "drop" ), listDropChecks ) + , ( ( [ "List" ], "member" ), collectionMemberChecks listCollection ) , ( ( [ "Set" ], "map" ), collectionMapChecks setCollection ) , ( ( [ "Set" ], "filter" ), collectionFilterChecks setCollection ) , ( ( [ "Set" ], "remove" ), collectionRemoveChecks setCollection ) @@ -1432,6 +1397,7 @@ functionCallChecks = , ( ( [ "Dict" ], "fromList" ), collectionFromListChecks dictCollection ) , ( ( [ "Dict" ], "toList" ), collectionToListChecks dictCollection ) , ( ( [ "Dict" ], "size" ), collectionSizeChecks dictCollection ) + , ( ( [ "Dict" ], "member" ), collectionMemberChecks dictCollection ) , ( ( [ "String" ], "isEmpty" ), stringIsEmptyChecks ) , ( ( [ "String" ], "concat" ), stringConcatChecks ) , ( ( [ "String" ], "join" ), stringJoinChecks ) @@ -1453,6 +1419,7 @@ functionCallChecks = type alias OperatorCheckInfo = { lookupTable : ModuleNameLookupTable + , inferredConstants : ( Infer.Inferred, List Infer.Inferred ) , parentRange : Range , operator : String , left : Node Expression @@ -1528,7 +1495,7 @@ removeAlongWithOtherFunctionCheck : -> CheckInfo -> List (Error {}) removeAlongWithOtherFunctionCheck errorMessage secondFunctionCheck checkInfo = - case Node.value (removeParens checkInfo.firstArg) of + case Node.value (AstHelpers.removeParens checkInfo.firstArg) of Expression.Application (secondFn :: firstArgOfSecondCall :: _) -> case secondFunctionCheck checkInfo.lookupTable secondFn of Just secondRange -> @@ -1702,20 +1669,6 @@ divisionChecks { leftRange, rightRange, right } = [] -find : (a -> Bool) -> List a -> Maybe a -find predicate nodes = - case nodes of - [] -> - Nothing - - node :: rest -> - if predicate node then - Just node - - else - find predicate rest - - findMap : (a -> Maybe b) -> List a -> Maybe b findMap mapper nodes = case nodes of @@ -1776,7 +1729,7 @@ plusplusChecks { parentRange, leftRange, rightRange, left, right, isOnTheRightSi ] ] - ( Expression.ListExpr [ _ ], _ ) -> + ( Expression.ListExpr [ listElement ], _ ) -> if isOnTheRightSideOfPlusPlus then [] @@ -1786,17 +1739,13 @@ plusplusChecks { parentRange, leftRange, rightRange, left, right, isOnTheRightSi , details = [ "Concatenating a list with a single value is the same as using (::) on the list with the value." ] } parentRange - [ Fix.replaceRangeBy - { start = leftRange.start - , end = { row = leftRange.start.row, column = leftRange.start.column + 1 } - } - "(" - , Fix.replaceRangeBy - { start = { row = leftRange.end.row, column = leftRange.end.column - 1 } + (Fix.replaceRangeBy + { start = leftRange.end , end = rightRange.start } - ") :: " - ] + " :: " + :: replaceBySubExpressionFix leftRange listElement + ) ] _ -> @@ -1919,7 +1868,7 @@ negateCompositionCheck { lookupTable, fromLeftToRight, parentRange, left, right, getNegateComposition : ModuleNameLookupTable -> Bool -> Node Expression -> Maybe Range getNegateComposition lookupTable takeFirstFunction node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.OperatorApplication "<<" _ left right -> if takeFirstFunction then getNegateFunction lookupTable right @@ -1949,7 +1898,7 @@ basicsNegateChecks checkInfo = getNegateFunction : ModuleNameLookupTable -> Node Expression -> Maybe Range getNegateFunction lookupTable baseNode = - case removeParens baseNode of + case AstHelpers.removeParens baseNode of Node range (Expression.FunctionOrValue _ "negate") -> case ModuleNameLookupTable.moduleNameAt lookupTable range of Just [ "Basics" ] -> @@ -1968,7 +1917,7 @@ getNegateFunction lookupTable baseNode = basicsNotChecks : CheckInfo -> List (Error {}) basicsNotChecks checkInfo = - case getBoolean checkInfo.lookupTable checkInfo.firstArg of + case Evaluate.getBoolean checkInfo checkInfo.firstArg of Determined bool -> [ Rule.errorWithFix { message = "Expression is equal to " ++ boolToString (not bool) @@ -2042,7 +1991,7 @@ notNotCompositionErrorMessage = getNotComposition : ModuleNameLookupTable -> Bool -> Node Expression -> Maybe Range getNotComposition lookupTable takeFirstFunction node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.OperatorApplication "<<" _ left right -> if takeFirstFunction then getNotFunction lookupTable right @@ -2097,7 +2046,7 @@ findSimilarConditionsError operatorCheckInfo = errorsForNode nodeToCompareTo = List.concatMap (areSimilarConditionsError - operatorCheckInfo.lookupTable + operatorCheckInfo operatorCheckInfo.operator nodeToCompareTo ) @@ -2108,9 +2057,9 @@ findSimilarConditionsError operatorCheckInfo = |> List.concatMap (Tuple.second >> errorsForNode) -areSimilarConditionsError : ModuleNameLookupTable -> String -> Node Expression -> ( RedundantConditionResolution, Node Expression ) -> List (Error {}) -areSimilarConditionsError lookupTable operator nodeToCompareTo ( redundantConditionResolution, nodeToLookAt ) = - case Normalize.compare lookupTable nodeToCompareTo nodeToLookAt of +areSimilarConditionsError : Infer.Resources a -> String -> Node Expression -> ( RedundantConditionResolution, Node Expression ) -> List (Error {}) +areSimilarConditionsError resources operator nodeToCompareTo ( redundantConditionResolution, nodeToLookAt ) = + case Normalize.compare resources nodeToCompareTo nodeToLookAt of Normalize.ConfirmedEquality -> errorForRedundantCondition operator redundantConditionResolution nodeToLookAt @@ -2188,11 +2137,11 @@ listConditions operatorToLookFor redundantConditionResolution node = or_isLeftSimplifiableError : OperatorCheckInfo -> List (Error {}) -or_isLeftSimplifiableError { lookupTable, parentRange, left, leftRange, rightRange } = - case getBoolean lookupTable left of +or_isLeftSimplifiableError ({ parentRange, left, leftRange, rightRange } as checkInfo) = + case Evaluate.getBoolean checkInfo left of Determined True -> [ Rule.errorWithFix - { message = "Condition is always True" + { message = "Comparison is always True" , details = alwaysSameDetails } parentRange @@ -2221,8 +2170,8 @@ or_isLeftSimplifiableError { lookupTable, parentRange, left, leftRange, rightRan or_isRightSimplifiableError : OperatorCheckInfo -> List (Error {}) -or_isRightSimplifiableError { lookupTable, parentRange, right, leftRange, rightRange } = - case getBoolean lookupTable right of +or_isRightSimplifiableError ({ parentRange, right, leftRange, rightRange } as checkInfo) = + case Evaluate.getBoolean checkInfo right of Determined True -> [ Rule.errorWithFix { message = unnecessaryMessage @@ -2267,8 +2216,8 @@ andChecks operatorCheckInfo = and_isLeftSimplifiableError : OperatorCheckInfo -> List (Rule.Error {}) -and_isLeftSimplifiableError { lookupTable, parentRange, left, leftRange, rightRange } = - case getBoolean lookupTable left of +and_isLeftSimplifiableError ({ parentRange, left, leftRange, rightRange } as checkInfo) = + case Evaluate.getBoolean checkInfo left of Determined True -> [ Rule.errorWithFix { message = unnecessaryMessage @@ -2284,7 +2233,7 @@ and_isLeftSimplifiableError { lookupTable, parentRange, left, leftRange, rightRa Determined False -> [ Rule.errorWithFix - { message = "Condition is always False" + { message = "Comparison is always False" , details = alwaysSameDetails } parentRange @@ -2300,8 +2249,8 @@ and_isLeftSimplifiableError { lookupTable, parentRange, left, leftRange, rightRa and_isRightSimplifiableError : OperatorCheckInfo -> List (Rule.Error {}) -and_isRightSimplifiableError { lookupTable, parentRange, leftRange, right, rightRange } = - case getBoolean lookupTable right of +and_isRightSimplifiableError ({ parentRange, leftRange, right, rightRange } as checkInfo) = + case Evaluate.getBoolean checkInfo right of Determined True -> [ Rule.errorWithFix { message = unnecessaryMessage @@ -2317,7 +2266,7 @@ and_isRightSimplifiableError { lookupTable, parentRange, leftRange, right, right Determined False -> [ Rule.errorWithFix - { message = "Condition is always False" + { message = "Comparison is always False" , details = alwaysSameDetails } parentRange @@ -2337,8 +2286,8 @@ and_isRightSimplifiableError { lookupTable, parentRange, leftRange, right, right equalityChecks : Bool -> OperatorCheckInfo -> List (Error {}) -equalityChecks isEqual { lookupTable, parentRange, left, right, leftRange, rightRange } = - if getBoolean lookupTable right == Determined isEqual then +equalityChecks isEqual ({ lookupTable, parentRange, left, right, leftRange, rightRange } as checkInfo) = + if Evaluate.getBoolean checkInfo right == Determined isEqual then [ Rule.errorWithFix { message = "Unnecessary comparison with boolean" , details = [ "The result of the expression will be the same with or without the comparison." ] @@ -2347,7 +2296,7 @@ equalityChecks isEqual { lookupTable, parentRange, left, right, leftRange, right [ Fix.removeRange { start = leftRange.end, end = rightRange.end } ] ] - else if getBoolean lookupTable left == Determined isEqual then + else if Evaluate.getBoolean checkInfo left == Determined isEqual then [ Rule.errorWithFix { message = "Unnecessary comparison with boolean" , details = [ "The result of the expression will be the same with or without the comparison." ] @@ -2368,26 +2317,39 @@ equalityChecks isEqual { lookupTable, parentRange, left, right, leftRange, right ] _ -> - case Normalize.compare lookupTable left right of + let + inferred : Infer.Inferred + inferred = + Tuple.first checkInfo.inferredConstants + + normalizeAndInfer : Node Expression -> Node Expression + normalizeAndInfer node = + let + newNode : Node Expression + newNode = + Normalize.normalize checkInfo node + in + case Infer.get (Node.value newNode) inferred of + Just expr -> + Node Range.emptyRange expr + + Nothing -> + newNode + + normalizedLeft : Node Expression + normalizedLeft = + normalizeAndInfer left + + normalizedRight : Node Expression + normalizedRight = + normalizeAndInfer right + in + case Normalize.compareWithoutNormalization normalizedLeft normalizedRight of Normalize.ConfirmedEquality -> - [ Rule.errorWithFix - { message = "Condition is always " ++ boolToString isEqual - , details = sameThingOnBothSidesDetails isEqual - } - parentRange - [ Fix.replaceRangeBy parentRange (boolToString isEqual) - ] - ] + [ comparisonError isEqual parentRange ] Normalize.ConfirmedInequality -> - [ Rule.errorWithFix - { message = "Condition is always " ++ boolToString (not isEqual) - , details = sameThingOnBothSidesDetails (not isEqual) - } - parentRange - [ Fix.replaceRangeBy parentRange (boolToString (not isEqual)) - ] - ] + [ comparisonError (not isEqual) parentRange ] Normalize.Unconfirmed -> [] @@ -2395,7 +2357,7 @@ equalityChecks isEqual { lookupTable, parentRange, left, right, leftRange, right getNotCall : ModuleNameLookupTable -> Node Expression -> Maybe Range getNotCall lookupTable baseNode = - case Node.value (removeParens baseNode) of + case Node.value (AstHelpers.removeParens baseNode) of Expression.Application ((Node notRange (Expression.FunctionOrValue _ "not")) :: _) -> case ModuleNameLookupTable.moduleNameAt lookupTable notRange of Just [ "Basics" ] -> @@ -2410,7 +2372,7 @@ getNotCall lookupTable baseNode = getNotFunction : ModuleNameLookupTable -> Node Expression -> Maybe Range getNotFunction lookupTable baseNode = - case removeParens baseNode of + case AstHelpers.removeParens baseNode of Node notRange (Expression.FunctionOrValue _ "not") -> case ModuleNameLookupTable.moduleNameAt lookupTable notRange of Just [ "Basics" ] -> @@ -2425,7 +2387,7 @@ getNotFunction lookupTable baseNode = getSpecificFunction : ( ModuleName, String ) -> ModuleNameLookupTable -> Node Expression -> Maybe Range getSpecificFunction ( moduleName, name ) lookupTable baseNode = - case removeParens baseNode of + case AstHelpers.removeParens baseNode of Node fnRange (Expression.FunctionOrValue _ foundName) -> if (foundName == name) @@ -2445,7 +2407,7 @@ getSpecificFunctionCall ( moduleName, name ) lookupTable baseNode = let match : Maybe ( Range, String ) match = - case Node.value (removeParens baseNode) of + case Node.value (AstHelpers.removeParens baseNode) of Expression.Application ((Node fnRange (Expression.FunctionOrValue _ foundName)) :: _ :: _) -> Just ( fnRange, foundName ) @@ -2490,21 +2452,6 @@ unnecessaryDetails = ] -sameThingOnBothSidesDetails : Bool -> List String -sameThingOnBothSidesDetails computedResult = - let - computedResultString : String - computedResultString = - if computedResult then - "True" - - else - "False" - in - [ "The value on the left and on the right are the same. Therefore we can determine that the expression will always be " ++ computedResultString ++ "." - ] - - -- COMPARISONS @@ -2515,34 +2462,37 @@ comparisonChecks operatorFunction operatorCheckInfo = Maybe.map2 operatorFunction (Normalize.getNumberValue operatorCheckInfo.left) (Normalize.getNumberValue operatorCheckInfo.right) - |> Maybe.map boolToString of - Just value -> - [ Rule.errorWithFix - { message = "Comparison is always " ++ value - , details = - [ "The value on the left and on the right are the same. Therefore we can determine that the expression will always be " ++ value ++ "." - ] - } - operatorCheckInfo.parentRange - [ Fix.replaceRangeBy operatorCheckInfo.parentRange value - ] - ] + Just bool -> + [ comparisonError bool operatorCheckInfo.parentRange ] Nothing -> [] +comparisonError : Bool -> Range -> Error {} +comparisonError bool range = + let + boolAsString : String + boolAsString = + boolToString bool + in + Rule.errorWithFix + { message = "Comparison is always " ++ boolAsString + , details = + [ "Based on the values and/or the context, we can determine that the value of this operation will always be " ++ boolAsString ++ "." + ] + } + range + [ Fix.replaceRangeBy range boolAsString ] + + -- IF EXPRESSIONS -targetIf : Node a -> Range -targetIf node = - let - { start } = - Node.range node - in +targetIfKeyword : Range -> Range +targetIfKeyword { start } = { start = start , end = { start | column = start.column + 2 } } @@ -2644,7 +2594,7 @@ alwaysCompositionCheck { lookupTable, fromLeftToRight, left, right, leftRange, r isAlwaysCall : ModuleNameLookupTable -> Node Expression -> Bool isAlwaysCall lookupTable node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Application ((Node alwaysRange (Expression.FunctionOrValue _ "always")) :: _ :: []) -> ModuleNameLookupTable.moduleNameAt lookupTable alwaysRange == Just [ "Basics" ] @@ -2654,7 +2604,7 @@ isAlwaysCall lookupTable node = getAlwaysArgument : ModuleNameLookupTable -> Node Expression -> Maybe { alwaysRange : Range, rangeToRemove : Range } getAlwaysArgument lookupTable node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Application ((Node alwaysRange (Expression.FunctionOrValue _ "always")) :: arg :: []) -> if ModuleNameLookupTable.moduleNameAt lookupTable alwaysRange == Just [ "Basics" ] then Just @@ -2696,7 +2646,7 @@ reportEmptyListSecondArgument ( ( moduleName, name ), function ) = case checkInfo.secondArg of Just (Node _ (Expression.ListExpr [])) -> [ Rule.errorWithFix - { message = "Using " ++ String.join "." moduleName ++ "." ++ name ++ " on an empty list will result in a empty list" + { message = "Using " ++ String.join "." moduleName ++ "." ++ name ++ " on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] } checkInfo.fnRange @@ -2715,7 +2665,7 @@ reportEmptyListFirstArgument ( ( moduleName, name ), function ) = case checkInfo.firstArg of Node _ (Expression.ListExpr []) -> [ Rule.errorWithFix - { message = "Using " ++ String.join "." moduleName ++ "." ++ name ++ " on an empty list will result in a empty list" + { message = "Using " ++ String.join "." moduleName ++ "." ++ name ++ " on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] } checkInfo.fnRange @@ -2773,7 +2723,7 @@ stringWordsChecks { parentRange, fnRange, firstArg } = case Node.value firstArg of Expression.Literal "" -> [ Rule.errorWithFix - { message = "Using String.words on an empty string will result in a empty list" + { message = "Using String.words on an empty string will result in an empty list" , details = [ "You can replace this call by an empty list." ] } fnRange @@ -2789,7 +2739,7 @@ stringLinesChecks { parentRange, fnRange, firstArg } = case Node.value firstArg of Expression.Literal "" -> [ Rule.errorWithFix - { message = "Using String.lines on an empty string will result in a empty list" + { message = "Using String.lines on an empty string will result in an empty list" , details = [ "You can replace this call by an empty list." ] } fnRange @@ -2870,7 +2820,7 @@ stringLengthChecks { parentRange, fnRange, firstArg } = stringRepeatChecks : CheckInfo -> List (Error {}) -stringRepeatChecks { parentRange, fnRange, firstArg, secondArg } = +stringRepeatChecks ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = case secondArg of Just (Node _ (Expression.Literal "")) -> [ Rule.errorWithFix @@ -2882,7 +2832,7 @@ stringRepeatChecks { parentRange, fnRange, firstArg, secondArg } = ] _ -> - case getIntValue firstArg of + case Evaluate.getInt checkInfo firstArg of Just intValue -> if intValue == 1 then [ Rule.errorWithFix @@ -2910,10 +2860,10 @@ stringRepeatChecks { parentRange, fnRange, firstArg, secondArg } = stringReplaceChecks : CheckInfo -> List (Error {}) -stringReplaceChecks { lookupTable, fnRange, firstArg, secondArg, thirdArg } = +stringReplaceChecks ({ fnRange, firstArg, secondArg, thirdArg } as checkInfo) = case secondArg of Just secondArg_ -> - case Normalize.compare lookupTable firstArg secondArg_ of + case Normalize.compare checkInfo firstArg secondArg_ of Normalize.ConfirmedEquality -> [ Rule.errorWithFix { message = "The result of String.replace will be the original string" @@ -3016,7 +2966,7 @@ maybeMapChecks checkInfo = maybeMapCompositionChecks : CompositionCheckInfo -> List (Error {}) maybeMapCompositionChecks { lookupTable, fromLeftToRight, parentRange, left, right } = if fromLeftToRight then - case ( removeParens left, Node.value (removeParens right) ) of + case ( AstHelpers.removeParens left, Node.value (AstHelpers.removeParens right) ) of ( Node justRange (Expression.FunctionOrValue _ "Just"), Expression.Application ((Node maybeMapRange (Expression.FunctionOrValue _ "map")) :: mapperFunction :: []) ) -> if (ModuleNameLookupTable.moduleNameAt lookupTable justRange == Just [ "Maybe" ]) @@ -3039,7 +2989,7 @@ maybeMapCompositionChecks { lookupTable, fromLeftToRight, parentRange, left, rig [] else - case ( Node.value (removeParens left), removeParens right ) of + case ( Node.value (AstHelpers.removeParens left), AstHelpers.removeParens right ) of ( Expression.Application ((Node maybeMapRange (Expression.FunctionOrValue _ "map")) :: mapperFunction :: []), Node justRange (Expression.FunctionOrValue _ "Just") ) -> if ModuleNameLookupTable.moduleNameAt lookupTable justRange == Just [ "Maybe" ] && ModuleNameLookupTable.moduleNameAt lookupTable maybeMapRange == Just [ "Maybe" ] then [ Rule.errorWithFix @@ -3062,7 +3012,7 @@ maybeMapCompositionChecks { lookupTable, fromLeftToRight, parentRange, left, rig resultMapCompositionChecks : CompositionCheckInfo -> List (Error {}) resultMapCompositionChecks { lookupTable, fromLeftToRight, parentRange, left, right } = if fromLeftToRight then - case ( removeParens left, Node.value (removeParens right) ) of + case ( AstHelpers.removeParens left, Node.value (AstHelpers.removeParens right) ) of ( Node justRange (Expression.FunctionOrValue _ "Ok"), Expression.Application ((Node resultMapRange (Expression.FunctionOrValue _ "map")) :: mapperFunction :: []) ) -> if (ModuleNameLookupTable.moduleNameAt lookupTable justRange == Just [ "Result" ]) @@ -3085,7 +3035,7 @@ resultMapCompositionChecks { lookupTable, fromLeftToRight, parentRange, left, ri [] else - case ( Node.value (removeParens left), removeParens right ) of + case ( Node.value (AstHelpers.removeParens left), AstHelpers.removeParens right ) of ( Expression.Application ((Node resultMapRange (Expression.FunctionOrValue _ "map")) :: mapperFunction :: []), Node justRange (Expression.FunctionOrValue _ "Ok") ) -> if ModuleNameLookupTable.moduleNameAt lookupTable justRange == Just [ "Result" ] && ModuleNameLookupTable.moduleNameAt lookupTable resultMapRange == Just [ "Result" ] then [ Rule.errorWithFix @@ -3226,7 +3176,7 @@ findConsecutiveListLiterals firstListElement restOfListElements = listConcatMapChecks : CheckInfo -> List (Error {}) -listConcatMapChecks { lookupTable, parentRange, fnRange, firstArg, secondArg, usingRightPizza } = +listConcatMapChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = if isIdentity lookupTable firstArg then [ Rule.errorWithFix { message = "Using List.concatMap with an identity function is the same as using List.concat" @@ -3252,23 +3202,14 @@ listConcatMapChecks { lookupTable, parentRange, fnRange, firstArg, secondArg, us Nothing -> case secondArg of - Just (Node listRange (Expression.ListExpr [ Node singleElementRange _ ])) -> + Just (Node listRange (Expression.ListExpr [ listElement ])) -> [ Rule.errorWithFix { message = "Using List.concatMap on an element with a single item is the same as calling the function directly on that lone element." , details = [ "You can replace this call by a call to the function directly." ] } fnRange - (if usingRightPizza then - [ Fix.replaceRangeBy { start = listRange.start, end = singleElementRange.start } "(" - , Fix.replaceRangeBy { start = singleElementRange.end, end = listRange.end } ")" - , Fix.removeRange fnRange - ] - - else - [ Fix.removeRange fnRange - , Fix.replaceRangeBy { start = listRange.start, end = singleElementRange.start } "(" - , Fix.replaceRangeBy { start = singleElementRange.end, end = listRange.end } ")" - ] + (Fix.removeRange fnRange + :: replaceBySubExpressionFix listRange listElement ) ] @@ -3280,7 +3221,7 @@ concatAndMapCompositionCheck : CompositionCheckInfo -> List (Error {}) concatAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = if fromLeftToRight then if isSpecificFunction [ "List" ] "concat" lookupTable right then - case Node.value (removeParens left) of + case Node.value (AstHelpers.removeParens left) of Expression.Application [ leftFunction, _ ] -> if isSpecificFunction [ "List" ] "map" lookupTable leftFunction then [ Rule.errorWithFix @@ -3303,7 +3244,7 @@ concatAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = [] else if isSpecificFunction [ "List" ] "concat" lookupTable left then - case Node.value (removeParens right) of + case Node.value (AstHelpers.removeParens right) of Expression.Application [ rightFunction, _ ] -> if isSpecificFunction [ "List" ] "map" lookupTable rightFunction then [ Rule.errorWithFix @@ -3328,9 +3269,9 @@ concatAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = listIndexedMapChecks : CheckInfo -> List (Error {}) listIndexedMapChecks { lookupTable, fnRange, firstArg } = - case removeParens firstArg of + case AstHelpers.removeParens firstArg of Node lambdaRange (Expression.LambdaExpression { args, expression }) -> - case Maybe.map removeParensFromPattern (List.head args) of + case Maybe.map AstHelpers.removeParensFromPattern (List.head args) of Just (Node patternRange Pattern.AllPattern) -> let rangeToRemove : Range @@ -3377,8 +3318,8 @@ listIndexedMapChecks { lookupTable, fnRange, firstArg } = listAllChecks : CheckInfo -> List (Error {}) -listAllChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = - case Maybe.map (removeParens >> Node.value) secondArg of +listAllChecks ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = + case Maybe.map (AstHelpers.removeParens >> Node.value) secondArg of Just (Expression.ListExpr []) -> [ Rule.errorWithFix { message = "The call to List.all will result in True" @@ -3389,7 +3330,7 @@ listAllChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = ] _ -> - case isAlwaysBoolean lookupTable firstArg of + case Evaluate.isAlwaysBoolean checkInfo firstArg of Determined True -> [ Rule.errorWithFix { message = "The call to List.all will result in True" @@ -3404,8 +3345,8 @@ listAllChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = listAnyChecks : CheckInfo -> List (Error {}) -listAnyChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = - case Maybe.map (removeParens >> Node.value) secondArg of +listAnyChecks ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = + case Maybe.map (AstHelpers.removeParens >> Node.value) secondArg of Just (Expression.ListExpr []) -> [ Rule.errorWithFix { message = "The call to List.any will result in False" @@ -3416,7 +3357,7 @@ listAnyChecks { lookupTable, parentRange, fnRange, firstArg, secondArg } = ] _ -> - case isAlwaysBoolean lookupTable firstArg of + case Evaluate.isAlwaysBoolean checkInfo firstArg of Determined False -> [ Rule.errorWithFix { message = "The call to List.any will result in False" @@ -3530,10 +3471,10 @@ collectJusts lookupTable list acc = filterAndMapCompositionCheck : CompositionCheckInfo -> List (Error {}) filterAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = if fromLeftToRight then - case Node.value (removeParens right) of + case Node.value (AstHelpers.removeParens right) of Expression.Application [ rightFunction, arg ] -> if isSpecificFunction [ "List" ] "filterMap" lookupTable rightFunction && isIdentity lookupTable arg then - case Node.value (removeParens left) of + case Node.value (AstHelpers.removeParens left) of Expression.Application [ leftFunction, _ ] -> if isSpecificFunction [ "List" ] "map" lookupTable leftFunction then [ Rule.errorWithFix @@ -3559,10 +3500,10 @@ filterAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = [] else - case Node.value (removeParens left) of + case Node.value (AstHelpers.removeParens left) of Expression.Application [ leftFunction, arg ] -> if isSpecificFunction [ "List" ] "filterMap" lookupTable leftFunction && isIdentity lookupTable arg then - case Node.value (removeParens right) of + case Node.value (AstHelpers.removeParens right) of Expression.Application [ rightFunction, _ ] -> if isSpecificFunction [ "List" ] "map" lookupTable rightFunction then [ Rule.errorWithFix @@ -3589,28 +3530,33 @@ filterAndMapCompositionCheck { lookupTable, fromLeftToRight, left, right } = listRangeChecks : CheckInfo -> List (Error {}) -listRangeChecks { parentRange, fnRange, firstArg, secondArg } = - case Maybe.map2 Tuple.pair (getIntValue firstArg) (Maybe.andThen getIntValue secondArg) of - Just ( first, second ) -> - if first > second then - [ Rule.errorWithFix - { message = "The call to List.range will result in []" - , details = [ "The second argument to List.range is bigger than the first one, therefore you can replace this list by an empty list." ] - } - fnRange - (replaceByEmptyFix "[]" parentRange secondArg) - ] +listRangeChecks ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = + case Maybe.andThen (Evaluate.getInt checkInfo) secondArg of + Just second -> + case Evaluate.getInt checkInfo firstArg of + Just first -> + if first > second then + [ Rule.errorWithFix + { message = "The call to List.range will result in []" + , details = [ "The second argument to List.range is bigger than the first one, therefore you can replace this list by an empty list." ] + } + fnRange + (replaceByEmptyFix "[]" parentRange secondArg) + ] - else - [] + else + [] + + Nothing -> + [] Nothing -> [] listRepeatChecks : CheckInfo -> List (Error {}) -listRepeatChecks { parentRange, fnRange, firstArg, secondArg } = - case getIntValue firstArg of +listRepeatChecks ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = + case Evaluate.getInt checkInfo firstArg of Just intValue -> if intValue < 1 then [ Rule.errorWithFix @@ -3630,7 +3576,7 @@ listRepeatChecks { parentRange, fnRange, firstArg, secondArg } = listReverseChecks : CheckInfo -> List (Error {}) listReverseChecks ({ parentRange, fnRange, firstArg } as checkInfo) = - case Node.value (removeParens firstArg) of + case Node.value (AstHelpers.removeParens firstArg) of Expression.ListExpr [] -> [ Rule.errorWithFix { message = "Using List.reverse on [] will result in []" @@ -3733,15 +3679,13 @@ subAndCmdBatchChecks moduleName { lookupTable, parentRange, fnRange, firstArg } [ Fix.replaceRangeBy parentRange (moduleName ++ ".none") ] ] - Expression.ListExpr [ arg ] -> + Expression.ListExpr [ listElement ] -> [ Rule.errorWithFix { message = "Unnecessary " ++ moduleName ++ ".batch" , details = [ moduleName ++ ".batch with a single element is equal to that element." ] } fnRange - [ Fix.replaceRangeBy { start = parentRange.start, end = (Node.range arg).start } "(" - , Fix.replaceRangeBy { start = (Node.range arg).end, end = parentRange.end } ")" - ] + (replaceBySubExpressionFix parentRange listElement) ] Expression.ListExpr args -> @@ -3751,7 +3695,7 @@ subAndCmdBatchChecks moduleName { lookupTable, parentRange, fnRange, firstArg } (List.map (Node.range >> Just) (List.drop 1 args) ++ [ Nothing ]) |> List.filterMap (\( prev, arg, next ) -> - case removeParens arg of + case AstHelpers.removeParens arg of Node batchRange (Expression.FunctionOrValue _ "none") -> if ModuleNameLookupTable.moduleNameAt lookupTable batchRange == Just [ "Platform", moduleName ] then Just @@ -3790,17 +3734,15 @@ subAndCmdBatchChecks moduleName { lookupTable, parentRange, fnRange, firstArg } oneOfChecks : CheckInfo -> List (Error {}) -oneOfChecks { fnRange, firstArg } = - case removeParens firstArg of - Node listRange (Expression.ListExpr [ Node singleElementRange _ ]) -> +oneOfChecks { parentRange, fnRange, firstArg } = + case AstHelpers.removeParens firstArg of + Node _ (Expression.ListExpr [ listElement ]) -> [ Rule.errorWithFix { message = "Unnecessary oneOf" , details = [ "There is only a single element in the list of elements to try out." ] } fnRange - [ Fix.replaceRangeBy { start = fnRange.start, end = singleElementRange.start } "(" - , Fix.replaceRangeBy { start = singleElementRange.end, end = listRange.end } ")" - ] + (replaceBySubExpressionFix parentRange listElement) ] _ -> @@ -4099,7 +4041,7 @@ resultWithDefaultChecks checkInfo = collectionFilterChecks : Collection -> CheckInfo -> List (Error {}) -collectionFilterChecks collection ({ lookupTable, parentRange, fnRange, firstArg, secondArg } as checkInfo) = +collectionFilterChecks collection ({ parentRange, fnRange, firstArg, secondArg } as checkInfo) = case Maybe.andThen (collection.determineSize checkInfo.lookupTable) checkInfo.secondArg of Just (Exactly 0) -> [ Rule.errorWithFix @@ -4111,7 +4053,7 @@ collectionFilterChecks collection ({ lookupTable, parentRange, fnRange, firstArg ] _ -> - case isAlwaysBoolean lookupTable firstArg of + case Evaluate.isAlwaysBoolean checkInfo firstArg of Determined True -> [ Rule.errorWithFix { message = "Using " ++ collection.moduleName ++ ".filter with a function that will always return True is the same as not using " ++ collection.moduleName ++ ".filter" @@ -4377,7 +4319,7 @@ collectionPartitionChecks collection checkInfo = ] _ -> - case isAlwaysBoolean checkInfo.lookupTable checkInfo.firstArg of + case Evaluate.isAlwaysBoolean checkInfo checkInfo.firstArg of Determined True -> case checkInfo.secondArg of Just listArg -> @@ -4449,7 +4391,7 @@ type CollectionSize determineListLength : ModuleNameLookupTable -> Node Expression -> Maybe CollectionSize determineListLength lookupTable node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.ListExpr list -> Just (Exactly (List.length list)) @@ -4474,7 +4416,7 @@ determineListLength lookupTable node = replaceSingleElementListBySingleValue_RENAME : ModuleNameLookupTable -> Range -> Node Expression -> Maybe (List (Error {})) replaceSingleElementListBySingleValue_RENAME lookupTable fnRange node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.LambdaExpression { expression } -> case replaceSingleElementListBySingleValue lookupTable expression of Just fixes -> @@ -4495,17 +4437,10 @@ replaceSingleElementListBySingleValue_RENAME lookupTable fnRange node = replaceSingleElementListBySingleValue : ModuleNameLookupTable -> Node Expression -> Maybe (List Fix) -replaceSingleElementListBySingleValue lookupTable rawNode = - let - (Node { start, end } nodeValue) = - removeParens rawNode - in - case nodeValue of - Expression.ListExpr [ _ ] -> - Just - [ Fix.replaceRangeBy { start = start, end = { start | column = start.column + 1 } } "(" - , Fix.replaceRangeBy { start = { end | column = end.column - 1 }, end = end } ")" - ] +replaceSingleElementListBySingleValue lookupTable node = + case Node.value (AstHelpers.removeParens node) of + Expression.ListExpr [ listElement ] -> + Just (replaceBySubExpressionFix (Node.range node) listElement) Expression.Application ((Node fnRange (Expression.FunctionOrValue _ "singleton")) :: _ :: []) -> if ModuleNameLookupTable.moduleNameAt lookupTable fnRange == Just [ "List" ] then @@ -4545,7 +4480,7 @@ determineIfCollectionIsEmpty moduleName singletonNumberOfArgs lookupTable node = Just (Exactly 0) else - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Application ((Node fnRange (Expression.FunctionOrValue _ "singleton")) :: args) -> if List.length args == singletonNumberOfArgs && ModuleNameLookupTable.moduleNameAt lookupTable fnRange == Just moduleName then Just (Exactly 1) @@ -4678,7 +4613,7 @@ removeRecordFields recordUpdateRange variable fields = let value : Node Expression value = - removeParens valueWithParens + AstHelpers.removeParens valueWithParens in if isUnnecessaryRecordUpdateSetter variable field value then [ Rule.errorWithFix @@ -4700,7 +4635,7 @@ removeRecordFields recordUpdateRange variable fields = let value : Node Expression value = - removeParens valueWithParens + AstHelpers.removeParens valueWithParens in if isUnnecessaryRecordUpdateSetter variable field value then Just @@ -4736,6 +4671,129 @@ isUnnecessaryRecordUpdateSetter variable field value = +-- IF + + +ifChecks : + ModuleContext + -> Range + -> + { condition : Node Expression + , trueBranch : Node Expression + , falseBranch : Node Expression + } + -> { errors : List (Error {}), rangesToIgnore : List Range, rightSidesOfPlusPlus : List Range, inferredConstants : List ( Range, Infer.Inferred ) } +ifChecks context nodeRange { condition, trueBranch, falseBranch } = + case Evaluate.getBoolean context condition of + Determined True -> + errorsAndRangesToIgnore + [ Rule.errorWithFix + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + } + (targetIfKeyword nodeRange) + [ Fix.removeRange + { start = nodeRange.start + , end = (Node.range trueBranch).start + } + , Fix.removeRange + { start = (Node.range trueBranch).end + , end = nodeRange.end + } + ] + ] + [ Node.range condition ] + + Determined False -> + errorsAndRangesToIgnore + [ Rule.errorWithFix + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + } + (targetIfKeyword nodeRange) + [ Fix.removeRange + { start = nodeRange.start + , end = (Node.range falseBranch).start + } + ] + ] + [ Node.range condition ] + + Undetermined -> + case ( Evaluate.getBoolean context trueBranch, Evaluate.getBoolean context falseBranch ) of + ( Determined True, Determined False ) -> + onlyErrors + [ Rule.errorWithFix + { message = "The if expression's value is the same as the condition" + , details = [ "The expression can be replaced by the condition." ] + } + (targetIfKeyword nodeRange) + [ Fix.removeRange + { start = nodeRange.start + , end = (Node.range condition).start + } + , Fix.removeRange + { start = (Node.range condition).end + , end = nodeRange.end + } + ] + ] + + ( Determined False, Determined True ) -> + onlyErrors + [ Rule.errorWithFix + { message = "The if expression's value is the inverse of the condition" + , details = [ "The expression can be replaced by the condition wrapped by `not`." ] + } + (targetIfKeyword nodeRange) + [ Fix.replaceRangeBy + { start = nodeRange.start + , end = (Node.range condition).start + } + "not (" + , Fix.replaceRangeBy + { start = (Node.range condition).end + , end = nodeRange.end + } + ")" + ] + ] + + _ -> + case Normalize.compare context trueBranch falseBranch of + Normalize.ConfirmedEquality -> + onlyErrors + [ Rule.errorWithFix + { message = "The values in both branches is the same." + , details = [ "The expression can be replaced by the contents of either branch." ] + } + (targetIfKeyword nodeRange) + [ Fix.removeRange + { start = nodeRange.start + , end = (Node.range trueBranch).start + } + , Fix.removeRange + { start = (Node.range trueBranch).end + , end = nodeRange.end + } + ] + ] + + _ -> + { errors = [] + , rangesToIgnore = [] + , rightSidesOfPlusPlus = [] + , inferredConstants = + Infer.inferForIfCondition + (Node.value (Normalize.normalize context condition)) + { trueBranchRange = Node.range trueBranch + , falseBranchRange = Node.range falseBranch + } + (Tuple.first context.inferredConstants) + } + + + -- CASE OF @@ -4754,57 +4812,33 @@ sameBodyForCaseOfChecks context parentRange cases = [] -> [] - first :: rest -> + ( firstPattern, firstBody ) :: rest -> + let + restPatterns : List (Node Pattern) + restPatterns = + List.map Tuple.first rest + in if - List.any (Tuple.first >> introducesVariable) (first :: rest) - || not (Normalize.areAllTheSame context.lookupTable (Tuple.second first) (List.map Tuple.second rest)) + introducesVariableOrUsesTypeConstructor context (firstPattern :: restPatterns) + || not (Normalize.areAllTheSame context firstBody (List.map Tuple.second rest)) then [] else let - constructorsUsed : () -> List ( ModuleName, String ) - constructorsUsed () = - List.concatMap (Tuple.first >> findUsedConstructors context) (first :: rest) - |> Set.fromList - |> Set.toList + firstBodyRange : Range + firstBodyRange = + Node.range firstBody in - if not (List.isEmpty context.ignoredCustomTypes) && allConstructorsWereUsedOfAType context.ignoredCustomTypes (constructorsUsed ()) then - [] - - else - let - firstBodyRange : Range - firstBodyRange = - Node.range (Tuple.second first) - in - [ Rule.errorWithFix - { message = "Unnecessary case expression" - , details = [ "All the branches of this case expression resolve to the same value. You can remove the case expression and replace it with the body of one of the branches." ] - } - (caseKeyWordRange parentRange) - [ Fix.removeRange { start = parentRange.start, end = firstBodyRange.start } - , Fix.removeRange { start = firstBodyRange.end, end = parentRange.end } - ] + [ Rule.errorWithFix + { message = "Unnecessary case expression" + , details = [ "All the branches of this case expression resolve to the same value. You can remove the case expression and replace it with the body of one of the branches." ] + } + (caseKeyWordRange parentRange) + [ Fix.removeRange { start = parentRange.start, end = firstBodyRange.start } + , Fix.removeRange { start = firstBodyRange.end, end = parentRange.end } ] - - -allConstructorsWereUsedOfAType : List Constructor -> List ( ModuleName, String ) -> Bool -allConstructorsWereUsedOfAType ignoredCustomTypes constructorsUsed = - case constructorsUsed of - [] -> - False - - ( moduleName, constructorName ) :: rest -> - case find (\type_ -> type_.moduleName == moduleName && List.member constructorName type_.constructors) ignoredCustomTypes of - Just customType -> - List.all - (\constructor -> List.member ( moduleName, constructor ) (( moduleName, constructorName ) :: rest)) - customType.constructors - || allConstructorsWereUsedOfAType ignoredCustomTypes rest - - Nothing -> - allConstructorsWereUsedOfAType ignoredCustomTypes rest + ] caseKeyWordRange : Range -> Range @@ -4814,86 +4848,49 @@ caseKeyWordRange range = } -introducesVariable : Node Pattern -> Bool -introducesVariable node = - case Node.value node of - Pattern.VarPattern _ -> - True - - Pattern.RecordPattern _ -> - True - - Pattern.AsPattern _ _ -> - True - - Pattern.ParenthesizedPattern pattern -> - introducesVariable pattern - - Pattern.TuplePattern nodes -> - List.any introducesVariable nodes - - Pattern.UnConsPattern first rest -> - List.any introducesVariable [ first, rest ] - - Pattern.ListPattern nodes -> - List.any introducesVariable nodes - - Pattern.NamedPattern _ nodes -> - List.any introducesVariable nodes - - _ -> +introducesVariableOrUsesTypeConstructor : ModuleContext -> List (Node Pattern) -> Bool +introducesVariableOrUsesTypeConstructor context nodesToLookAt = + case nodesToLookAt of + [] -> False + node :: remaining -> + case Node.value node of + Pattern.VarPattern _ -> + True -findUsedConstructors : ModuleContext -> Node Pattern -> List ( ModuleName, String ) -findUsedConstructors context node = - case Node.value node of - Pattern.NamedPattern { name } nodes -> - case isIgnoredConstructor2 context (Node.range node) name of - Just moduleName -> - ( moduleName, name ) :: List.concatMap (findUsedConstructors context) nodes + Pattern.RecordPattern _ -> + True - Nothing -> - List.concatMap (findUsedConstructors context) nodes + Pattern.AsPattern _ _ -> + True - Pattern.AsPattern pattern _ -> - findUsedConstructors context pattern + Pattern.ParenthesizedPattern pattern -> + introducesVariableOrUsesTypeConstructor context (pattern :: remaining) - Pattern.ParenthesizedPattern pattern -> - findUsedConstructors context pattern + Pattern.TuplePattern nodes -> + introducesVariableOrUsesTypeConstructor context (nodes ++ remaining) - Pattern.TuplePattern nodes -> - List.concatMap (findUsedConstructors context) nodes + Pattern.UnConsPattern first rest -> + introducesVariableOrUsesTypeConstructor context (first :: rest :: remaining) - Pattern.UnConsPattern first rest -> - List.concatMap (findUsedConstructors context) [ first, rest ] + Pattern.ListPattern nodes -> + introducesVariableOrUsesTypeConstructor context (nodes ++ remaining) - Pattern.ListPattern nodes -> - List.concatMap (findUsedConstructors context) nodes + Pattern.NamedPattern { name } nodes -> + case ModuleNameLookupTable.fullModuleNameFor context.lookupTable node of + Just moduleName -> + if Set.member ( moduleName, name ) context.customTypesToReportInCases then + introducesVariableOrUsesTypeConstructor context (nodes ++ remaining) - _ -> - [] + else + True + Nothing -> + True -isIgnoredConstructor2 : ModuleContext -> Range -> String -> Maybe ModuleName -isIgnoredConstructor2 context range name = - case ModuleNameLookupTable.moduleNameAt context.lookupTable range of - Just [] -> - if Set.member ( context.moduleName, name ) context.constructorsToIgnore then - Just context.moduleName - - else - Nothing - - Just moduleName -> - if Set.member ( moduleName, name ) context.constructorsToIgnore then - Just moduleName - - else - Nothing - - Nothing -> - Nothing + _ -> + introducesVariableOrUsesTypeConstructor context remaining booleanCaseOfChecks : ModuleNameLookupTable -> Range -> Expression.CaseBlock -> List (Error {}) @@ -4933,7 +4930,7 @@ booleanCaseOfChecks lookupTable parentRange { expression, cases } = isSpecificFunction : ModuleName -> String -> ModuleNameLookupTable -> Node Expression -> Bool isSpecificFunction moduleName fnName lookupTable node = - case removeParens node of + case AstHelpers.removeParens node of Node noneRange (Expression.FunctionOrValue _ foundFnName) -> (foundFnName == fnName) && (ModuleNameLookupTable.moduleNameAt lookupTable noneRange == Just moduleName) @@ -4944,7 +4941,7 @@ isSpecificFunction moduleName fnName lookupTable node = isSpecificCall : ModuleName -> String -> ModuleNameLookupTable -> Node Expression -> Bool isSpecificCall moduleName fnName lookupTable node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Application ((Node noneRange (Expression.FunctionOrValue _ foundFnName)) :: _ :: []) -> (foundFnName == fnName) && (ModuleNameLookupTable.moduleNameAt lookupTable noneRange == Just moduleName) @@ -4953,25 +4950,9 @@ isSpecificCall moduleName fnName lookupTable node = False -getIntValue : Node Expression -> Maybe Int -getIntValue node = - case Node.value (removeParens node) of - Expression.Integer n -> - Just n - - Expression.Hex n -> - Just n - - Expression.Negation expr -> - Maybe.map negate (getIntValue expr) - - _ -> - Nothing - - getUncomputedNumberValue : Node Expression -> Maybe Float getUncomputedNumberValue node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Integer n -> Just (toFloat n) @@ -5082,7 +5063,7 @@ isIdentity lookupTable baseNode = let node : Node Expression node = - removeParens baseNode + AstHelpers.removeParens baseNode in case Node.value node of Expression.FunctionOrValue _ "identity" -> @@ -5125,7 +5106,7 @@ getVarPattern node = getExpressionName : Node Expression -> Maybe String getExpressionName node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.FunctionOrValue [] name -> Just name @@ -5143,72 +5124,6 @@ isListLiteral node = False -removeParens : Node Expression -> Node Expression -removeParens node = - case Node.value node of - Expression.ParenthesizedExpression expr -> - removeParens expr - - _ -> - node - - -removeParensFromPattern : Node Pattern -> Node Pattern -removeParensFromPattern node = - case Node.value node of - Pattern.ParenthesizedPattern pattern -> - removeParensFromPattern pattern - - _ -> - node - - -isAlwaysBoolean : ModuleNameLookupTable -> Node Expression -> Match Bool -isAlwaysBoolean lookupTable node = - case Node.value (removeParens node) of - Expression.Application ((Node alwaysRange (Expression.FunctionOrValue _ "always")) :: boolean :: []) -> - case ModuleNameLookupTable.moduleNameAt lookupTable alwaysRange of - Just [ "Basics" ] -> - getBoolean lookupTable boolean - - _ -> - Undetermined - - Expression.LambdaExpression { expression } -> - getBoolean lookupTable expression - - _ -> - Undetermined - - -getBoolean : ModuleNameLookupTable -> Node Expression -> Match Bool -getBoolean lookupTable baseNode = - let - node : Node Expression - node = - removeParens baseNode - in - case Node.value node of - Expression.FunctionOrValue _ "True" -> - case ModuleNameLookupTable.moduleNameFor lookupTable node of - Just [ "Basics" ] -> - Determined True - - _ -> - Undetermined - - Expression.FunctionOrValue _ "False" -> - case ModuleNameLookupTable.moduleNameFor lookupTable node of - Just [ "Basics" ] -> - Determined False - - _ -> - Undetermined - - _ -> - Undetermined - - getBooleanPattern : ModuleNameLookupTable -> Node Pattern -> Maybe Bool getBooleanPattern lookupTable node = case Node.value node of @@ -5243,7 +5158,7 @@ isAlwaysMaybe lookupTable baseNode = let node : Node Expression node = - removeParens baseNode + AstHelpers.removeParens baseNode in case Node.value node of Expression.FunctionOrValue _ "Just" -> @@ -5276,7 +5191,7 @@ getMaybeValues lookupTable baseNode = let node : Node Expression node = - removeParens baseNode + AstHelpers.removeParens baseNode in case Node.value node of Expression.Application ((Node justRange (Expression.FunctionOrValue _ "Just")) :: arg :: []) -> @@ -5373,7 +5288,7 @@ isAlwaysResult lookupTable baseNode = let node : Node Expression node = - removeParens baseNode + AstHelpers.removeParens baseNode in case Node.value node of Expression.FunctionOrValue _ "Ok" -> @@ -5414,7 +5329,7 @@ getResultValues lookupTable baseNode = let node : Node Expression node = - removeParens baseNode + AstHelpers.removeParens baseNode in case Node.value node of Expression.Application ((Node justRange (Expression.FunctionOrValue _ "Ok")) :: arg :: []) -> @@ -5524,7 +5439,7 @@ combineResultValuesHelp lookupTable nodes soFar = isAlwaysEmptyList : ModuleNameLookupTable -> Node Expression -> Bool isAlwaysEmptyList lookupTable node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.Application ((Node alwaysRange (Expression.FunctionOrValue _ "always")) :: alwaysValue :: []) -> case ModuleNameLookupTable.moduleNameAt lookupTable alwaysRange of Just [ "Basics" ] -> @@ -5542,7 +5457,7 @@ isAlwaysEmptyList lookupTable node = isEmptyList : Node Expression -> Bool isEmptyList node = - case Node.value (removeParens node) of + case Node.value (AstHelpers.removeParens node) of Expression.ListExpr [] -> True diff --git a/tests/Simplify/AstHelpers.elm b/tests/Simplify/AstHelpers.elm new file mode 100644 index 00000000..71eb2133 --- /dev/null +++ b/tests/Simplify/AstHelpers.elm @@ -0,0 +1,25 @@ +module Simplify.AstHelpers exposing (removeParens, removeParensFromPattern) + +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Node as Node exposing (Node) +import Elm.Syntax.Pattern as Pattern exposing (Pattern) + + +removeParens : Node Expression -> Node Expression +removeParens node = + case Node.value node of + Expression.ParenthesizedExpression expr -> + removeParens expr + + _ -> + node + + +removeParensFromPattern : Node Pattern -> Node Pattern +removeParensFromPattern node = + case Node.value node of + Pattern.ParenthesizedPattern pattern -> + removeParensFromPattern pattern + + _ -> + node diff --git a/tests/Simplify/Evaluate.elm b/tests/Simplify/Evaluate.elm new file mode 100644 index 00000000..4979670f --- /dev/null +++ b/tests/Simplify/Evaluate.elm @@ -0,0 +1,113 @@ +module Simplify.Evaluate exposing (getBoolean, getInt, isAlwaysBoolean) + +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Review.ModuleNameLookupTable as ModuleNameLookupTable +import Simplify.AstHelpers as AstHelpers +import Simplify.Infer as Infer +import Simplify.Match exposing (Match(..)) +import Simplify.Normalize as Normalize + + +getBoolean : Infer.Resources a -> Node Expression -> Match Bool +getBoolean resources baseNode = + let + node : Node Expression + node = + AstHelpers.removeParens baseNode + in + case Node.value node of + Expression.FunctionOrValue _ "True" -> + case ModuleNameLookupTable.moduleNameFor resources.lookupTable node of + Just [ "Basics" ] -> + Determined True + + _ -> + Undetermined + + Expression.FunctionOrValue _ "False" -> + case ModuleNameLookupTable.moduleNameFor resources.lookupTable node of + Just [ "Basics" ] -> + Determined False + + _ -> + Undetermined + + Expression.FunctionOrValue _ name -> + case + ModuleNameLookupTable.moduleNameFor resources.lookupTable node + |> Maybe.andThen (\moduleName -> Infer.get (Expression.FunctionOrValue moduleName name) (Tuple.first resources.inferredConstants)) + of + Just (Expression.FunctionOrValue [ "Basics" ] "True") -> + Determined True + + Just (Expression.FunctionOrValue [ "Basics" ] "False") -> + Determined False + + Just _ -> + Undetermined + + Nothing -> + Undetermined + + _ -> + case + Infer.isBoolean + (Node.value (Normalize.normalize resources node)) + (Tuple.first resources.inferredConstants) + of + Just bool -> + Determined bool + + Nothing -> + Undetermined + + +isAlwaysBoolean : Infer.Resources a -> Node Expression -> Match Bool +isAlwaysBoolean resources node = + case Node.value (AstHelpers.removeParens node) of + Expression.Application ((Node alwaysRange (Expression.FunctionOrValue _ "always")) :: boolean :: []) -> + case ModuleNameLookupTable.moduleNameAt resources.lookupTable alwaysRange of + Just [ "Basics" ] -> + getBoolean resources boolean + + _ -> + Undetermined + + Expression.LambdaExpression { expression } -> + getBoolean resources expression + + _ -> + Undetermined + + +getInt : Infer.Resources a -> Node Expression -> Maybe Int +getInt resources baseNode = + let + node : Node Expression + node = + AstHelpers.removeParens baseNode + in + case Node.value node of + Expression.Integer n -> + Just n + + Expression.Hex n -> + Just n + + Expression.Negation expr -> + Maybe.map negate (getInt resources expr) + + Expression.FunctionOrValue _ name -> + case + ModuleNameLookupTable.moduleNameFor resources.lookupTable node + |> Maybe.andThen (\moduleName -> Infer.get (Expression.FunctionOrValue moduleName name) (Tuple.first resources.inferredConstants)) + of + Just (Expression.Integer int) -> + Just int + + _ -> + Nothing + + _ -> + Nothing diff --git a/tests/Simplify/Infer.elm b/tests/Simplify/Infer.elm new file mode 100644 index 00000000..f4be8083 --- /dev/null +++ b/tests/Simplify/Infer.elm @@ -0,0 +1,532 @@ +module Simplify.Infer exposing + ( DeducedValue(..) + , Fact(..) + , Inferred(..) + , Resources + , deduceNewFacts + , empty + , falseExpr + , fromList + , get + , infer + , inferForIfCondition + , isBoolean + , trueExpr + ) + +{-| Infers values from `if` conditions. + +This is meant to simplify expressions like the following: + +```diff +if a then + -- we know that `a` is True +- if a && b then ++ if b then +``` + + +### Mechanism + +The way that this is done is by collecting "facts" about the conditions we've found. Given the following expression: + + if a && b == 1 then + 1 + + else + 2 + +we can infer that in the `then` branch, the following facts are true: + + - `a && b == 1` is True + - `a` is True + - `b == 1` is True + - `b` equals `1` + +and for the `else` branch, that: + + - `a && b == 1` is False + - `a` is False OR `b == 1` is False (or that `b` does not equal `1`, not sure how we represent this at the moment) + +For a condition like `a || b`, we know that in the `then` branch: + + - `a` is True OR `b` is True + +and that in the `else` branch: + + - `a || b` is `False` + - `a` is `False` + - `b` is `False` + +Whenever we get a new fact from a new `if` condition, we then go through all the previously known facts and see if the +new one can simplify some of the old ones to generate new facts. + +For instance, if we knew that `a` is True OR `b` is True, and we encounter `if a then`, then we can infer that for the `else` branch `a` is False. +When comparing that to `a` is True OR `b` is True, we can infer that `b` is True. + +Every new fact that we uncover from this comparison will also repeat the process of going through the previous list of facts. + +Another thing that we do whenever we encounter a new fact os to try and "deduce" a value from it, which we add to a list +of "deduced" values. A few examples: + + - `a` -> `a` is True + - `a == 1` -> `a` is equal to `1` + - `a /= 1` -> Can't infer individual values when this is True + - `a` OR `b` -> Can't infer individual values when this is True + +(with the exception that we can infer that the whole expression is `True` or `False`) + +Before we do all of this analysis, we normalize the AST, so we have a more predictable AST and don't have to do as many checks. + + +### Application + +This data is then used in `Normalize` to change the AST, so that a reference to `a` whose value we have "deduced" is +replaced by that value. Finally, that data is also used in functions like `Evaluate.getBoolean`. +(Note: This might be a bit redundant but that's a simplification for later on) + +Whenever we see a boolean expression, we will look at whether we can simplify it, and report an error when that happens. + + +### Limits + +The current system has a few holes meaning some things we could infer aren't properly handled, and I'd love help with that. +From the top of my mind, I think that `if x /= 1 then (if x == 1 then ...)` (or some variation) does not get simplified when it could. + +We are making special exception for numbers for equality, but we could do more: handling `String`, `Char` and probably more. + +The system does not currently handle `case` expressions. While handling pattern matching against literals should not be +too hard with the current system, storing "shapes" of the value (the value is a `Just` of something) probably requires +some work. + +-} + +import AssocList +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Range exposing (Range) +import Review.ModuleNameLookupTable exposing (ModuleNameLookupTable) + + +type Inferred + = Inferred + { facts : List Fact + , deduced : AssocList.Dict Expression DeducedValue + } + + +type DeducedValue + = DTrue + | DFalse + | DNumber Float + | DString String + + +type Fact + = Equals Expression Expression + | NotEquals Expression Expression + | Or (List Fact) (List Fact) + + +type alias Resources a = + { a + | lookupTable : ModuleNameLookupTable + , inferredConstants : ( Inferred, List Inferred ) + } + + +empty : Inferred +empty = + Inferred + { facts = [] + , deduced = AssocList.empty + } + + +fromList : List ( Expression, DeducedValue ) -> Inferred +fromList list = + Inferred + { facts = [] + , deduced = AssocList.fromList list + } + + +get : Expression -> Inferred -> Maybe Expression +get expr (Inferred inferred) = + AssocList.get expr inferred.deduced + |> Maybe.map + (\value -> + case value of + DTrue -> + trueExpr + + DFalse -> + falseExpr + + DNumber float -> + Expression.Floatable float + + DString str -> + Expression.Literal str + ) + + +isBoolean : Expression -> Inferred -> Maybe Bool +isBoolean expr (Inferred inferred) = + AssocList.get expr inferred.deduced + |> Maybe.andThen + (\value -> + case value of + DTrue -> + Just True + + DFalse -> + Just False + + DNumber _ -> + Nothing + + DString _ -> + Nothing + ) + + +inferForIfCondition : Expression -> { trueBranchRange : Range, falseBranchRange : Range } -> Inferred -> List ( Range, Inferred ) +inferForIfCondition condition { trueBranchRange, falseBranchRange } inferred = + [ ( trueBranchRange, infer [ condition ] True inferred ) + , ( falseBranchRange, infer [ condition ] False inferred ) + ] + + +trueExpr : Expression +trueExpr = + Expression.FunctionOrValue [ "Basics" ] "True" + + +falseExpr : Expression +falseExpr = + Expression.FunctionOrValue [ "Basics" ] "False" + + +convertToFact : Expression -> Bool -> List Fact +convertToFact expr shouldBe = + if shouldBe then + [ Equals expr trueExpr, NotEquals expr falseExpr ] + + else + [ Equals expr falseExpr, NotEquals expr trueExpr ] + + +infer : List Expression -> Bool -> Inferred -> Inferred +infer nodes shouldBe acc = + List.foldl (inferHelp shouldBe) acc nodes + + +inferHelp : Bool -> Expression -> Inferred -> Inferred +inferHelp shouldBe node acc = + let + dict : Inferred + dict = + injectFacts (convertToFact node shouldBe) acc + in + case node of + Expression.Application [ Node _ (Expression.FunctionOrValue [ "Basics" ] "not"), expression ] -> + inferHelp (not shouldBe) (Node.value expression) dict + + Expression.OperatorApplication "&&" _ (Node _ left) (Node _ right) -> + if shouldBe then + infer [ left, right ] shouldBe dict + + else + injectFacts + [ Or + (convertToFact left False) + (convertToFact right False) + ] + dict + + Expression.OperatorApplication "||" _ (Node _ left) (Node _ right) -> + if shouldBe then + injectFacts + [ Or + (convertToFact left True) + (convertToFact right True) + ] + dict + + else + infer [ left, right ] shouldBe dict + + Expression.OperatorApplication "==" inf left right -> + dict + |> (if shouldBe then + injectFacts [ NotEquals (Expression.OperatorApplication "/=" inf left right) trueExpr ] + + else + identity + ) + |> inferOnEquality left right shouldBe + |> inferOnEquality right left shouldBe + + Expression.OperatorApplication "/=" inf left right -> + dict + |> (if shouldBe then + injectFacts [ NotEquals (Expression.OperatorApplication "==" inf left right) trueExpr ] + + else + identity + ) + |> inferOnEquality left right (not shouldBe) + |> inferOnEquality right left (not shouldBe) + + _ -> + dict + + +injectFacts : List Fact -> Inferred -> Inferred +injectFacts newFacts (Inferred inferred) = + case newFacts of + [] -> + Inferred inferred + + newFact :: restOfFacts -> + if List.member newFact inferred.facts then + injectFacts + restOfFacts + (Inferred inferred) + + else + let + newFactsToVisit : List Fact + newFactsToVisit = + deduceNewFacts newFact inferred.facts + + deducedFromNewFact : Maybe ( Expression, DeducedValue ) + deducedFromNewFact = + case newFact of + Equals a b -> + equalsFact a b + + NotEquals a b -> + equalsFact a b + |> Maybe.andThen notDeduced + + Or _ _ -> + -- TODO Add "a || b || ..."? + Nothing + in + injectFacts + (newFactsToVisit ++ restOfFacts) + (Inferred + { facts = newFact :: inferred.facts + , deduced = + case deducedFromNewFact of + Just ( a, b ) -> + AssocList.insert a b inferred.deduced + + Nothing -> + inferred.deduced + } + ) + + +deduceNewFacts : Fact -> List Fact -> List Fact +deduceNewFacts newFact facts = + case newFact of + Equals factTarget factValue -> + case expressionToDeduced factValue of + Just value -> + List.concatMap (mergeEqualFacts ( factTarget, value )) facts + + Nothing -> + [ Equals factValue factTarget ] + + NotEquals _ _ -> + [] + + Or _ _ -> + [] + + +equalsFact : Expression -> Expression -> Maybe ( Expression, DeducedValue ) +equalsFact a b = + case expressionToDeduced a of + Just deducedValue -> + Just ( b, deducedValue ) + + Nothing -> + case expressionToDeduced b of + Just deducedValue -> + Just ( a, deducedValue ) + + Nothing -> + Nothing + + +expressionToDeduced : Expression -> Maybe DeducedValue +expressionToDeduced expression = + case expression of + Expression.FunctionOrValue [ "Basics" ] "True" -> + Just DTrue + + Expression.FunctionOrValue [ "Basics" ] "False" -> + Just DFalse + + Expression.Floatable float -> + Just (DNumber float) + + Expression.Literal string -> + Just (DString string) + + _ -> + Nothing + + +notDeduced : ( a, DeducedValue ) -> Maybe ( a, DeducedValue ) +notDeduced ( a, deducedValue ) = + case deducedValue of + DTrue -> + Just ( a, DFalse ) + + DFalse -> + Just ( a, DTrue ) + + _ -> + Nothing + + +mergeEqualFacts : ( Expression, DeducedValue ) -> Fact -> List Fact +mergeEqualFacts equalFact fact = + case fact of + Or left right -> + List.filterMap (ifSatisfy equalFact) + (List.map (\cond -> ( cond, right )) left + ++ List.map (\cond -> ( cond, left )) right + ) + |> List.concat + + _ -> + [] + + +ifSatisfy : ( Expression, DeducedValue ) -> ( Fact, a ) -> Maybe a +ifSatisfy ( target, value ) ( targetFact, otherFact ) = + case targetFact of + Equals factTarget factValue -> + if factTarget == target && areIncompatible value factValue then + Just otherFact + + else + Nothing + + NotEquals factTarget factValue -> + if factTarget == target && areCompatible value factValue then + Just otherFact + + else + Nothing + + _ -> + Nothing + + +areIncompatible : DeducedValue -> Expression -> Bool +areIncompatible value factValue = + case ( value, factValue ) of + ( DTrue, Expression.FunctionOrValue [ "Basics" ] "False" ) -> + True + + ( DFalse, Expression.FunctionOrValue [ "Basics" ] "True" ) -> + True + + ( DNumber valueFloat, Expression.Floatable factFloat ) -> + valueFloat /= factFloat + + ( DString valueString, Expression.Literal factString ) -> + valueString /= factString + + _ -> + False + + +areCompatible : DeducedValue -> Expression -> Bool +areCompatible value factValue = + case ( value, factValue ) of + ( DTrue, Expression.FunctionOrValue [ "Basics" ] "True" ) -> + True + + ( DFalse, Expression.FunctionOrValue [ "Basics" ] "False" ) -> + True + + ( DNumber valueFloat, Expression.Floatable factFloat ) -> + valueFloat == factFloat + + ( DString valueString, Expression.Literal factString ) -> + valueString == factString + + _ -> + False + + +inferOnEquality : Node Expression -> Node Expression -> Bool -> Inferred -> Inferred +inferOnEquality (Node _ expr) (Node _ other) shouldBe dict = + case expr of + Expression.Integer int -> + if shouldBe then + injectFacts + [ Equals other (Expression.Floatable (Basics.toFloat int)) ] + dict + + else + injectFacts + [ NotEquals other (Expression.Floatable (Basics.toFloat int)) ] + dict + + Expression.Floatable float -> + if shouldBe then + injectFacts + [ Equals other (Expression.Floatable float) ] + dict + + else + injectFacts + [ NotEquals other (Expression.Floatable float) ] + dict + + Expression.Literal str -> + if shouldBe then + injectFacts + [ Equals other (Expression.Literal str) ] + dict + + else + injectFacts + [ NotEquals other (Expression.Literal str) ] + dict + + Expression.FunctionOrValue [ "Basics" ] "True" -> + injectFacts + [ Equals other + (if shouldBe then + trueExpr + + else + falseExpr + ) + ] + dict + + Expression.FunctionOrValue [ "Basics" ] "False" -> + injectFacts + [ Equals other + (if shouldBe then + falseExpr + + else + trueExpr + ) + ] + dict + + _ -> + dict diff --git a/tests/Simplify/InferTest.elm b/tests/Simplify/InferTest.elm new file mode 100644 index 00000000..beeb1329 --- /dev/null +++ b/tests/Simplify/InferTest.elm @@ -0,0 +1,535 @@ +module Simplify.InferTest exposing (all) + +import AssocList +import Elm.Syntax.Expression exposing (Expression(..)) +import Elm.Syntax.Infix as Infix exposing (InfixDirection(..)) +import Elm.Syntax.Node exposing (Node(..)) +import Elm.Syntax.Range as Range +import Expect exposing (Expectation) +import Simplify.Infer exposing (DeducedValue(..), Fact(..), Inferred(..), deduceNewFacts, empty, falseExpr, get, infer, trueExpr) +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "Infer" + [ simpleTests + , detailedTests + , deduceNewFactsTests + ] + + +simpleTests : Test +simpleTests = + describe "get" + [ test "should infer a is true when a is True" <| + \() -> + empty + |> infer [ FunctionOrValue [] "a" ] True + |> get (FunctionOrValue [] "a") + |> Expect.equal (Just trueExpr) + , test "should infer a is true when a is False" <| + \() -> + empty + |> infer [ FunctionOrValue [] "a" ] False + |> get (FunctionOrValue [] "a") + |> Expect.equal (Just falseExpr) + , test "should infer a is 1 when a == 1 is True" <| + \() -> + empty + |> infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Floatable 1)) + ] + True + |> get (FunctionOrValue [] "a") + |> Expect.equal (Just (Floatable 1)) + , test "should not infer a when a == 1 is False" <| + \() -> + empty + |> infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Floatable 1)) + ] + False + |> get (FunctionOrValue [] "a") + |> Expect.equal Nothing + , test "should infer a is true when a && b is True" <| + \() -> + empty + |> infer + [ OperatorApplication "&&" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> get (FunctionOrValue [] "a") + |> Expect.equal (Just trueExpr) + , test "should infer b is true when a && b is True" <| + \() -> + empty + |> infer + [ OperatorApplication "&&" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> get (FunctionOrValue [] "b") + |> Expect.equal (Just trueExpr) + , test "should not infer a when a || b is True" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> get (FunctionOrValue [] "a") + |> Expect.equal Nothing + , test "should not infer b when a || b is True" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> get (FunctionOrValue [] "b") + |> Expect.equal Nothing + , test "should infer a is false when a || b is False" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + False + |> get (FunctionOrValue [] "a") + |> Expect.equal (Just falseExpr) + , test "should infer b is false when a || b is False" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + False + |> get (FunctionOrValue [] "b") + |> Expect.equal (Just falseExpr) + , test "should infer b is true when a || b is True and a is False" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> infer [ FunctionOrValue [] "a" ] + False + |> get (FunctionOrValue [] "b") + |> Expect.equal (Just trueExpr) + , test "should infer b is true when b || a is True and a is False" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "b")) + (n (FunctionOrValue [] "a")) + ] + True + |> infer [ FunctionOrValue [] "a" ] + False + |> get (FunctionOrValue [] "b") + |> Expect.equal (Just trueExpr) + , test "should not infer b when a || b is True and a is True" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> infer [ FunctionOrValue [] "a" ] + True + |> get (FunctionOrValue [] "b") + |> Expect.equal Nothing + ] + + +detailedTests : Test +detailedTests = + describe "infer" + [ test "should infer a when True" <| + \() -> + infer + [ FunctionOrValue [] "a" ] + True + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "a") falseExpr + , Equals (FunctionOrValue [] "a") trueExpr + ] + , deduced = + [ ( FunctionOrValue [] "a" + , DTrue + ) + ] + } + , test "should infer a when False" <| + \() -> + infer + [ FunctionOrValue [] "a" ] + False + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "a") trueExpr + , Equals (FunctionOrValue [] "a") falseExpr + ] + , deduced = + [ ( FunctionOrValue [] "a" + , DFalse + ) + ] + } + , test "should infer a == True when True" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n trueExpr) + ] + True + empty + |> expectEqual + { deduced = + [ ( FunctionOrValue [] "a", DTrue ) + , ( OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n trueExpr), DFalse ) + , ( OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n trueExpr), DTrue ) + ] + , facts = + [ Equals (FunctionOrValue [] "a") trueExpr + , NotEquals (OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n trueExpr)) trueExpr + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n trueExpr)) falseExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n trueExpr)) trueExpr + ] + } + , test "should infer a == True when False" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n trueExpr) + ] + False + empty + |> expectEqual + { facts = + [ Equals (FunctionOrValue [] "a") falseExpr + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n trueExpr)) trueExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n trueExpr)) falseExpr + ] + , deduced = + [ ( FunctionOrValue [] "a" + , DFalse + ) + , ( OperatorApplication "==" + Non + (n (FunctionOrValue [] "a")) + (n trueExpr) + , DFalse + ) + ] + } + , test "should infer a == 1 when True" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Floatable 1)) + ] + True + empty + |> expectEqual + { deduced = + [ ( FunctionOrValue [] "a", DNumber 1 ) + , ( OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n (Floatable 1)), DFalse ) + , ( OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Floatable 1)), DTrue ) + ] + , facts = + [ Equals (FunctionOrValue [] "a") (Floatable 1) + , NotEquals (OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n (Floatable 1))) trueExpr + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Floatable 1))) falseExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Floatable 1))) trueExpr + ] + } + , test "should infer a == 1 when False" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Floatable 1)) + ] + False + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "a") (Floatable 1) + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Floatable 1))) trueExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Floatable 1))) falseExpr + ] + , deduced = + [ ( OperatorApplication "==" + Non + (n (FunctionOrValue [] "a")) + (n (Floatable 1)) + , DFalse + ) + ] + } + , test "should infer a == \"ok\" when True" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Literal "\"ok\"")) + ] + True + empty + |> expectEqual + { deduced = + [ ( FunctionOrValue [] "a", DString "\"ok\"" ) + , ( OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\"")), DFalse ) + , ( OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\"")), DTrue ) + ] + , facts = + [ Equals (FunctionOrValue [] "a") (Literal "\"ok\"") + , NotEquals (OperatorApplication "/=" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\""))) trueExpr + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\""))) falseExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\""))) trueExpr + ] + } + , test "should infer a == \"ok\" when False" <| + \() -> + infer + [ OperatorApplication "==" + Infix.Non + (n (FunctionOrValue [] "a")) + (n (Literal "\"ok\"")) + ] + False + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "a") (Literal "\"ok\"") + , NotEquals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\""))) trueExpr + , Equals (OperatorApplication "==" Non (n (FunctionOrValue [] "a")) (n (Literal "\"ok\""))) falseExpr + ] + , deduced = + [ ( OperatorApplication "==" + Non + (n (FunctionOrValue [] "a")) + (n (Literal "\"ok\"")) + , DFalse + ) + ] + } + , test "should infer a && b when True" <| + \() -> + infer + [ OperatorApplication "&&" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "b") falseExpr + , Equals (FunctionOrValue [] "b") trueExpr + , NotEquals (FunctionOrValue [] "a") falseExpr + , Equals (FunctionOrValue [] "a") trueExpr + , NotEquals (OperatorApplication "&&" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) falseExpr + , Equals (OperatorApplication "&&" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) trueExpr + ] + , deduced = + [ ( FunctionOrValue [] "b", DTrue ) + , ( FunctionOrValue [] "a", DTrue ) + , ( OperatorApplication "&&" + Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + , DTrue + ) + ] + } + , test "should infer a && b when False" <| + \() -> + infer + [ OperatorApplication "&&" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + False + empty + |> expectEqual + { facts = + [ Or [ Equals (FunctionOrValue [] "a") falseExpr, NotEquals (FunctionOrValue [] "a") trueExpr ] [ Equals (FunctionOrValue [] "b") falseExpr, NotEquals (FunctionOrValue [] "b") trueExpr ] + , NotEquals (OperatorApplication "&&" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) trueExpr + , Equals (OperatorApplication "&&" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) falseExpr + ] + , deduced = + [ ( OperatorApplication "&&" + Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + , DFalse + ) + ] + } + , test "should infer a || b when True" <| + \() -> + infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + empty + |> expectEqual + { facts = + [ Or [ Equals (FunctionOrValue [] "a") trueExpr, NotEquals (FunctionOrValue [] "a") falseExpr ] [ Equals (FunctionOrValue [] "b") trueExpr, NotEquals (FunctionOrValue [] "b") falseExpr ] + , NotEquals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) falseExpr + , Equals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) trueExpr + ] + , deduced = + [ ( OperatorApplication "||" + Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + , DTrue + ) + ] + } + , test "should infer a || b when False" <| + \() -> + infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + False + empty + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "b") trueExpr + , Equals (FunctionOrValue [] "b") falseExpr + , NotEquals (FunctionOrValue [] "a") trueExpr + , Equals (FunctionOrValue [] "a") falseExpr + , NotEquals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) trueExpr + , Equals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) falseExpr + ] + , deduced = + [ ( FunctionOrValue [] "b", DFalse ) + , ( FunctionOrValue [] "a", DFalse ) + , ( OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b")), DFalse ) + ] + } + , test "should infer a || b when True and a when False" <| + \() -> + empty + |> infer + [ OperatorApplication "||" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ] + True + |> infer [ FunctionOrValue [] "a" ] + False + |> expectEqual + { facts = + [ NotEquals (FunctionOrValue [] "a") trueExpr + , NotEquals (FunctionOrValue [] "b") falseExpr + , Equals (FunctionOrValue [] "b") trueExpr + , Equals (FunctionOrValue [] "a") falseExpr + , Or [ Equals (FunctionOrValue [] "a") trueExpr, NotEquals (FunctionOrValue [] "a") falseExpr ] [ Equals (FunctionOrValue [] "b") trueExpr, NotEquals (FunctionOrValue [] "b") falseExpr ] + , NotEquals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) falseExpr + , Equals (OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) trueExpr + ] + , deduced = + [ ( FunctionOrValue [] "a", DFalse ) + , ( FunctionOrValue [] "b", DTrue ) + , ( OperatorApplication "||" Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b")), DTrue ) + ] + } + ] + + +deduceNewFactsTests : Test +deduceNewFactsTests = + describe "deduceNewFacts" + [ test "should not deduce anything when facts don't share anything (a == True, b == True)" <| + \() -> + deduceNewFacts + (Equals (FunctionOrValue [] "b") trueExpr) + [ Equals (FunctionOrValue [] "a") trueExpr ] + |> Expect.equal [] + , test "should deduce b is True when (a || b) and (a == False)" <| + \() -> + deduceNewFacts + (Equals (FunctionOrValue [] "a") falseExpr) + [ Or + [ Equals (FunctionOrValue [] "a") trueExpr ] + [ Equals (FunctionOrValue [] "b") trueExpr ] + ] + |> Expect.equal + [ Equals (FunctionOrValue [] "b") trueExpr ] + ] + + +expectEqual : + { facts : List Fact + , deduced : List ( Expression, DeducedValue ) + } + -> Inferred + -> Expectation +expectEqual record (Inferred inferred) = + { facts = inferred.facts + , deduced = AssocList.toList inferred.deduced + } + |> Expect.equal record + + +n : Expression -> Node Expression +n = + Node Range.emptyRange diff --git a/tests/Simplify/Normalize.elm b/tests/Simplify/Normalize.elm index 657938af..8982542d 100644 --- a/tests/Simplify/Normalize.elm +++ b/tests/Simplify/Normalize.elm @@ -1,58 +1,154 @@ -module Simplify.Normalize exposing (Comparison(..), areAllTheSame, compare, getNumberValue) +module Simplify.Normalize exposing (Comparison(..), areAllTheSame, compare, compareWithoutNormalization, getNumberValue, normalize) import Dict 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.Syntax.Range as Range +import Elm.Writer import Review.ModuleNameLookupTable as ModuleNameLookupTable exposing (ModuleNameLookupTable) +import Simplify.Infer as Infer -areTheSame : ModuleNameLookupTable -> Node Expression -> Node Expression -> Bool -areTheSame lookupTable left right = - normalize lookupTable left == normalize lookupTable right - - -areAllTheSame : ModuleNameLookupTable -> Node Expression -> List (Node Expression) -> Bool -areAllTheSame lookupTable first rest = +areAllTheSame : Infer.Resources a -> Node Expression -> List (Node Expression) -> Bool +areAllTheSame resources first rest = let normalizedFirst : Node Expression normalizedFirst = - normalize lookupTable first + normalize resources first in - List.all (\node -> normalize lookupTable node == normalizedFirst) rest + List.all (\node -> normalize resources node == normalizedFirst) rest -normalize : ModuleNameLookupTable -> Node Expression -> Node Expression -normalize lookupTable node = +normalize : Infer.Resources a -> Node Expression -> Node Expression +normalize resources node = case Node.value node of Expression.ParenthesizedExpression expr -> - normalize lookupTable expr + normalize resources expr Expression.Application nodes -> - toNode (Expression.Application (List.map (normalize lookupTable) nodes)) + case nodes of + fn :: arg1 :: restOrArgs -> + let + normalizedArg1 : Node Expression + normalizedArg1 = + normalize resources arg1 + in + case normalize resources fn of + Node _ (Expression.RecordAccessFunction fieldAccess) -> + let + recordAccess : Node Expression + recordAccess = + Expression.RecordAccess normalizedArg1 (toNode (String.dropLeft 1 fieldAccess)) + |> toNode + in + if List.isEmpty restOrArgs then + recordAccess - Expression.OperatorApplication string infixDirection left right -> - toNode (Expression.OperatorApplication string infixDirection (normalize lookupTable left) (normalize lookupTable right)) + else + (recordAccess :: List.map (normalize resources) restOrArgs) + |> Expression.Application + |> toNode + + normalizedFn -> + (normalizedFn :: normalizedArg1 :: List.map (normalize resources) restOrArgs) + |> Expression.Application + |> toNode + + _ -> + node + + Expression.OperatorApplication "<|" _ function extraArgument -> + addToFunctionCall + (normalize resources function) + (normalize resources extraArgument) + + Expression.OperatorApplication "|>" _ extraArgument function -> + addToFunctionCall + (normalize resources function) + (normalize resources extraArgument) + + Expression.OperatorApplication "::" infixDirection element list -> + let + normalizedElement : Node Expression + normalizedElement = + normalize resources element + + normalizedList : Node Expression + normalizedList = + normalize resources list + in + case Node.value normalizedList of + Expression.ListExpr elements -> + toNode (Expression.ListExpr (normalizedElement :: elements)) + + _ -> + toNode (Expression.OperatorApplication "::" infixDirection normalizedElement normalizedList) + + Expression.OperatorApplication ">" infixDirection left right -> + toNode (Expression.OperatorApplication "<" infixDirection (normalize resources right) (normalize resources left)) + + Expression.OperatorApplication ">=" infixDirection left right -> + toNode (Expression.OperatorApplication "<=" infixDirection (normalize resources right) (normalize resources left)) + + Expression.OperatorApplication operator infixDirection l r -> + let + left : Node Expression + left = + normalize resources l + + right : Node Expression + right = + normalize resources r + in + if List.member operator [ "+", "*", "||", "&&", "==", "/=" ] && toComparable left > toComparable right then + toNode (Expression.OperatorApplication operator infixDirection right left) + + else + toNode (Expression.OperatorApplication operator infixDirection left right) Expression.FunctionOrValue rawModuleName string -> - let - moduleName : ModuleName - moduleName = - ModuleNameLookupTable.moduleNameFor lookupTable node - |> Maybe.withDefault rawModuleName - in - toNode (Expression.FunctionOrValue moduleName string) + Expression.FunctionOrValue + (ModuleNameLookupTable.moduleNameFor resources.lookupTable node + |> Maybe.withDefault rawModuleName + ) + string + |> toNodeAndInfer resources Expression.IfBlock cond then_ else_ -> - toNode (Expression.IfBlock (normalize lookupTable cond) (normalize lookupTable then_) (normalize lookupTable else_)) + let + reverseIfConditionIsNegated : Node Expression -> Node Expression -> Node Expression -> Node Expression + reverseIfConditionIsNegated condArg thenArg elseArg = + case Node.value condArg of + Expression.Application [ Node _ (Expression.FunctionOrValue [ "Basics" ] "not"), negatedCondition ] -> + reverseIfConditionIsNegated negatedCondition elseArg thenArg + + _ -> + toNode (Expression.IfBlock condArg thenArg elseArg) + in + reverseIfConditionIsNegated + (normalize resources cond) + (normalize resources then_) + (normalize resources else_) Expression.Negation expr -> - toNode (Expression.Negation (normalize lookupTable expr)) + let + normalized : Node Expression + normalized = + normalize resources expr + in + case Node.value normalized of + Expression.Integer int -> + toNode (Expression.Integer -int) + + Expression.Floatable float -> + toNode (Expression.Floatable -float) + + _ -> + toNode (Expression.Negation normalized) Expression.TupledExpression nodes -> - toNode (Expression.TupledExpression (List.map (normalize lookupTable) nodes)) + toNode (Expression.TupledExpression (List.map (normalize resources) nodes)) Expression.LetExpression letBlock -> toNode @@ -74,83 +170,156 @@ normalize lookupTable node = , declaration = toNode { name = toNode (Node.value declaration.name) - , arguments = List.map normalizePattern declaration.arguments - , expression = normalize lookupTable declaration.expression + , arguments = List.map (normalizePattern resources.lookupTable) declaration.arguments + , expression = normalize resources declaration.expression } } ) Expression.LetDestructuring pattern expr -> - toNode (Expression.LetDestructuring (normalizePattern pattern) (normalize lookupTable expr)) + toNode (Expression.LetDestructuring (normalizePattern resources.lookupTable pattern) (normalize resources expr)) ) letBlock.declarations - , expression = normalize lookupTable letBlock.expression + , expression = normalize resources letBlock.expression } ) Expression.CaseExpression caseBlock -> toNode (Expression.CaseExpression - { cases = List.map (\( pattern, expr ) -> ( normalizePattern pattern, normalize lookupTable expr )) caseBlock.cases - , expression = toNode <| Node.value caseBlock.expression + { cases = List.map (\( pattern, expr ) -> ( normalizePattern resources.lookupTable pattern, normalize resources expr )) caseBlock.cases + , expression = normalize resources caseBlock.expression } ) Expression.LambdaExpression lambda -> toNode (Expression.LambdaExpression - { args = List.map normalizePattern lambda.args - , expression = normalize lookupTable lambda.expression + { args = List.map (normalizePattern resources.lookupTable) lambda.args + , expression = normalize resources lambda.expression } ) Expression.ListExpr nodes -> - toNode (Expression.ListExpr (List.map (normalize lookupTable) nodes)) + toNode (Expression.ListExpr (List.map (normalize resources) nodes)) Expression.RecordAccess expr (Node _ field) -> - toNode (Expression.RecordAccess (normalize lookupTable expr) (toNode field)) + toNode (Expression.RecordAccess (normalize resources expr) (toNode field)) Expression.RecordExpr nodes -> nodes |> List.sortBy (\(Node _ ( Node _ fieldName, _ )) -> fieldName) - |> List.map (\(Node _ ( Node _ fieldName, expr )) -> toNode ( toNode fieldName, normalize lookupTable expr )) + |> List.map (\(Node _ ( Node _ fieldName, expr )) -> toNode ( toNode fieldName, normalize resources expr )) |> Expression.RecordExpr |> toNode Expression.RecordUpdateExpression (Node _ value) nodes -> nodes |> List.sortBy (\(Node _ ( Node _ fieldName, _ )) -> fieldName) - |> List.map (\(Node _ ( Node _ fieldName, expr )) -> toNode ( toNode fieldName, normalize lookupTable expr )) + |> List.map (\(Node _ ( Node _ fieldName, expr )) -> toNode ( toNode fieldName, normalize resources expr )) |> Expression.RecordUpdateExpression (toNode value) |> toNode + Expression.Hex int -> + Expression.Integer int + |> toNode + expr -> toNode expr -normalizePattern : Node Pattern -> Node Pattern -normalizePattern node = +toNodeAndInfer : Infer.Resources a -> Expression -> Node Expression +toNodeAndInfer resources element = + case Infer.get element (Tuple.first resources.inferredConstants) of + Just value -> + toNode value + + Nothing -> + toNode element + + +toComparable : Node Expression -> String +toComparable a = + Elm.Writer.write (Elm.Writer.writeExpression a) + + +addToFunctionCall : Node Expression -> Node Expression -> Node Expression +addToFunctionCall functionCall extraArgument = + case Node.value functionCall of + Expression.ParenthesizedExpression expr -> + addToFunctionCall expr extraArgument + + Expression.Application (fnCall :: args) -> + Expression.Application (fnCall :: (args ++ [ extraArgument ])) + |> toNode + + Expression.LetExpression { declarations, expression } -> + Expression.LetExpression { declarations = declarations, expression = addToFunctionCall expression extraArgument } + |> toNode + + Expression.IfBlock condition ifBranch elseBranch -> + Expression.IfBlock condition (addToFunctionCall ifBranch extraArgument) (addToFunctionCall elseBranch extraArgument) + |> toNode + + Expression.CaseExpression { expression, cases } -> + Expression.CaseExpression { expression = expression, cases = List.map (\( cond, expr ) -> ( cond, addToFunctionCall expr extraArgument )) cases } + |> toNode + + Expression.RecordAccessFunction fieldAccess -> + Expression.RecordAccess extraArgument (toNode (String.dropLeft 1 fieldAccess)) + |> toNode + + _ -> + Expression.Application [ functionCall, extraArgument ] + |> toNode + + +normalizePattern : ModuleNameLookupTable -> Node Pattern -> Node Pattern +normalizePattern lookupTable node = case Node.value node of Pattern.TuplePattern patterns -> - toNode (Pattern.TuplePattern (List.map normalizePattern patterns)) + toNode (Pattern.TuplePattern (List.map (normalizePattern lookupTable) patterns)) Pattern.RecordPattern fields -> - toNode (Pattern.RecordPattern (List.map (\(Node _ field) -> toNode field) fields)) + toNode + (Pattern.RecordPattern + (fields + |> List.sortBy (\(Node _ fieldName) -> fieldName) + |> List.map (\(Node _ field) -> toNode field) + ) + ) Pattern.UnConsPattern element list -> - toNode (Pattern.UnConsPattern (normalizePattern element) (normalizePattern list)) + case normalizePattern lookupTable list of + Node _ (Pattern.ListPattern elements) -> + toNode (Pattern.ListPattern (normalizePattern lookupTable element :: elements)) + + normalizedList -> + toNode (Pattern.UnConsPattern (normalizePattern lookupTable element) normalizedList) Pattern.ListPattern patterns -> - toNode (Pattern.ListPattern (List.map normalizePattern patterns)) + toNode (Pattern.ListPattern (List.map (normalizePattern lookupTable) patterns)) Pattern.NamedPattern qualifiedNameRef patterns -> - toNode (Pattern.NamedPattern qualifiedNameRef (List.map normalizePattern patterns)) + let + nameRef : Pattern.QualifiedNameRef + nameRef = + { moduleName = + ModuleNameLookupTable.moduleNameFor lookupTable node + |> Maybe.withDefault qualifiedNameRef.moduleName + , name = qualifiedNameRef.name + } + in + toNode (Pattern.NamedPattern nameRef (List.map (normalizePattern lookupTable) patterns)) Pattern.AsPattern pattern (Node _ asName) -> - toNode (Pattern.AsPattern (normalizePattern pattern) (toNode asName)) + toNode (Pattern.AsPattern (normalizePattern lookupTable pattern) (toNode asName)) Pattern.ParenthesizedPattern pattern -> - normalizePattern pattern + normalizePattern lookupTable pattern + + Pattern.HexPattern int -> + toNode (Pattern.IntPattern int) pattern -> toNode pattern @@ -171,46 +340,40 @@ type Comparison | Unconfirmed -compare : ModuleNameLookupTable -> Node Expression -> Node Expression -> Comparison -compare lookupTable leftNode right = - compareHelp lookupTable leftNode right True +compare : Infer.Resources a -> Node Expression -> Node Expression -> Comparison +compare resources leftNode right = + compareHelp + (normalize resources leftNode) + (normalize resources right) + True -compareHelp : ModuleNameLookupTable -> Node Expression -> Node Expression -> Bool -> Comparison -compareHelp lookupTable leftNode right canFlip = +compareWithoutNormalization : Node Expression -> Node Expression -> Comparison +compareWithoutNormalization leftNode right = + compareHelp leftNode right True + + +compareHelp : Node Expression -> Node Expression -> Bool -> Comparison +compareHelp leftNode right canFlip = let - fallback : Node Expression -> Comparison - fallback rightNode = + fallback : () -> Comparison + fallback () = if canFlip then - compareHelp lookupTable rightNode leftNode False + compareHelp right leftNode False - else if areTheSame lookupTable leftNode right then + else if leftNode == right then ConfirmedEquality else Unconfirmed in case Node.value leftNode of - Expression.ParenthesizedExpression expr -> - compareHelp lookupTable expr right canFlip - Expression.Integer left -> compareNumbers (Basics.toFloat left) right Expression.Floatable left -> compareNumbers left right - Expression.Hex left -> - compareNumbers (Basics.toFloat left) right - - Expression.Negation left -> - case getNumberValue left of - Just leftValue -> - compareNumbers -leftValue right - - Nothing -> - fallback right - Expression.OperatorApplication leftOp _ leftLeft leftRight -> if List.member leftOp [ "+", "-", "*", "/" ] then case getNumberValue leftNode of @@ -220,25 +383,24 @@ compareHelp lookupTable leftNode right canFlip = fromEquality (leftValue == rightValue) Nothing -> - fallback right + fallback () Nothing -> - fallback right + fallback () else case Node.value (removeParens right) of Expression.OperatorApplication rightOp _ rightLeft rightRight -> if leftOp == rightOp then compareEqualityOfAll - lookupTable [ leftLeft, leftRight ] [ rightLeft, rightRight ] else - fallback right + fallback () _ -> - fallback right + fallback () Expression.Literal left -> case Node.value (removeParens right) of @@ -246,7 +408,7 @@ compareHelp lookupTable leftNode right canFlip = fromEquality (left == rightValue) _ -> - fallback right + fallback () Expression.CharLiteral left -> case Node.value (removeParens right) of @@ -254,29 +416,19 @@ compareHelp lookupTable leftNode right canFlip = fromEquality (left == rightValue) _ -> - fallback right + fallback () - Expression.FunctionOrValue _ leftName -> - let - right_ : Node Expression - right_ = - removeParens right - in - case Node.value right_ of - Expression.FunctionOrValue _ rightName -> - if - isSameReference - lookupTable - ( Node.range leftNode, leftName ) - ( Node.range right_, rightName ) - then + Expression.FunctionOrValue moduleNameLeft leftName -> + case Node.value right of + Expression.FunctionOrValue moduleNameRight rightName -> + if leftName == rightName && moduleNameRight == moduleNameLeft then ConfirmedEquality else - fallback right_ + fallback () _ -> - fallback right + fallback () Expression.ListExpr leftList -> case Node.value (removeParens right) of @@ -285,58 +437,58 @@ compareHelp lookupTable leftNode right canFlip = ConfirmedInequality else - compareLists lookupTable leftList rightList ConfirmedEquality + compareLists leftList rightList ConfirmedEquality _ -> - fallback right + fallback () Expression.TupledExpression leftList -> case Node.value (removeParens right) of Expression.TupledExpression rightList -> - compareLists lookupTable leftList rightList ConfirmedEquality + compareLists leftList rightList ConfirmedEquality _ -> - fallback right + fallback () Expression.RecordExpr leftList -> case Node.value (removeParens right) of Expression.RecordExpr rightList -> - compareRecords lookupTable leftList rightList ConfirmedEquality + compareRecords leftList rightList ConfirmedEquality _ -> - fallback right + fallback () Expression.RecordUpdateExpression leftBaseValue leftList -> case Node.value (removeParens right) of Expression.RecordUpdateExpression rightBaseValue rightList -> if Node.value leftBaseValue == Node.value rightBaseValue then - compareRecords lookupTable leftList rightList ConfirmedEquality + compareRecords leftList rightList ConfirmedEquality else - compareRecords lookupTable leftList rightList Unconfirmed + compareRecords leftList rightList Unconfirmed _ -> - fallback right + fallback () Expression.Application leftArgs -> case Node.value (removeParens right) of Expression.Application rightArgs -> - compareEqualityOfAll lookupTable leftArgs rightArgs + compareEqualityOfAll leftArgs rightArgs _ -> - fallback right + fallback () Expression.RecordAccess leftExpr leftName -> case Node.value (removeParens right) of Expression.RecordAccess rightExpr rightName -> if Node.value leftName == Node.value rightName then - compareHelp lookupTable leftExpr rightExpr canFlip + compareHelp leftExpr rightExpr canFlip else Unconfirmed _ -> - fallback right + fallback () Expression.UnitExpr -> ConfirmedEquality @@ -345,27 +497,14 @@ compareHelp lookupTable leftNode right canFlip = case Node.value (removeParens right) of Expression.IfBlock rightCond rightThen rightElse -> compareEqualityOfAll - lookupTable [ leftCond, leftThen, leftElse ] [ rightCond, rightThen, rightElse ] _ -> - fallback right + fallback () _ -> - fallback right - - -isSameReference : ModuleNameLookupTable -> ( Range, String ) -> ( Range, String ) -> Bool -isSameReference lookupTable ( leftFnRange, leftFnName ) ( rightFnRange, rightFnName ) = - if leftFnName == rightFnName then - Maybe.map2 (==) - (ModuleNameLookupTable.moduleNameAt lookupTable leftFnRange) - (ModuleNameLookupTable.moduleNameAt lookupTable rightFnRange) - |> Maybe.withDefault False - - else - False + fallback () compareNumbers : Float -> Node Expression -> Comparison @@ -424,31 +563,31 @@ getNumberValue node = Nothing -compareLists : ModuleNameLookupTable -> List (Node Expression) -> List (Node Expression) -> Comparison -> Comparison -compareLists lookupTable leftList rightList acc = +compareLists : List (Node Expression) -> List (Node Expression) -> Comparison -> Comparison +compareLists leftList rightList acc = case ( leftList, rightList ) of ( left :: restOfLeft, right :: restOfRight ) -> - case compareHelp lookupTable left right True of + case compareWithoutNormalization left right of ConfirmedEquality -> - compareLists lookupTable restOfLeft restOfRight acc + compareLists restOfLeft restOfRight acc ConfirmedInequality -> ConfirmedInequality Unconfirmed -> - compareLists lookupTable restOfLeft restOfRight Unconfirmed + compareLists restOfLeft restOfRight Unconfirmed _ -> acc -compareEqualityOfAll : ModuleNameLookupTable -> List (Node Expression) -> List (Node Expression) -> Comparison -compareEqualityOfAll lookupTable leftList rightList = +compareEqualityOfAll : List (Node Expression) -> List (Node Expression) -> Comparison +compareEqualityOfAll leftList rightList = case ( leftList, rightList ) of ( left :: restOfLeft, right :: restOfRight ) -> - case compareHelp lookupTable left right True of + case compareHelp left right True of ConfirmedEquality -> - compareEqualityOfAll lookupTable restOfLeft restOfRight + compareEqualityOfAll restOfLeft restOfRight ConfirmedInequality -> Unconfirmed @@ -465,8 +604,8 @@ type RecordFieldComparison | HasBothValues (Node Expression) (Node Expression) -compareRecords : ModuleNameLookupTable -> List (Node Expression.RecordSetter) -> List (Node Expression.RecordSetter) -> Comparison -> Comparison -compareRecords lookupTable leftList rightList acc = +compareRecords : List (Node Expression.RecordSetter) -> List (Node Expression.RecordSetter) -> Comparison -> Comparison +compareRecords leftList rightList acc = let leftFields : List ( String, Node Expression ) leftFields = @@ -487,28 +626,28 @@ compareRecords lookupTable leftList rightList acc = Dict.empty |> Dict.values in - compareRecordFields lookupTable recordFieldComparisons acc + compareRecordFields recordFieldComparisons acc -compareRecordFields : ModuleNameLookupTable -> List RecordFieldComparison -> Comparison -> Comparison -compareRecordFields lookupTable recordFieldComparisons acc = +compareRecordFields : List RecordFieldComparison -> Comparison -> Comparison +compareRecordFields recordFieldComparisons acc = case recordFieldComparisons of [] -> acc MissingOtherValue :: rest -> - compareRecordFields lookupTable rest Unconfirmed + compareRecordFields rest Unconfirmed (HasBothValues a b) :: rest -> - case compare lookupTable a b of + case compareHelp a b True of ConfirmedInequality -> ConfirmedInequality ConfirmedEquality -> - compareRecordFields lookupTable rest acc + compareRecordFields rest acc Unconfirmed -> - compareRecordFields lookupTable rest Unconfirmed + compareRecordFields rest Unconfirmed fromEquality : Bool -> Comparison diff --git a/tests/Simplify/NormalizeTest.elm b/tests/Simplify/NormalizeTest.elm new file mode 100644 index 00000000..30037411 --- /dev/null +++ b/tests/Simplify/NormalizeTest.elm @@ -0,0 +1,849 @@ +module Simplify.NormalizeTest exposing (all) + +import Dict +import Elm.Dependency +import Elm.Interface as Interface +import Elm.Parser as Parser +import Elm.Processing +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Expression exposing (Expression(..), LetDeclaration(..)) +import Elm.Syntax.File exposing (File) +import Elm.Syntax.Infix as Infix +import Elm.Syntax.ModuleName exposing (ModuleName) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Pattern exposing (Pattern(..)) +import Elm.Syntax.Range as Range exposing (Range) +import Expect +import Review.ModuleNameLookupTable as ModuleNameLookupTable +import Simplify.Infer as Infer +import Simplify.Normalize as Normalize +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "Normalize" + [ simpleNormalizationTests + , moduleNameTests + , inferTests + ] + + +simpleNormalizationTests : Test +simpleNormalizationTests = + describe "Simple normalizations" + [ test "should remove parentheses" <| + \() -> + "(1)" + |> normalizeAndExpect (Integer 1) + , test "should remove ranges of 'f a'" <| + \() -> + "f a" + |> normalizeAndExpect + (Application + [ n (FunctionOrValue [] "f") + , n (FunctionOrValue [] "a") + ] + ) + , test "should turn '.field a' into 'a.field'" <| + \() -> + ".field a" + |> normalizeAndExpect + (RecordAccess + (n (FunctionOrValue [] "a")) + (n "field") + ) + , test "should turn '.field <| a' into 'a.field'" <| + \() -> + "(.field) <| (a)" + |> normalizeAndExpect + (RecordAccess + (n (FunctionOrValue [] "a")) + (n "field") + ) + , test "should turn '(a).field' into 'a.field'" <| + \() -> + "(a).field" + |> normalizeAndExpect + (RecordAccess + (n (FunctionOrValue [] "a")) + (n "field") + ) + , test "should turn '.field a b' into 'a.field b'" <| + \() -> + ".field a b" + |> normalizeAndExpect + (Application + [ n + (RecordAccess + (n (FunctionOrValue [] "a")) + (n "field") + ) + , n (FunctionOrValue [] "b") + ] + ) + , test "should normalize a function and function arguments" <| + \() -> + "(a) (b) (c)" + |> normalizeAndExpect + (Application + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + , n (FunctionOrValue [] "c") + ] + ) + , test "should remove <|" <| + \() -> + "a b <| c" + |> normalizeAndExpect + (Application + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + , n (FunctionOrValue [] "c") + ] + ) + , test "should remove |>" <| + \() -> + "c |> a b" + |> normalizeAndExpect + (Application + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + , n (FunctionOrValue [] "c") + ] + ) + , test "should remove <| when the left is not already an application " <| + \() -> + "(a) <| (b)" + |> normalizeAndExpect + (Application + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + ] + ) + , test "should remove <| when the left is a let expression" <| + \() -> + "(let a = (fn) in a) <| (b)" + |> normalizeAndExpect + (LetExpression + { declarations = + [ n + (LetFunction + { declaration = + n + { arguments = [] + , expression = n (FunctionOrValue [] "fn") + , name = n "a" + } + , documentation = Nothing + , signature = Nothing + } + ) + ] + , expression = + n + (Application + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + ] + ) + } + ) + , test "should remove <| when the left is an if expression" <| + \() -> + "(if (cond) then (fn1) else (fn2)) <| (b)" + |> normalizeAndExpect + (IfBlock (n (FunctionOrValue [] "cond")) + (n + (Application + [ n (FunctionOrValue [] "fn1") + , n (FunctionOrValue [] "b") + ] + ) + ) + (n + (Application + [ n (FunctionOrValue [] "fn2") + , n (FunctionOrValue [] "b") + ] + ) + ) + ) + , test "should remove <| when the left is a case expression" <| + \() -> + """ + (case (x) of + 1 -> (fn1) + 2 -> (fn2) + ) <| (b) +""" + |> normalizeAndExpect + (CaseExpression + { expression = n (FunctionOrValue [] "x") + , cases = + [ ( n (IntPattern 1) + , n + (Application + [ n (FunctionOrValue [] "fn1") + , n (FunctionOrValue [] "b") + ] + ) + ) + , ( n (IntPattern 2) + , n + (Application + [ n (FunctionOrValue [] "fn2") + , n (FunctionOrValue [] "b") + ] + ) + ) + ] + } + ) + , test "should normalize an operator application" <| + \() -> + """ + [ (a) < (b) + , (a) <= (b) + , (a) == (b) + , (a) /= (b) + , (a) ++ (b) + , (a) + (b) + , (a) - (b) + , (a) * (b) + , (a) / (b) + , (a) // (b) + , (a) ^ (b) + , (a) && (b) + , (a) || (b) + , (a) (b) + , (a) (b) + , (a) |. (b) + , (a) |= (b) + ]""" + |> normalizeAndExpect + (ListExpr + [ n (OperatorApplication "<" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "<=" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "==" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "/=" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "++" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "+" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "-" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "*" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "/" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "//" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "^" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "&&" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "||" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "|." Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "|=" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + ] + ) + , test "should re-order operands of '+', '*', '||', '&&', '==', '/=' alphabetically" <| + \() -> + """ + [ (b) + (a) + , (b) * (a) + , (b) || (a) + , (b) && (a) + , (b) == (a) + , (b) /= (a) + ]""" + |> normalizeAndExpect + (ListExpr + [ n (OperatorApplication "+" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "*" Infix.Left (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "||" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "&&" Infix.Right (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "==" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + , n (OperatorApplication "/=" Infix.Non (n (FunctionOrValue [] "a")) (n (FunctionOrValue [] "b"))) + ] + ) + , test "should replace > by <" <| + \() -> + "(a) > (b)" + |> normalizeAndExpect + (OperatorApplication + "<" + Infix.Non + (n (FunctionOrValue [] "b")) + (n (FunctionOrValue [] "a")) + ) + , test "should replace >= by <=" <| + \() -> + "(a) >= (b)" + |> normalizeAndExpect + (OperatorApplication + "<=" + Infix.Non + (n (FunctionOrValue [] "b")) + (n (FunctionOrValue [] "a")) + ) + , test "should normalize uncons with a list literal into a list literal" <| + \() -> + "(a) :: [(b)]" + |> normalizeAndExpect + (ListExpr + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + ] + ) + , test "should normalize uncons with a non-list literal into a cons operation still" <| + \() -> + "(a) :: (b)" + |> normalizeAndExpect + (OperatorApplication + "::" + Infix.Right + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + ) + , test "should normalize an if expression" <| + \() -> + "if (a) then (b) else (c)" + |> normalizeAndExpect + (IfBlock + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + (n (FunctionOrValue [] "c")) + ) + , test "should remove the `not` calls in the condition by switching the branches" <| + \() -> + "if (Basics.not a) then (b) else (c)" + |> normalizeAndExpect + (IfBlock + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "c")) + (n (FunctionOrValue [] "b")) + ) + , test "should remove the `not` calls in the condition by switching the branches multiple times" <| + \() -> + "if (Basics.not <| Basics.not a) then (b) else (c)" + |> normalizeAndExpect + (IfBlock + (n (FunctionOrValue [] "a")) + (n (FunctionOrValue [] "b")) + (n (FunctionOrValue [] "c")) + ) + , test "should normalize an integer negation" <| + \() -> + "-1" + |> normalizeAndExpect (Integer -1) + , test "should normalize a float negation" <| + \() -> + "-1.1" + |> normalizeAndExpect (Floatable -1.1) + , test "should normalize negations of something else" <| + \() -> + "-(a)" + |> normalizeAndExpect + (Negation (n (FunctionOrValue [] "a"))) + , test "should normalize tuples" <| + \() -> + "( (a), (b) )" + |> normalizeAndExpect + (TupledExpression + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + ] + ) + , test "should normalize lists" <| + \() -> + "[ (a), (b) ]" + |> normalizeAndExpect + (ListExpr + [ n (FunctionOrValue [] "a") + , n (FunctionOrValue [] "b") + ] + ) + , test "should normalize lambdas" <| + \() -> + "\\(a) -> (a)" + |> normalizeAndExpect + (LambdaExpression + { args = [ n (VarPattern "a") ] + , expression = n (FunctionOrValue [] "a") + } + ) + , test "should normalize records, and sort fields alphabetically" <| + \() -> + "{ field = (2), a = (1), z = (3) }" + |> normalizeAndExpect + (RecordExpr + [ n ( n "a", n (Integer 1) ) + , n ( n "field", n (Integer 2) ) + , n ( n "z", n (Integer 3) ) + ] + ) + , test "should normalize record updates, and sort fields alphabetically" <| + \() -> + "{ record | field = (2), a = (1), z = (3) }" + |> normalizeAndExpect + (RecordUpdateExpression + (n "record") + [ n ( n "a", n (Integer 1) ) + , n ( n "field", n (Integer 2) ) + , n ( n "z", n (Integer 3) ) + ] + ) + , test "should normalize hex literals to integers" <| + \() -> + "0x100" + |> normalizeAndExpect + (Integer 256) + , test "should normalize let expressions" <| + \() -> + """ + let + (a) = (1) + f : toBeRemoved + f (n) = (2) + in + (2) +""" + |> normalizeAndExpect + (LetExpression + { declarations = + [ n + (LetDestructuring + (n (VarPattern "a")) + (n (Integer 1)) + ) + , n + (LetFunction + { declaration = + n + { arguments = + [ n (VarPattern "n") + ] + , expression = n (Integer 2) + , name = n "f" + } + , documentation = Nothing + , signature = Nothing + } + ) + ] + , expression = n (Integer 2) + } + ) + , test "should normalize case expressions" <| + \() -> + """case (x) of + ( (0x100), (b) ) :: (c) -> (1) + a :: [ (b) ] -> (2) + { field, a, z } -> (3) + Basics.Just True -> (4) + ((a) as b) -> (5) +""" + |> normalizeAndExpect + (CaseExpression + { cases = + [ ( n + (UnConsPattern + (n + (TuplePattern + [ n (IntPattern 256) + , n (VarPattern "b") + ] + ) + ) + (n (VarPattern "c")) + ) + , n (Integer 1) + ) + , ( n + (ListPattern + [ n (VarPattern "a") + , n (VarPattern "b") + ] + ) + , n (Integer 2) + ) + , ( n + (RecordPattern + [ n "a" + , n "field" + , n "z" + ] + ) + , n (Integer 3) + ) + , ( n + (NamedPattern + { moduleName = [ "Basics" ] + , name = "Just" + } + [ n (NamedPattern { moduleName = [], name = "True" } []) ] + ) + , n (Integer 4) + ) + , ( n (AsPattern (n (VarPattern "a")) (n "b")) + , n (Integer 5) + ) + ] + , expression = n (FunctionOrValue [] "x") + } + ) + ] + + +moduleNameTests : Test +moduleNameTests = + describe "Module name normalization" + [ test "should normalize module name in expression (unknown)" <| + \() -> + "A.b" + |> normalizeWithModuleNamesAndExpect [] + (FunctionOrValue [ "A" ] "b") + , test "should normalize module name in expression (unchanged)" <| + \() -> + "A.b" + |> normalizeWithModuleNamesAndExpect + [ ( { start = { row = 2, column = 9 }, end = { row = 2, column = 12 } } + , [ "A" ] + ) + ] + (FunctionOrValue [ "A" ] "b") + , test "should normalize module name in expression (aliased)" <| + \() -> + "A.b" + |> normalizeWithModuleNamesAndExpect + [ ( { start = { row = 2, column = 9 }, end = { row = 2, column = 12 } } + , [ "Something", "Else" ] + ) + ] + (FunctionOrValue [ "Something", "Else" ] "b") + , test "should normalize module name in expression (unqualified)" <| + \() -> + "b" + |> normalizeWithModuleNamesAndExpect + [ ( { start = { row = 2, column = 9 }, end = { row = 2, column = 10 } } + , [ "A" ] + ) + ] + (FunctionOrValue [ "A" ] "b") + , test "should normalize module name in patterns" <| + \() -> + """case x of + B.Just (a) -> (1) + Nothing -> (2) + _ -> (3) +""" + |> normalizeWithModuleNamesAndExpect + [ ( { start = { row = 3, column = 3 }, end = { row = 3, column = 13 } } + , [ "Basics" ] + ) + , ( { start = { row = 4, column = 3 }, end = { row = 4, column = 10 } } + , [ "Basics" ] + ) + ] + (CaseExpression + { cases = + [ ( n (NamedPattern { moduleName = [ "Basics" ], name = "Just" } [ n (VarPattern "a") ]) + , n (Integer 1) + ) + , ( n (NamedPattern { moduleName = [ "Basics" ], name = "Nothing" } []) + , n (Integer 2) + ) + , ( n AllPattern + , n (Integer 3) + ) + ] + , expression = n (FunctionOrValue [] "x") + } + ) + ] + + +inferTests : Test +inferTests = + describe "Normalization through inferred values" + [ test "should not replace anything when nothing has been inferred" <| + \() -> + "a" + |> normalizeWithInferredAndExpect + [] + [] + (FunctionOrValue [] "a") + , test "should replace reference when its value is known" <| + \() -> + "a" + |> normalizeWithInferredAndExpect + [ ( { start = { row = 2, column = 9 }, end = { row = 2, column = 10 } } + , [ "A" ] + ) + ] + [ ( FunctionOrValue [ "A" ] "a", Infer.DTrue ) ] + (FunctionOrValue [ "Basics" ] "True") + , test "should not replace reference when module name is unknown" <| + \() -> + "a" + |> normalizeWithInferredAndExpect + [] + [ ( FunctionOrValue [ "A" ] "a", Infer.DTrue ) ] + (FunctionOrValue [] "a") + , test "should not replace reference when module name is not the same" <| + \() -> + "a" + |> normalizeWithInferredAndExpect + [ ( { start = { row = 2, column = 9 }, end = { row = 2, column = 10 } } + , [ "B" ] + ) + ] + [ ( FunctionOrValue [ "A" ] "a", Infer.DTrue ) ] + (FunctionOrValue [ "B" ] "a") + ] + + +n : a -> Node a +n = + Node Range.emptyRange + + +normalizeAndExpect : Expression -> String -> Expect.Expectation +normalizeAndExpect expected source = + normalizeBase [] Infer.empty expected source + + +normalizeWithModuleNamesAndExpect : List ( Range, ModuleName ) -> Expression -> String -> Expect.Expectation +normalizeWithModuleNamesAndExpect moduleNames expected source = + normalizeBase moduleNames Infer.empty expected source + + +normalizeWithInferredAndExpect : List ( Range, ModuleName ) -> List ( Expression, Infer.DeducedValue ) -> Expression -> String -> Expect.Expectation +normalizeWithInferredAndExpect moduleNames inferredList expected source = + normalizeBase moduleNames (Infer.fromList inferredList) expected source + + +normalizeBase : List ( Range, ModuleName ) -> Infer.Inferred -> Expression -> String -> Expect.Expectation +normalizeBase moduleNames inferred expected source = + ("module A exposing (..)\nvalue = " ++ source) + |> parse + |> getValue + |> Normalize.normalize + { lookupTable = ModuleNameLookupTable.createForTests [ "A" ] moduleNames + , inferredConstants = ( inferred, [] ) + } + |> Node.value + |> Expect.equal expected + + +{-| Parse source code into a AST. +-} +parse : String -> File +parse source = + case Parser.parse source of + Ok ast -> + Elm.Processing.process elmProcessContext ast + + Err _ -> + Debug.todo "Source code given to test contained invalid syntax" + + +getValue : File -> Node Expression +getValue file = + case findValueDeclaration file.declarations of + Just expression -> + expression + + Nothing -> + Debug.todo "Source code did not contain a value declaration" + + +findValueDeclaration : List (Node Declaration) -> Maybe (Node Expression) +findValueDeclaration declarations = + findMap + (\node -> + case Node.value node of + Declaration.FunctionDeclaration { declaration } -> + if Node.value (Node.value declaration).name == "value" then + Just (Node.value declaration).expression + + else + Nothing + + _ -> + Nothing + ) + declarations + + +elmProcessContext : Elm.Processing.ProcessContext +elmProcessContext = + Elm.Processing.init + |> Elm.Processing.addDependency elmCore + + +elmCore : Elm.Dependency.Dependency +elmCore = + { name = "elm/core" + , version = "1.0.0" + , interfaces = + Dict.fromList + [ ( [ "Basics" ] + , [ -- infix right 0 (<|) = apL + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 0 + , operator = Node.Node Range.emptyRange "<|" + , function = Node.Node Range.emptyRange "apL" + } + , -- infix left 0 (|>) = apR + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 0 + , operator = Node.Node Range.emptyRange "|>" + , function = Node.Node Range.emptyRange "apR" + } + , -- infix right 2 (||) = or + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 2 + , operator = Node.Node Range.emptyRange "||" + , function = Node.Node Range.emptyRange "or" + } + , -- infix right 3 (&&) = and + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 3 + , operator = Node.Node Range.emptyRange "&&" + , function = Node.Node Range.emptyRange "and" + } + , -- infix non 4 (==) = eq + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange "==" + , function = Node.Node Range.emptyRange "eq" + } + , -- infix non 4 (/=) = neq + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange "/=" + , function = Node.Node Range.emptyRange "neq" + } + , -- infix non 4 (<) = lt + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange "<" + , function = Node.Node Range.emptyRange "lt" + } + , -- infix non 4 (>) = gt + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange ">" + , function = Node.Node Range.emptyRange "gt" + } + , -- infix non 4 (<=) = le + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange "<=" + , function = Node.Node Range.emptyRange "le" + } + , -- infix non 4 (>=) = ge + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Non + , precedence = Node.Node Range.emptyRange 4 + , operator = Node.Node Range.emptyRange ">=" + , function = Node.Node Range.emptyRange "ge" + } + , -- infix right 5 (++) = append + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 5 + , operator = Node.Node Range.emptyRange "++" + , function = Node.Node Range.emptyRange "append" + } + , -- infix left 6 (+) = add + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 6 + , operator = Node.Node Range.emptyRange "+" + , function = Node.Node Range.emptyRange "add" + } + , -- infix left 6 (-) = sub + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 6 + , operator = Node.Node Range.emptyRange "-" + , function = Node.Node Range.emptyRange "sub" + } + , -- infix left 7 (*) = mul + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 7 + , operator = Node.Node Range.emptyRange "*" + , function = Node.Node Range.emptyRange "mul" + } + , -- infix left 7 (/) = fdiv + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 7 + , operator = Node.Node Range.emptyRange "/" + , function = Node.Node Range.emptyRange "fdiv" + } + , -- infix left 7 (//) = idiv + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 7 + , operator = Node.Node Range.emptyRange "//" + , function = Node.Node Range.emptyRange "idiv" + } + , -- infix right 8 (^) = pow + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 8 + , operator = Node.Node Range.emptyRange "^" + , function = Node.Node Range.emptyRange "pow" + } + , -- infix left 9 (<<) = composeL + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Left + , precedence = Node.Node Range.emptyRange 9 + , operator = Node.Node Range.emptyRange "<<" + , function = Node.Node Range.emptyRange "composeL" + } + , -- infix right 9 (>>) = composeR + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 9 + , operator = Node.Node Range.emptyRange ">>" + , function = Node.Node Range.emptyRange "composeR" + } + ] + ) + , ( [ "List" ] + , [ -- infix right 5 (::) = cons + Interface.Operator + { direction = Node.Node Range.emptyRange Infix.Right + , precedence = Node.Node Range.emptyRange 5 + , operator = Node.Node Range.emptyRange "::" + , function = Node.Node Range.emptyRange "cons" + } + ] + ) + ] + } + + +findMap : (a -> Maybe b) -> List a -> Maybe b +findMap mapper nodes = + case nodes of + [] -> + Nothing + + node :: rest -> + case mapper node of + Just value -> + Just value + + Nothing -> + findMap mapper rest diff --git a/tests/Simplify/RangeDict.elm b/tests/Simplify/RangeDict.elm new file mode 100644 index 00000000..095968c3 --- /dev/null +++ b/tests/Simplify/RangeDict.elm @@ -0,0 +1,39 @@ +module Simplify.RangeDict exposing (RangeDict, empty, get, insert, member) + +import Dict exposing (Dict) +import Elm.Syntax.Range exposing (Range) + + +type alias RangeDict v = + Dict String v + + +empty : RangeDict v +empty = + Dict.empty + + +insert : Range -> v -> RangeDict v -> RangeDict v +insert range = + Dict.insert (rangeAsString range) + + +get : Range -> RangeDict v -> Maybe v +get range = + Dict.get (rangeAsString range) + + +member : Range -> RangeDict v -> Bool +member range = + Dict.member (rangeAsString range) + + +rangeAsString : Range -> String +rangeAsString range = + [ range.start.row + , range.start.column + , range.end.row + , range.end.column + ] + |> List.map String.fromInt + |> String.join "_" diff --git a/tests/SimplifyTest.elm b/tests/SimplifyTest.elm index ca043784..57345cb3 100644 --- a/tests/SimplifyTest.elm +++ b/tests/SimplifyTest.elm @@ -15,6 +15,7 @@ all = , caseOfTests , booleanCaseOfTests , ifTests + , duplicatedIfTests , recordUpdateTests , numberTests , fullyAppliedPrefixOperatorTests @@ -30,6 +31,7 @@ all = , subTests , parserTests , jsonDecodeTests + , recordAccessTests ] @@ -39,63 +41,100 @@ all = configurationTests : Test configurationTests = - let - details : List String - details = - [ "I expect valid type names to be passed to Simplify.ignoreCaseOfForTypes, that include the module name, like `Module.Name.TypeName`." - ] - in describe "Configuration" [ test "should not report configuration error if all ignored constructors exist" <| \() -> - [ """module A exposing (..) + """module A exposing (..) type B = B type C = C -""", """module B.C exposing (..) -type D = D -""" ] - |> Review.Test.runOnModules (rule <| ignoreCaseOfForTypes [ "A.B", "A.C", "B.C.D" ] defaults) +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "Maybe.Maybe", "Result.Result" ] defaults) |> Review.Test.expectNoErrors , test "should report configuration error if passed an invalid module name" <| \() -> - ignoreCaseOfForTypes [ "_.B" ] defaults - |> rule - |> Review.Test.expectConfigurationError - { message = "Invalid type names: `_.B`" - , details = details - } + """module A exposing (..) +a = 1 +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "_.B" ] defaults) + |> Review.Test.expectGlobalErrors + [ { message = "Could not find type names: `_.B`" + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } + ] , test "should report configuration error if passed an invalid type name" <| \() -> - ignoreCaseOfForTypes [ "A.f" ] defaults - |> rule - |> Review.Test.expectConfigurationError - { message = "Invalid type names: `A.f`" - , details = details - } + """module A exposing (..) +a = 1 +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "A.f" ] defaults) + |> Review.Test.expectGlobalErrors + [ { message = "Could not find type names: `A.f`" + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } + ] , test "should report configuration error if passed an empty type name" <| \() -> - ignoreCaseOfForTypes [ "" ] defaults - |> rule - |> Review.Test.expectConfigurationError - { message = "Invalid type names: ``" - , details = details - } + """module A exposing (..) +a = 1 +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "" ] defaults) + |> Review.Test.expectGlobalErrors + [ { message = "Could not find type names: ``" + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } + ] , test "should report configuration error if passed a type name without a module name" <| \() -> - ignoreCaseOfForTypes [ "B" ] defaults - |> rule - |> Review.Test.expectConfigurationError - { message = "Invalid type names: `B`" - , details = details - } + """module A exposing (..) +a = 1 +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "B" ] defaults) + |> Review.Test.expectGlobalErrors + [ { message = "Could not find type names: `B`" + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } + ] , test "should report configuration error if passed multiple invalid types" <| \() -> - ignoreCaseOfForTypes [ "_.B", "A.f", "B", "Is.Valid" ] defaults - |> rule - |> Review.Test.expectConfigurationError - { message = "Invalid type names: `_.B`, `A.f`, `B`" - , details = details - } + """module A exposing (..) +a = 1 +""" + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "_.B", "A.f", "B", "Maybe.Maybe" ] defaults) + |> Review.Test.expectGlobalErrors + [ { message = "Could not find type names: `A.f`, `B`, `_.B`" + , details = + [ "I expected to find these custom types in the dependencies, but I could not find them." + , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." + , "If you find that these types have been moved or renamed, please update your configuration." + , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." + ] + } + ] , test "should report global error if ignored types were not found in the project" <| \() -> """module A exposing (..) @@ -105,10 +144,11 @@ a = 1 |> Review.Test.expectGlobalErrors [ { message = "Could not find type names: `A.B`, `B.C`" , details = - [ "I expected to find these custom types in the code or dependencies, but I could not find them." + [ "I expected to find these custom types in the dependencies, but I could not find them." , "Please check whether these types and have not been removed, and if so, remove them from the configuration of this rule." , "If you find that these types have been moved or renamed, please update your configuration." , "Note that I may have provided fixes for things you didn't wish to be fixed, so you might want to undo the changes I have applied." + , "Also note that the configuration for this rule changed in v2.0.19: types that are custom to your project are ignored by default, so this configuration setting can only be used to avoid simplifying case expressions that use custom types defined in dependencies." ] } ] @@ -392,7 +432,7 @@ unnecessaryDetails = sameThingOnBothSidesDetails : String -> List String sameThingOnBothSidesDetails value = - [ "The value on the left and on the right are the same. Therefore we can determine that the expression will always be " ++ value ++ "." + [ "Based on the values and/or the context, we can determine that the value of this operation will always be " ++ value ++ "." ] @@ -401,12 +441,6 @@ comparisonIsAlwaysMessage value = "Comparison is always " ++ value -comparisonIsAlwaysDetails : String -> List String -comparisonIsAlwaysDetails value = - [ "The value on the left and on the right are the same. Therefore we can determine that the expression will always be " ++ value ++ "." - ] - - orTests : Test orTests = describe "||" @@ -418,7 +452,7 @@ a = True || x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = alwaysSameDetails , under = "True || x" } @@ -498,7 +532,7 @@ a = (True) || x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = alwaysSameDetails , under = "(True) || x" } @@ -700,7 +734,7 @@ a = False && x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = alwaysSameDetails , under = "False && x" } @@ -716,7 +750,7 @@ a = x && False |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = alwaysSameDetails , under = "x && False" } @@ -1061,12 +1095,32 @@ a = case value of a = x """ ] - , test "should replace case of with a single case by the body of the case" <| + , test "should not replace case of with a single case by the body of the case" <| \() -> """module A exposing (..) type B = C a = case value of C -> x +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + + -- TODO Create a project with a union with a single constructor + -- , test "should not replace case of with a single case when the constructor is ignored" <| + -- \() -> + -- """module A exposing (..) + --type B = C + --a = case value of + -- C -> x + --""" + -- |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "A.B" ] <| defaults) + -- |> Review.Test.expectNoErrors + , test "should replace case of with multiple cases that have the same body" <| + \() -> + """module A exposing (..) +a = case value of + Just _ -> x + Nothing -> x """ |> Review.Test.run (rule defaults) |> Review.Test.expectErrors @@ -1076,32 +1130,9 @@ a = case value of , under = "case" } |> Review.Test.whenFixed """module A exposing (..) -type B = C a = x """ ] - , test "should not replace case of with a single case when the constructor is ignored" <| - \() -> - """module A exposing (..) -type B = C -a = case value of - C -> x -""" - |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "A.B" ] <| defaults) - |> Review.Test.expectNoErrors - , test "should not replace case of with a single case when the constructor from a different file is ignored" <| - \() -> - [ """module A exposing (..) -import Other exposing (B(..)) -a = case value of - C -> x -""" - , """module Other exposing (..) -type B = C -""" - ] - |> Review.Test.runOnModules (rule <| ignoreCaseOfForTypes [ "Other.B" ] <| defaults) - |> Review.Test.expectNoErrors , test "should not replace case of with a single case when the constructor from a dependency is ignored" <| \() -> """module A exposing (..) @@ -1113,63 +1144,30 @@ a = case value of |> Review.Test.expectNoErrors , test "should not replace case of with multiple cases when all constructors of ignored type are used" <| \() -> - [ """module A exposing (..) -import Other exposing (B(..)) + """module A exposing (..) a = case value of - C -> x - D -> x + Just _ -> x + Nothing -> x """ - , """module Other exposing (..) -type B = C | D -""" - ] - |> Review.Test.runOnModules (rule <| ignoreCaseOfForTypes [ "Other.B" ] <| defaults) + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "Maybe.Maybe" ] <| defaults) |> Review.Test.expectNoErrors , test "should replace case of with multiple cases when not all constructors of ignored type are used" <| \() -> - [ """module A exposing (..) -import Other exposing (B(..)) + """module A exposing (..) a = case value of - C -> x - D -> x + Just _ -> x _ -> x """ - , """module Other exposing (..) -type B = C | D | E -""" - ] - |> Review.Test.runOnModules (rule <| ignoreCaseOfForTypes [ "Other.B" ] <| defaults) - |> Review.Test.expectErrorsForModules - [ ( "A" - , [ Review.Test.error - { message = "Unnecessary case expression" - , details = [ "All the branches of this case expression resolve to the same value. You can remove the case expression and replace it with the body of one of the branches." ] - , under = "case" - } - |> Review.Test.whenFixed """module A exposing (..) -import Other exposing (B(..)) -a = x -""" - ] - ) - ] - , test "should replace case of with a single case with ignored arguments by the body of the case" <| + |> Review.Test.run (rule <| ignoreCaseOfForTypes [ "Maybe.Maybe" ] <| defaults) + |> Review.Test.expectNoErrors + , test "should not replace case of with a single case with ignored arguments by the body of the case" <| \() -> """module A exposing (..) a = case value of A (_) (B C) -> x """ |> Review.Test.run (rule defaults) - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Unnecessary case expression" - , details = [ "All the branches of this case expression resolve to the same value. You can remove the case expression and replace it with the body of one of the branches." ] - , under = "case" - } - |> Review.Test.whenFixed """module A exposing (..) -a = x -""" - ] + |> Review.Test.expectNoErrors , test "should not replace case of where a pattern introduces a variable" <| \() -> """module A exposing (..) @@ -1178,23 +1176,6 @@ a = case value of """ |> Review.Test.run (rule defaults) |> Review.Test.expectNoErrors - , test "should replace case of with multiple cases that have the same body" <| - \() -> - """module A exposing (..) -a = case value of - A (_) (B C) -> x -""" - |> Review.Test.run (rule defaults) - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Unnecessary case expression" - , details = [ "All the branches of this case expression resolve to the same value. You can remove the case expression and replace it with the body of one of the branches." ] - , under = "case" - } - |> Review.Test.whenFixed """module A exposing (..) -a = x -""" - ] , test "should replace boolean case of with the same body by that body" <| \() -> """module A exposing (..) @@ -1704,7 +1685,23 @@ a = -(-n) , under = "-(-" } |> Review.Test.whenFixed """module A exposing (..) -a = (n) +a = n +""" + ] + , test "should simplify -(-(f n)) to (f n)" <| + \() -> + """module A exposing (..) +a = -(-(f n)) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Unnecessary double number negation" + , details = [ "Negating a number twice is the same as the number itself." ] + , under = "-(-" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (f n) """ ] ] @@ -1870,7 +1867,7 @@ a = 1 < 2 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "True" - , details = comparisonIsAlwaysDetails "True" + , details = sameThingOnBothSidesDetails "True" , under = "1 < 2" } |> Review.Test.whenFixed """module A exposing (..) @@ -1886,7 +1883,7 @@ a = 1 < 2 + 3 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "True" - , details = comparisonIsAlwaysDetails "True" + , details = sameThingOnBothSidesDetails "True" , under = "1 < 2 + 3" } |> Review.Test.whenFixed """module A exposing (..) @@ -1902,7 +1899,7 @@ a = 2 < 1 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "False" - , details = comparisonIsAlwaysDetails "False" + , details = sameThingOnBothSidesDetails "False" , under = "2 < 1" } |> Review.Test.whenFixed """module A exposing (..) @@ -1918,7 +1915,7 @@ a = 1 > 2 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "False" - , details = comparisonIsAlwaysDetails "False" + , details = sameThingOnBothSidesDetails "False" , under = "1 > 2" } |> Review.Test.whenFixed """module A exposing (..) @@ -1934,7 +1931,7 @@ a = 1 >= 2 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "False" - , details = comparisonIsAlwaysDetails "False" + , details = sameThingOnBothSidesDetails "False" , under = "1 >= 2" } |> Review.Test.whenFixed """module A exposing (..) @@ -1950,7 +1947,7 @@ a = 1 <= 2 |> Review.Test.expectErrors [ Review.Test.error { message = comparisonIsAlwaysMessage "True" - , details = comparisonIsAlwaysDetails "True" + , details = sameThingOnBothSidesDetails "True" , under = "1 <= 2" } |> Review.Test.whenFixed """module A exposing (..) @@ -2102,7 +2099,7 @@ a = x == x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "x == x" } @@ -2118,7 +2115,7 @@ a = x == (x) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "x == (x)" } @@ -2134,7 +2131,7 @@ a = x /= x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "x /= x" } @@ -2150,12 +2147,188 @@ a = List.map (\\a -> a.value) things == List.map (\\a -> a.value) things |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "List.map (\\a -> a.value) things == List.map (\\a -> a.value) things" } |> Review.Test.whenFixed """module A exposing (..) a = True +""" + ] + , test "should simplify calls with a single arg that use `<|`" <| + \() -> + """module A exposing (..) +a = (f b) == (f <| b) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(f b) == (f <| b)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify calls with multiple args that use `<|`" <| + \() -> + """module A exposing (..) +a = (f b c) == (f b <| c) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(f b c) == (f b <| c)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify calls with a single arg that use `|>`" <| + \() -> + """module A exposing (..) +a = (f b) == (b |> f) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(f b) == (b |> f)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify calls with multiple args that use `|>`" <| + \() -> + """module A exposing (..) +a = (f b c) == (c |> f b) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(f b c) == (c |> f b)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify nested function calls using `|>`" <| + \() -> + """module A exposing (..) +a = (f b c) == (c |> (b |> f)) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(f b c) == (c |> (b |> f))" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify nested function calls using `|>`, even when function is wrapped in let expression" <| + \() -> + """module A exposing (..) +a = (let x = 1 in f b c) == (c |> (let x = 1 in f b)) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(let x = 1 in f b c) == (c |> (let x = 1 in f b))" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify nested function calls using `|>`, even when function is wrapped in if expression" <| + \() -> + """module A exposing (..) +a = (if cond then f b c else g d c) == (c |> (if cond then f b else g d)) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(if cond then f b c else g d c) == (c |> (if cond then f b else g d))" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify nested function calls using `|>`, even when function is wrapped in case expression" <| + \() -> + """module A exposing (..) +a = (case x of + X -> f b c + Y -> g d c + ) + == + ((case x of + X -> f b + Y -> g d + ) <| c) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = """(case x of + X -> f b c + Y -> g d c + ) + == + ((case x of + X -> f b + Y -> g d + ) <| c)""" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify record access comparison" <| + \() -> + """module A exposing (..) +a = (b.c) == (.c b) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(b.c) == (.c b)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True +""" + ] + , test "should simplify record access comparison using pipeline" <| + \() -> + """module A exposing (..) +a = (b.c) == (.c <| b) +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "(b.c) == (.c <| b)" + } + |> Review.Test.whenFixed """module A exposing (..) +a = True """ ] , test "should simplify equality of different literals to False" <| @@ -2166,7 +2339,7 @@ a = "a" == "b" |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "\"a\" == \"b\"" } @@ -2182,7 +2355,7 @@ a = 'a' == 'b' |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "'a' == 'b'" } @@ -2198,7 +2371,7 @@ a = "a" /= "b" |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "\"a\" /= \"b\"" } @@ -2214,7 +2387,7 @@ a = 1 == 2 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1 == 2" } @@ -2230,7 +2403,7 @@ a = 1 == 2.0 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1 == 2.0" } @@ -2246,7 +2419,7 @@ a = 1.0 == 2 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1.0 == 2" } @@ -2262,7 +2435,7 @@ a = 0x10 == 2 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "0x10 == 2" } @@ -2278,7 +2451,7 @@ a = 1 + 3 == 2 + 5 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1 + 3 == 2 + 5" } @@ -2294,7 +2467,7 @@ a = 1 - 3 == 2 - 5 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1 - 3 == 2 - 5" } @@ -2310,7 +2483,7 @@ a = 2 * 3 == 2 * 5 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "2 * 3 == 2 * 5" } @@ -2326,7 +2499,7 @@ a = 1 / 3 == 2 / 5 |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "1 / 3 == 2 / 5" } @@ -2342,7 +2515,7 @@ a = () == x |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "() == x" } @@ -2358,7 +2531,7 @@ a = x == () |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "x == ()" } @@ -2374,7 +2547,7 @@ a = [ 1 ] == [ 1, 1 ] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "[ 1 ] == [ 1, 1 ]" } @@ -2390,7 +2563,7 @@ a = [ 1, 2 ] == [ 1, 1 ] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "[ 1, 2 ] == [ 1, 1 ]" } @@ -2406,7 +2579,7 @@ a = [ 1, 2 - 1 ] == [ 1, 1 ] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "[ 1, 2 - 1 ] == [ 1, 1 ]" } @@ -2422,7 +2595,7 @@ a = (1) == (2) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "(1) == (2)" } @@ -2438,7 +2611,7 @@ a = ( 1, 2 ) == ( 1, 1 ) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "( 1, 2 ) == ( 1, 1 )" } @@ -2454,7 +2627,7 @@ a = { a = 1, b = 2 } == { b = 1, a = 1 } |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "{ a = 1, b = 2 } == { b = 1, a = 1 }" } @@ -2470,7 +2643,7 @@ a = { x | a = 1 } == { x | a = 2 } |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "{ x | a = 1 } == { x | a = 2 }" } @@ -2486,7 +2659,7 @@ a = { x | a = 1 } == { x | a = 1 } |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "{ x | a = 1 } == { x | a = 1 }" } @@ -2509,7 +2682,7 @@ a = { x | a = 1 } == { y | a = 2 } |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always False" + { message = "Comparison is always False" , details = sameThingOnBothSidesDetails "False" , under = "{ x | a = 1 } == { y | a = 2 }" } @@ -2550,7 +2723,7 @@ b = 1 |> Review.Test.expectErrorsForModules [ ( "A" , [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "B.b == b" } @@ -2570,7 +2743,7 @@ a = List.map fn 1 == map fn (2 - 1) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "List.map fn 1 == map fn (2 - 1)" } @@ -2595,7 +2768,7 @@ a = (if 1 then 2 else 3) == (if 2 - 1 then 3 - 1 else 4 - 1) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "(if 1 then 2 else 3) == (if 2 - 1 then 3 - 1 else 4 - 1)" } @@ -2618,7 +2791,7 @@ a = -1 == -(2 - 1) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "-1 == -(2 - 1)" } @@ -2634,12 +2807,28 @@ a = ({ a = 1 }).a == ({ a = 2 - 1 }).a |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "({ a = 1 }).a == ({ a = 2 - 1 }).a" } |> Review.Test.whenFixed """module A exposing (..) a = True +""" + , Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field of a record or record update can be simplified to just that field's value" ] + , under = "({ a = 2 - 1 }).a" + } + |> Review.Test.whenFixed """module A exposing (..) +a = ({ a = 1 }).a == (2 - 1) +""" + , Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field of a record or record update can be simplified to just that field's value" ] + , under = "({ a = 1 }).a" + } + |> Review.Test.whenFixed """module A exposing (..) +a = 1 == ({ a = 2 - 1 }).a """ ] , test "should simplify operator expressions" <| @@ -2650,7 +2839,7 @@ a = (1 |> fn) == (2 - 1 |> fn) |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Condition is always True" + { message = "Comparison is always True" , details = sameThingOnBothSidesDetails "True" , under = "(1 |> fn) == (2 - 1 |> fn)" } @@ -2664,7 +2853,16 @@ a = True a = ({ a = 1 }).a == ({ a = 1 }).b """ |> Review.Test.run (rule defaults) - |> Review.Test.expectNoErrors + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field of a record or record update can be simplified to just that field's value" ] + , under = "({ a = 1 }).a" + } + |> Review.Test.whenFixed """module A exposing (..) +a = 1 == ({ a = 1 }).b +""" + ] ] @@ -2782,6 +2980,1011 @@ a = 1 +-- DUPLICATED IF CONDITIONS + + +duplicatedIfTests : Test +duplicatedIfTests = + describe "Duplicated if conditions" + [ test "should not remove nested conditions if they're not duplicate" <| + \() -> + """module A exposing (..) +a = + if x then + if y then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should remove duplicate nested conditions (x inside the then)" <| + \() -> + """module A exposing (..) +a = + if x then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + 1 + else + 3 +""" + ] + , test "should remove duplicate nested conditions (x inside the then, with parens)" <| + \() -> + """module A exposing (..) +a = + if (x) then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if (x) then + 1 + else + 3 +""" + ] + , test "should remove duplicate nested conditions (not x inside the top condition)" <| + \() -> + """module A exposing (..) +a = + if not x then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if not x then + 2 + else + 3 +""" + ] + , test "should remove duplicate nested conditions (not <| x inside the top condition)" <| + \() -> + """module A exposing (..) +a = + if not <| x then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if not <| x then + 2 + else + 3 +""" + ] + , test "should remove opposite nested conditions (not x inside the then)" <| + \() -> + """module A exposing (..) +a = + if x then + if not x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Expression is equal to False" + , details = [ "You can replace the call to `not` by the boolean value directly." ] + , under = "not x" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + if False then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove duplicate nested conditions (x inside the else)" <| + \() -> + """module A exposing (..) +a = + if x then + 1 + else + if x then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 6, column = 5 }, end = { row = 6, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + 1 + else + 3 +""" + ] + , test "should remove opposite nested conditions (not x inside the else)" <| + \() -> + """module A exposing (..) +a = + if x then + 1 + else + if not x then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Expression is equal to True" + , details = [ "You can replace the call to `not` by the boolean value directly." ] + , under = "not x" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + 1 + else + if True then + 2 + else + 3 +""" + ] + , test "should remove duplicate nested conditions (x part of a nested condition)" <| + \() -> + """module A exposing (..) +a = + if x then + if x && y then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Part of the expression is unnecessary" + , details = [ "A part of this condition is unnecessary. You can remove it and it would not impact the behavior of the program." ] + , under = "x && y" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + if y then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove duplicate deeply nested conditions" <| + \() -> + """module A exposing (..) +a = + if x then + if y then + if x then + 1 + else + 2 + else + 3 + else + 4 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 7 }, end = { row = 5, column = 9 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x then + if y then + 1 + else + 3 + else + 4 +""" + ] + , test "should remove duplicate nested conditions (x part of the top && condition)" <| + \() -> + """module A exposing (..) +a = + if x && y then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x && y then + 1 + else + 3 +""" + ] + , test "should remove opposite nested conditions (x part of the top || condition)" <| + \() -> + """module A exposing (..) +a = + if x || y then + 1 + else + if x then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 6, column = 5 }, end = { row = 6, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x || y then + 1 + else + 3 +""" + ] + , test "should not remove condition when we don't have enough information (x part of the top && condition, other condition in the else)" <| + \() -> + """module A exposing (..) +a = + if x && y then + 1 + else + if x then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should not remove condition when we don't have enough information (x part of the top || condition, other condition in the then)" <| + \() -> + """module A exposing (..) +a = + if x || y then + if x then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should remove branches where the condition always matches" <| + \() -> + """module A exposing (..) +a = + if x == 1 then + if x == 1 then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "x == 1" + } + |> Review.Test.atExactly { start = { row = 4, column = 8 }, end = { row = 4, column = 14 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x == 1 then + if True then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches (strings == with different values)" <| + \() -> + """module A exposing (..) +a = + if x == "a" then + if x == "b" then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always False" + , details = sameThingOnBothSidesDetails "False" + , under = "x == \"b\"" + } + |> Review.Test.atExactly { start = { row = 4, column = 8 }, end = { row = 4, column = 16 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x == "a" then + if False then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches (not function or value)" <| + \() -> + """module A exposing (..) +a = + if item.name == "Aged Brie" then + if item.name == "Sulfuras, Hand of Ragnaros" then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always False" + , details = sameThingOnBothSidesDetails "False" + , under = "item.name == \"Sulfuras, Hand of Ragnaros\"" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if item.name == "Aged Brie" then + if False then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches" <| + \() -> + """module A exposing (..) +a = + if x == 1 then + if x == 2 then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always False" + , details = sameThingOnBothSidesDetails "False" + , under = "x == 2" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x == 1 then + if False then + 1 + else + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches (strings)" <| + \() -> + """module A exposing (..) +a = + if x /= "a" then + if x == "a" then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x /= "a" then + 2 + else + 3 +""" + ] + , test "should not spread inferred things from one branch to another" <| + \() -> + """module A exposing (..) +a = + if x == 1 then + 1 + else if x == 2 then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should remove branches where the condition always matches (/=)" <| + \() -> + """module A exposing (..) +a = + if x /= 1 then + if x /= 1 then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x /= 1 then + 1 + else + 3 +""" + ] + , test "should remove branches where the condition always matches (/= in else)" <| + \() -> + """module A exposing (..) +a = + if x /= 1 then + 1 + else if x /= 1 then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always False" + , details = sameThingOnBothSidesDetails "False" + , under = "x /= 1" + } + |> Review.Test.atExactly { start = { row = 5, column = 11 }, end = { row = 5, column = 17 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x /= 1 then + 1 + else if False then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition always matches (== in else)" <| + \() -> + """module A exposing (..) +a = + if x /= 1 then + 1 + else if x == 1 then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "x == 1" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x /= 1 then + 1 + else if True then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition always matches (/= == in else)" <| + \() -> + """module A exposing (..) +a = + if x /= 1 then + if x == 1 then + 1 + else + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x /= 1 then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches (literal on the left using ==, second if)" <| + \() -> + """module A exposing (..) +a = + if x == 1 then + 1 + else if 1 == x then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 10 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if x == 1 then + 1 + else 3 +""" + ] + , test "should remove branches where the condition never matches (literal on the left using ==, first if)" <| + \() -> + """module A exposing (..) +a = + if 1 == x then + 1 + else if x == 1 then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 10 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if 1 == x then + 1 + else 3 +""" + ] + , test "should remove branches where the condition always matches (literal on the left using /=)" <| + \() -> + """module A exposing (..) +a = + if 1 /= x then + 1 + else if x == 1 then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Comparison is always True" + , details = sameThingOnBothSidesDetails "True" + , under = "x == 1" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if 1 /= x then + 1 + else if True then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition never matches (&&)" <| + \() -> + """module A exposing (..) +a = + if a && b then + 1 + else if a && b then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 10 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a && b then + 1 + else 3 +""" + ] + , test "should remove branches where the condition may not match (a || b --> a)" <| + \() -> + """module A exposing (..) +a = + if a || b then + 1 + else if a then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 10 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a || b then + 1 + else 3 +""" + ] + , test "should remove branches where the condition may not match (a || b --> a && b)" <| + \() -> + """module A exposing (..) +a = + if a || b then + 1 + else if a && b then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + -- TODO Order of the errors seem to matter here. Should be fixed in `elm-review` + [ Review.Test.error + { message = "Comparison is always False" + , details = alwaysSameDetails + , under = "a && b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a || b then + 1 + else if a then + 2 + else + 3 +""" + , Review.Test.error + { message = "Comparison is always False" + , details = alwaysSameDetails + , under = "a && b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a || b then + 1 + else if b then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition may not match (a && b --> a --> b)" <| + \() -> + """module A exposing (..) +a = + if a && b then + 1 + else if a then + if b then + 2 + else + 3 + else + 4 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to False" + , details = [ "The expression can be replaced by what is inside the 'else' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 6, column = 5 }, end = { row = 6, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a && b then + 1 + else if a then + 3 + else + 4 +""" + ] + , test "should remove branches where the condition may not match (a || b --> a --> b)" <| + \() -> + """module A exposing (..) +a = + if a || b then + if not a then + if b then + 1 + else + 2 + else + 3 + else + 4 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 5, column = 7 }, end = { row = 5, column = 9 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a || b then + if not a then + 1 + else + 3 + else + 4 +""" + ] + , test "should remove branches where the condition may not match (a || b --> not a)" <| + \() -> + """module A exposing (..) +a = + if a || b then + 1 + else if not a then + 2 + else + 3 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Expression is equal to True" + , details = [ "You can replace the call to `not` by the boolean value directly." ] + , under = "not a" + } + |> Review.Test.whenFixed """module A exposing (..) +a = + if a || b then + 1 + else if True then + 2 + else + 3 +""" + ] + , test "should remove branches where the condition may not match (not (a || b) --> not a --> not b)" <| + \() -> + -- TODO Probably best to normalize inside Evaluate.getBoolean? + """module A exposing (..) +a = + if not (a || b) then + 1 + else if not a then + if b then + 2 + else + 3 + else + 4 +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "The condition will always evaluate to True" + , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + , under = "if" + } + |> Review.Test.atExactly { start = { row = 6, column = 5 }, end = { row = 6, column = 7 } } + |> Review.Test.whenFixed """module A exposing (..) +a = + if not (a || b) then + 1 + else if not a then + 2 + else + 4 +""" + ] + + -- , test "should not lose information as more conditions add up" <| + -- \() -> + -- """module A exposing (..) + --a = + -- if a == 1 then + -- if a == f b then + -- if a == 1 then + -- 1 + -- else + -- 2 + -- else + -- 3 + -- else + -- 4 + --""" + -- |> Review.Test.run (rule defaults) + -- |> Review.Test.expectErrors + -- [ Review.Test.error + -- { message = "The condition will always evaluate to True" + -- , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + -- , under = "if" + -- } + -- |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 7 } } + -- |> Review.Test.whenFixed """module A exposing (..) + --a = + -- if a == 1 then + -- if a == 1 then + -- 1 + -- else + -- 2 + -- else + -- 4 + --""" + -- , Review.Test.error + -- { message = "The condition will always evaluate to True" + -- , details = [ "The expression can be replaced by what is inside the 'then' branch." ] + -- , under = "if" + -- } + -- |> Review.Test.atExactly { start = { row = 5, column = 7 }, end = { row = 5, column = 9 } } + -- |> Review.Test.whenFixed """module A exposing (..) + --a = + -- if a == 1 then + -- if a /= 2 then + -- 1 + -- else + -- 3 + -- else + -- 4 + --""" + -- ] + -- TODO + -- Unhappy && and || cases: + -- if a && b then ... else + -- if a || b then ... else + ] + + + -- RECORD UPDATE @@ -3285,7 +4488,23 @@ a = [ b ] ++ c , under = "[ b ] ++ c" } |> Review.Test.whenFixed """module A exposing (..) -a = ( b ) :: c +a = b :: c +""" + ] + , test "should replace [ f n ] ++ c by (f n) :: c" <| + \() -> + """module A exposing (..) +a = [ f n ] ++ c +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Should use (::) instead of (++)" + , details = [ "Concatenating a list with a single value is the same as using (::) on the list with the value." ] + , under = "[ f n ] ++ c" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (f n) :: c """ ] , test "should not replace [b] ++ c when on the right of a ++ operator" <| @@ -3673,7 +4892,7 @@ a = String.words "" |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using String.words on an empty string will result in a empty list" + { message = "Using String.words on an empty string will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "String.words" } @@ -3703,7 +4922,7 @@ a = String.lines "" |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using String.lines on an empty string will result in a empty list" + { message = "Using String.lines on an empty string will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "String.lines" } @@ -3784,6 +5003,8 @@ listSimplificationTests = , listReverseTests , listTakeTests , listDropTests + , listIntersperseTests + , listMemberTests ] @@ -3853,7 +5074,7 @@ a = List.concat [] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.concat on an empty list will result in a empty list" + { message = "Using List.concat on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.concat" } @@ -4065,7 +5286,7 @@ a = List.concatMap f [] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.concatMap on an empty list will result in a empty list" + { message = "Using List.concatMap on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.concatMap" } @@ -4108,7 +5329,7 @@ a = (always []) , test "should replace List.concatMap (\\_ -> [a]) x by List.map (\\_ -> a) x" <| \() -> """module A exposing (..) -a = List.concatMap (\\_ -> [a]) x +a = List.concatMap (\\_ -> ([a])) x """ |> Review.Test.run (rule defaults) |> Review.Test.expectErrors @@ -4118,7 +5339,7 @@ a = List.concatMap (\\_ -> [a]) x , under = "List.concatMap" } |> Review.Test.whenFixed """module A exposing (..) -a = List.map (\\_ -> (a)) x +a = List.map (\\_ -> a) x """ ] , test "should replace List.concatMap (\\_ -> List.singleton a) x by List.map (\\_ -> a) x" <| @@ -4150,7 +5371,7 @@ a = List.concatMap (\\_ -> if cond then [a] else [b]) x , under = "List.concatMap" } |> Review.Test.whenFixed """module A exposing (..) -a = List.map (\\_ -> if cond then (a) else (b)) x +a = List.map (\\_ -> if cond then a else b) x """ ] , test "should replace List.concatMap (\\_ -> case y of A -> [a] ; B -> [b]) x by List.map (\\_ -> case y of A -> a ; B -> b) x" <| @@ -4174,8 +5395,8 @@ a = List.concatMap a = List.map (\\_ -> case y of - A -> (a) - B -> (b) + A -> a + B -> b ) x """ ] @@ -4192,7 +5413,7 @@ a = List.concatMap f [ a ] , under = "List.concatMap" } |> Review.Test.whenFixed """module A exposing (..) -a = f (a) +a = f a """ ] , test "should replace List.concatMap f <| [ b c ] by f <| (b c)" <| @@ -4209,6 +5430,22 @@ a = List.concatMap f <| [ b c ] } |> Review.Test.whenFixed """module A exposing (..) a = f <| (b c) +""" + ] + , test "should replace List.concatMap f <| [ c ] by c |> f" <| + \() -> + """module A exposing (..) +a = [ c ] |> List.concatMap f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Using List.concatMap on an element with a single item is the same as calling the function directly on that lone element." + , details = [ "You can replace this call by a call to the function directly." ] + , under = "List.concatMap" + } + |> Review.Test.whenFixed """module A exposing (..) +a = c |> f """ ] , test "should replace List.concatMap f <| [ b c ] by (b c) |> f" <| @@ -4690,7 +5927,7 @@ a = List.filterMap f [] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.filterMap on an empty list will result in a empty list" + { message = "Using List.filterMap on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.filterMap" } @@ -4706,7 +5943,7 @@ a = List.filterMap f <| [] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.filterMap on an empty list will result in a empty list" + { message = "Using List.filterMap on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.filterMap" } @@ -4722,7 +5959,7 @@ a = [] |> List.filterMap f |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.filterMap on an empty list will result in a empty list" + { message = "Using List.filterMap on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.filterMap" } @@ -5093,7 +6330,7 @@ a = List.indexedMap f [] |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error - { message = "Using List.indexedMap on an empty list will result in a empty list" + { message = "Using List.indexedMap on an empty list will result in an empty list" , details = [ "You can replace this call by an empty list." ] , under = "List.indexedMap" } @@ -5938,6 +7175,65 @@ a = (Tuple.pair []) ] +listIntersperseTests : Test +listIntersperseTests = + describe "List.intersperse" + [ test "should not report List.intersperse that contains a variable or expression" <| + \() -> + """module A exposing (..) +a = List.intersperse 2 x +b = List.intersperse y [ 1, 2, 3 ] +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should replace List.intersperse n [] by []" <| + \() -> + """module A exposing (..) +a = List.intersperse x [] +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Using List.intersperse on an empty list will result in an empty list" + , details = [ "You can replace this call by an empty list." ] + , under = "List.intersperse" + } + |> Review.Test.whenFixed """module A exposing (..) +a = [] +""" + ] + ] + + +listMemberTests : Test +listMemberTests = + describe "List.member" + [ test "should not report List.member used with okay arguments" <| + \() -> + """module A exposing (..) +a = List.member x y +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should replace List.member x List.empty by False" <| + \() -> + """module A exposing (..) +a = List.member x [] +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Using List.member on [] will result in False" + , details = [ "You can replace this call by False." ] + , under = "List.member" + } + |> Review.Test.whenFixed """module A exposing (..) +a = False +""" + ] + ] + + -- Maybe @@ -8126,7 +9422,7 @@ setMemberTests = [ test "should not report Set.member used with okay arguments" <| \() -> """module A exposing (..) -a = Set.member x x +a = Set.member x y """ |> Review.Test.run (rule defaults) |> Review.Test.expectNoErrors @@ -8388,6 +9684,7 @@ dictSimplificationTests = , dictFromListTests , dictToListTests , dictSizeTests + , dictMemberTests ] @@ -8634,6 +9931,35 @@ a = 1 ] +dictMemberTests : Test +dictMemberTests = + describe "Dict.member" + [ test "should not report Dict.member used with okay arguments" <| + \() -> + """module A exposing (..) +a = Dict.member x y +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should replace Dict.member x Dict.empty by False" <| + \() -> + """module A exposing (..) +a = Dict.member x Dict.empty +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Using Dict.member on Dict.empty will result in False" + , details = [ "You can replace this call by False." ] + , under = "Dict.member" + } + |> Review.Test.whenFixed """module A exposing (..) +a = False +""" + ] + ] + + -- Cmd @@ -8695,7 +10021,7 @@ a = Cmd.batch [ Cmd.none ] , under = "Cmd.batch" } |> Review.Test.whenFixed """module A exposing (..) -a = (Cmd.none) +a = Cmd.none """ ] , test "should replace Cmd.batch [ b ] by b" <| @@ -8711,7 +10037,7 @@ a = Cmd.batch [ b ] , under = "Cmd.batch" } |> Review.Test.whenFixed """module A exposing (..) -a = (b) +a = b """ ] , test "should replace Cmd.batch [ b, Cmd.none ] by Cmd.batch []" <| @@ -8842,7 +10168,7 @@ a = Sub.batch [ Sub.none ] , under = "Sub.batch" } |> Review.Test.whenFixed """module A exposing (..) -a = (Sub.none) +a = Sub.none """ ] , test "should replace Sub.batch [ b ] by b" <| @@ -8858,7 +10184,23 @@ a = Sub.batch [ b ] , under = "Sub.batch" } |> Review.Test.whenFixed """module A exposing (..) -a = (b) +a = b +""" + ] + , test "should replace Sub.batch [ f n ] by (f n)" <| + \() -> + """module A exposing (..) +a = Sub.batch [ f n ] +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Unnecessary Sub.batch" + , details = [ "Sub.batch with a single element is equal to that element." ] + , under = "Sub.batch" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (f n) """ ] , test "should replace Sub.batch [ b, Sub.none ] by Sub.batch []" <| @@ -8947,7 +10289,7 @@ d = Parser.Advanced.oneOf [ y, z ] """ |> Review.Test.run (rule defaults) |> Review.Test.expectNoErrors - , test "should replace Parser.oneOf [ x ] by ( x )" <| + , test "should replace Parser.oneOf [ x ] by x" <| \() -> """module A exposing (..) import Parser @@ -8962,10 +10304,10 @@ a = Parser.oneOf [ x ] } |> Review.Test.whenFixed """module A exposing (..) import Parser -a = (x) +a = x """ ] - , test "should replace Parser.Advanced.oneOf [ x ] by ( x )" <| + , test "should replace Parser.Advanced.oneOf [ x ] by x" <| \() -> """module A exposing (..) import Parser.Advanced @@ -8980,7 +10322,7 @@ a = Parser.Advanced.oneOf [ x ] } |> Review.Test.whenFixed """module A exposing (..) import Parser.Advanced -a = (x) +a = x """ ] ] @@ -9002,7 +10344,7 @@ b = Json.Decode.oneOf [ y, z ] """ |> Review.Test.run (rule defaults) |> Review.Test.expectNoErrors - , test "should replace Json.Decode.oneOf [ x ] by ( x )" <| + , test "should replace Json.Decode.oneOf [ x ] by x" <| \() -> """module A exposing (..) import Json.Decode @@ -9017,7 +10359,190 @@ a = Json.Decode.oneOf [ x ] } |> Review.Test.whenFixed """module A exposing (..) import Json.Decode -a = (x) +a = x +""" + ] + ] + + + +-- Record access + + +recordAccessTests : Test +recordAccessTests = + let + details : List String + details = + [ "Accessing the field of a record or record update can be simplified to just that field's value" + ] + in + describe "Simplify.RecordAccess" + [ test "should simplify record accesses for explicit records" <| + \() -> + """module A exposing (..) +a = { b = 3 }.b +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = details + , under = "{ b = 3 }.b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = 3 +""" + ] + , test "should simplify record accesses for explicit records and add parens when necessary" <| + \() -> + """module A exposing (..) +a = { b = f n }.b +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = details + , under = "{ b = f n }.b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (f n) +""" + ] + , test "should simplify record accesses for explicit records in parentheses" <| + \() -> + """module A exposing (..) +a = (({ b = 3 })).b +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = details + , under = "(({ b = 3 })).b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = 3 +""" + ] + , test "shouldn't simplify record accesses for explicit records if it can't find the field" <| + \() -> + """module A exposing (..) +a = { b = 3 }.c +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should simplify record accesses for record updates" <| + \() -> + """module A exposing (..) +a = foo { d | b = f x y }.b +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = details + , under = "{ d | b = f x y }.b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = foo (f x y) +""" + ] + , test "should simplify record accesses for record updates in parentheses" <| + \() -> + """module A exposing (..) +a = foo (({ d | b = f x y })).b +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = details + , under = "(({ d | b = f x y })).b" + } + |> Review.Test.whenFixed """module A exposing (..) +a = foo (f x y) +""" + ] + , test "should simplify record accesses for record updates if it can't find the field" <| + \() -> + """module A exposing (..) +a = { d | b = 3 }.c +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field of an unrelated record update can be simplified to just the original field's value" ] + , under = "{ d | b = 3 }.c" + } + |> Review.Test.whenFixed """module A exposing (..) +a = d.c +""" + ] + , test "should simplify record accesses for let/in expressions" <| + \() -> + """module A exposing (..) +a = (let b = c in f x).e +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] + , under = "(let b = c in f x).e" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (let b = c in (f x).e) +""" + ] + , test "should simplify record accesses for let/in expressions in parentheses" <| + \() -> + """module A exposing (..) +a = (((let b = c in f x))).e +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] + , under = "(((let b = c in f x))).e" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (((let b = c in (f x).e))) +""" + ] + , test "should simplify nested record accesses for let/in expressions (inner)" <| + \() -> + """module A exposing (..) +a = (let b = c in f x).e.f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] + , under = "(let b = c in f x).e" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (let b = c in (f x).e).f +""" + ] + , test "should simplify nested record accesses for let/in expressions (outer)" <| + \() -> + """module A exposing (..) +a = (let b = c in (f x).e).f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] + , under = "(let b = c in (f x).e).f" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (let b = c in (f x).e.f) """ ] ]