-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Mutually recursive field types, the easy version #32658
base: master
Are you sure you want to change the base?
Conversation
0507f5e
to
bd7129f
Compare
nice - I knew the day would come |
It is still possible to do the union thing via the following, right?
Is there any type (including data layout) that is inexpressible in this simplified version and would be expressible in the other? |
Yes that works (modulo bugs). The thing you can do is later decide that a forward declared name should be a Union. |
I think @chethega's point is a good one. The problem with |
Right, if you think of |
I think having the explicit |
This is an alternative to #32581, that is easier to implement, but more restrictivive. Here, the forward declaration must contain everything except for the field types: ``` incomplete type Foo{Bar} <: Baz; end ``` with it being an error to deviate from this specification when completing the type ``` struct Foo <: Baz; end # Error: Missing type parameter struct Foo{A, B} <: Baz; end # Error: Too many type parameters struct Foo{Qux} <: Baz; end # Error: Name of type parameter doesn't match struct Foo{Bar<:Int64} <: Baz; end # Error: Bounds of type parameter don't match struct Foo{Bar}; end; # Error supertype dosesn't match ``` This is easier because this way we have enough information for subtyping and type application and therefor do not need to delay this until the entire dependency graph is complete as we did in #32581. Of course this also means that we don't get the union feature that was requested in #269: ``` inomplete type U; end struct A; x::U; end struct B; x::U; end U = Union{A, B}; #ERROR ``` However, it could of course be emulated by wrapping the union type: ``` struct U data::Union{A, B} end ``` However, given the simplicity of this change and the difficulty of #32581, this seems like the way to go for the moment. We may want to revisit all this if we ever want to do computed field types, which will encounter similar challenges as #32581. Fixes #269.
I've tested this a little bit, but I think at this point the biggest thing this is missing is people trying it and breaking things. I'm not currently aware of any issues, but I'm sure there are some, so please test drive this. |
struct IncompleteTypeException | ||
dt::DataType | ||
end | ||
must_be_complete(t::DataType) = (t.incomplete && throw(IncompleteTypeException(t)); t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem like this should need a custom type (which should be subtype Exception if it was kept). Just error("informative message here about ", dt.name.name)
|
||
""" | ||
isbits(x) | ||
|
||
Return `true` if `x` is an instance of an `isbitstype` type. | ||
""" | ||
isbits(@nospecialize x) = (@_pure_meta; typeof(x).isbitstype) | ||
isbits(@nospecialize x) = typeof(x).isbitstype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inference can do this now? yay! but still, this might have performance implications as we may now infer and store a much larger set of methods.
but also, seems unrelated?
@@ -794,6 +794,8 @@ static jl_value_t *get_fieldtype(jl_value_t *t, jl_value_t *f, int dothrow) | |||
if (!jl_is_datatype(t)) | |||
jl_type_error("fieldtype", (jl_value_t*)jl_datatype_type, t); | |||
jl_datatype_t *st = (jl_datatype_t*)t; | |||
if (st->incomplete) | |||
jl_error("Type is incomplete"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jl_error("Type is incomplete"); | |
jl_error("fieldtype: this DataType's fields are incomplete"); |
(or something even longer and clearer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe: incomplete type `$name` declared but never defined
?
dt->mutabl = args[5]==jl_true ? 1 : 0; | ||
dt->name->names = (jl_svec_t*)temp; | ||
jl_gc_wb(dt->name, dt->name->names); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else { | |
} | |
else { |
@@ -1921,6 +1935,7 @@ void jl_init_types(void) JL_GC_DISABLED | |||
jl_true = jl_permbox8(jl_bool_type, 1); | |||
|
|||
jl_abstractstring_type = jl_new_abstracttype((jl_value_t*)jl_symbol("AbstractString"), core, jl_any_type, jl_emptysvec); | |||
jl_abstractstring_type->incomplete = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't jl_new_abstracttype
be setting incomplete = 0
always (since you can never complete an abstract type).
static void check_type_completion(jl_datatype_t *bdt, jl_value_t *super, jl_value_t *para) | ||
{ | ||
// Check that supertype matches | ||
if (!jl_types_equal((jl_value_t*)bdt->super, super)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, super
needs a rewrap_unionall to be used as a Type
if (v->name != w->name) | ||
jl_errorf("Name `%s` of type parameter does not match name `%s` from forward declaration.", | ||
jl_symbol_name(v->name), jl_symbol_name(w->name)); | ||
else if (!jl_types_equal(v->lb, w->lb) || !jl_types_equal(v->ub, w->ub)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lb
and ub
cannot be passed to jl_types_equal
directly (they might themselvers be TypeVar)
Could we pose this whole loop as a simple question to the type-system? jl_types_equal(bdt->name->wrapper, jl_apply_type(bdt->name->wrapper, jl_svec_data(para), jl_svec_len(para))
0, args[5]==jl_true ? 1 : 0, jl_unbox_long(args[6])); | ||
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1); | ||
jl_datatype_t *bdt = NULL; | ||
if (b->value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (b->value) | |
if (b->value && b->constp) |
assert(jl_is_symbol(name)); | ||
assert(jl_is_svec(para)); | ||
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1); | ||
if (b->value && jl_is_datatype(jl_unwrap_unionall(b->value))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (b->value && jl_is_datatype(jl_unwrap_unionall(b->value))) { | |
if (b->value && b->constp && jl_is_datatype(jl_unwrap_unionall(b->value))) { |
dt = jl_new_datatype((jl_sym_t*)name, modu, (jl_datatype_t*)super, | ||
(jl_svec_t*)para, NULL, NULL, 0, 0, 0); | ||
w = dt->name->wrapper; | ||
temp = b->value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is NULL or the assignment later will fail, per the check a few lines earlier to ensure the result is guaranteed to be compatible with equiv_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout algorithm (jl_compute_field_offsets, specifically references_name
) needs to be updated to take into account the new fact that "definition is self-referential" now might include any type that was incomplete at the time of definition. This currently computes a stable fixed-point for storage inlining (a result we don't currently use much, but will become much more significant once we gain the ability to handle inlining references) based on the ability to define mutually-referential types (which was previously limited to just self-references).
incomplete type Bar <: Foo2; end | ||
end) | ||
|
||
incomplete type FieldNamesTest; end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit concerned about what my happen in inference and/or codegen attempts here to speculate on the contents of dt->name->names
for this concrete type (e.g. nfields). Just looking at fieldcount_noerror
briefly, I see it assumes that dt->name->names
is required to always be assigned and available.
What's the status of this PR? It would be great if Julia could have this feature in v1.5. |
An enthusiastic JuliaCon 2020 bump here! Can we include this as a 1.6 release feature? |
The con of this is that this will split out def code in two parts : the params types and the containment ones.
Similar issues will raise - what (and when) is undef, |
This is an alternative to #32581, that is easier to implement, but
more restrictivive. Here, the forward declaration must contain
everything except for the field types:
with it being an error to deviate from this specification when
completing the type
This is easier because this way we have enough information for subtyping
and type application and therefor do not need to delay this until
the entire dependency graph is complete as we did in #32581.
Of course this also means that we don't get the union feature that
was requested in #269:
However, it could of course be emulated by wrapping the union type:
However, given the simplicity of this change and the difficulty of #32581,
this seems like the way to go for the moment. We may want to revisit all
this if we ever want to do computed field types, which will encounter similar
challenges as #32581.
Fixes #269.