add check_no_self_qualified_accesses (#64)

* add `check_no_self_qualified_accesses`

* up

* format

* up

* fix

README.md

@@ -8,14 +8,15 @@
 
 ExplicitImports.jl helps detect implicit imports and mitigate issues with the alternatives (explicit imports and qualified accesses).
 
-| Problem           | Example                             | Interactive detection                                  | 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` | `check_all_qualified_accesses_are_public` |
+| Problem               | Example                             | Interactive detection                                  | 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` | `check_all_qualified_accesses_are_public` |
+| Self-qualified access | `Foo.bar` within the module `Foo`   | `print_explicit_imports`                               | `improper_qualified_accesses` | `check_no_self_qualified_accesses`        |
 
 To understand these examples, note that:
 

docs/src/api.md

@@ -39,11 +39,12 @@ check_all_explicit_imports_via_owners
 check_all_explicit_imports_are_public
 ```
 
-Lastly, we have two checks related to detecting "improper" qualified accesses to names, which are analogous to checks related to improper explicit imports.  [`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`. Likewise, [`check_all_qualified_accesses_are_public`](@ref) is a stricter check which verifies all qualified accesses to names are via modules in which that name is public.
+Lastly, we have two checks related to detecting "improper" qualified accesses to names, which are analogous to checks related to improper explicit imports.  [`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`. Likewise, [`check_all_qualified_accesses_are_public`](@ref) is a stricter check which verifies all qualified accesses to names are via modules in which that name is public. Additionally, [`check_no_self_qualified_accesses`](@ref) checks there are no self-qualified accesses, like accessing `Foo.foo` from within the module `Foo`.
 
 ```@docs
 check_all_qualified_accesses_via_owners
 check_all_qualified_accesses_are_public
+check_no_self_qualified_accesses
 ```
 
 ## Usage with scripts (such as `runtests.jl`)

src/ExplicitImports.jl

@@ -9,13 +9,14 @@ export print_explicit_imports, explicit_imports, check_no_implicit_imports,
 export print_explicit_imports_script
 export improper_qualified_accesses,
        improper_qualified_accesses_nonrecursive, check_all_qualified_accesses_via_owners,
-       check_all_qualified_accesses_are_public
+       check_all_qualified_accesses_are_public,
+       check_no_self_qualified_accesses
 export improper_explicit_imports, improper_explicit_imports_nonrecursive,
        check_all_explicit_imports_via_owners, check_all_explicit_imports_are_public
 export ImplicitImportsException, UnanalyzableModuleException,
        FileNotFoundException, QualifiedAccessesFromNonOwnerException,
        ExplicitImportsFromNonOwnerException, NonPublicExplicitImportsException,
-       NonPublicQualifiedAccessException
+       NonPublicQualifiedAccessException, SelfQualifiedAccessException
 export StaleImportsException, check_no_stale_explicit_imports
 
 # deprecated

src/checks.jl

@@ -43,6 +43,21 @@ function Base.showerror(io::IO, e::QualifiedAccessesFromNonOwnerException)
     end
 end
 
+struct SelfQualifiedAccessException <: Exception
+    mod::Module
+    accesses::Vector{@NamedTuple{name::Symbol,location::String,value::Any}}
+end
+
+function Base.showerror(io::IO, e::SelfQualifiedAccessException)
+    println(io, "SelfQualifiedAccessException")
+    println(io,
+            "Module `$(e.mod)` has self-qualified accesses:")
+    for row in e.accesses
+        println(io,
+                "- `$(row.name)` was accessed as $(e.mod).$(row.name) inside $(e.mod) at $(row.location)")
+    end
+end
+
 struct ExplicitImportsFromNonOwnerException <: Exception
     mod::Module
     bad_imports::Vector{@NamedTuple{name::Symbol,location::String,value::Any,
@@ -294,6 +309,12 @@ function check_all_qualified_accesses_via_owners(mod::Module, file=pathof(mod);
         filter!(problematic) do nt
             return nt.name ∉ ignore
         end
+
+        # Skip self-qualified. Those can be non-owner, but it's kinda beside the point.
+        filter!(problematic) do nt
+            return !nt.self_qualified
+        end
+
         filter!(problematic) do row
             if require_submodule_access
                 !row.accessing_from_owns_name
@@ -364,6 +385,11 @@ function check_all_qualified_accesses_are_public(mod::Module, file=pathof(mod);
             return nt.name ∉ ignore
         end
 
+        # Skip self-qualified. Those can be non-public, but it's kinda beside the point.
+        filter!(problematic) do nt
+            return !nt.self_qualified
+        end
+
         # We don't just pass `skip` to `improper_explicit_imports`
         # since that works by "ownership" rather than publicness
         for (from, pub) in skip
@@ -386,6 +412,59 @@ function check_all_qualified_accesses_are_public(mod::Module, file=pathof(mod);
     return nothing
 end
 
+"""
+    check_no_self_qualified_accesses(mod::Module, file=pathof(mod);
+                                    ignore::Tuple=())
+
+Checks that neither `mod` nor any of its submodules has self-qualified accesses,
+throwing an `SelfQualifiedAccessException` if so, and returning `nothing` otherwise.
+
+This can be used in a package's tests, e.g.
+
+```julia
+@test check_no_self_qualified_accesses(MyPackage) === nothing
+```
+
+## Allowing some self-qualified accesses
+
+If `ignore` is supplied, it should be a tuple of `Symbol`s, representing names
+that are allowed to be self-qualified. For example,
+
+```julia
+@test check_no_self_qualified_accesses(MyPackage; ignore=(:foo,)) === nothing
+```
+
+would check there were no self-qualified accesses besides that of the name `foo`.
+
+## non-fully-analyzable modules do not cause exceptions
+
+Note that if a module is not fully analyzable (e.g. it has dynamic `include` calls), qualified accesess 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_qualified_accesses`](@ref) for programmatic access to the same information. Note that while `improper_qualified_accesses` may increase in scope and report other kinds of improper accesses, `check_all_qualified_accesses_are_public` will not.
+"""
+function check_no_self_qualified_accesses(mod::Module, file=pathof(mod);
+                                          ignore::Tuple=())
+    check_file(file)
+    for (submodule, problematic) in
+        improper_qualified_accesses(mod, file; skip=())
+        filter!(problematic) do nt
+            return nt.name ∉ ignore
+        end
+
+        # Keep only self-qualified modules
+        filter!(problematic) do nt
+            return nt.self_qualified
+        end
+
+        # drop unnecessary columns
+        problematic = NamedTuple{(:name, :location, :value)}.(problematic)
+        if !isempty(problematic)
+            throw(SelfQualifiedAccessException(submodule, problematic))
+        end
+    end
+    return nothing
+end
+
 """
     check_all_explicit_imports_via_owners(mod::Module, file=pathof(mod); ignore::Tuple=(),
                                           require_submodule_import=false,

src/improper_qualified_accesses.jl

@@ -29,7 +29,8 @@ function analyze_qualified_names(mod::Module, file=pathof(mod);
                         public_access::Bool,
                         accessing_from_owns_name::Bool,
                         accessing_from_submodule_owns_name::Bool,
-                        internal_access::Bool}[]
+                        internal_access::Bool,
+                        self_qualified::Bool}[]
     # Now check:
     for row in qualified
         output = process_qualified_row(row, mod)
@@ -39,9 +40,10 @@ function analyze_qualified_names(mod::Module, file=pathof(mod);
                                                           output.accessing_from)
 
         internal_access = Base.moduleroot(mod) == Base.moduleroot(output.accessing_from)
+        self_qualified = output.accessing_from == mod
         push!(table,
               (; output..., accessing_from_owns_name, accessing_from_submodule_owns_name,
-               internal_access))
+               internal_access, self_qualified))
     end
     # Sort first, so we get the "first" time each is used
     sort!(table; by=nt -> (; nt.name, nt.location))
@@ -125,19 +127,20 @@ function improper_qualified_accesses_nonrecursive(mod::Module, file=pathof(mod);
     end
     problematic = analyze_qualified_names(mod, file; file_analysis)
 
-    # Report non-public accesses
-    filter!(row -> !row.public_access, problematic)
+    # Report only non-public accesses or self-qualified ones
+    filter!(row -> !row.public_access || row.self_qualified, problematic)
 
     for (from, parent) in skip
         filter!(problematic) do row
             return !(has_ancestor(row.whichmodule, parent) && row.accessing_from == from)
         end
     end
 
-    # if `allow_internal_accesses=true`, the default, then we strip out any accesses where the accessing module shares a `moduleroot` with the current module
+    # if `allow_internal_accesses=true`, the default, then we strip out any accesses where the accessing module shares a `moduleroot` with the current module,
+    # unless it is self-qualified
     if allow_internal_accesses
         filter!(problematic) do row
-            return !row.internal_access
+            return !row.internal_access || row.self_qualified
         end
     end
     return problematic
@@ -152,9 +155,10 @@ Attempts do detect various kinds of "improper" qualified accesses taking place i
 Currently, only detects cases in which the name is being accessed from a module `mod` for which:
 
 - `name` is not exported from `mod`
-- `name` is not declared public in `mod` (requires Julia v1.11+)
+- or `name` is not declared public in `mod` (requires Julia v1.11+)
+- or `name` is "self-qualified": i.e. in the module `Foo`, `Foo.name` is being accessed.
 
-The keyword argument `allow_internal_accesses` determines whether or not "internal" qualified accesses to other modules in the same package (or more generally, sharing the same `Base.moduleroot`) are reported here. If `allow_internal_accesses=false`, then even such "internal" qualified accesses will be returned.
+The keyword argument `allow_internal_accesses` determines whether or not "internal" qualified accesses to other modules in the same package (or more generally, sharing the same `Base.moduleroot`) are reported here. If `allow_internal_accesses=false`, then even such "internal" qualified accesses will be returned. Note self-qualified accesses are reported regardless of the setting of `allow_internal_accesses`.
 
 The keyword argument `skip` is expected to be an iterator of `accessing_from => parent` pairs, where names which are accessed from `accessing_from` but who have an ancestor `parent` are ignored. By default, accesses from Base to names owned by Core are skipped.
 
@@ -171,6 +175,7 @@ Returns a nested structure providing information about improper accesses to name
 - `accessing_from_owns_name::Bool`: whether or not `accessing_from` matches `whichmodule` and therefore is considered to directly "own" the name
 - `accessing_from_submodule_owns_name::Bool`: whether or not `whichmodule` is a submodule of `accessing_from`
 - `internal_access::Bool`: whether or not the access is "internal", meaning the module it was accessed in and the module it was accessed from share the same `Base.moduleroot`.
+- `self_qualified::Bool`: whether or not the access is "self-qualified", meaning the module it was accessed in and the module it is accessed from are the same module.
 
 In non-breaking releases of ExplicitImports:
 

src/interactive_usage.jl

@@ -169,13 +169,28 @@ function print_explicit_imports(io::IO, mod::Module, file=pathof(mod);
                                                                    file_analysis=file_analysis[file],
                                                                    allow_internal_accesses)
 
+            self_qualified = filter(row -> row.self_qualified, problematic)
+            if !isempty(self_qualified)
+                println(io)
+                word = !isnothing(imports) && isempty(imports) &&
+                       isempty(problematic_imports_for_stale) ?
+                       "However" : "Additionally"
+                plural = length(self_qualified) > 1 ? "es" : ""
+                println(io,
+                        "$word, $(name_fn(mod)) has $(length(self_qualified)) self-qualified access$plural:")
+                for row in self_qualified
+                    println(io,
+                            "- `$(row.name)` was accessed as `$(mod).$(row.name)` inside $(mod) at $(row.location)")
+                end
+            end
+
             non_owner = filter(row -> !row.accessing_from_submodule_owns_name,
                                problematic)
 
             if !isempty(non_owner)
                 println(io)
                 word = !isnothing(imports) && isempty(imports) &&
-                       isempty(problematic_imports_for_stale) ?
+                       isempty(problematic_imports_for_stale) && isempty(self_qualified) ?
                        "However" : "Additionally"
                 plural = length(non_owner) > 1 ? "s" : ""
                 println(io,
@@ -196,7 +211,8 @@ function print_explicit_imports(io::IO, mod::Module, file=pathof(mod);
                 println(io)
 
                 word = !isnothing(imports) && isempty(imports) &&
-                       isempty(problematic_imports_for_stale) && isempty(non_owner) ?
+                       isempty(problematic_imports_for_stale) && isempty(self_qualified) &&
+                       isempty(non_owner) ?
                        "However" : "Additionally"
                 plural = length(non_public) > 1 ? "s" : ""
                 println(io,

test/runtests.jl

@@ -201,15 +201,17 @@ end
 @testset "qualified access" begin
     # analyze_qualified_names
     qualified = analyze_qualified_names(TestQualifiedAccess, "test_qualified_access.jl")
-    @test length(qualified) == 5
-    ABC, DEF, HIJ, X, map = qualified
+    @test length(qualified) == 6
+    ABC, DEF, HIJ, X, map, x = qualified
     @test ABC.name == :ABC
     @test DEF.public_access
     @test HIJ.public_access
     @test DEF.name == :DEF
     @test HIJ.name == :HIJ
     @test X.name == :X
     @test map.name == :map
+    @test x.name == :x
+    @test x.self_qualified
 
     # improper_qualified_accesses
     ret = Dict(improper_qualified_accesses(TestQualifiedAccess,
@@ -219,8 +221,8 @@ end
     @test isempty(ret[TestQualifiedAccess.FooModule])
     @test !isempty(ret[TestQualifiedAccess])
 
-    @test length(ret[TestQualifiedAccess]) == 3
-    ABC, X, map = ret[TestQualifiedAccess]
+    @test length(ret[TestQualifiedAccess]) == 4
+    ABC, X, map, x = ret[TestQualifiedAccess]
     # Can add keys, but removing them is breaking
     @test keys(ABC) ⊇
           [:name, :location, :value, :accessing_from, :whichmodule, :public_access,
@@ -240,12 +242,35 @@ end
 
     @test map.name == :map
 
+    @test x.name == :x
+    @test x.self_qualified
+
     imps = DataFrame(improper_qualified_accesses_nonrecursive(TestQualifiedAccess,
                                                               "test_qualified_access.jl";
                                                               allow_internal_accesses=true))
+    subset!(imps, :self_qualified => ByRow(!)) # drop self-qualified
     # in this case we rule out all the `Main` ones, so only LinearAlgebra is left:
     @test all(==(LinearAlgebra), imps.accessing_from)
 
+    # check_no_self_qualified_accesses
+    ex = SelfQualifiedAccessException
+    @test_throws ex check_no_self_qualified_accesses(TestQualifiedAccess,
+                                                     "test_qualified_access.jl")
+
+    str = exception_string() do
+        return check_no_self_qualified_accesses(TestQualifiedAccess,
+                                                "test_qualified_access.jl")
+    end
+    @test contains(str, "has self-qualified accesses:\n- `x` was accessed as")
+
+    @test check_no_self_qualified_accesses(TestQualifiedAccess,
+                                           "test_qualified_access.jl"; ignore=(:x,)) ===
+          nothing
+
+    str = sprint(print_explicit_imports, TestQualifiedAccess,
+                 "test_qualified_access.jl")
+    @test contains(str, "has 1 self-qualified access:\n- `x` was accessed as")
+
     # check_all_qualified_accesses_via_owners
     ex = QualifiedAccessesFromNonOwnerException
     @test_throws ex check_all_qualified_accesses_via_owners(TestQualifiedAccess,

test/test_mods.jl

@@ -210,7 +210,6 @@ end
 
 end # TestMod12
 
-
 # https://github.com/ericphanson/ExplicitImports.jl/issues/62
 module TestMod13
 

test/test_qualified_access.jl

@@ -49,4 +49,9 @@ using LinearAlgebra: LinearAlgebra
 
 LinearAlgebra.map
 
+x = 1
+
+# self-qualified access
+TestQualifiedAccess.x
+
 end # TestQualifiedAccess