add check_all_explicit_imports_are_public (#59)

* wip

* wip

* readme/docs

* update docs

* skips

* tests

README.md

@@ -8,14 +8,14 @@
 
 ExplicitImports.jl helps detect implicit imports and mitigate issues with the alternatives (explicit imports and qualified accesses).
 
-| Problem           | Example                             | Interactive detection of issue                         | Programmatic detection        | Regression-testing check                                     |
-| ----------------- | ----------------------------------- | ------------------------------------------------------ | ----------------------------- | ------------------------------------------------------------ |
-| Implicit imports  | `using LinearAlgebra`               | `print_explicit_imports`                               | `implicit_imports`            | `check_no_implicit_imports`                                  |
-| Non-owning import | `using LinearAlgebra: map`          | `print_explicit_imports`                               | `improper_explicit_imports`   | `check_all_explicit_imports_via_owners` |
-| Non-public import | `using LinearAlgebra: _svd!`        | `print_explicit_imports` with `report_non_public=true` | `improper_explicit_imports`   | not yet implemented: `check_all_explicit_imports_public`     |
-| Stale import      | `using LinearAlgebra: svd # unused` | `print_explicit_imports`                               | `improper_explicit_imports`   | `check_no_stale_explicit_imports`                            |
-| Non-owning access | `LinearAlgebra.map`                 | `print_explicit_imports`                               | `improper_qualified_accesses` | `check_all_qualified_accesses_via_owners`                    |
-| Non-public access | `LinearAlgebra._svd!`               | `print_explicit_imports` with `report_non_public=true` | `improper_qualified_accesses` | not yet implemented: `check_all_qualified_accesses_public`   |
+| Problem           | Example                             | Interactive detection of issue                         | Programmatic detection        | Regression-testing check                                   |
+| ----------------- | ----------------------------------- | ------------------------------------------------------ | ----------------------------- | ---------------------------------------------------------- |
+| Implicit imports  | `using LinearAlgebra`               | `print_explicit_imports`                               | `implicit_imports`            | `check_no_implicit_imports`                                |
+| Non-owning import | `using LinearAlgebra: map`          | `print_explicit_imports`                               | `improper_explicit_imports`   | `check_all_explicit_imports_via_owners`                    |
+| Non-public import | `using LinearAlgebra: _svd!`        | `print_explicit_imports` with `report_non_public=true` | `improper_explicit_imports`   | `check_all_explicit_imports_are_public`                    |
+| Stale import      | `using LinearAlgebra: svd # unused` | `print_explicit_imports`                               | `improper_explicit_imports`   | `check_no_stale_explicit_imports`                          |
+| Non-owning access | `LinearAlgebra.map`                 | `print_explicit_imports`                               | `improper_qualified_accesses` | `check_all_qualified_accesses_via_owners`                  |
+| Non-public access | `LinearAlgebra._svd!`               | `print_explicit_imports` with `report_non_public=true` | `improper_qualified_accesses` | not yet implemented: `check_all_qualified_accesses_public` |
 
 To understand these examples, note that:
 

docs/src/api.md

@@ -23,15 +23,27 @@ improper_qualified_accesses
 
 ## Checks to use in testing
 
-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`:
+ExplicitImports.jl provides several functions (all starting with `check_`) which introspect a module for various kinds of potential issues, and throws errors if these issues are encountered. These "check" functions are designed to be narrowly scoped to detect one specific type of issue, and stable so that they can be used in testing environments (with the aim that non-breaking releases of ExplicitExports.jl will generally not cause new test failures).
+
+The first such check is [`check_no_implicit_imports`](@ref) which aims to ensure there are no implicit exports used in the package.
 
 ```@docs
 check_no_implicit_imports
+```
+
+Next, we have several checks related to detecting "improper" explicit imports. The function [`check_no_stale_explicit_imports`](@ref) checks that a module has no "stale" (unused) explicit imports. Next [`check_all_explicit_imports_via_owners`](@ref) and [`check_all_explicit_imports_are_public`](@ref) provide related checks. [`check_all_explicit_imports_via_owners`](@ref) is a weaker check which errors for particularly problematic imports of non-public names, namely those for which the module they are being imported from does not "own" the name (since it was not defined there). The typical scenario here is that the name may be public in some other module, but just happens to be present in the namespace of that module (consider `using LinearAlgebra: map` which imports Base's `map` function). Next, [`check_all_explicit_imports_are_public`](@ref) provides a stricter check that all names being explicitly imported are in fact public in the module they are being imported from, whether or not they are "owned" by that module.
+
+```@docs
 check_no_stale_explicit_imports
-check_all_qualified_accesses_via_owners
 check_all_explicit_imports_via_owners
+check_all_explicit_imports_are_public
 ```
 
+Lastly, we have one check related to detecting "improper" qualified accesses to names.  [`check_all_qualified_accesses_via_owners`](@ref) checks that all qualified accesses (e.g. usage of names in the form `Foo.bar`) are such that the name being accessed is "owned" by the module it is being accessed from (just like [`check_all_explicit_imports_via_owners`](@ref)). This would detect, e.g., `LinearAlgebra.map`.
+
+```@docs
+check_all_qualified_accesses_via_owners
+```
 ## Usage with scripts (such as `runtests.jl`)
 
 We also provide a helper function to analyze scripts (rather than modules).

src/ExplicitImports.jl

@@ -10,10 +10,10 @@ export print_explicit_imports_script
 export improper_qualified_accesses,
        improper_qualified_accesses_nonrecursive, check_all_qualified_accesses_via_owners
 export improper_explicit_imports, improper_explicit_imports_nonrecursive,
-       check_all_explicit_imports_via_owners
+       check_all_explicit_imports_via_owners, check_all_explicit_imports_are_public
 export ImplicitImportsException, UnanalyzableModuleException,
        FileNotFoundException, QualifiedAccessesFromNonOwnerException,
-       ExplicitImportsFromNonOwnerException
+       ExplicitImportsFromNonOwnerException, NonPublicExplicitImportsException
 export StaleImportsException, check_no_stale_explicit_imports
 
 # deprecated

src/checks.jl

@@ -1,3 +1,5 @@
+const TUPLE_MODULE_PAIRS = NTuple{N,Pair{Module,Module}} where {N}
+
 struct ImplicitImportsException <: Exception
     mod::Module
     names::Vector{@NamedTuple{name::Symbol,source::Module,exporters::Vector{Module},
@@ -57,6 +59,22 @@ function Base.showerror(io::IO, e::ExplicitImportsFromNonOwnerException)
     end
 end
 
+struct NonPublicExplicitImportsException <: Exception
+    mod::Module
+    bad_imports::Vector{@NamedTuple{name::Symbol,location::String,value::Any,
+                                    importing_from::Module}}
+end
+
+function Base.showerror(io::IO, e::NonPublicExplicitImportsException)
+    println(io, "NonPublicExplicitImportsException")
+    println(io,
+            "Module `$(e.mod)` has explicit imports of names from modules in which they are not public (i.e. exported or declared public in Julia 1.11+):")
+    for row in e.bad_imports
+        println(io,
+                "- `$(row.name)` is not public in $(row.importing_from) but it was imported from $(row.importing_from) at $(row.location)")
+    end
+end
+
 struct StaleImportsException <: Exception
     mod::Module
     names::Vector{@NamedTuple{name::Symbol,location::String}}
@@ -118,7 +136,8 @@ function check_no_stale_explicit_imports(mod::Module, file=pathof(mod); ignore::
 end
 
 """
-    check_no_implicit_imports(mod::Module, file=pathof(mod); skip=(mod, Base, Core), ignore::Tuple=(), allow_unanalyzable::Tuple=())
+    check_no_implicit_imports(mod::Module, file=pathof(mod); skip=(mod, Base, Core), ignore::Tuple=(),
+                              allow_unanalyzable::Tuple=())
 
 Checks that neither `mod` nor any of its submodules is relying on implicit imports, throwing
 an `ImplicitImportsException` if so, and returning `nothing` otherwise.
@@ -207,7 +226,9 @@ function should_ignore!(::Nothing, mod; ignore)
 end
 
 """
-    check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(), require_submodule_access=false)
+    check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(),
+                                            require_submodule_access=false,
+                                            skip::$(TUPLE_MODULE_PAIRS)=(Base => Core,))
 
 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.
@@ -220,6 +241,16 @@ This can be used in a package's tests, e.g.
 
 ## Allowing some qualified accesses via non-owner modules
 
+The `skip` keyword argument can be passed to allow non-owning accesses via some modules (and their submodules). One pases a tuple of `accessing_from => parent` pairs, allowing cases in which a name is being imported from the module `accessing_from`, but is owned by the module `parent`. By default, `skip` is set to `(Base => Core,)`, meaning that names which are accessed from Base but are owned by Core are not flagged.
+
+For example:
+
+```julia
+@test check_all_qualified_accesses_via_owners(MyPackage; skip=(Base => Core, DataFrames => PrettyTables)) === nothing
+```
+
+would allow explicitly accessing names which are owned by PrettyTables from DataFrames.
+
 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,
 
@@ -235,10 +266,10 @@ See also: [`improper_qualified_accesses`](@ref). Note that while that function m
 """
 function check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod);
                                                  ignore::Tuple=(),
+                                                 skip::TUPLE_MODULE_PAIRS=(Base => Core,),
                                                  require_submodule_access=false)
     check_file(file)
-    for (submodule, problematic) in
-        improper_qualified_accesses(mod, file; skip=ignore)
+    for (submodule, problematic) in improper_qualified_accesses(mod, file; skip)
         filter!(problematic) do nt
             return nt.name ∉ ignore
         end
@@ -260,7 +291,9 @@ function check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod);
 end
 
 """
-    check_all_explicit_imports_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(), allow_unanalyzable::Tuple=(), require_submodule_import=false)
+    check_all_explicit_imports_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(),
+                                          require_submodule_import=false,
+                                          skip::$(TUPLE_MODULE_PAIRS)=(Base => Core,)))
 
 Checks that neither `mod` nor any of its submodules has imports 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 `ExplicitImportsFromNonOwnerException` if so, and returning `nothing` otherwise.
@@ -271,12 +304,17 @@ This can be used in a package's tests, e.g.
 @test check_all_explicit_imports_via_owners(MyPackage) === nothing
 ```
 
-## Allowing some submodules to be unanalyzable
+## Allowing some explicit imports via non-owner modules
 
-Pass `allow_unanalyzable` as a tuple of submodules which are allowed to be unanalyzable.
-Any other submodules found to be unanalyzable will result in an `UnanalyzableModuleException` being thrown.
+The `skip` keyword argument can be passed to allow non-owning imports from some modules (and their submodules). One pases a tuple of `importing_from => parent` pairs, allowing cases in which a name is being imported from the module `importing_from`, but is owned by the module `parent`. By default, `skip` is set to `(Base => Core,)`, meaning that names which are imported from Base but are owned by Core are not flagged.
 
-## Allowing some explicit imports via non-owner modules
+For example:
+
+```julia
+@test check_all_explicit_imports_are_public(MyPackage; skip=(Base => Core, DataFrames => PrettyTables)) === nothing
+```
+
+would allow explicitly importing names which are owned by PrettyTables from DataFrames.
 
 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,
@@ -291,20 +329,26 @@ would check there were no explicit imports from non-owner modules besides that o
 
 If `require_submodule_import=true`, then an error will be thrown if the name is imported from a non-owner module even if it is imported from a parent module of the owner module. For example, in June 2024, `JSON.parse` is actually defined in the submodule `JSON.Parser` and is not declared public inside `JSON`, but the name is present within the module `JSON`. If `require_submodule_import=false`, the default, in this scenario the access `using JSON: parse` will not trigger an error, since the name is being accessed by a parent of the owner. If `require_submodule_import=false`, then accessing the function as `using JSON.Parser: parse` will be required to avoid an error.
 
-See also: [`improper_explicit_imports`](@ref). Note that while that function may increase in scope and report other kinds of improper accesses, `check_all_explicit_imports_via_owners` will not.
+## non-fully-analyzable modules do not cause exceptions
+
+Note that if a module is not fully analyzable (e.g. it has dynamic `include` calls), explicit imports of non-public names which could not be analyzed will be missed. Unlike [`check_no_stale_explicit_imports`](@ref) and [`check_no_implicit_imports`](@ref), this function will *not* throw an `UnanalyzableModuleException` in such cases.
+
+See also: [`improper_explicit_imports`](@ref) for programmatic access to such imports and [`check_all_explicit_imports_are_public`](@ref) for a stricter version of this check. Note that while `improper_explicit_imports` may increase in scope and report other kinds of improper accesses, `check_all_explicit_imports_via_owners` will not.
 """
 function check_all_explicit_imports_via_owners(mod::Module, file=pathof(mod);
                                                ignore::Tuple=(),
-                                               allow_unanalyzable::Tuple=(),
+                                               skip::TUPLE_MODULE_PAIRS=(Base => Core,),
                                                require_submodule_import=false)
     check_file(file)
+    # `strict=false` because unanalyzability doesn't compromise our analysis
+    # that much, unlike in the stale case (in which we might miss usages of the
+    # "stale" name, making it not-stale). Here we might just miss bad imports
+    # hidden behind a dynamic include or such. IMO it's sufficient to have
+    # `check_no_stale_explicit_imports` or `check_no_implicit_imports`
+    # throw by default there and not require this function to also throw
+    # in the exact same cases.
     for (submodule, problematic) in
-        improper_explicit_imports(mod, file)
-        if isnothing(problematic)
-            submodule in allow_unanalyzable && continue
-            throw(UnanalyzableModuleException(submodule))
-        end
-
+        improper_explicit_imports(mod, file; strict=false, skip)
         filter!(problematic) do nt
             return nt.name ∉ ignore
         end
@@ -330,3 +374,76 @@ function check_all_explicit_imports_via_owners(mod::Module, file=pathof(mod);
     end
     return nothing
 end
+
+"""
+    check_all_explicit_imports_are_public(mod::Module, file=pathof(mod); ignore::Tuple=(),
+                                          skip::$(TUPLE_MODULE_PAIRS)=(Base => Core,))
+
+Checks that neither `mod` nor any of its submodules has imports to names which are non-public (i.e. not exported, nor declared public on Julia 1.11+)
+throwing an `NonPublicExplicitImportsException` if so, and returning `nothing` otherwise.
+
+This can be used in a package's tests, e.g.
+
+```julia
+@test check_all_explicit_imports_are_public(MyPackage) === nothing
+```
+
+## Allowing some non-public explicit imports
+
+The `skip` keyword argument can be passed to allow non-public imports from some modules (and their submodules). One pases a tuple of `importing_from => pub` pairs, allowing cases in which a name is being imported from the module `importing_from`, but is public in the module `pub`. By default, `skip` is set to `(Base => Core,)`, meaning that names which are imported from Base but are public in Core are not flagged.
+
+For example:
+
+```julia
+@test check_all_explicit_imports_are_public(MyPackage; skip=(Base => Core, DataFrames => PrettyTables)) === nothing
+```
+
+would allow explicitly importing names which are public in PrettyTables from DataFrames.
+
+If `ignore` is supplied, it should be a tuple of `Symbol`s, representing names
+that are allowed to be imported from modules in which they are not public. For example,
+
+```julia
+@test check_all_explicit_imports_are_public(MyPackage; ignore=(:DataFrame,)) === nothing
+```
+
+would check there were no non-public explicit imports besides that of the name `DataFrame`.
+
+## non-fully-analyzable modules do not cause exceptions
+
+Note that if a module is not fully analyzable (e.g. it has dynamic `include` calls), explicit imports of non-public names which could not be analyzed will be missed. Unlike [`check_no_stale_explicit_imports`](@ref) and [`check_no_implicit_imports`](@ref), this function will *not* throw an `UnanalyzableModuleException` in such cases.
+
+See also: [`improper_explicit_imports`](@ref) for programmatic access to such imports, and [`check_all_explicit_imports_via_owners`] for a weaker version of this check. Note that while `improper_explicit_imports` may increase in scope and report other kinds of improper accesses, `check_all_explicit_imports_are_public` will not.
+"""
+function check_all_explicit_imports_are_public(mod::Module, file=pathof(mod);
+                                               skip::TUPLE_MODULE_PAIRS=(Base => Core,),
+                                               ignore::Tuple=())
+    check_file(file)
+    for (submodule, problematic) in
+        # We pass `skip=()` since we will do our own filtering after
+        improper_explicit_imports(mod, file; strict=false, skip=())
+        filter!(problematic) do nt
+            return nt.name ∉ ignore
+        end
+
+        # We don't just pass `skip` to `improper_explicit_imports`
+        # since that works by "ownership" rather than publicness
+        for (from, pub) in skip
+            filter!(problematic) do row
+                return !(row.importing_from == from && public_or_exported(pub, row.name))
+            end
+        end
+
+        # Discard imports from names that are public in their module; that's OK
+        filter!(problematic) do nt
+            return !nt.public_import
+        end
+
+        # drop unnecessary columns
+        problematic = NamedTuple{(:name, :location, :value, :importing_from)}.(problematic)
+        if !isempty(problematic)
+            throw(NonPublicExplicitImportsException(submodule, problematic))
+        end
+    end
+    return nothing
+end