Print a module that exports the name rather than the module that defi…

…nes the name (#25)

* add test module

* print the exporter, not the source

* update docstring

* bump version

Project.toml

@@ -1,7 +1,7 @@
 name = "ExplicitImports"
 uuid = "7d51a73a-1435-4ff3-83d9-f097790105c7"
 authors = ["Eric P. Hanson"]
-version = "1.2.0"
+version = "1.3.0"
 
 [deps]
 AbstractTrees = "1520ce14-60c1-5f80-bbc7-55ef81b5835c"

src/ExplicitImports.jl

@@ -57,7 +57,7 @@ end
 """
     explicit_imports(mod::Module, file=pathof(mod); skip=(mod, Base, Core), warn_stale=true, strict=true)
 
-Returns a nested structure providing information about explicit import statements one could make for each submodule of `mod`. This information is structured as a collection of pairs, where the keys are the submodules of `mod` (including `mod` itself), and the values are `NamedTuple`s, with at least the keys `name`, `source`, and `location`, showing which names are being used implicitly, which modules they came from, and the location of those usages. Additional keys may be added to the `NamedTuple`'s in the future in non-breaking releases of ExplicitImports.jl.
+Returns a nested structure providing information about explicit import statements one could make for each submodule of `mod`. This information is structured as a collection of pairs, where the keys are the submodules of `mod` (including `mod` itself), and the values are `NamedTuple`s, with at least the keys `name`, `source`, `exporters`, and `location`, showing which names are being used implicitly, which modules they were defined in, which modules they were exported from, and the location of those usages. Additional keys may be added to the `NamedTuple`'s in the future in non-breaking releases of ExplicitImports.jl.
 
 ## Arguments
 
@@ -179,9 +179,17 @@ function print_explicit_imports(io::IO, mod::Module, file=pathof(mod);
     end
 end
 
-function using_statement((; name, source))
+# TODO; there may be a better way to make this choice
+function choose_exporter(name, exporters)
+    by = mod -> reverse(module_path(mod))
+    sorted = sort(exporters; by, lt=is_prefix)
+    return first(sorted)
+end
+
+function using_statement((; name, exporters))
     # skip `Main.X`, just do `.X`
-    v = replace(string(source), "Main" => "")
+    e = choose_exporter(name, exporters)
+    v = replace(string(e), "Main" => "")
     return "using $v: $name"
 end
 
@@ -219,13 +227,14 @@ function explicit_imports_nonrecursive(mod::Module, file=pathof(mod);
     needed_names = Set(nt.name for nt in needs_explicit_import)
     filter!(all_implicit_imports) do (k, v)
         k in needed_names || return false
-        should_skip(v; skip) && return false
+        should_skip(v.source; skip) && return false
+        any(mod -> should_skip(mod; skip), v.exporters) && return false
         return true
     end
 
     location_lookup = Dict(nt.name => nt.location for nt in needs_explicit_import)
 
-    to_make_explicit = [(; name=k, source=v, location=location_lookup[k])
+    to_make_explicit = [(; name=k, v..., location=location_lookup[k])
                         for (k, v) in all_implicit_imports]
 
     function lt((k1, v1), (k2, v2))

src/checks.jl

@@ -1,6 +1,7 @@
 struct ImplicitImportsException <: Exception
     mod::Module
-    names::Vector{@NamedTuple{name::Symbol,source::Module,location::String}}
+    names::Vector{@NamedTuple{name::Symbol,source::Module,exporters::Vector{Module},
+                              location::String}}
 end
 
 function Base.showerror(io::IO, e::ImplicitImportsException)

src/find_implicit_imports.jl

@@ -2,21 +2,24 @@
 modules_from_using(m::Module) = ccall(:jl_module_usings, Any, (Any,), m)
 
 function get_implicit_names(mod; skip=(mod, Base, Core))
-    implicit_names = Symbol[]
+    implicit_names = Dict{Symbol,Vector{Module}}()
     for mod in modules_from_using(mod)
         should_skip(mod; skip) && continue
-        append!(implicit_names, names(mod))
+        for name in names(mod)
+            v = get!(Vector{Module}, implicit_names, name)
+            push!(v, mod)
+        end
     end
-    return unique!(implicit_names)
+    return implicit_names
 end
 
 """
     find_implicit_imports(mod::Module; skip=(mod, Base, Core))
 
-Given a module `mod`, returns a `Dict{Symbol, Module}` showing
+Given a module `mod`, returns a `Dict{Symbol, @NamedTuple{source::Module,exporters::Vector{Module}}}` showing
 names exist in `mod`'s namespace which are available due to implicit
 exports by other modules. The dict's keys are those names, and the values
-are the module that the name comes from.
+are the source module that the name comes from, along with the modules which export the same binding that are available in `mod` due to implicit imports.
 
 In the case of ambiguities (two modules exporting the same name), the name
 is unavailable in the module, and hence the name will not be present in the dict.
@@ -28,9 +31,9 @@ function find_implicit_imports(mod::Module; skip=(mod, Base, Core))
 
     # Build a dictionary to lookup modules from names
     # we use `which` to figure out what the name resolves to in `mod`
-    mod_lookup = Dict{Symbol,Module}()
-    for name in implicit_names
-        resolved_module = try
+    mod_lookup = Dict{Symbol,@NamedTuple{source::Module,exporters::Vector{Module}}}()
+    for (name, exporters) in implicit_names
+        source_module = try
             # I would like to suppress this warning:
             # WARNING: both X and Y export "parse"; uses of it in module Z must be qualified
             # However, `redirect_stdio` does not help!
@@ -47,12 +50,37 @@ function find_implicit_imports(mod::Module; skip=(mod, Base, Core))
             missing
         end
         # for unambiguous names, we can figure them out
-        # note `resolved_module` can equal `mod` if both `mod` and some other module
+        # note `source_module` can equal `mod` if both `mod` and some other module
         # define the same name. If it resolves to `mod` though, we don't want to
         # explicitly import anything!
-        if !ismissing(resolved_module) && resolved_module !== mod
-            mod_lookup[name] = resolved_module
+        if !ismissing(source_module) && source_module !== mod
+            source = source_module
+
+            es = Module[]
+            # Now figure out what names it was exported from
+            binding = getglobal(source_module, name)
+            # which one to use if more than 1?
+            # currently we will use the last one...
+            for e in exporters
+                exported_binding = try_getglobal(e, name)
+                if exported_binding === binding
+                    push!(es, e)
+                end
+            end
+            mod_lookup[name] = (; source, exporters=es)
         end
     end
     return mod_lookup
 end
+
+function try_getglobal(mod, name)
+    try
+        getglobal(mod, name)
+    catch e
+        if e isa UndefVarError
+            nothing
+        else
+            rethrow()
+        end
+    end
+end

test/examples.jl

@@ -1,3 +1,4 @@
+# https://github.com/ericphanson/ExplicitImports.jl/issues/1
 module ThreadPinning
 
 using LinearAlgebra
@@ -20,6 +21,7 @@ end
 
 end
 
+# https://github.com/ericphanson/ExplicitImports.jl/issues/20
 module Foo20
 
 using Markdown
@@ -41,3 +43,10 @@ testing docs
 function testing_docstr end
 
 end
+
+# https://github.com/ericphanson/ExplicitImports.jl/issues/24
+module Mod24
+using ..Exporter2
+
+exported_a
+end

test/runtests.jl

@@ -20,15 +20,18 @@ function restrict_to_module(df, mod)
                   :module_path => ByRow(ms -> all(Base.splat(isequal), zip(ms, mod_path))))
 end
 
-function drop_location(nt::@NamedTuple{name::Symbol,source::Module,location::String})
+function only_name_source(nt::@NamedTuple{name::Symbol,source::Module,
+                                          exporters::Vector{Module},location::String})
+    @test !isempty(nt.exporters)
     return (; nt.name, nt.source)
 end
-function drop_location(nt::@NamedTuple{name::Symbol,location::String})
+
+function only_name_source(nt::@NamedTuple{name::Symbol,location::String})
     return (; nt.name)
 end
-drop_location(::Nothing) = nothing
-drop_location(v::Vector) = drop_location.(v)
-drop_location(p::Pair) = first(p) => drop_location(last(p))
+only_name_source(::Nothing) = nothing
+only_name_source(v::Vector) = only_name_source.(v)
+only_name_source(p::Pair) = first(p) => only_name_source(last(p))
 
 include("Exporter.jl")
 include("TestModA.jl")
@@ -46,7 +49,7 @@ if VERSION > v"1.9-"
         DataFramesExt = Base.get_extension(TestPkg, :DataFramesExt)
         @test haskey(Dict(submods), DataFramesExt)
 
-        ext_imports = Dict(drop_location(explicit_imports(TestPkg)))[DataFramesExt]
+        ext_imports = Dict(only_name_source(explicit_imports(TestPkg)))[DataFramesExt]
         @test ext_imports == [(; name=:DataFrames, source=DataFrames),
                               (; name=:DataFrame, source=DataFrames),
                               (; name=:groupby, source=DataFrames)]
@@ -62,8 +65,15 @@ end
     @test contains(str, "- qr")
 end
 
+@testset "Exported module (#24)" begin
+    statements = using_statement.(explicit_imports_nonrecursive(Mod24, "examples.jl"))
+    # The key thing here is we do not have `using .Exporter: exported_a`,
+    # since we haven't done `using .Exporter` in `Mod24`, only `using .Exporter2`
+    @test statements == ["using .Exporter2: exported_a", "using .Exporter2: Exporter2"]
+end
+
 @testset "string macros (#20)" begin
-    foo = drop_location(explicit_imports_nonrecursive(Foo20, "examples.jl"))
+    foo = only_name_source(explicit_imports_nonrecursive(Foo20, "examples.jl"))
     @test foo == [(; name=:Markdown, source=Markdown),
                   (; name=Symbol("@doc_str"), source=Markdown)]
     bar = explicit_imports_nonrecursive(Bar20, "examples.jl")
@@ -129,7 +139,7 @@ end
 @testset "ExplicitImports.jl" begin
     @test using_statement.(explicit_imports_nonrecursive(TestModA, "TestModA.jl")) ==
           ["using .Exporter: Exporter", "using .Exporter: @mac",
-           "using .Exporter: exported_a",
+           "using .Exporter2: exported_a",
            "using .Exporter2: Exporter2", "using .Exporter3: Exporter3"]
 
     per_usage_info, _ = analyze_all_names("TestModA.jl")
@@ -202,12 +212,12 @@ end
     @test_logs explicit_imports_nonrecursive(TestModA.SubModB.TestModA.TestModC,
                                              "TestModC.jl"; warn_stale=false)
 
-    @test drop_location(stale_explicit_imports_nonrecursive(TestModA.SubModB.TestModA.TestModC,
-                                                            "TestModC.jl")) ==
+    @test only_name_source(stale_explicit_imports_nonrecursive(TestModA.SubModB.TestModA.TestModC,
+                                                               "TestModC.jl")) ==
           [(; name=:exported_c), (; name=:exported_d)]
 
     # Recursive version
-    lookup = Dict(drop_location(stale_explicit_imports(TestModA, "TestModA.jl")))
+    lookup = Dict(only_name_source(stale_explicit_imports(TestModA, "TestModA.jl")))
     @test lookup[TestModA.SubModB.TestModA.TestModC] ==
           [(; name=:exported_c), (; name=:exported_d)]
     @test isempty(lookup[TestModA])
@@ -224,7 +234,8 @@ end
     # Recursion
     nested = @test_logs (:warn, r"stale") explicit_imports(TestModA, "TestModA.jl")
     @test nested isa Vector{Pair{Module,
-                                 Vector{@NamedTuple{name::Symbol,source::Module,location::String}}}}
+                                 Vector{@NamedTuple{name::Symbol,source::Module,
+                                                    exporters::Vector{Module},location::String}}}}
     @test TestModA in first.(nested)
     @test TestModA.SubModB in first.(nested)
     @test TestModA.SubModB.TestModA in first.(nested)
@@ -237,7 +248,7 @@ end
     # should be no logs
     str = @test_logs sprint(print_explicit_imports, TestModA, "TestModA.jl")
     @test contains(str, "Module Main.TestModA is relying on implicit imports")
-    @test contains(str, "using .Exporter: exported_a")
+    @test contains(str, "using .Exporter2: exported_a")
     @test contains(str,
                    "However, module Main.TestModA.SubModB.TestModA.TestModC has stale explicit imports for these unused names")
 
@@ -350,20 +361,20 @@ end
     @testset "Tainted modules" begin
         log = (:warn, r"Dynamic")
 
-        @test_logs log @test drop_location(explicit_imports(DynMod, "DynMod.jl")) ==
+        @test_logs log @test only_name_source(explicit_imports(DynMod, "DynMod.jl")) ==
                              [DynMod => nothing, DynMod.Hidden => nothing]
-        @test_logs log @test drop_location(explicit_imports(DynMod, "DynMod.jl";
-                                                            strict=false)) ==
+        @test_logs log @test only_name_source(explicit_imports(DynMod, "DynMod.jl";
+                                                               strict=false)) ==
                              [DynMod => [(; name=:print_explicit_imports,
                                           source=ExplicitImports)],
                               # Wrong! Missing explicit export
                               DynMod.Hidden => []]
 
         @test_logs log @test explicit_imports_nonrecursive(DynMod, "DynMod.jl") === nothing
 
-        @test_logs log @test drop_location(explicit_imports_nonrecursive(DynMod,
-                                                                         "DynMod.jl";
-                                                                         strict=false)) ==
+        @test_logs log @test only_name_source(explicit_imports_nonrecursive(DynMod,
+                                                                            "DynMod.jl";
+                                                                            strict=false)) ==
                              [(; name=:print_explicit_imports, source=ExplicitImports)]
         @test_logs log @test stale_explicit_imports(DynMod, "DynMod.jl") ==
                              [DynMod => nothing,