Invalid JETLS warning, buggy kwdef macro

General Usage
, , ,
1

The following code:

Base.@kwdef mutable struct SysState{P}
    acc::Float64 = 0
    "vector of particle positions in x [m]"
    X::MVector{P, Float64} = zeros(P)
end

gives the warning:

`KiteUtils.P` is not defined

I think this is a bug, but the issue I created was closed: Invalid warning about type parameter · Issue #412 · aviatesk/JETLS.jl · GitHub

I don’t understand why. Is there a bug in the Base.@kwdef macro?

It looks like that:

julia> ex = @macroexpand Base.@kwdef mutable struct SysState{P}
           acc::Float64 = 0
           "vector of particle positions in x [m]"
           X::Vector{P} = zeros(P)
       end
quote
    #= util.jl:630 =#
    begin
        $(Expr(:meta, :doc))
        mutable struct SysState{P}
            #= REPL[4]:2 =#
            acc::Float64
            #= REPL[4]:3 =#
            "vector of particle positions in x [m]"
            X::Vector{P}
        end
    end
    #= util.jl:631 =#
    begin
        function SysState(; acc = 0, X = zeros(P))
            #= REPL[4]:1 =#
            SysState(acc, X)
        end
        function SysState{P}(; acc = 0, X = zeros(P)) where P
            #= REPL[4]:1 =#
            SysState{P}(acc, X)
        end
    end
end

Any comments?

I think I get the warning, because SysState() is not defined. But shouldn’t it be allowed to define a type that requires a type parameter?

2

This method is trying to access a global P, unrelated to the P of the SysState{P} struct block. This would be a lot easier to fix if the struct’s field annotations were carried over to the generated methods, but introducing that would be a breaking restriction.

at-kwdef generates incorrect generic constructors for structs with type parameters · Issue #54601 · JuliaLang/julia

This issue also mentions a failure of field type conversion that I’m pretty sure you’d also run into if the first issue were evaded. SysState{P} gets a method with the unannotated positional arguments that tries to convert all the fields, but SysState does not.

3

Thank you for providing a link to this issue in Julia. But is there any known workaround?

4

I didn’t think through my last comment, it would actually make no sense to annotate X there to infer P from a default value with an unknown P. In other words, if you don’t specify P with an explicit parameter or an explicit X value, a SysState call has no reasonable way to infer P. So, function SysState(; acc = 0, X) would be a reasonable fix, no breaking changes needed. Extra annotations could only help if P were inferred from another field annotated with P and its default value were either absent or not using the P typevar, but that’s too weirdly indirect a way to specify P.

The field type conversion problem would still stop this example at acc::Float64 = 0. That’s not an issue with @kwdef anymore, just a question of whether we want more default outer constructors to handle type conversions for parametric types. Maybe the questions are why default constructors are documented very vaguely.

5

To summarize:

  • My code is correct
  • The problem is the Base.@kwdef macro
  • There is no known workaround

Therefore, I created this issue: Feature request: Allow to disable warnings for a single line or a block · Issue #414 · aviatesk/JETLS.jl · GitHub

I would already be happy if I could suppress this warning until a proper fix is found.

6

Arguably not, you’re still attempting to specify an untenable default expression for how @kwdef is designed and documented to work. @kwdef could do the extra work to opt out of default expressions with static parameter symbols, but it could also double down on all symbols in default expressions being global variables or prior field names in the expanded constructors without the parameter. A lack of a possible characteristic doesn’t make it a bug, it could just be a limitation.

On the topic of possible mistakes, your expanded expression has the edit X::Vector{P} = zeros(P), which tries to convert a 0-dimensional array to a vector given P is an element type, not static length.

7

I don’t understand what you mean. If my code were wrong, then someone should be able to suggest a fix. But nobody is suggesting a fix. Therefore, I must assume that my code is correct.

I mean, the code works fine. Do I have to specify a valid range or type for P?

OK, AI suggested:

Base.@kwdef mutable struct SysState{P<:Int}
    acc::Float64 = 0.0
    "vector of particle positions in x [m]"
    X::MVector{P,Float64} = MVector{P,Float64}(zeros(P))
end

A little bit too verbose from my point of view. And it doesn’t help. Still the same warning. So I have to conclude this is a bug in the macro, which is part of Julia Base.

From my point of view, the macro should NOT generate this code if a type parameter is present:

        function SysState(; acc = 0, X = zeros(P))
            #= REPL[4]:1 =#
            SysState(acc, X)
        end
8

SysState() would unconditionally throw the error UndefVarError: `P` not defined, I wouldn’t call that part fine.

If I were in charge of @kwdef, this would also be one of the first things I try to prevent. But @kwdef is plainly documented to do the transform:

The default argument is supplied by declaring fields of the form field::T = default or field = default.

No adjustment for the method and struct’s different scopes or the feasibility of the default expression, basically just a paste from the struct expression to the method header. The macro is doing exactly what it says, so it’s hard to argue it’s a bug. You could argue it should change, but you’d have to make the case for increasing the work the macro does just to prevent a small fraction of runtime errors (at an extreme, it should be possible to specify a call of a function you haven’t even defined yet, so no amount of parsing can prevent all runtime errors).