From 1bbbb46c5167cb7b114fc8da826e0c8e8e108bad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mos=C3=A8=20Giordano?= Date: Mon, 18 May 2026 17:29:45 +0200 Subject: [PATCH 1/3] Make `Aqua.test_piracy` recursive --- src/piracies.jl | 36 +++++++++++++++++++++++++++++++++--- test/test_piracy.jl | 16 +++++++++++----- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/piracies.jl b/src/piracies.jl index d82bd795..e0ac2d65 100644 --- a/src/piracies.jl +++ b/src/piracies.jl @@ -4,6 +4,30 @@ using ..Aqua: is_kwcall using Test: is_in_mods +# Helper function to find all submodules of the given module +function walk_submodules!(result, visited, mod::Module) + for name in sort(names(mod; all=true, imported=false)) + isdefined(mod, name) || continue + value = getproperty(mod, name) + if value isa Module && + parentmodule(value) === mod && + !(value in visited) && + value !== mod + + push!(visited, value) + push!(result, value) + walk_submodules!(result, visited, value) + end + end +end +function get_submodules(mod::Module) + result = Module[mod] + visited = Set{Module}() + + walk_submodules!(result, visited, mod) + return result +end + # 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 = true) @@ -182,9 +206,14 @@ function is_pirate(meth::Method; treat_as_own = Union{Function,Type}[]) ) end -function hunt(mod::Module; skip_deprecated::Bool = true, kwargs...) - piracies = filter(all_methods(mod; skip_deprecated = skip_deprecated)) do method - method.module === mod && is_pirate(method; kwargs...) +function hunt(mod::Module; skip_deprecated::Bool = true, recursive::Bool = true, kwargs...) + mods = recursive ? get_submodules(mod) : [mod] + piracies = Method[] + for mod in mods + append!(piracies, + filter(all_methods(mod; skip_deprecated = skip_deprecated)) do method + method.module === mod && is_pirate(method; kwargs...) + end) end sort!(piracies, by = (m -> m.name)) return piracies @@ -200,6 +229,7 @@ Test that `m` does not commit type piracies. # Keyword Arguments - `broken::Bool = false`: If true, it uses `@test_broken` instead of `@test` and shortens the error message. +- `recursive::Bool = true`: If true, also looks for type piracies in all submodules of `m`. - `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 diff --git a/test/test_piracy.jl b/test/test_piracy.jl index 2ba30df3..30ba709e 100644 --- a/test/test_piracy.jl +++ b/test/test_piracy.jl @@ -1,5 +1,6 @@ push!(LOAD_PATH, joinpath(@__DIR__, "pkgs", "PiracyForeignProject")) +baremodule OuterPiracyModule baremodule PiracyModule using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType @@ -59,10 +60,13 @@ Base.findmin(::Set{Vector{ForeignParameterizedType{Int}}}, x::Int) = x + 1 Base.findmin(::Union{Foo,ForeignParameterizedType{Int}}, x::Int) = x + 1 end # PiracyModule +end # OuterPiracyModule using Aqua: Piracy using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType +const PiracyModule = OuterPiracyModule.PiracyModule + # Get all methods - test length meths = filter(Piracy.all_methods(PiracyModule)) do m m.module == PiracyModule @@ -83,7 +87,6 @@ end # Test what is foreign BasePkg = Base.PkgId(Base) CorePkg = Base.PkgId(Core) -ThisPkg = Base.PkgId(PiracyModule) @test Piracy.is_foreign(Int, BasePkg; treat_as_own = []) # from Core @test !Piracy.is_foreign(Int, CorePkg; treat_as_own = []) # from Core @@ -91,7 +94,7 @@ ThisPkg = Base.PkgId(PiracyModule) @test !Piracy.is_foreign(Set{Int}, CorePkg; treat_as_own = []) # Test what is pirate -pirates = Piracy.hunt(PiracyModule) +pirates = Piracy.hunt(OuterPiracyModule; recursive=true) @test length(pirates) == 3 + # findfirst 3 + # findmax @@ -103,7 +106,7 @@ pirates = Piracy.hunt(PiracyModule) end # Test what is pirate (with treat_as_own=[ForeignType]) -pirates = Piracy.hunt(PiracyModule, treat_as_own = [ForeignType]) +pirates = Piracy.hunt(OuterPiracyModule, treat_as_own = [ForeignType]) @test length(pirates) == 3 + # findfirst 3 + # findmin @@ -113,7 +116,7 @@ pirates = Piracy.hunt(PiracyModule, treat_as_own = [ForeignType]) end # Test what is pirate (with treat_as_own=[ForeignParameterizedType]) -pirates = Piracy.hunt(PiracyModule, treat_as_own = [ForeignParameterizedType]) +pirates = Piracy.hunt(OuterPiracyModule, treat_as_own = [ForeignParameterizedType]) @test length(pirates) == 3 + # findfirst 3 + # findmax @@ -136,7 +139,7 @@ pirates = filter( end # Test what is pirate (with treat_as_own=[Base.findfirst, Base.findmax]) -pirates = Piracy.hunt(PiracyModule, treat_as_own = [Base.findfirst, Base.findmax]) +pirates = Piracy.hunt(OuterPiracyModule, treat_as_own = [Base.findfirst, Base.findmax]) @test length(pirates) == 3 + # findmin 1 + # ForeignType callable @@ -159,3 +162,6 @@ pirates = filter( meths, ) @test length(pirates) == 0 + +# No piracy is found in the outer module when `recursive=false`. +@test isempty(Piracy.hunt(OuterPiracyModule; recursive=false)) From d29c189a33e5e9fb47f957b45b3ef80312a8499c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mos=C3=A8=20Giordano?= Date: Tue, 19 May 2026 16:44:12 +0200 Subject: [PATCH 2/3] Add more tests for recursive piracy --- test/test_piracy.jl | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/test_piracy.jl b/test/test_piracy.jl index 30ba709e..9abb5742 100644 --- a/test/test_piracy.jl +++ b/test/test_piracy.jl @@ -60,6 +60,12 @@ Base.findmin(::Set{Vector{ForeignParameterizedType{Int}}}, x::Int) = x + 1 Base.findmin(::Union{Foo,ForeignParameterizedType{Int}}, x::Int) = x + 1 end # PiracyModule + +baremodule NonPiracyModule +baremodule InnerModule +end # InnerModule +end # NonPiracyModule + end # OuterPiracyModule using Aqua: Piracy @@ -93,6 +99,10 @@ CorePkg = Base.PkgId(Core) @test !Piracy.is_foreign(Set{Int}, BasePkg; treat_as_own = []) @test !Piracy.is_foreign(Set{Int}, CorePkg; treat_as_own = []) +# Check that `Piracy.get_submodules` finds all submodules. +@test Set(Piracy.get_submodules(OuterPiracyModule)) == + Set([OuterPiracyModule, OuterPiracyModule.PiracyModule, OuterPiracyModule.NonPiracyModule, OuterPiracyModule.NonPiracyModule.InnerModule]) + # Test what is pirate pirates = Piracy.hunt(OuterPiracyModule; recursive=true) @test length(pirates) == @@ -165,3 +175,5 @@ pirates = filter( # No piracy is found in the outer module when `recursive=false`. @test isempty(Piracy.hunt(OuterPiracyModule; recursive=false)) +# There's no piracy inside `NonPiracyModule`, not even recursively +@test isempty(Piracy.hunt(OuterPiracyModule.NonPiracyModule; recursive=true)) From bb5ae5c8bc66806983c62333c09c759dd54912d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mos=C3=A8=20Giordano?= Date: Wed, 10 Jun 2026 20:29:13 +0100 Subject: [PATCH 3/3] Reuse existing `walkmodules` function, but drop `recursive` argument --- src/piracies.jl | 36 +++++------------------------------- test/test_piracy.jl | 18 ++++++++++-------- 2 files changed, 15 insertions(+), 39 deletions(-) diff --git a/src/piracies.jl b/src/piracies.jl index 572080fc..51ffe42f 100644 --- a/src/piracies.jl +++ b/src/piracies.jl @@ -1,33 +1,9 @@ module Piracy -using ..Aqua: is_kwcall, isType, type_parameter +using ..Aqua: is_kwcall, isType, type_parameter, walkmodules using Test: is_in_mods -# Helper function to find all submodules of the given module -function walk_submodules!(result, visited, mod::Module) - for name in sort(names(mod; all=true, imported=false)) - isdefined(mod, name) || continue - value = getproperty(mod, name) - if value isa Module && - parentmodule(value) === mod && - !(value in visited) && - value !== mod - - push!(visited, value) - push!(result, value) - walk_submodules!(result, visited, value) - end - end -end -function get_submodules(mod::Module) - result = Module[mod] - visited = Set{Module}() - - walk_submodules!(result, visited, mod) - return result -end - # 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 = true) @@ -205,13 +181,12 @@ function is_pirate(meth::Method; treat_as_own = Union{Function,Type}[]) ) end -function hunt(mod::Module; skip_deprecated::Bool = true, recursive::Bool = true, kwargs...) - mods = recursive ? get_submodules(mod) : [mod] +function hunt(mod::Module; skip_deprecated::Bool = true, kwargs...) piracies = Method[] - for mod in mods + walkmodules(mod) do m append!(piracies, - filter(all_methods(mod; skip_deprecated = skip_deprecated)) do method - method.module === mod && is_pirate(method; kwargs...) + filter(all_methods(m; skip_deprecated = skip_deprecated)) do method + method.module === m && is_pirate(method; kwargs...) end) end sort!(piracies, by = (m -> m.name)) @@ -228,7 +203,6 @@ Test that `m` does not commit type piracies. # Keyword Arguments - `broken::Bool = false`: If true, it uses `@test_broken` instead of `@test` and shortens the error message. -- `recursive::Bool = true`: If true, also looks for type piracies in all submodules of `m`. - `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 diff --git a/test/test_piracy.jl b/test/test_piracy.jl index 9abb5742..bf131ce8 100644 --- a/test/test_piracy.jl +++ b/test/test_piracy.jl @@ -68,7 +68,7 @@ end # NonPiracyModule end # OuterPiracyModule -using Aqua: Piracy +using Aqua: Aqua, Piracy using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType const PiracyModule = OuterPiracyModule.PiracyModule @@ -99,12 +99,16 @@ CorePkg = Base.PkgId(Core) @test !Piracy.is_foreign(Set{Int}, BasePkg; treat_as_own = []) @test !Piracy.is_foreign(Set{Int}, CorePkg; treat_as_own = []) -# Check that `Piracy.get_submodules` finds all submodules. -@test Set(Piracy.get_submodules(OuterPiracyModule)) == +# Check that `Aqua.walkmodules` finds all submodules. +all_mods = Module[] +Aqua.walkmodules(OuterPiracyModule) do x + push!(all_mods, x) +end +@test Set(all_mods) == Set([OuterPiracyModule, OuterPiracyModule.PiracyModule, OuterPiracyModule.NonPiracyModule, OuterPiracyModule.NonPiracyModule.InnerModule]) # Test what is pirate -pirates = Piracy.hunt(OuterPiracyModule; recursive=true) +pirates = Piracy.hunt(OuterPiracyModule) @test length(pirates) == 3 + # findfirst 3 + # findmax @@ -173,7 +177,5 @@ pirates = filter( ) @test length(pirates) == 0 -# No piracy is found in the outer module when `recursive=false`. -@test isempty(Piracy.hunt(OuterPiracyModule; recursive=false)) -# There's no piracy inside `NonPiracyModule`, not even recursively -@test isempty(Piracy.hunt(OuterPiracyModule.NonPiracyModule; recursive=true)) +# There's no piracy inside `NonPiracyModule` +@test isempty(Piracy.hunt(OuterPiracyModule.NonPiracyModule))