Enhance Privacy.all_methods

Author: lgoettgens

src/piracy.jl

@@ -1,47 +1,46 @@
 module Piracy
 
-using Test: @test, @test_broken
-using ..Aqua: walkmodules
-
-const DEFAULT_PKGS = (Base.PkgId(Base), Base.PkgId(Core))
-
-function all_methods!(
-    mod::Module,
-    done_callables::Base.IdSet{Any},      # cached to prevent duplicates
-    result::Vector{Method},
-    filter_default::Bool,
-)::Vector{Method}
-    for name in names(mod; all = true, imported = true)
-        # names can list undefined symbols which cannot be eval'd
-        isdefined(mod, name) || continue
-
-        # Skip closures
-        startswith(String(name), "#") && continue
-        val = getfield(mod, name)
-
-        if !in(val, done_callables)
-            # In old versions of Julia, Vararg errors when methods is called on it
-            val === Vararg && continue
-            for method in methods(val)
-                # Default filtering removes all methods defined in DEFAULT_PKGs,
-                # since these may pirate each other.
-                if !(filter_default && in(Base.PkgId(method.module), DEFAULT_PKGS))
-                    push!(result, method)
-                end
-            end
-            push!(done_callables, val)
+import Test
+
+# based on Test/Test.jl#detect_ambiguities
+# https://github.com/JuliaLang/julia/blob/v1.9.1/stdlib/Test/src/Test.jl#L1838-L1896
+function all_methods(mods::Module...; skip_deprecated::Bool)
+    meths = Method[]
+    mods = collect(mods)::Vector{Module}
+
+    function examine(mt::Core.MethodTable)
+        examine(Base.MethodList(mt))
+    end
+    function examine(ml::Base.MethodList)
+        for m in ml
+            Test.is_in_mods(m.module, true, mods) || continue
+            push!(meths, m)
         end
     end
-    result
-end
 
-function all_methods(mod::Module; filter_default::Bool = true)
-    result = Method[]
-    done_callables = Base.IdSet()
-    walkmodules(mod) do mod
-        all_methods!(mod, done_callables, result, filter_default)
+    work = Base.loaded_modules_array()
+    filter!(mod -> mod === parentmodule(mod), work) # some items in loaded_modules_array are not top modules (really just Base)
+    while !isempty(work)
+        mod = pop!(work)
+        for name in names(mod; all = true)
+            (skip_deprecated && Base.isdeprecated(mod, name)) && continue
+            isdefined(mod, name) || continue
+            f = Base.unwrap_unionall(getfield(mod, name))
+            if isa(f, Module) && f !== mod && parentmodule(f) === mod && nameof(f) === name
+                push!(work, f)
+            elseif isa(f, DataType) &&
+                   isdefined(f.name, :mt) &&
+                   parentmodule(f) === mod &&
+                   nameof(f) === name &&
+                   f.name.mt !== Symbol.name.mt &&
+                   f.name.mt !== DataType.name.mt
+                examine(f.name.mt)
+            end
+        end
     end
-    return result
+    examine(Symbol.name.mt)
+    examine(DataType.name.mt)
+    return meths
 end
 
 ##################################
@@ -141,7 +140,7 @@ function is_foreign_method(@nospecialize(T::DataType), pkg::Base.PkgId; treat_as
 
     # fallback to general code
     return !(T in treat_as_own) &&
-           !(T <: Function && T.instance in treat_as_own) &&
+           !(T <: Function && isdefined(T, :instance) && T.instance in treat_as_own) &&
            is_foreign(T, pkg; treat_as_own = treat_as_own)
 end
 
@@ -165,8 +164,8 @@ end
 hunt(mod::Module; from::Module = mod, kwargs...) =
     hunt(Base.PkgId(mod); from = from, kwargs...)
 
-function hunt(pkg::Base.PkgId; from::Module, kwargs...)
-    filter(all_methods(from)) do method
+function hunt(pkg::Base.PkgId; from::Module, skip_deprecated::Bool = true, kwargs...)
+    filter(all_methods(from; skip_deprecated = skip_deprecated)) do method
         Base.PkgId(method.module) === pkg && is_pirate(method; kwargs...)
     end
 end
@@ -182,6 +181,7 @@ See [Julia documentation](https://docs.julialang.org/en/v1/manual/style-guide/#A
 # Keyword Arguments
 - `broken::Bool = false`: If true, it uses `@test_broken` instead of
   `@test`.
+- `skip_deprecated::Bool = true`: If true, it does not check deprecated methods.
 - `treat_as_own = Union{Function, Type}[]`: The types in this container 
   are considered to be "owned" by the module `m`. This is useful for 
   testing packages that deliberately commit some type piracy, e.g. modules 

test/test_piracy.jl

@@ -57,31 +57,27 @@ Base.findmin(::ForeignParameterizedType{Int}, x::Int) = x + 1
 Base.findmin(::Set{Vector{ForeignParameterizedType{Int}}}, x::Int) = x + 1
 Base.findmin(::Union{Foo,ForeignParameterizedType{Int}}, x::Int) = x + 1
 
-# Assign them names in this module so they can be found by all_methods
-a = Base.findfirst
-b = Base.findlast
-c = Base.findmax
-d = Base.findmin
 end # PiracyModule
 
 using Aqua: Piracy
-using PiracyForeignProject: ForeignType, ForeignParameterizedType
+using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType
 
 # Get all methods - test length
-meths = filter(Piracy.all_methods(PiracyModule)) do m
+meths = filter(Piracy.all_methods(PiracyModule; skip_deprecated = true)) do m
     m.module == PiracyModule
 end
 
-# 2 Foo constructors
-# 2 from f
-# 1 from MyUnion
-# 6 from findlast
-# 3 from findfirst
-# 1 from ForeignType
-# 1 from ForeignNonSingletonType
-# 3 from findmax
-# 3 from findmin
-@test length(meths) == 2 + 2 + 1 + 6 + 3 + 1 + 1 + 3 + 3
+@test length(meths) ==
+      2 + # Foo constructors
+      1 + # Bar constructor
+      2 + # f
+      1 + # MyUnion
+      6 + # findlast
+      3 + # findfirst
+      1 + # ForeignType callable
+      1 + # ForeignNonSingletonType callable
+      3 + # findmax
+      3   # findmin
 
 # Test what is foreign
 BasePkg = Base.PkgId(Base)
@@ -95,49 +91,71 @@ ThisPkg = Base.PkgId(PiracyModule)
 
 # Test what is pirate
 pirates = filter(m -> Piracy.is_pirate(m), meths)
-@test length(pirates) == 3 + 3 + 3 + 1 + 1
-@test_broken all(pirates) do m
-    m.name in [:findfirst, :findmax, :findmin]
+@test length(pirates) ==
+      3 + # findfirst
+      3 + # findmax
+      3 + # findmin
+      1 + # ForeignType callable
+      1   # ForeignNonSingletonType callable
+@test all(pirates) do m
+    m.name in [:findfirst, :findmax, :findmin, :ForeignType, :ForeignNonSingletonType]
 end
 
 # Test what is pirate (with treat_as_own=[ForeignType])
 pirates = filter(m -> Piracy.is_pirate(m; treat_as_own = [ForeignType]), meths)
-@test_broken length(pirates) == 3 + 3
-@test_broken all(pirates) do m
-    m.name in [:findfirst, :findmin]
+@test length(pirates) ==
+      3 + # findfirst
+      3 + # findmin
+      1   # ForeignNonSingletonType callable
+@test all(pirates) do m
+    m.name in [:findfirst, :findmin, :ForeignNonSingletonType]
 end
 
 # Test what is pirate (with treat_as_own=[ForeignParameterizedType])
 pirates = filter(m -> Piracy.is_pirate(m; treat_as_own = [ForeignParameterizedType]), meths)
-@test_broken length(pirates) == 3 + 3
-@test_broken all(pirates) do m
-    m.name in [:findfirst, :findmax]
+@test length(pirates) ==
+      3 + # findfirst
+      3 + # findmax
+      1 + # ForeignType callable
+      1   # ForeignNonSingletonType callable
+@test all(pirates) do m
+    m.name in [:findfirst, :findmax, :ForeignType, :ForeignNonSingletonType]
 end
 
 # Test what is pirate (with treat_as_own=[ForeignType, ForeignParameterizedType])
 pirates = filter(
     m -> Piracy.is_pirate(m; treat_as_own = [ForeignType, ForeignParameterizedType]),
     meths,
 )
-@test_broken length(pirates) == 3
-@test_broken all(pirates) do m
-    m.name in [:findfirst]
+@test length(pirates) ==
+      3 + # findfirst
+      1   # ForeignNonSingletonType callable
+@test all(pirates) do m
+    m.name in [:findfirst, :ForeignNonSingletonType]
 end
 
 # Test what is pirate (with treat_as_own=[Base.findfirst, Base.findmax])
 pirates =
     filter(m -> Piracy.is_pirate(m; treat_as_own = [Base.findfirst, Base.findmax]), meths)
-@test_broken length(pirates) == 3
-@test_broken all(pirates) do m
-    m.name in [:findmin]
+@test length(pirates) ==
+      3 + # findmin
+      1 + # ForeignType callable
+      1   # ForeignNonSingletonType callable
+@test all(pirates) do m
+    m.name in [:findmin, :ForeignType, :ForeignNonSingletonType]
 end
 
 # Test what is pirate (excluding a cover of everything)
 pirates = filter(
     m -> Piracy.is_pirate(
         m;
-        treat_as_own = [ForeignType, ForeignParameterizedType, Base.findfirst],
+        treat_as_own = [
+            ForeignType,
+            ForeignParameterizedType,
+            ForeignNonSingletonType,
+            Base.findfirst,
+        ],
     ),
     meths,
 )
-@test_broken length(pirates) == 0
+@test length(pirates) == 0