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

@kwdef fails on structs with supertypes #29307

Closed
arthurp opened this issue Sep 21, 2018 · 1 comment · Fixed by #29316
Closed

@kwdef fails on structs with supertypes #29307

arthurp opened this issue Sep 21, 2018 · 1 comment · Fixed by #29316

Comments

@arthurp
Copy link

arthurp commented Sep 21, 2018

This is best explained with an example:

julia> abstract type Super end

julia> Base.@kwdef struct S <: Super; v; end
ERROR: syntax: invalid function name "S <: Super"

julia> @macroexpand Base.@kwdef struct S <: Super; v; end
quote
    #= util.jl:660 =#
    begin
        $(Expr(:meta, :doc))
        struct S1 <: Super
            #= none:1 =#
            v
        end
    end
    (S1 <: Super)(; v) = (S1 <: Super)(v)
end

@kwdef is blindly taking the first argument of the struct Expr and using as the function name. But the first argument includes the supertype when it is present. This means the function name includes the supertype annotation which is invalid.

I think the fix for this is simple and safe: Check for the case where the struct "name" is not a Symbol and further deconstruct it to get the real struct name. Patch below.

From 22f0ccfe0e97ad157aca6a917ccc2c299a1329c8 Mon Sep 17 00:00:00 2001
From: Arthur Peters <amp@singingwizard.org>
Date: Fri, 21 Sep 2018 12:32:24 -0500
Subject: [PATCH] Fix invalid code generation in @kwdef when struct has
 supertype.

---
 base/util.jl | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/base/util.jl b/base/util.jl
index 80c7907..d27dc03 100644
--- a/base/util.jl
+++ b/base/util.jl
@@ -652,7 +652,12 @@ Stacktrace:
 """
 macro kwdef(expr)
     expr = macroexpand(__module__, expr) # to expand @static
-    T = expr.args[2]
+    if expr.args[2] isa Symbol
+        T = expr.args[2]
+    else
+        @assert expr.args[2].head == :<:
+        T = expr.args[2].args[1]
+    end
     params_ex = Expr(:parameters)
     call_ex = Expr(:call, T)
     _kwdef!(expr.args[3], params_ex, call_ex)
-- 
2.17.1

(I explicitly release my copy rights, if any, to this patch, putting my contribution to this patch in the public domain.)

@simonbyrne
Copy link
Contributor

Thanks for the issue and the patch. It occurs to me we don't handle parametric types very well either: I'll open pull request.

simonbyrne added a commit that referenced this issue Oct 5, 2018
* at-kwdef support for parametric types and subtypes

Fixes #29307.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants