Provide functionality to detect "improper qualified access" (#50)

* track qualifying modules

* wip

* tweak

* add more functionality

* wip

* more docs and checks

* wip

* wip

* bump project

* switch to `which`

* wip

* wip

* update readme

* don't skip source modules

.github/workflows/CI.yml

@@ -21,17 +21,16 @@ jobs:
         version:
           - '1.7' # earliest supported version
           - '1' # current release
+          - 'beta'
           - 'nightly'
         os:
           - ubuntu-latest
-        arch:
-          - x64
     steps:
       - uses: actions/checkout@v4
-      - uses: julia-actions/setup-julia@v1
+      # Using `juliaup` over `setup-julia` so we can access beta versions easier
+      - uses: julia-actions/install-juliaup@v1
         with:
-          version: ${{ matrix.version }}
-          arch: ${{ matrix.arch }}
+          julia-version: ${{ matrix.version }}
       - uses: julia-actions/cache@v1
       - uses: julia-actions/julia-buildpkg@v1
       - uses: julia-actions/julia-runtest@v1

Project.toml

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

README.md

@@ -29,6 +29,14 @@ There are various takes on _how problematic_ this issue is, to what extent this
 
 Personally, I don't think this is always a huge issue, and that it's basically fine for packages to use implicit imports if that is their preferred style and they understand the risk. But I do think this issue is somewhat a "hole" in the semver system as it applies to Julia packages, and I wanted to create some tooling to make it easier to mitigate the issue for package authors who would prefer to not rely on implicit imports.
 
+## Ways ExplicitImports.jl can and cannot help
+
+As mentioned, there are two ways to avoid implicit imports: by using explicit imports such as `using X: foo`, or by qualifying names (`X.foo`).
+
+ExplicitImports.jl was initially designed to help make explicit imports more ergonomic, by providing functionality to convert implicit imports into explicit ones (e.g. `print_explicit_imports`), and by providing testing tools to keep a codebase free of implicit imports and without stale explicit imports. There are two missing features here still: checking the explicitly imported names are public in the module they are being imported from (e.g. avoid `using X: _internal_foo`), and a weaker condition, checking that they are owned by the module they are being imported from (e.g. avoid `using LinearAlgebra: map`, since `map` comes from Base).
+
+Since v1.5, ExplicitImports.jl has also gained functionality to make using qualifying names more ergonomic. One pitfall of qualified accesses like `X.foo` is that `foo` may be internal to `X` or may be owned by another module (the same issues faced by explicit imports). The function `improper_qualified_accesses` can detect the latter case.
+
 ## Implementation status
 
 ExplicitImports.jl has been used successfully on several codebases, but I would still not describe it as fully mature. That said, it should be ready for use; please file issues if problems arise.
@@ -38,6 +46,7 @@ See the [API docs](https://ericphanson.github.io/ExplicitImports.jl/dev/api/) fo
 - functionality to help convert implicit imports to explicit exports
 - functionality to warn about "stale" (unused) explicit imports
 - functionality to add to package tests to ensure all imports continue to be explicit (and non-stale)
+- functionality to detect "improper" qualified access to names, such as accessing `LinearAlgebra.map` instead of `Base.map` (since `map` is owned by Base, and just happens to be available in the `LinearAlgebra` namespace)
 
 ## Example
 
@@ -53,6 +62,9 @@ using AbstractTrees: AbstractTrees, Leaves, TreeCursor, children, nodevalue
 using JuliaSyntax: JuliaSyntax, @K_str
 ```
 
+Additionally, module ExplicitImports accesses names from non-owner modules:
+- `parent` has owner AbstractTrees but it was accessed from ExplicitImports at /Users/eph/ExplicitImports/src/qualified_names.jl:217:21
+
 ````
 
 Note: the `WARNING` is more or less harmless; the way this package is written, it will happen any time there is a clash, even if that clash is not realized in your code. I cannot figure out how to suppress it.
@@ -71,6 +83,9 @@ using AbstractTrees: children # used at /Users/eph/ExplicitImports/src/get_names
 using AbstractTrees: nodevalue # used at /Users/eph/ExplicitImports/src/parse_utilities.jl:96:34
 using JuliaSyntax: JuliaSyntax # used at /Users/eph/ExplicitImports/src/parse_utilities.jl:103:15
 ```
+
+Additionally, module ExplicitImports accesses names from non-owner modules:
+- `parent` has owner AbstractTrees but it was accessed from ExplicitImports at /Users/eph/ExplicitImports/src/qualified_names.jl:217:21
 ````
 
 Note the paths of course will differ depending on the location of the code on your system.

docs/src/api.md

@@ -18,13 +18,22 @@ print_stale_explicit_imports
 stale_explicit_imports
 ```
 
+## Detecting "improper" access of names from other modules
+
+```@docs
+improper_qualified_accesses
+print_improper_qualified_accesses
+improper_qualified_accesses_nonrecursive
+```
+
 ## Checks to use in testing
 
-ExplicitImports.jl provides two functions which can be used to regression test that there is no reliance on implicit imports or stale explicit imports:
+ExplicitImports.jl provides three functions which can be used to regression test that there is no reliance on implicit imports, no stale explicit imports, and no qualified accesses to names from modules other than their owner as determined by `Base.which`:
 
 ```@docs
 check_no_implicit_imports
 check_no_stale_explicit_imports
+check_all_qualified_accesses_via_owners
 ```
 
 ## Usage with scripts (such as `runtests.jl`)

docs/src/internals.md

@@ -21,4 +21,5 @@ ExplicitImports.find_implicit_imports
 ExplicitImports.get_names_used
 ExplicitImports.analyze_all_names
 ExplicitImports.inspect_session
+ExplicitImports.FileAnalysis
 ```

examples/qualified.jl

@@ -0,0 +1,5 @@
+module MyMod
+using LinearAlgebra
+# sum is in `Base`, so we shouldn't access it from LinearAlgebra:
+n = LinearAlgebra.sum([1, 2, 3])
+end

src/ExplicitImports.jl

@@ -9,12 +9,15 @@ export print_explicit_imports, explicit_imports, check_no_implicit_imports,
 export print_explicit_imports_script
 export print_stale_explicit_imports, stale_explicit_imports,
        check_no_stale_explicit_imports, stale_explicit_imports_nonrecursive
+export print_improper_qualified_accesses, improper_qualified_accesses,
+       improper_qualified_accesses_nonrecursive, check_all_qualified_accesses_via_owners
 export StaleImportsException, ImplicitImportsException, UnanalyzableModuleException,
-       FileNotFoundException
+       FileNotFoundException, QualifiedAccessesFromNonOwnerException
 
 include("parse_utilities.jl")
 include("find_implicit_imports.jl")
 include("get_names_used.jl")
+include("qualified_names.jl")
 include("checks.jl")
 
 const SKIPS_KWARG = """
@@ -112,22 +115,28 @@ function print_explicit_imports(mod::Module, file=pathof(mod); kw...)
 end
 
 """
-    print_explicit_imports([io::IO=stdout,] mod::Module, file=pathof(mod); skip=(mod, Base, Core), warn_stale=true, strict=true)
+    print_explicit_imports([io::IO=stdout,] mod::Module, file=pathof(mod); skip=(mod, Base, Core), warn_stale=true,
+                           warn_improper_qualified_accesses=true, strict=true)
 
-Runs [`explicit_imports`](@ref) and prints the results, along with those of [`stale_explicit_imports`](@ref).
+Runs [`explicit_imports`](@ref) and prints the results, along with those of [`stale_explicit_imports`](@ref) and [`improper_qualified_accesses`](@ref).
+
+Note that the particular printing may change in future non-breaking releases of ExplicitImports.
 
 ## Keyword arguments
 
 $SKIPS_KWARG
 * `warn_stale=true`: if set, this function will also print information about stale explicit imports.
+* `warn_improper_qualified_accesses=true`: if set, this function will also print information about any "improper" qualified accesses to names from other modules.
 $STRICT_PRINTING_KWARG
 * `show_locations=false`: whether or not to print locations of where the names are being used (and, if `warn_stale=true`, where the stale explicit imports are).
 * `linewidth=80`: format into lines of up to this length. Set to 0 to indicate one name should be printed per line.
 
-See also [`check_no_implicit_imports`](@ref) and [`check_no_stale_explicit_imports`](@ref).
+See also [`check_no_implicit_imports`](@ref), [`check_no_stale_explicit_imports`](@ref), and [`check_all_qualified_accesses_via_owners`](@ref).
 """
 function print_explicit_imports(io::IO, mod::Module, file=pathof(mod);
-                                skip=(mod, Base, Core), warn_stale=true, strict=true,
+                                skip=(mod, Base, Core), warn_stale=true,
+                                warn_improper_qualified_accesses=true,
+                                strict=true,
                                 show_locations=false,
                                 linewidth=80,
                                 # internal kwargs
@@ -170,6 +179,23 @@ function print_explicit_imports(io::IO, mod::Module, file=pathof(mod);
                     println(io, "- $name", proof)
                 end
             end
+        else
+            stale = ()
+        end
+        if warn_improper_qualified_accesses
+            problematic = improper_qualified_accesses_nonrecursive(mod, file;
+                                                                   file_analysis=file_analysis[file])
+            if !isnothing(problematic) && !isempty(problematic)
+                word = !isnothing(imports) && isempty(imports) && isempty(stale) ?
+                       "However" : "Additionally"
+                println(io)
+                println(io,
+                        "$word, $(name_fn(mod)) accesses names from non-owner modules:")
+                for row in problematic
+                    println(io,
+                            "- `$(row.name)` has owner $(row.whichmodule) but it was accessed from $(row.accessing_from) at $(row.location)")
+                end
+            end
         end
     end
 end
@@ -292,6 +318,8 @@ end
 
 Runs [`stale_explicit_imports`](@ref) and prints the results.
 
+Note that the particular printing may change in future non-breaking releases of ExplicitImports.
+
 ## Keyword arguments
 
 $STRICT_PRINTING_KWARG
@@ -341,6 +369,8 @@ More keys may be added to the NamedTuples in the future in non-breaking releases
 
     This is an unusual situation (generally `B` should just get `x` directly from `X`, rather than indirectly via `A`), but there are situations in which it arises, so one may need to be careful about naively removing all "stale" explicit imports flagged by this function.
 
+    Running [`improper_qualified_accesses`](@ref) on downstream code can help identify such "improper" accesses to names via modules other than their owner.
+
 ## Keyword arguments
 
 $STRICT_KWARG
@@ -423,6 +453,9 @@ function filter_to_module(file_analysis::FileAnalysis, mod::Module)
     # Don't do that!
     match = module_path -> all(Base.splat(isequal), zip(module_path, mod_path))
 
+    per_usage_info = filter(file_analysis.per_usage_info) do nt
+        return match(nt.module_path)
+    end
     needs_explicit_import = filter(file_analysis.needs_explicit_import) do nt
         return match(nt.module_path)
     end
@@ -431,7 +464,7 @@ function filter_to_module(file_analysis::FileAnalysis, mod::Module)
     end
     mods_found = filter(!isempty, file_analysis.untainted_modules)
     tainted = !any(match, mods_found)
-    return (; needs_explicit_import, unnecessary_explicit_import, tainted)
+    return (; needs_explicit_import, unnecessary_explicit_import, tainted, per_usage_info)
 end
 
 if VERSION < v"1.9-"
@@ -542,6 +575,8 @@ end
 
 Analyzes the script located at `path` and prints information about reliance on implicit exports as well as any stale explicit imports (if `warn_stale=true`).
 
+Note that the particular printing may change in future non-breaking releases of ExplicitImports.
+
 !!! warning
   The script (or at least, all imports in the script) must be run before this function can give reliable results, since it relies on introspecting what names are present in `Main`.
 

src/checks.jl

@@ -36,6 +36,23 @@ function Base.showerror(io::IO, e::UnanalyzableModuleException)
     return nothing
 end
 
+struct QualifiedAccessesFromNonOwnerException <: Exception
+    mod::Module
+    accesses::Vector{@NamedTuple{name::Symbol,location::String,value::Any,
+                                 accessing_from::Module,
+                                 whichmodule::Module}}
+end
+
+function Base.showerror(io::IO, e::QualifiedAccessesFromNonOwnerException)
+    println(io, "QualifiedAccessesFromNonOwnerException")
+    println(io,
+            "Module `$(e.mod)` has qualified accesses to names via modules other than their owner as determined via `Base.which`:")
+    for row in e.accesses
+        println(io,
+                "- `$(row.name)` has owner $(row.whichmodule) but it was accessed from $(row.accessing_from) at $(row.location)")
+    end
+end
+
 """
     check_no_implicit_imports(mod::Module, file=pathof(mod); skip=(mod, Base, Core), ignore::Tuple=(), allow_unanalyzable::Tuple=())
 
@@ -170,3 +187,48 @@ function check_no_stale_explicit_imports(mod::Module, file=pathof(mod); ignore::
     end
     return nothing
 end
+
+"""
+    check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(), require_submodule_access=false)
+
+Checks that neither `mod` nor any of its submodules has accesses to names via modules other than their owner as determined by `Base.which` (unless the name is public or exported in that module),
+throwing an `QualifiedAccessesFromNonOwnerException` if so, and returning `nothing` otherwise.
+
+This can be used in a package's tests, e.g.
+
+```julia
+@test check_all_qualified_accesses_via_owners(MyPackage) === nothing
+```
+
+## Allowing some qualified accesses via non-owner modules
+
+If `ignore` is supplied, it should be a tuple of `Symbol`s, representing names
+that are allowed to be accessed from non-owner modules. For example,
+
+```julia
+@test check_all_qualified_accesses_via_owners(MyPackage; ignore=(:DataFrame,)) === nothing
+```
+
+would check there were no qualified accesses from non-owner modules besides that of the name `DataFrame`.
+
+See also: [`improper_qualified_accesses`](@ref), which also describes the meaning of the keyword argument `require_submodule_access`. Note that while that function may increase in scope and report other kinds of improper accesses, `check_all_qualified_accesses_via_owners` will not.
+"""
+function check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod);
+                                                 ignore::Tuple=(),
+                                                 require_submodule_access=false)
+    check_file(file)
+    for (submodule, problematic) in
+        improper_qualified_accesses(mod, file; skip=ignore, require_submodule_access)
+        # drop unnecessary columns
+        problematic = [(;
+                        (k => v for (k, v) in pairs(row) if k ∉ (:public_access,))...)
+                       for row in problematic]
+        filter!(problematic) do nt
+            return nt.name ∉ ignore
+        end
+        if !isempty(problematic)
+            throw(QualifiedAccessesFromNonOwnerException(submodule, problematic))
+        end
+    end
+    return nothing
+end