Skip to content

test_unbound_args: add a functionality to ignore specific methods #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+37 −1
Open

test_unbound_args: add a functionality to ignore specific methods #316

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a9f275a
add ignore keyword for specific methods in test_unbound_args
arnaud-ma Jan 8, 2025
cbe7926
rename 'ignore' keyword to 'exclude' in test_unbound_args to align wi…
arnaud-ma Jan 26, 2025
549620e
handle the case of callable objects in test_unbound_args
arnaud-ma Jan 26, 2025
35b2dc8
remove the test_unbound_args(unbounds) method
arnaud-ma Jan 26, 2025
f30844d
update the doc of test_unbound_args to align with test_ambiguities
arnaud-ma Jan 26, 2025
df69299
use exactly the same signature style as Julia to exclude methods in t…
arnaud-ma Feb 4, 2025
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
16 changes: 15 additions & 1 deletion src/unbound_args.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,23 @@ of the method.
# Keyword Arguments
- `broken::Bool = false`: If true, it uses `@test_broken` instead of
`@test` and shortens the error message.
- `exclude::AbstractVector{Tuple{Base.Callable, DataType...}} = []`: A vector of
signatures of functions or callable to exclude from testing. A signature is given
as a tuple, where the first element is the callable and the rest are
the types of the arguments. For example, to exclude `foo(x::Int, y::Float64)`,
pass `(foo, Int, Float64)`.
"""
function test_unbound_args(m::Module; broken::Bool = false)
function test_unbound_args(m::Module; broken::Bool = false, exclude = [])
unbounds = detect_unbound_args_recursively(m)
for i in exclude
callable, args = i[1], i[2:end] # i[2:end] is empty if length(i) == 1

# the type of the function is the function itself if it is a callable object
callable_t = callable isa Function ? typeof(callable) : callable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I thinks this still isn't quite what we want here.
For custom structs, say struct S end, there are both function signatures containing S and Type{S} (which isn't even quite typeof(S)), since the former is for callable objects and the latter for constructors.

Furthermore, there are subtypes of Function, that are not a singleton type of function xxx, e.g. Base.ComposedFunction.

Maybe you have an idea that makes all of these cases work. If not, I think we just need the user to specify the exact signature, i.e. foo(x::Int, y::Float64) as (typeof(foo), Int, Float64).

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I understand now. Thank you very much for your patience. After some research, I can't find any way to be 100% sure that we are dealing with a "simple" method.

But specifying the exact signature requires some knowledge from the user. To help the user, I think adding what method.sig returns in the error message can be a good idea. For example from this:

Unbound type parameters detected:
[1] f(x) where T @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:15
[2] (::Main.Foo.S{U})(::NTuple{N, T}) where {N, T, U} @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:8
[3] Main.Foo.S(::NTuple{N, T}) where {N, T} @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:11

to this:

Unbound type parameters detected:

[1] f(x) where T @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:15
    signature: Tuple{typeof(Main.Foo.f), Any} where T

[2] (::Main.Foo.S{U})(::NTuple{N, T}) where {N, T, U} @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:8
    signature: Tuple{Main.Foo.S{U}, NTuple{N, T}} where {N, T, U}

[3] Main.Foo.S(::NTuple{N, T}) where {N, T} @ Main.Foo ~/projets/forks/Aqua/Aqua.jl/a.jl:11
    signature: Tuple{Type{Main.Foo.S}, NTuple{N, T}} where {N, T}

What do you think about that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not a fan of requiring 3x of vertical space. Instead, I think we could make a function public that returns the list of methods, so that a user can query this for the signature themselves. But I would put that to a follow-up PR.
For this PR, it would be great if you could do the small adaptions needed for this, and adapt the docstring as well.

exclude_signature = Tuple{callable_t, args...}
filter!(method -> (method.sig != exclude_signature), unbounds)
end

if broken
if !isempty(unbounds)
printstyled(
Expand Down
14 changes: 14 additions & 0 deletions test/pkgs/PkgUnboundArgs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,18 @@ end
# `_totuple` is taken from
# https://github.com/JuliaLang/julia/blob/48634f9f8669e1dc1be0a1589cd5be880c04055a/test/ambiguous.jl#L257-L259

# taken from https://github.com/JuliaTesting/Aqua.jl/issues/86
module Issue86
f(::NTuple{N,T}) where {N,T} = (N, T)
f(::Tuple{}) = (0, Any)
end

module ExcludeCallableObject
struct Callable{U}
s::U
end

(::Callable{U})(::NTuple{N,T}) where {N,T,U} = (N, T, U)
(::Callable{U})(::Tuple{}) where {U} = (0, Any, U)
end
end # module
9 changes: 9 additions & 0 deletions test/test_unbound_args.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ using PkgUnboundArgs
@test length(results) == 1
@test results[1] isa Test.Fail

Aqua.test_unbound_args(
PkgUnboundArgs,
exclude = [
(PkgUnboundArgs.M25341._totuple, Type{Tuple{Vararg{E}}} where {E}, Any, Vararg),
(PkgUnboundArgs.Issue86.f, NTuple),
(PkgUnboundArgs.ExcludeCallableObject.Callable, NTuple),
],
)

# It works with other tests:
Aqua.test_ambiguities(PkgUnboundArgs)
Aqua.test_undefined_exports(PkgUnboundArgs)
Expand Down
Loading