From b4f11c5ddfe501e087a979dea10fad3d0631795f Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Tue, 23 Aug 2022 17:09:29 +0200 Subject: [PATCH] Fix ModuleNameLookupTable resolving to indirect dependencies (#135) --- src/Review/Rule.elm | 6 +- tests/ModuleNameLookupTableTest.elm | 136 ++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 3 deletions(-) diff --git a/src/Review/Rule.elm b/src/Review/Rule.elm index f3204a31..b2263949 100644 --- a/src/Review/Rule.elm +++ b/src/Review/Rule.elm @@ -5712,7 +5712,7 @@ scope_pairWithNoErrors fn visited context = -- DEPENDENCIES -scope_dependenciesVisitor : Dict String Dependency -> { context | dependenciesModules : Dict String Elm.Docs.Module } -> { context | dependenciesModules : Dict String Elm.Docs.Module } +scope_dependenciesVisitor : Dict String Dependency -> ScopeProjectContext -> ScopeProjectContext scope_dependenciesVisitor dependencies innerContext = let dependenciesModules : Dict String Elm.Docs.Module @@ -6143,12 +6143,12 @@ registerImportExposed import_ innerContext = module_ : Elm.Docs.Module module_ = - (case Dict.get (joinModuleName moduleName) innerContext.dependenciesModules of + (case Dict.get moduleName innerContext.modules of Just m -> Just m Nothing -> - Dict.get moduleName innerContext.modules + Dict.get (joinModuleName moduleName) innerContext.dependenciesModules ) |> Maybe.withDefault { name = joinModuleName moduleName diff --git a/tests/ModuleNameLookupTableTest.elm b/tests/ModuleNameLookupTableTest.elm index f5acde11..147d9f9b 100644 --- a/tests/ModuleNameLookupTableTest.elm +++ b/tests/ModuleNameLookupTableTest.elm @@ -1,5 +1,9 @@ module ModuleNameLookupTableTest exposing (all) +import Elm.Docs +import Elm.License +import Elm.Package +import Elm.Project import Elm.Syntax.Declaration as Declaration exposing (Declaration) import Elm.Syntax.Expression as Expression exposing (Expression) import Elm.Syntax.ModuleName exposing (ModuleName) @@ -7,12 +11,15 @@ import Elm.Syntax.Node as Node exposing (Node(..)) import Elm.Syntax.Pattern as Pattern import Elm.Syntax.Range exposing (Range) import Elm.Syntax.TypeAnnotation as TypeAnnotation exposing (TypeAnnotation) +import Elm.Version import Fixtures.Dependencies as Dependencies import Review.ModuleNameLookupTable as ModuleNameLookupTable exposing (ModuleNameLookupTable) import Review.Project as Project exposing (Project) +import Review.Project.Dependency as Dependency exposing (Dependency) import Review.Rule as Rule exposing (Rule) import Review.Test import Review.Test.Dependencies +import Review.Test.Dependencies.Unsafe as Unsafe import Test exposing (Test, describe, test) @@ -21,6 +28,7 @@ all = describe "ModuleNameLookupTable" [ moduleNameAtTest , fullModuleNameAtest + , dependenciesTest ] @@ -446,6 +454,44 @@ type alias BAlias = {} ] +dependenciesTest : Test +dependenciesTest = + describe "Dependencies" + [ test "should not confuse a function from a local module with a module from an indirect dependency" <| + \() -> + let + lookupFunction : ModuleNameLookupTable -> Range -> Maybe ModuleName + lookupFunction = + ModuleNameLookupTable.moduleNameAt + + rule : Rule + rule = + createRule + (Rule.withExpressionEnterVisitor (expressionVisitor lookupFunction)) + in + [ """module A exposing (..) +import Element exposing (..) + +a = value +""", """module Element exposing (value) +value = 1 +""" ] + |> Review.Test.runOnModulesWithProjectData project rule + |> Review.Test.expectErrorsForModules + [ ( "A" + , [ Review.Test.error + { message = """ +.value -> Element.value +""" + , details = [ "details" ] + , under = "module" + } + ] + ) + ] + ] + + type alias ModuleContext = { lookupTable : ModuleNameLookupTable , texts : List String @@ -455,10 +501,100 @@ type alias ModuleContext = project : Project project = Project.new + |> Project.addElmJson applicationElmJson |> Project.addDependency Dependencies.elmCore |> Project.addDependency Dependencies.elmHtml |> Project.addDependency Review.Test.Dependencies.elmParser |> Project.addDependency Review.Test.Dependencies.elmUrl + |> Project.addDependency xyz2dependency + + +applicationElmJson : { path : String, raw : String, project : Elm.Project.Project } +applicationElmJson = + { path = "elm.json" + , raw = """{ + "type": "application", + "source-directories": [ + "src" + ], + "elm-version": "0.19.1", + "dependencies": { + "direct": { + "elm/core": "1.0.0", + "elm/html": "1.0.0", + "elm/parser": "1.0.0", + "elm/url": "1.0.0", + "abc/xyz": "1.0.0" + }, + "indirect": { + "abc/xyz2": "1.0.0" + } + }, + "test-dependencies": { + "direct": {}, + "indirect": {} + } +}""" + , project = + Elm.Project.Application + { elm = Elm.Version.one + , dirs = [] + , depsDirect = + [ ( unsafePackageName "elm/core", Elm.Version.one ) + , ( unsafePackageName "elm/html", Elm.Version.one ) + , ( unsafePackageName "elm/parser", Elm.Version.one ) + , ( unsafePackageName "elm/url", Elm.Version.one ) + , ( unsafePackageName "abc/xyz", Elm.Version.one ) + ] + , depsIndirect = [ ( unsafePackageName "abc/xyz2", Elm.Version.one ) ] + , testDepsDirect = [] + , testDepsIndirect = [] + } + } + + +xyz2dependency : Dependency +xyz2dependency = + Dependency.create "abc/xyz2" + xyz2elmJson + xyz2dependencyModules + + +xyz2elmJson : Elm.Project.Project +xyz2elmJson = + Elm.Project.Package + { elm = Unsafe.constraint "0.19.0 <= v < 0.20.0" + , exposed = Elm.Project.ExposedList [ Unsafe.moduleName "Element" ] + , license = Elm.License.fromString "BSD-3-Clause" |> Maybe.withDefault Elm.License.bsd3 + , name = Unsafe.packageName "elm/xyz" + , summary = "Fake stuff" + , deps = [ ( Unsafe.packageName "elm/core", Unsafe.constraint "1.0.0 <= v < 2.0.0" ) ] + , testDeps = [] + , version = Elm.Version.fromString "1.0.0" |> Maybe.withDefault Elm.Version.one + } + + +xyz2dependencyModules : List Elm.Docs.Module +xyz2dependencyModules = + [ { name = "Element" + , comment = "" + , unions = [] + , aliases = [] + , values = [] + , binops = [] + } + ] + + +unsafePackageName : String -> Elm.Package.Name +unsafePackageName packageName = + case Elm.Package.fromString packageName of + Just name -> + name + + Nothing -> + -- unsafe, but if the generation went well, it should all be good. + unsafePackageName packageName createRule : (Rule.ModuleRuleSchema {} ModuleContext -> Rule.ModuleRuleSchema { hasAtLeastOneVisitor : () } ModuleContext) -> Rule