diff --git a/CHANGELOG.md b/CHANGELOG.md index 93a45bb0..009df9fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Fix some world age issues in `test_ambiguities`. ([#366]) +- Symbols as type-parameters do not disable piracy checks in `detect_piracies`. ([#375]) - The minimum supported julia version is increased to 1.6. ([#328]) ## Version [v0.8.14] - 2025-08-04 diff --git a/src/piracies.jl b/src/piracies.jl index d82bd795..a5afa6e5 100644 --- a/src/piracies.jl +++ b/src/piracies.jl @@ -56,36 +56,40 @@ end is_foreign(@nospecialize(x), pkg::Base.PkgId; treat_as_own) = is_foreign(typeof(x), pkg; treat_as_own = treat_as_own) -# Symbols can be used as type params - we assume these are unique and not -# piracy. This implies that we have -# -# julia> Aqua.Piracy.is_foreign(1, Base.PkgId(Aqua)) -# true -# -# julia> Aqua.Piracy.is_foreign(:hello, Base.PkgId(Aqua)) -# false -# -# and thus -# -# julia> Aqua.Piracy.is_foreign(Val{1}, Base.PkgId(Aqua)) -# true -# -# julia> Aqua.Piracy.is_foreign(Val{:hello}, Base.PkgId(Aqua)) -# false -# -# Admittedly, this asymmetry is rather worrisome. We do need to treat 1 foreign -# to consider `Vector{Char}` (i.e., `Array{Char,1}`) foreign. This may suggest -# to treat the `Symbol` type foreign as well. However, it means that we treat -# definition such as -# -# ForeignModule.api_function(::Val{:MyPackageName}) = ... -# -# as a type piracy even if this is actually the intended use-case (which is not -# a crazy API). The symbol name may also come from `gensym`. Since the aim of -# `Aqua.test_piracies` is to detect only "obvious" piracies, let us play on the -# safe side. +# With Symbols, the challenge is to distinguish between uses that were expected +# by the foreign module and which are novel to the package. A pattern like +# `ForeignModule.api_function(::Val{:MyPkg}) = ...` is an accepted dispatch +# idiom and we do not want to flag it as piracy. On the other hand, IntervalSets +# defines the type alias +# const ClosedInterval{T} = Interval{:closed, :closed, T} +# and that should be treated as foreign. + +# Historically all symbols were treated as non-foreign. Preserve that behavior +# for the fallback case: is_foreign(x::Symbol, pkg::Base.PkgId; treat_as_own) = false +# But when symbols appear as type-parameters, check whether the specific symbol +# and enclosing type match a user-defined type alias in the owning module. This +# check is somewhat loose in not insisting on a specific positional index within +# the parameter list: +function is_symbol_param_structural(sym::Symbol, @nospecialize(T::DataType)) + parent_mod = parentmodule(T) + Tname = nameof(T) + for name in names(parent_mod; all = true) + isdefined(parent_mod, name) || continue + Base.isdeprecated(parent_mod, name) && continue + obj = getfield(parent_mod, name) + body = Base.unwrap_unionall(obj) + if isa(body, DataType) && + parentmodule(body) === parent_mod && + nameof(body) === Tname && + sym in body.parameters + return true + end + end + return false +end + is_foreign_module(mod::Module, pkg::Base.PkgId) = Base.PkgId(mod) != pkg function is_foreign(@nospecialize(T::DataType), pkg::Base.PkgId; treat_as_own) @@ -96,10 +100,19 @@ function is_foreign(@nospecialize(T::DataType), pkg::Base.PkgId; treat_as_own) @assert length(params) == 1 return is_foreign(first(params), pkg; treat_as_own = treat_as_own) else - # Both the type itself and all of its parameters must be foreign + # Both the type itself and all of its parameters must be foreign. + # Symbol parameters are handled via is_symbol_param_structural: a + # Symbol is foreign only if the owning module explicitly uses it as + # a type parameter of T (e.g. ClosedInterval = Interval{:closed,…}). return !((C in treat_as_own)::Bool) && is_foreign_module(parentmodule(T), pkg) && - all(param -> is_foreign(param, pkg; treat_as_own = treat_as_own), params) + all(params) do param + if param isa Symbol + is_symbol_param_structural(param, T) + else + is_foreign(param, pkg; treat_as_own = treat_as_own) + end + end end end diff --git a/test/pkgs/PiracyForeignProject/src/PiracyForeignProject.jl b/test/pkgs/PiracyForeignProject/src/PiracyForeignProject.jl index fc2c5c5e..7db7c361 100644 --- a/test/pkgs/PiracyForeignProject/src/PiracyForeignProject.jl +++ b/test/pkgs/PiracyForeignProject/src/PiracyForeignProject.jl @@ -7,4 +7,12 @@ struct ForeignNonSingletonType x::Int end +# Type with a Symbol type parameter plus a type alias that fixes the Symbol. +# ForeignTaggedType tests that dispatching on the alias is detected as piracy +# (the :tag symbol is structural — it lives in this package's type alias), +# while dispatching on ForeignSymbolParamType{:user_symbol, T} directly is not +# (the :user_symbol could be a caller-defined dispatch tag). +struct ForeignSymbolParamType{S, T} end +const ForeignTaggedType{T} = ForeignSymbolParamType{:tag, T} + end diff --git a/test/test_piracy.jl b/test/test_piracy.jl index 2ba30df3..176c7052 100644 --- a/test/test_piracy.jl +++ b/test/test_piracy.jl @@ -2,7 +2,12 @@ push!(LOAD_PATH, joinpath(@__DIR__, "pkgs", "PiracyForeignProject")) baremodule PiracyModule -using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType +using PiracyForeignProject: + ForeignType, + ForeignParameterizedType, + ForeignNonSingletonType, + ForeignSymbolParamType, + ForeignTaggedType using Base: Base, @@ -33,6 +38,9 @@ Base.findlast(::Type{Val{Foo}}, x::Int) = x + 1 Base.findlast(::Tuple{Vararg{Bar{Set{Int}}}}, x::Int) = x + 1 Base.findlast(::Val{:foo}, x::Int) = x + 1 Base.findlast(::ForeignParameterizedType{Foo}, x::Int) = x + 1 +# Not piracy: :caller_tag is not defined in PiracyForeignProject's type aliases, +# so it is treated as a user-defined dispatch tag (like Val{:foo} above) +Base.findlast(::ForeignSymbolParamType{:caller_tag, T}, x::Int) where T = x + 1 # Not piracy const MyUnion = Union{Int,Foo} @@ -58,10 +66,21 @@ 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 +# Piracy: ForeignTaggedType = ForeignSymbolParamType{:tag, T} is a type alias +# defined in PiracyForeignProject, so :tag is structural — not a user-defined +# dispatch tag — and must not suppress piracy detection. +# Compare with Val{:foo} and ForeignSymbolParamType{:caller_tag,T} above. +Base.findlast(::ForeignTaggedType{T}, x::Int) where T = x + 1 + end # PiracyModule using Aqua: Piracy -using PiracyForeignProject: ForeignType, ForeignParameterizedType, ForeignNonSingletonType +using PiracyForeignProject: + ForeignType, + ForeignParameterizedType, + ForeignNonSingletonType, + ForeignSymbolParamType, + ForeignTaggedType # Get all methods - test length meths = filter(Piracy.all_methods(PiracyModule)) do m @@ -73,7 +92,7 @@ end 1 + # Bar constructor 2 + # f 4 + # MyUnion (incl. kwcall) - 6 + # findlast + 8 + # findlast (7 non-piracy + 1 ForeignTaggedType piracy) 3 + # findfirst 1 + # ForeignType callable 1 + # ForeignNonSingletonType callable @@ -97,9 +116,22 @@ pirates = Piracy.hunt(PiracyModule) 3 + # findmax 3 + # findmin 1 + # ForeignType callable - 1 # ForeignNonSingletonType callable + 1 + # ForeignNonSingletonType callable + 1 # findlast on ForeignTaggedType — :tag is structural in PiracyForeignProject @test all(pirates) do m - m.name in [:findfirst, :findmax, :findmin, :ForeignType, :ForeignNonSingletonType] + m.name in [:findfirst, :findmax, :findmin, :ForeignType, :ForeignNonSingletonType, :findlast] +end + +# Specifically verify which findlast is the pirate: the one whose arg2 contains +# :tag (structural — defined in PiracyForeignProject's ForeignTaggedType alias) +# must be piracy, while the one with :caller_tag (user-defined) must not be. +let arg2_params = m -> let sig = Base.unwrap_unionall(m.sig), p2 = sig.parameters[2] + p2 isa DataType ? p2.parameters : () + end + tagged_findlast = filter(m -> m.name === :findlast && :tag in arg2_params(m), meths) + caller_findlast = filter(m -> m.name === :findlast && :caller_tag in arg2_params(m), meths) + @test length(tagged_findlast) == 1 && Piracy.is_pirate(only(tagged_findlast)) + @test length(caller_findlast) == 1 && !Piracy.is_pirate(only(caller_findlast)) end # Test what is pirate (with treat_as_own=[ForeignType]) @@ -107,9 +139,10 @@ pirates = Piracy.hunt(PiracyModule, treat_as_own = [ForeignType]) @test length(pirates) == 3 + # findfirst 3 + # findmin - 1 # ForeignNonSingletonType callable + 1 + # ForeignNonSingletonType callable + 1 # findlast on ForeignSymbolParamType{:tag,T} @test all(pirates) do m - m.name in [:findfirst, :findmin, :ForeignNonSingletonType] + m.name in [:findfirst, :findmin, :ForeignNonSingletonType, :findlast] end # Test what is pirate (with treat_as_own=[ForeignParameterizedType]) @@ -118,9 +151,10 @@ pirates = Piracy.hunt(PiracyModule, treat_as_own = [ForeignParameterizedType]) 3 + # findfirst 3 + # findmax 1 + # ForeignType callable - 1 # ForeignNonSingletonType callable + 1 + # ForeignNonSingletonType callable + 1 # findlast on ForeignSymbolParamType{:tag,T} @test all(pirates) do m - m.name in [:findfirst, :findmax, :ForeignType, :ForeignNonSingletonType] + m.name in [:findfirst, :findmax, :ForeignType, :ForeignNonSingletonType, :findlast] end # Test what is pirate (with treat_as_own=[ForeignType, ForeignParameterizedType]) @@ -130,9 +164,10 @@ pirates = filter( ) @test length(pirates) == 3 + # findfirst - 1 # ForeignNonSingletonType callable + 1 + # ForeignNonSingletonType callable + 1 # findlast on ForeignSymbolParamType{:tag,T} @test all(pirates) do m - m.name in [:findfirst, :ForeignNonSingletonType] + m.name in [:findfirst, :ForeignNonSingletonType, :findlast] end # Test what is pirate (with treat_as_own=[Base.findfirst, Base.findmax]) @@ -140,9 +175,10 @@ pirates = Piracy.hunt(PiracyModule, treat_as_own = [Base.findfirst, Base.findmax @test length(pirates) == 3 + # findmin 1 + # ForeignType callable - 1 # ForeignNonSingletonType callable + 1 + # ForeignNonSingletonType callable + 1 # findlast on ForeignSymbolParamType{:tag,T} @test all(pirates) do m - m.name in [:findmin, :ForeignType, :ForeignNonSingletonType] + m.name in [:findmin, :ForeignType, :ForeignNonSingletonType, :findlast] end # Test what is pirate (excluding a cover of everything) @@ -153,6 +189,7 @@ pirates = filter( ForeignType, ForeignParameterizedType, ForeignNonSingletonType, + ForeignSymbolParamType, Base.findfirst, ], ),