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

Mutually recursive field types, the easy version #32658

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions base/compiler/tfuncs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const DATATYPE_TYPES_FIELDINDEX = fieldindex(DataType, :types)
const DATATYPE_SUPER_FIELDINDEX = fieldindex(DataType, :super)
const DATATYPE_MUTABLE_FIELDINDEX = fieldindex(DataType, :mutable)
const DATATYPE_INSTANCE_FIELDINDEX = fieldindex(DataType, :instance)
const DATATYPE_INCOMPLETE_FIELDINDEX = fieldindex(DataType, :incomplete)

const TYPENAME_NAME_FIELDINDEX = fieldindex(Core.TypeName, :name)
const TYPENAME_MODULE_FIELDINDEX = fieldindex(Core.TypeName, :module)
Expand Down Expand Up @@ -547,19 +548,26 @@ function subtype_tfunc(@nospecialize(a), @nospecialize(b))
end
add_tfunc(<:, 2, 2, subtype_tfunc, 0)

is_dt_complete_const_field(fld::Int) = (
fld == DATATYPE_TYPES_FIELDINDEX ||
fld == DATATYPE_MUTABLE_FIELDINDEX ||
fld == DATATYPE_INSTANCE_FIELDINDEX ||
fld == DATATYPE_INCOMPLETE_FIELDINDEX
)

is_dt_const_field(fld::Int) = (
fld == DATATYPE_NAME_FIELDINDEX ||
fld == DATATYPE_PARAMETERS_FIELDINDEX ||
fld == DATATYPE_TYPES_FIELDINDEX ||
fld == DATATYPE_SUPER_FIELDINDEX ||
fld == DATATYPE_MUTABLE_FIELDINDEX ||
fld == DATATYPE_INSTANCE_FIELDINDEX
fld == DATATYPE_SUPER_FIELDINDEX
)

function const_datatype_getfield_tfunc(@nospecialize(sv), fld::Int)
if fld == DATATYPE_INSTANCE_FIELDINDEX
return isdefined(sv, fld) ? Const(getfield(sv, fld)) : Union{}
elseif is_dt_const_field(fld)
return Const(getfield(sv, fld))
elseif !getfield(sv, :incomplete) && is_dt_complete_const_field(fld)
return Const(getfield(sv, fld))
end
return nothing
end
Expand Down
11 changes: 8 additions & 3 deletions base/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,11 @@ function isprimitivetype(@nospecialize(t::Type))
return !hasfield && t.size != 0 && !t.abstract
end

struct IncompleteTypeException
dt::DataType
end
must_be_complete(t::DataType) = (t.incomplete && throw(IncompleteTypeException(t)); t)
Copy link
Sponsor Member

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)


"""
isbitstype(T)

Expand All @@ -449,14 +454,14 @@ julia> isbitstype(Complex)
false
```
"""
isbitstype(@nospecialize(t::Type)) = (@_pure_meta; isa(t, DataType) && t.isbitstype)
isbitstype(@nospecialize(t::Type)) = (@_pure_meta; isa(t, DataType) && must_be_complete(t).isbitstype)

"""
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
Copy link
Sponsor Member

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?


"""
isdispatchtuple(T)
Expand All @@ -465,7 +470,7 @@ Determine whether type `T` is a tuple "leaf type",
meaning it could appear as a type signature in dispatch
and has no subtypes (or supertypes) which could appear in a call.
"""
isdispatchtuple(@nospecialize(t)) = (@_pure_meta; isa(t, DataType) && t.isdispatchtuple)
isdispatchtuple(@nospecialize(t)) = (@_pure_meta; isa(t, DataType) && must_be_complete(t).isdispatchtuple)

iskindtype(@nospecialize t) = (t === DataType || t === UnionAll || t === Union || t === typeof(Bottom))
isconcretedispatch(@nospecialize t) = isconcretetype(t) && !iskindtype(t)
Expand Down
2 changes: 2 additions & 0 deletions src/ast.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ jl_sym_t *splatnew_sym;
jl_sym_t *const_sym; jl_sym_t *thunk_sym;
jl_sym_t *abstracttype_sym; jl_sym_t *primtype_sym;
jl_sym_t *structtype_sym; jl_sym_t *foreigncall_sym;
jl_sym_t *incompletetype_sym;
jl_sym_t *global_sym; jl_sym_t *list_sym;
jl_sym_t *dot_sym; jl_sym_t *newvar_sym;
jl_sym_t *boundscheck_sym; jl_sym_t *inbounds_sym;
Expand Down Expand Up @@ -334,6 +335,7 @@ void jl_init_frontend(void)
abstracttype_sym = jl_symbol("abstract_type");
primtype_sym = jl_symbol("primitive_type");
structtype_sym = jl_symbol("struct_type");
incompletetype_sym = jl_symbol("incomplete_type");
toplevel_sym = jl_symbol("toplevel");
dot_sym = jl_symbol(".");
colon_sym = jl_symbol(":");
Expand Down
2 changes: 2 additions & 0 deletions src/builtins.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(or something even longer and clearer)

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski Aug 2, 2019

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?

int field_index;
if (jl_is_long(f)) {
field_index = jl_unbox_long(f) - 1;
Expand Down
2 changes: 1 addition & 1 deletion src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4248,7 +4248,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaval)
return ghostValue(jl_void_type);
}
if (head == abstracttype_sym || head == structtype_sym ||
head == primtype_sym) {
head == primtype_sym || head == incompletetype_sym) {
jl_errorf("type definition not allowed inside a local scope");
}
else {
Expand Down
3 changes: 3 additions & 0 deletions src/datatype.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ jl_datatype_t *jl_new_uninitialized_datatype(void)
t->has_concrete_subtype = 1;
t->layout = NULL;
t->names = NULL;
t->incomplete = 1;
return t;
}

Expand Down Expand Up @@ -539,6 +540,7 @@ JL_DLLEXPORT jl_datatype_t *jl_new_primitivetype(jl_value_t *name, jl_module_t *
bt->size = nbytes;
bt->layout = jl_get_layout(0, alignm, 0, NULL);
bt->instance = NULL;
bt->incomplete = 0;
return bt;
}

Expand Down Expand Up @@ -567,6 +569,7 @@ JL_DLLEXPORT jl_datatype_t * jl_new_foreign_type(jl_sym_t *name,
desc->sweepfunc = sweepfunc;
bt->layout = layout;
bt->instance = NULL;
bt->incomplete = 0;
return bt;
}

Expand Down
4 changes: 3 additions & 1 deletion src/dump.c
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,8 @@ static void jl_serialize_datatype(jl_serializer_state *s, jl_datatype_t *dt) JL_
write_int32(s->s, dt->size);
int has_instance = (dt->instance != NULL);
int has_layout = (dt->layout != NULL);
write_uint8(s->s, dt->abstract | (dt->mutabl << 1) | (has_layout << 2) | (has_instance << 3));
write_uint8(s->s, dt->abstract | (dt->mutabl << 1) | (has_layout << 2) |
(has_instance << 3) | (dt->incomplete << 4));
write_uint8(s->s, dt->hasfreetypevars
| (dt->isconcretetype << 1)
| (dt->isdispatchtuple << 2)
Expand Down Expand Up @@ -1398,6 +1399,7 @@ static jl_value_t *jl_deserialize_datatype(jl_serializer_state *s, int pos, jl_v
dt->mutabl = (flags >> 1) & 1;
int has_layout = (flags >> 2) & 1;
int has_instance = (flags >> 3) & 1;
dt->incomplete = (flags >> 4) & 1;
dt->hasfreetypevars = memflags & 1;
dt->isconcretetype = (memflags >> 1) & 1;
dt->isdispatchtuple = (memflags >> 2) & 1;
Expand Down
1 change: 1 addition & 0 deletions src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -2367,6 +2367,7 @@ jl_function_t *jl_new_generic_function_with_supertype(jl_sym_t *name, jl_module_
jl_set_const(module, tname, (jl_value_t*)ftype);
jl_value_t *f = jl_new_struct(ftype);
ftype->instance = f; jl_gc_wb(ftype, f);
ftype->incomplete = 0;
JL_GC_POP();
return (jl_function_t*)f;
}
Expand Down
111 changes: 105 additions & 6 deletions src/interpreter.c
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,82 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s)
if (temp == NULL || !equiv_type(dt, (jl_datatype_t*)jl_unwrap_unionall(temp))) {
jl_checked_assignment(b, w);
}
dt->incomplete = 0;
JL_GC_POP();
}

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))
Copy link
Sponsor Member

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

jl_error("Super type does not match forward declaration");
// Check that parameters match
if (jl_svec_len(para) != jl_svec_len(bdt->parameters)) {
jl_error("number of type parameters does not match forward declared type");
}
for (int i = 0; i < jl_svec_len(para); ++i) {
jl_tvar_t *v = (jl_tvar_t*)jl_svecref(para, i);
jl_tvar_t *w = (jl_tvar_t*)jl_svecref(bdt->parameters, i);
assert(jl_is_typevar(v) && jl_is_typevar(w));
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)) {
Copy link
Sponsor Member

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

jl_errorf("Bounds of type parameter `%s` do not match forward declaration",
jl_symbol_name(v->name));
}
}
}

static void eval_incompletetype(jl_expr_t *ex, interpreter_state *s)
{
jl_value_t **args = jl_array_ptr_data(ex->args);
if (inside_typedef)
jl_error("cannot eval a new incomplete type definition while defining another type");
jl_value_t *name = args[0];
jl_value_t *para = eval_value(args[1], s);
jl_value_t *super = NULL;
jl_value_t *temp = NULL;
jl_datatype_t *dt = NULL;
jl_value_t *w = NULL;
jl_module_t *modu = s->module;
JL_GC_PUSH5(&para, &super, &temp, &w, &dt);
assert(jl_is_svec(para));
if (jl_is_globalref(name)) {
modu = jl_globalref_mod(name);
name = (jl_value_t*)jl_globalref_name(name);
}
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))) {
Copy link
Sponsor Member

@vtjnash vtjnash Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super = eval_value(args[2], s);
check_type_completion(
(jl_datatype_t*)jl_unwrap_unionall(b->value), super, para);
return;
}
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;
Copy link
Sponsor Member

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

check_can_assign_type(b, w);
b->value = w;
jl_gc_wb_binding(b, w);
JL_TRY {
inside_typedef = 1;
super = eval_value(args[2], s);
jl_set_datatype_super(dt, super);
jl_reinstantiate_inner_types(dt);
}
JL_CATCH {
jl_reset_instantiate_inner_types(dt);
b->value = temp;
jl_rethrow();
}
b->value = temp;
if (temp == NULL || !equiv_type(dt, (jl_datatype_t*)jl_unwrap_unionall(temp))) {
jl_checked_assignment(b, w);
}
JL_GC_POP();
}

Expand Down Expand Up @@ -217,6 +293,7 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s)
if (temp == NULL || !equiv_type(dt, (jl_datatype_t*)jl_unwrap_unionall(temp))) {
jl_checked_assignment(b, w);
}
dt->incomplete = 0;
JL_GC_POP();
}

Expand All @@ -240,12 +317,28 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s)
assert(jl_is_symbol(name));
assert(jl_is_svec(para));
temp = eval_value(args[2], s); // field names
dt = jl_new_datatype((jl_sym_t*)name, modu, NULL, (jl_svec_t*)para,
(jl_svec_t*)temp, NULL,
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)
Copy link
Sponsor Member

@vtjnash vtjnash Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bdt = (jl_datatype_t *)jl_unwrap_unionall(b->value);
if (bdt &&
jl_is_datatype(bdt) &&
bdt->name->name == (jl_sym_t*)name &&
bdt->name->module == modu &&
bdt->incomplete) {
super = eval_value(args[3], s);
check_type_completion(bdt, super, para);
dt = bdt;
dt->mutabl = args[5]==jl_true ? 1 : 0;
dt->name->names = (jl_svec_t*)temp;
jl_gc_wb(dt->name, dt->name->names);
} else {
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else {
}
else {

dt = jl_new_datatype((jl_sym_t*)name, modu, NULL, (jl_svec_t*)para,
(jl_svec_t*)temp, NULL,
0, args[5]==jl_true ? 1 : 0, jl_unbox_long(args[6]));
}
w = dt->name->wrapper;

jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
temp = b->value; // save old value
// temporarily assign so binding is available for field types
check_can_assign_type(b, w);
Expand All @@ -255,8 +348,10 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s)
JL_TRY {
inside_typedef = 1;
// operations that can fail
super = eval_value(args[3], s);
jl_set_datatype_super(dt, super);
if (!super) {
super = eval_value(args[3], s);
jl_set_datatype_super(dt, super);
}
dt->types = (jl_svec_t*)eval_value(args[4], s);
jl_gc_wb(dt, dt->types);
for (size_t i = 0; i < jl_svec_len(dt->types); i++) {
Expand All @@ -280,6 +375,7 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s)
if (temp == NULL || !equiv_type(dt, (jl_datatype_t*)jl_unwrap_unionall(temp))) {
jl_checked_assignment(b, w);
}
dt->incomplete = 0;

JL_GC_POP();
}
Expand Down Expand Up @@ -745,6 +841,9 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
else if (head == structtype_sym) {
eval_structtype((jl_expr_t*)stmt, s);
}
else if (head == incompletetype_sym) {
eval_incompletetype((jl_expr_t*)stmt, s);
}
else if (jl_is_toplevel_only_expr(stmt)) {
jl_toplevel_eval(s->module, stmt);
}
Expand Down
Loading