Invalid JETLS warning, buggy kwdef macro

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?

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.

1 Like

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

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.

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.

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.

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

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).

It is a bug because it generates invalid code for parametric structs. and the docs say: " Base.@kwdef for parametric structs, and structs with supertypes requires at least Julia 1.1."

It works fine for many parametric structs.

julia> @kwdef struct MyComplex{T}
         re::T = 0
         im::T = 0
       end

julia> MyComplex()
MyComplex{Int64}(0, 0)

julia> MyComplex(re=1)
MyComplex{Int64}(1, 0)

The problem is struct parameters in the default expressions.

julia> @kwdef struct BadComplex{T}
         re::T = zero(T)
         im::T = zero(T)
       end

julia> BadComplex()
ERROR: UndefVarError: `T` not defined in `Main`

And again, it’s only pasting the expressions into method default arguments as promised. An input expression that doesn’t work could just be a bad input, not a bug. Until we get clarification on the intent from @kwdef’s developers (which the issue I linked left up for debate), there’s not much we can assert here.

AFAICT, Benny and the previous issue is saying that the @kwdef macro shouldn’t be used in this situation. Would you be comfortable to put the constructor outside the struct def? That would create a bit more code to maintain, however may workaround your problem. I generate this with help of Gemini

mutable struct SysState{P}
    acc::Float64
    "vector of particle positions in x [m]"
    X::MVector{P, Float64}
end

# Outer constructor to handle keyword arguments and default values
function SysState{P}(; acc::Float64 = 0.0, X = zeros(P)) where {P}
    return SysState{P}(acc, X)
end

No, in my real struct I have 20 fields, not just two. And I have many similar structs.

I mean, I am happy using a custom kwdef macro that can handle this case without creating invalid code, but I am not good at writing macros.

This is the original macro:

macro kwdef(expr)
    expr = macroexpand(__module__, expr) # to expand @static
    isexpr(expr, :struct) || error("Invalid usage of @kwdef")
    _, T, fieldsblock = expr.args
    if T isa Expr && T.head === :<:
        T = T.args[1]
    end

    fieldnames = Any[]
    defvals = Any[]
    extract_names_and_defvals_from_kwdef_fieldblock!(fieldsblock, fieldnames, defvals)
    parameters = map(fieldnames, defvals) do fieldname, defval
        if isnothing(defval)
            return fieldname
        else
            return Expr(:kw, fieldname, esc(defval))
        end
    end

    # Only define a constructor if the type has fields, otherwise we'll get a stack
    # overflow on construction
    if !isempty(parameters)
        T_no_esc = Meta.unescape(T)
        if T_no_esc isa Symbol
            sig = Expr(:call, esc(T), Expr(:parameters, parameters...))
            body = Expr(:block, __source__, Expr(:call, esc(T), fieldnames...))
            kwdefs = Expr(:function, sig, body)
        elseif isexpr(T_no_esc, :curly)
            # if T == S{A<:AA,B<:BB}, define two methods
            #   S(...) = ...
            #   S{A,B}(...) where {A<:AA,B<:BB} = ...
            S = T.args[1]
            P = T.args[2:end]
            Q = Any[isexpr(U, :<:) ? U.args[1] : U for U in P]
            SQ = :($S{$(Q...)})
            body1 = Expr(:block, __source__, Expr(:call, esc(S), fieldnames...))
            sig1 = Expr(:call, esc(S), Expr(:parameters, parameters...))
            def1 = Expr(:function, sig1, body1)
            body2 = Expr(:block, __source__, Expr(:call, esc(SQ), fieldnames...))
            sig2 = :($(Expr(:call, esc(SQ), Expr(:parameters, parameters...))) where {$(esc.(P)...)})
            def2 = Expr(:function, sig2, body2)
            kwdefs = Expr(:block, def1, def2)
        else
            error("Invalid usage of @kwdef")
        end
    else
        kwdefs = nothing
    end
    return quote
        $(esc(:($Base.@__doc__ $expr)))
        $kwdefs
    end
end

# @kwdef helper function
# mutates arguments inplace
function extract_names_and_defvals_from_kwdef_fieldblock!(block, names, defvals)
    for (i, item) in pairs(block.args)
        if isexpr(item, :block)
            extract_names_and_defvals_from_kwdef_fieldblock!(item, names, defvals)
        elseif item isa Expr && item.head in (:escape, :var"hygienic-scope")
            n = length(names)
            extract_names_and_defvals_from_kwdef_fieldblock!(item, names, defvals)
            for j in n+1:length(defvals)
                if !isnothing(defvals[j])
                    defvals[j] = Expr(item.head, defvals[j])
                end
            end
        else
            def, name, defval = @something(def_name_defval_from_kwdef_fielddef(item), continue)
            block.args[i] = def
            push!(names, name)
            push!(defvals, defval)
        end
    end
end

function def_name_defval_from_kwdef_fielddef(kwdef)
    if kwdef isa Symbol
        return kwdef, kwdef, nothing
    elseif isexpr(kwdef, :(::))
        name, _ = kwdef.args
        return kwdef, Meta.unescape(name), nothing
    elseif isexpr(kwdef, :(=))
        lhs, rhs = kwdef.args
        def, name, _ = @something(def_name_defval_from_kwdef_fielddef(lhs), return nothing)
        return def, name, rhs
    elseif kwdef isa Expr && kwdef.head in (:const, :atomic)
        def, name, defval = @something(def_name_defval_from_kwdef_fielddef(kwdef.args[1]), return nothing)
        return Expr(kwdef.head, def), name, defval
    elseif kwdef isa Expr && kwdef.head in (:escape, :var"hygienic-scope")
        def, name, defval = @something(def_name_defval_from_kwdef_fielddef(kwdef.args[1]), return nothing)
        return Expr(kwdef.head, def), name, isnothing(defval) ? defval : Expr(kwdef.head, defval)
    end
end

The question is, how to modify it such that it does not create this function:

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

I’ve had similar problems quite often, and I usually solved them in one of two ways:

  1. Give up on Base.@kwdef
  2. Retrieve the type parameter from an earlier field whose default value I already gave, something like
Base.@kwdef mutable struct SysState{P}
    acc::Float64 = 0
    stuff::P
    X::MVector{P, Float64} = zeros(typeof(stuff))
end
1 Like

But stuff has no default value in your example…

And my code works. The only problem is that the macro inserts an invalid, unused function.

UPDATE
I changed my code to:

using Parameters, StaticArrays

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

This expands to:

julia> ex = @macroexpand @with_kw_noshow mutable struct SysState{P}
                  acc::Float64 = 0
                  "vector of particle positions in x [m]"
                  X::Vector{P} = zeros(P)
              end
quote
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:611 =#
    begin
        $(Expr(:meta, :doc))
        mutable struct SysState{P}
            "Default: 0"
            acc::Float64
            "vector of particle positions in x [m] Default: zeros(P)"
            X::Vector{P}
            (SysState{P}(; acc = 0, X = zeros(P)) where P) = begin
                    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:491 =#
                    SysState{P}(acc, X)
                end
            (SysState{P}(acc, X) where P) = begin
                    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:503 =#
                    new{P}(acc, X)
                end
        end
    end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:612 =#
    (SysState(acc, X::Vector{P}) where P) = begin
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:525 =#
            SysState{P}(acc, X)
        end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:613 =#
    SysState(; acc = 0, X = zeros(P)) = begin
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:545 =#
            SysState(acc, X)
        end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:614 =#
    begin
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:569 =#
        SysState(pp::SysState; kws...) = begin
                #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:569 =#
                (Parameters).reconstruct(pp, kws)
            end
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:572 =#
        SysState(pp::SysState, di::(Parameters).AbstractDict) = begin
                #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:572 =#
                (Parameters).reconstruct(pp, di)
            end
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:573 =#
        SysState(pp::SysState, di::Vararg{Tuple{Symbol, Any}}) = begin
                #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:573 =#
                (Parameters).reconstruct(pp, di)
            end
    end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:615 =#
    nothing
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:616 =#
    macro unpack_SysState(ex)
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:616 =#
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:617 =#
        esc((Parameters)._unpack(ex, Any[:acc, :X]))
    end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:619 =#
    begin
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:594 =#
        macro pack_SysState!(ex)
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:594 =#
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:595 =#
            esc((Parameters)._pack_mutable(ex, Any[:acc, :X]))
        end
        #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:597 =#
        macro pack_SysState()
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:597 =#
            #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:598 =#
            esc((Parameters)._pack_new(SysState, Any[:acc, :X]))
        end
    end
    #= /home/ufechner/.julia/packages/Parameters/MK0O4/src/Parameters.jl:620 =#
    SysState
end

Now everything works as expected.

Conclusion: This is a bug (or missing feature) in Base.@kwdef, not present in Parameters.@with_kw_noshow .

I created a new issue: GitHub · Where software is built

Wouldn’t this have the same global P problem at a SysState() call?

julia> using Parameters, StaticArrays

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

julia> SysState()
ERROR: UndefVarError: `P` not defined in `Main`

Yes. But a SysState() call doesn’t make sense and never happens. Correct is a call of SysState{7}() or similar. The issue I had was the JETLS warning, which no longer occurs.

So you didn’t want to change the method generated by @kwdef after all, you just wanted JETLS to ignore it when generated by @with_kw_noshow? If so, that Github issue presenting the generated method as the problem and @with_kw_noshow as a workaround is incredibly misleading, especially on the base Julia repo where people would be more interested in improving the macro than what JETLS chooses to warn.

It does surprise me because I thought JETLS expanded macros and should see the same problematic method signature generated by both macros. If this JETLS warning is being triggered by the kwdef symbol specifically, then you would’ve been able to dodge it with an alias or a forwarding macro, worth a try:

julia> var"@kwdef1" = var"@kwdef" # don't need a const alias for macro expansions
@kwdef (macro with 1 method)

julia> macro kwdef2(args...)
         esc(:(@kwdef $(args...)))
       end
@kwdef2 (macro with 1 method)

Of course, the best solution would be a macro that doesn’t generate a wrong method. I thought that the macro I am using now would do that, but you found out that this is not the case. I will update the issue accordingly.

1 Like