Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 19 additions & 33 deletions src/ambiguities.jl
Original file line number Diff line number Diff line change
Expand Up @@ -154,37 +154,6 @@ function reprpkgid(pkg::PkgId)
return "Base.PkgId(Base.UUID($(repr(uuid.value))), $(repr(name)))"
end

struct _NoValue end

function getobj(m::Method)
signature = Base.unwrap_unionall(m.sig)
ty = if is_kwcall(signature)
signature.parameters[3]
else
signature.parameters[1]
end
ty = Base.unwrap_unionall(ty)
if ty <: Function
try
return ty.instance # this should work for functions
catch
end
end
try
if ty.name.wrapper === Type
return ty.parameters[1]
else
return ty.name.wrapper
end
catch err
@error(
"Failed to obtain a function from `Method`.",
exception = (err, catch_backtrace())
)
end
return _NoValue()
end

function test_ambiguities_impl(
packages::Vector{PkgId},
options::NamedTuple,
Expand All @@ -196,9 +165,26 @@ function test_ambiguities_impl(
ambiguities = detect_ambiguities(modules...; options...)

if !isempty(exspecs)
exclude_objs = getobj.(exspecs)
exclude_ft = Any[getobj(spec) for spec in exspecs]
exclude_sig = Any[]
for ft in exclude_ft
if ft isa Type
push!(exclude_sig, Tuple{ft, Vararg})
push!(exclude_sig, Tuple{Core.kwftype(ft), Any, ft, Vararg})
ft = Type{<:ft} # alternatively, Type{ft}
else
ft = typeof(ft)
end
push!(exclude_sig, Tuple{ft, Vararg})
push!(exclude_sig, Tuple{Core.kwftype(ft), Any, ft, Vararg})
end
ambiguities = filter(ambiguities) do (m1, m2)
getobj(m1) ∉ exclude_objs && getobj(m2) ∉ exclude_objs
for excl in exclude_sig
if m1.sig <: excl || m2.sig <: excl
return false
end
end
return true
end
end

Expand Down
2 changes: 1 addition & 1 deletion src/piracies.jl
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ function is_pirate(meth::Method; treat_as_own = Union{Function,Type}[])
signature = Base.unwrap_unionall(meth.sig)

function_type_index = 1
if is_kwcall(signature)
if is_kwcall(meth.sig)
# kwcall is a special case, since it is not a real function
# but a wrapper around a function, the third parameter is the original
# function, its positional arguments follow.
Expand Down
5 changes: 3 additions & 2 deletions src/utils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ function checked_repr(obj)
return code
end

function is_kwcall(signature::DataType)
function is_kwcall(signature::Type)
@static if VERSION < v"1.9"
signature = Base.unwrap_unionall(signature)::DataType
try
length(signature.parameters) >= 3 || return false
signature <: Tuple{Function,Any,Any,Vararg} || return false
Expand All @@ -69,6 +70,6 @@ function is_kwcall(signature::DataType)
return false
end
else
return signature.parameters[1] === typeof(Core.kwcall)
return signature <: Tuple{typeof(Core.kwcall), Any, Any, Vararg}
end
end
7 changes: 6 additions & 1 deletion test/test_ambiguities.jl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ include("preamble.jl")
if broken
@test_broken num_ambiguities_ == num_ambiguities
else
if num_ambiguities_ != num_ambiguities
@show exclude
println(strout)
println(strerr)
end
Comment on lines +27 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to the rest of the changes in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just debugging for the relevant tests if they change reporting

@test num_ambiguities_ == num_ambiguities
end
end
Expand All @@ -50,7 +55,7 @@ include("preamble.jl")
check_testcase([PkgWithAmbiguities.ConcreteType], total - num_ambs_ConcreteType)

# exclude abstract supertype without callables and constructors
check_testcase([PkgWithAmbiguities.AbstractType], total)
check_testcase([PkgWithAmbiguities.AbstractType], total - num_ambs_SingletonType - num_ambs_ConcreteType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an unwanted behavioral change.
If I exclude AbstractType, according to the docstring, this excludes ambiguities of the constructor AbstractType and the functor (::AbstractType).
However, after this change, it also ignores ambiguities of the constructor SingletonType (which IMO is a completely different function) and the functor (::SingletonType) (which shares the methodtable with (::AbstractType) for each SingletonType object created).
The former change seems clearly unwanted to me, for the latter one I am not sure what to do best here. If possible, I think I would like to keep the current behavior.

Note, however, that with the current behavior, excluding ConcreteParameterizedType also excludes all constructors ConcreteParameterizedType{T} for any T.

Since we already have a breaking change in master, it would be fine to slightly change the semantics of exclusion here, but that would need some good explanation of the new semantics in the docstring of test_ambiguities and in the changelog.

Copy link
Contributor Author

@vtjnash vtjnash May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've switched it back to doing only structural inspection instead of the subtyping queries and just simplified them to stay in the type domain where the comparisons can be made more reliable. It is less powerful (since it cannot realize that SingletonType is an implementation of AbstractType) but that keeps the existing named-based exclusion behavior.


@static if VERSION >= v"1.3-"
# for ambiguities between abstract and concrete type callables, only one needs to be excluded
Expand Down
20 changes: 0 additions & 20 deletions test/test_getobj.jl

This file was deleted.

Loading