Mutually recursive field types, the easy version #32658

Keno · 2019-07-23T03:27:23Z

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:

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

cwiese · 2019-07-26T02:13:23Z

nice - I knew the day would come

chethega · 2019-07-28T14:15:44Z

It is still possible to do the union thing via the following, right?

incomplete type A; end
incomplete type B; end
struct A; x::Union{A,B}; end
struct B; x::Union{A,B}; end

Is there any type (including data layout) that is inexpressible in this simplified version and would be expressible in the other?

Keno · 2019-07-28T20:01:25Z

It is still possible to do the union thing via the following, right?

Yes that works (modulo bugs). The thing you can do is later decide that a forward declared name should be a Union.

StefanKarpinski · 2019-07-31T19:13:12Z

I think @chethega's point is a good one. The problem with U = Union{A, B}; #ERROR seems to me that rather than defining what the incomplete type U is as a type declaration would, this expression constructs a new union type and then happens to assign it to the name U. This should be an error since U already has an (incomplete) meaning, which is presumably const, even though it's not fully defined yet (this is like a const binding to an incompletely constructed object, a perfectly coherent thing). This problem ends up being somewhat specific to unions because there is no syntax for declaring a union—you just make a union and then give it a name if you want to. I think we can live with that since it doesn't limit what can be expressed.

Keno · 2019-07-31T19:16:07Z

Right, if you think of U as an incomplete data type this behavior makes perfect sense (which is why I like the incomplete type syntax). There was briefly a consideration to have a more general delayed binding mechanism to implement this, but that turned out too complicated.

StefanKarpinski · 2019-07-31T20:09:35Z

I think having the explicit incomplete type syntax is kind of nice; we could potentially auto-insert such statements when an undefined type is referenced and then error if it isn't defined by the end of the current evaluation unit.

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.

Keno · 2019-08-02T16:47:28Z

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.

vtjnash · 2019-08-02T17:23:22Z

base/reflection.jl

+struct IncompleteTypeException
+    dt::DataType
+end
+must_be_complete(t::DataType) = (t.incomplete && throw(IncompleteTypeException(t)); t)


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)

vtjnash · 2019-08-02T17:25:21Z

base/reflection.jl


 """
    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


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?

vtjnash · 2019-08-02T17:26:48Z

src/builtins.c

@@ -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");


Suggested change

jl_error("Type is incomplete");

jl_error("fieldtype: this DataType's fields are incomplete");

(or something even longer and clearer)

Maybe: incomplete type `$name` declared but never defined?

vtjnash · 2019-08-02T17:27:59Z

src/interpreter.c

+        dt->mutabl = args[5]==jl_true ? 1 : 0;
+        dt->name->names = (jl_svec_t*)temp;
+        jl_gc_wb(dt->name, dt->name->names);
+    } else {


Suggested change

} else {

}

else {

vtjnash · 2019-08-02T17:32:45Z

src/jltypes.c

@@ -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;


shouldn't jl_new_abstracttype be setting incomplete = 0 always (since you can never complete an abstract type).

vtjnash · 2019-08-02T17:35:42Z

src/interpreter.c

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


IIRC, super needs a rewrap_unionall to be used as a Type

vtjnash · 2019-08-02T17:44:32Z

src/interpreter.c

+        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)) {


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

vtjnash · 2019-08-02T17:46:10Z

src/interpreter.c

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


Suggested change

if (b->value)

if (b->value && b->constp)

vtjnash · 2019-08-02T17:46:35Z

src/interpreter.c

+    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))) {


Suggested change

if (b->value && jl_is_datatype(jl_unwrap_unionall(b->value))) {

if (b->value && b->constp && jl_is_datatype(jl_unwrap_unionall(b->value))) {

vtjnash · 2019-08-02T17:51:24Z

src/interpreter.c

+    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;


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

vtjnash

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

vtjnash · 2019-08-02T18:04:26Z

test/incompletetype.jl

+    incomplete type Bar <: Foo2; end
+end)
+
+incomplete type FieldNamesTest; end


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.

Gnimuc · 2020-03-30T05:07:53Z

What's the status of this PR? It would be great if Julia could have this feature in v1.5.

quinnj · 2020-07-31T08:12:48Z

An enthusiastic JuliaCon 2020 bump here! Can we include this as a 1.6 release feature?

o314 · 2022-09-01T06:00:49Z

The con of this is that this will split out def code in two parts : the params types and the containment ones.
When programming in the large , this may (will) lead to very dirty codebase .
As an example

how programmers are able to locate import statement in code today (grep is not enough) ;
and how do they know
- which parts of code have already run before ?
- which libs are loaded ?
- which config/platform is active ?

Similar issues will raise - what (and when) is undef, @lazy, kwdef, autohash, etc. . Maintenance, quality over time will fall.
BTW that does not seem to be a commons practices in the zoo languages today, isn't it ?

Keno force-pushed the kf/incomplete2 branch from 2850da9 to 2ba2412 Compare July 24, 2019 19:31

kshyatt added the domain:types and dispatch Types, subtyping and method dispatch label Jul 25, 2019

Keno force-pushed the kf/incomplete2 branch 2 times, most recently from 0507f5e to bd7129f Compare July 26, 2019 02:08

Keno mentioned this pull request Jul 26, 2019

handle mutually-circular type declarations #269

Open

Keno mentioned this pull request Aug 2, 2019

Very WIP: Mutually recursive types #32581

Closed

Keno added 3 commits August 2, 2019 12:32

Add parsing for incomplete type

b89af39

Add incomplete flag to jl_datatype_t

45ba7aa

Keno force-pushed the kf/incomplete2 branch from bd7129f to 2d468b1 Compare August 2, 2019 16:45

vtjnash reviewed Aug 2, 2019

View reviewed changes

bhalonen mentioned this pull request Aug 27, 2019

Automate the wrapper generation, upgrade CEnum dep. scipopt/SCIP.jl#77

Closed

vtjnash mentioned this pull request Oct 30, 2019

Reorder self-referential types checking to prevent inline allocation #33718

Merged

This was referenced Jun 2, 2020

fix #36104, hack to allow accessing incomplete struct via dot syntax #36111

Closed

fix #36104, assign global name during type definitions #36121

Merged

StefanKarpinski added this to the 1.6 milestone Jul 31, 2020

ali-ramadhan mentioned this pull request Sep 20, 2020

Includes at the module level cannot have circular dependencies. CliMA/Oceananigans.jl#69

Closed

JeffBezanson removed this from the 1.6 features milestone Oct 1, 2020

Gnimuc mentioned this pull request Jan 3, 2021

Improvement Meta-list v2 JuliaInterop/Clang.jl#228

Closed

10 tasks

Keno mentioned this pull request Mar 20, 2024

Segfault when using improperly defined struct #53754

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutually recursive field types, the easy version #32658

Mutually recursive field types, the easy version #32658

Keno commented Jul 23, 2019 •

edited

Loading

cwiese commented Jul 26, 2019

chethega commented Jul 28, 2019

Keno commented Jul 28, 2019

StefanKarpinski commented Jul 31, 2019

Keno commented Jul 31, 2019

StefanKarpinski commented Jul 31, 2019

Keno commented Aug 2, 2019

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019

StefanKarpinski Aug 2, 2019 •

edited

Loading

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019

vtjnash Aug 2, 2019 •

edited

Loading

vtjnash Aug 2, 2019 •

edited

Loading

vtjnash Aug 2, 2019

vtjnash left a comment

vtjnash Aug 2, 2019

Gnimuc commented Mar 30, 2020

quinnj commented Jul 31, 2020

o314 commented Sep 1, 2022 •

edited

Loading

	jl_error("Type is incomplete");
	jl_error("fieldtype: this DataType's fields are incomplete");

	if (b->value && jl_is_datatype(jl_unwrap_unionall(b->value))) {
	if (b->value && b->constp && jl_is_datatype(jl_unwrap_unionall(b->value))) {

Mutually recursive field types, the easy version #32658

Are you sure you want to change the base?

Mutually recursive field types, the easy version #32658

Conversation

Keno commented Jul 23, 2019 • edited Loading

cwiese commented Jul 26, 2019

chethega commented Jul 28, 2019

Keno commented Jul 28, 2019

StefanKarpinski commented Jul 31, 2019

Keno commented Jul 31, 2019

StefanKarpinski commented Jul 31, 2019

Keno commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

vtjnash Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnimuc commented Mar 30, 2020

quinnj commented Jul 31, 2020

o314 commented Sep 1, 2022 • edited Loading

Keno commented Jul 23, 2019 •

edited

Loading

StefanKarpinski Aug 2, 2019 •

edited

Loading

vtjnash Aug 2, 2019 •

edited

Loading

vtjnash Aug 2, 2019 •

edited

Loading

o314 commented Sep 1, 2022 •

edited

Loading