JuliaTesting / Aqua.jl

Auto QUality Assurance for Julia packages
MIT License
339 stars 25 forks source link

False-positive unbound argument for struct constructors with unions #265

Open staticfloat opened 8 months ago

staticfloat commented 8 months ago

If I have a struct with a union type that contains the only instance of a parametric typevar in it, Aqua complains that my struct's constructor has an unbound typevar:

struct Foo{T}
    x::Union{Nothing,Vector{T}}
end

Results in:

Unbound type parameters: Test Failed at /Users/sabae/.julia/packages/Aqua/9p8ck/src/unbound_args.jl:28
  Expression: isempty(unbounds)
   Evaluated: isempty(Method[Example.Foo(x::Union{Nothing, Vector{T}}) where T @ Example ~/.julia/dev/Example/src/Example.jl:20])
topolarity commented 8 months ago

I think Aqua is complaining that Foo(nothing) results in T being unbound (so that the constructor fails), which is true.

You probably need to separate the Foo(x::Vector) and Foo{T}(::Nothing) constructors:

struct Foo{T}
     x::Union{Nothing,Vector{T}}

     Foo(v::Vector{T}) where T = new{T}(v)
     Foo{T}(v::Vector{T}) where T = new{T}(v)
     Foo{T}(::Nothing) where {T} = new{T}(nothing)
 end
staticfloat commented 8 months ago

Hmmm, that's a little annoying that the default constructor that Julia gives you fails this check though. Too bad.

topolarity commented 8 months ago

Agreed - seems like it might be worth an exception in the Aqua ruleset

lgoettgens commented 8 months ago

Using the example from above, I can easily get a real error (see below), so I think that Aqua reports a genuine issue here.

julia> struct Foo{T}
           x::Union{Nothing,Vector{T}}
       end

julia> Foo(nothing)
ERROR: UndefVarError: `T` not defined
Stacktrace:
 [1] Foo(x::Nothing)
   @ Main ./REPL[1]:2
 [2] top-level scope
   @ REPL[2]:1
Tortar commented 7 months ago

Can something be done in Julia itself to improve this?

topolarity commented 7 months ago

Yes, the error is real but the question is whether it's worth reporting.

This gives an UndefVarError, but if the default constructor were changed to "fix" this, it would still give a MethodError. It's not clear to me why Aqua considers one worth reporting, but not the other.

Tortar commented 7 months ago

but couldn't all needed constructors be created so that all cases work out of the box?

lgoettgens commented 7 months ago

but couldn't all needed constructors be created so that all cases work out of the box?

This would be fixed, if julia wouldn't create inner constructors without explicit type parameters at all. In the example in https://github.com/JuliaTesting/Aqua.jl/issues/265#issuecomment-1959078509, I see no way how julia could automatically resolve this. What a user want's can be wastly different based on the context. If, however, you would only have a constructor with type parameters, i.e. call it as Foo{String}(nothing), this would be fine. But that would be a breaking change in the julia language.

gdalle commented 4 months ago

I am having (what I think is) a similar problem with an NTuple wrapper. Here's the code for MyPackage:

module MyPackage

struct MyStruct{N,T}
    elements::NTuple{N,T}
end

end

and here's the error:

julia> Aqua.test_all(MyPackage)
Skipping Base.active_repl
Skipping Base.active_repl_backend
Test Summary:    | Pass  Total  Time
Method ambiguity |    1      1  1.6s
Unbound type parameters detected:
[1] MyPackage.MyStruct(elements::Tuple{Vararg{T, N}}) where {N, T} @ MyPackage /tmp/MyPackage/src/MyPackage.jl:6
Unbound type parameters: Test Failed at /home/gdalle/.julia/packages/Aqua/tHrmY/src/unbound_args.jl:37
  Expression: isempty(unbounds)
   Evaluated: isempty(Method[MyPackage.MyStruct(elements::Tuple{Vararg{T, N}}) where {N, T} @ MyPackage /tmp/MyPackage/src/MyPackage.jl:6])