Skip to content
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.

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

Test.detect_unbound_args reports cases that are reasonable and harmless #56698

Open
LilithHafner opened this issue Nov 27, 2024 · 4 comments
Open
Labels
testsystem The unit testing framework and Test stdlib types and dispatch Types, subtyping and method dispatch

Comments

@LilithHafner
Copy link
Member

If the unbound static parameter is not used in the function body, then it should not get reported by detect_unbound_args. There are perfectly valid functions that don't use their static parameters. For example:

julia> module Mod
           f(x::NTuple{<:Any, T}, y::NTuple{<:Any, T}) where T = (x..., y...)
       end
Main.Mod

julia> Test.detect_unbound_args(Mod)
[1] f(x::NTuple{var"#s5", T} where var"#s5", y::NTuple{var"#s4", T} where var"#s4") where T @ Main.Mod REPL[14]:2

julia> Mod.f((1,2),(3,4))
(1, 2, 3, 4)

julia> Mod.f((1,2),(3,4.0))
ERROR: MethodError: no method matching f(::Tuple{Int64, Int64}, ::Tuple{Int64, Float64})
The function `f` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  f(::NTuple{var"#s5", T} where var"#s5", ::NTuple{var"#s4", T} where var"#s4") where T
   @ Main.Mod REPL[14]:2

Stacktrace:
 [1] top-level scope
   @ REPL[17]:1

julia> Mod.f((),())
()

This causes spurious test failures in #54494

@LilithHafner LilithHafner added testsystem The unit testing framework and Test stdlib types and dispatch Types, subtyping and method dispatch labels Nov 27, 2024
LilithHafner added a commit that referenced this issue Nov 27, 2024
@lgoettgens
Copy link
Contributor

I think in your example above, the problematic input is Mod.f((), ()), as in this case the type variable T is unbounded (as the empty tuple is a valid instance of NTuple{0, T} for every type T)

@LilithHafner
Copy link
Member Author

Yes, Mod.f((),()) is the purported problem. But when we run it, we can see that it's not a problem because T is not used in the function body.

@vtjnash
Copy link
Member

vtjnash commented Nov 30, 2024

As you did in that issue, note that it is considered completely acceptable to add cases above the @test_broken need_to_handle_undef_sparam, as long as you can argue that they are reasonable and harmless, like this case. This could probably be detected since there are no uses of the value in the body, while many other similar cases can be rewritten with the where T on the inside like f(::() where T), this case does not share that simple solutino

@LilithHafner
Copy link
Member Author

Unfortunately, in the sort case, probably due to kwargs, this produces three cases, all of which contain compiler generated names. That makes it hard to filter them out in test/ambiguous.jl:

x@x:~$ julia +pr54494
A new version of Julia for the `pr54494` channel is available. Run:

  juliaup update

to install the latest Julia for the `pr54494` channel.
  o  | Version 1.12.0-DEV.1704 (2024-11-26)
 o o | lh/sort-tuple/5001cf98200 (fork: 11 commits, 3 days)
julia> using Test

julia> Test.detect_unbound_args(Base.Sort)
[1] var"#sort#25"(alg::Base.Sort.Algorithm, lt, by, rev::Union{Nothing, Bool}, order::Base.Order.Ordering, scratch::Union{Nothing, Vector{T}}, ::typeof(sort), x::NTuple{var"#s697", T} where var"#s697") where T @ Base.Sort sort.jl:1762
[2] sort(x::NTuple{var"#s696", T} where var"#s696"; alg, lt, by, rev, order, scratch) where T @ Base.Sort sort.jl:1762
[3] kwcall(::NamedTuple, ::typeof(sort), x::NTuple{var"#s695", T} where var"#s695") where T @ Base.Sort sort.jl:1762

This could probably be detected since there are no uses of the value in the body

That would be lovely, especially considering this behavior used throughout the package ecosystem via Aqua.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

3 participants