Skip to content

Commit

Permalink
jltypes: always run parameter normalization (#49845)
Browse files Browse the repository at this point in the history 
This simplifies the types, which may help subtyping other other similar
lookup code any time this is later used as a parameter, so it is
probably worthwhile to do.

This is a followup to #49820, where we reorganized the code to make
this more straightforward.
  • Loading branch information
@vtjnash
vtjnash authored May 22, 2023
1 parent 1c21cbb commit e02d3ba
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 38 deletions.
6 changes: 3 additions & 3 deletions src/builtins.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1363,11 +1363,11 @@ JL_CALLABLE(jl_f_apply_type)
jl_vararg_t *vm = (jl_vararg_t*)args[0];
if (!vm->T) {
JL_NARGS(apply_type, 2, 3);
return (jl_value_t*)jl_wrap_vararg(args[1], nargs == 3 ? args[2] : NULL);
return (jl_value_t*)jl_wrap_vararg(args[1], nargs == 3 ? args[2] : NULL, 1);
}
else if (!vm->N) {
JL_NARGS(apply_type, 2, 2);
return (jl_value_t*)jl_wrap_vararg(vm->T, args[1]);
return (jl_value_t*)jl_wrap_vararg(vm->T, args[1], 1);
}
}
else if (jl_is_unionall(args[0])) {
Expand Down Expand Up @@ -2060,7 +2060,7 @@ void jl_init_primitives(void) JL_GC_DISABLED
add_builtin("Tuple", (jl_value_t*)jl_anytuple_type);
add_builtin("TypeofVararg", (jl_value_t*)jl_vararg_type);
add_builtin("SimpleVector", (jl_value_t*)jl_simplevector_type);
add_builtin("Vararg", (jl_value_t*)jl_wrap_vararg(NULL, NULL));
add_builtin("Vararg", (jl_value_t*)jl_wrap_vararg(NULL, NULL, 0));

add_builtin("Module", (jl_value_t*)jl_module_type);
add_builtin("MethodTable", (jl_value_t*)jl_methtable_type);
Expand Down
4 changes: 2 additions & 2 deletions src/gf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,7 @@ static jl_value_t *inst_varargp_in_env(jl_value_t *decl, jl_svec_t *sparams)
vm = T_has_tv ? jl_type_unionall(v, T) : T;
if (N_has_tv)
N = NULL;
vm = (jl_value_t*)jl_wrap_vararg(vm, N); // this cannot throw for these inputs
vm = (jl_value_t*)jl_wrap_vararg(vm, N, 1); // this cannot throw for these inputs
}
sp++;
decl = ((jl_unionall_t*)decl)->body;
Expand Down Expand Up @@ -984,7 +984,7 @@ static void jl_compilation_sig(
// avoid Vararg{Type{Type{...}}}
if (jl_is_type_type(type_i) && jl_is_type_type(jl_tparam0(type_i)))
type_i = (jl_value_t*)jl_type_type;
type_i = (jl_value_t*)jl_wrap_vararg(type_i, (jl_value_t*)NULL); // this cannot throw for these inputs
type_i = (jl_value_t*)jl_wrap_vararg(type_i, (jl_value_t*)NULL, 1); // this cannot throw for these inputs
}
else {
type_i = inst_varargp_in_env(decl, sparams);
Expand Down
65 changes: 36 additions & 29 deletions src/jltypes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -847,14 +847,14 @@ JL_DLLEXPORT jl_value_t *jl_type_unionall(jl_tvar_t *v, jl_value_t *body)
if (T_has_tv) {
jl_value_t *wrapped = jl_type_unionall(v, vm->T);
JL_GC_PUSH1(&wrapped);
wrapped = (jl_value_t*)jl_wrap_vararg(wrapped, vm->N);
wrapped = (jl_value_t*)jl_wrap_vararg(wrapped, vm->N, 1);
JL_GC_POP();
return wrapped;
}
else {
assert(N_has_tv);
assert(vm->N == (jl_value_t*)v);
return (jl_value_t*)jl_wrap_vararg(vm->T, NULL);
return (jl_value_t*)jl_wrap_vararg(vm->T, NULL, 1);
}
}
if (!jl_is_type(body) && !jl_is_typevar(body))
Expand Down Expand Up @@ -1889,7 +1889,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
}
// if some normalization might be needed, do that now
// it is probably okay to mutate iparams, and we only store globally rooted objects here
if (check && cacheable) {
if (check) {
size_t i;
for (i = 0; i < ntp; i++) {
jl_value_t *pi = iparams[i];
Expand All @@ -1898,8 +1898,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
if (jl_is_datatype(pi))
continue;
if (jl_is_vararg(pi))
// This would require some special handling, but is not needed
// at the moment (and might be better handled in jl_wrap_vararg instead).
// This is already handled in jl_wrap_vararg instead
continue;
if (!cacheable && jl_has_free_typevars(pi))
continue;
Expand Down Expand Up @@ -2327,7 +2326,7 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t
N = inst_type_w_(v->N, env, stack, check);
}
if (T != v->T || N != v->N) {
t = (jl_value_t*)jl_wrap_vararg(T, N);
t = (jl_value_t*)jl_wrap_vararg(T, N, check);
}
JL_GC_POP();
return t;
Expand Down Expand Up @@ -2400,36 +2399,44 @@ jl_datatype_t *jl_wrap_Type(jl_value_t *t)
return (jl_datatype_t*)jl_instantiate_unionall(jl_type_type, t);
}

jl_vararg_t *jl_wrap_vararg(jl_value_t *t, jl_value_t *n)
jl_vararg_t *jl_wrap_vararg(jl_value_t *t, jl_value_t *n, int check)
{
if (n) {
if (jl_is_typevar(n) || jl_is_uniontype(jl_unwrap_unionall(n))) {
// TODO: this is disabled due to #39698; it is also inconsistent
// with other similar checks, where we usually only check substituted
// values and not the bounds of variables.
/*
jl_tvar_t *N = (jl_tvar_t*)n;
if (!(N->lb == jl_bottom_type && N->ub == (jl_value_t*)jl_any_type))
jl_error("TypeVar in Vararg length must have bounds Union{} and Any");
*/
}
else if (!jl_is_long(n)) {
jl_type_error_rt("Vararg", "count", (jl_value_t*)jl_long_type, n);
}
else if (jl_unbox_long(n) < 0) {
jl_errorf("Vararg length is negative: %zd", jl_unbox_long(n));
jl_task_t *ct = jl_current_task;
JL_GC_PUSH1(&t);
if (check) {
if (n) {
if (jl_is_typevar(n) || jl_is_uniontype(jl_unwrap_unionall(n))) {
// TODO: this is disabled due to #39698; it is also inconsistent
// with other similar checks, where we usually only check substituted
// values and not the bounds of variables.
/*
jl_tvar_t *N = (jl_tvar_t*)n;
if (!(N->lb == jl_bottom_type && N->ub == (jl_value_t*)jl_any_type))
jl_error("TypeVar in Vararg length must have bounds Union{} and Any");
*/
}
else if (!jl_is_long(n)) {
jl_type_error_rt("Vararg", "count", (jl_value_t*)jl_long_type, n);
}
else if (jl_unbox_long(n) < 0) {
jl_errorf("Vararg length is negative: %zd", jl_unbox_long(n));
}
}
}
if (t) {
if (!jl_valid_type_param(t)) {
jl_type_error_rt("Vararg", "type", (jl_value_t*)jl_type_type, t);
if (t) {
if (!jl_valid_type_param(t)) {
jl_type_error_rt("Vararg", "type", (jl_value_t*)jl_type_type, t);
}
t = normalize_unionalls(t);
jl_value_t *tw = extract_wrapper(t);
if (tw && t != tw && jl_types_equal(t, tw))
t = tw;
}
}
jl_task_t *ct = jl_current_task;
jl_vararg_t *vm = (jl_vararg_t *)jl_gc_alloc(ct->ptls, sizeof(jl_vararg_t), jl_vararg_type);
jl_set_typetagof(vm, jl_vararg_tag, 0);
vm->T = t;
vm->N = n;
JL_GC_POP();
return vm;
}

Expand Down Expand Up @@ -2712,7 +2719,7 @@ void jl_init_types(void) JL_GC_DISABLED
// It seems like we probably usually end up needing the box for kinds (often used in an Any context), so force it to exist
jl_vararg_type->name->mayinlinealloc = 0;

jl_svec_t *anytuple_params = jl_svec(1, jl_wrap_vararg((jl_value_t*)jl_any_type, (jl_value_t*)NULL));
jl_svec_t *anytuple_params = jl_svec(1, jl_wrap_vararg((jl_value_t*)jl_any_type, (jl_value_t*)NULL, 0));
jl_anytuple_type = jl_new_datatype(jl_symbol("Tuple"), core, jl_any_type, anytuple_params,
jl_emptysvec, anytuple_params, jl_emptysvec, 0, 0, 0);
jl_tuple_typename = jl_anytuple_type->name;
Expand Down
2 changes: 1 addition & 1 deletion src/julia_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ jl_datatype_t *jl_new_abstracttype(jl_value_t *name, jl_module_t *module,
jl_datatype_t *jl_new_uninitialized_datatype(void);
void jl_precompute_memoized_dt(jl_datatype_t *dt, int cacheable);
JL_DLLEXPORT jl_datatype_t *jl_wrap_Type(jl_value_t *t); // x -> Type{x}
jl_vararg_t *jl_wrap_vararg(jl_value_t *t, jl_value_t *n);
jl_vararg_t *jl_wrap_vararg(jl_value_t *t, jl_value_t *n, int check);
void jl_reinstantiate_inner_types(jl_datatype_t *t);
jl_datatype_t *jl_lookup_cache_type_(jl_datatype_t *type);
void jl_cache_type_(jl_datatype_t *type);
Expand Down
6 changes: 3 additions & 3 deletions src/subtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ static int subtype_unionall(jl_value_t *t, jl_unionall_t *u, jl_stenv_t *e, int8
if (R && ans && e->envidx < e->envsz) {
jl_value_t *val;
if (vb.intvalued && vb.lb == (jl_value_t*)jl_any_type)
val = (jl_value_t*)jl_wrap_vararg(NULL, NULL); // special token result that represents N::Int in the envout
val = (jl_value_t*)jl_wrap_vararg(NULL, NULL, 0); // special token result that represents N::Int in the envout
else if (!vb.occurs_inv && vb.lb != jl_bottom_type)
val = is_leaf_bound(vb.lb) ? vb.lb : (jl_value_t*)jl_new_typevar(u->var->name, jl_bottom_type, vb.lb);
else if (vb.lb == vb.ub)
Expand Down Expand Up @@ -3092,7 +3092,7 @@ static jl_value_t *intersect_varargs(jl_vararg_t *vmx, jl_vararg_t *vmy, ssize_t
ii = (jl_value_t*)vmy;
else {
JL_GC_PUSH1(&ii);
ii = (jl_value_t*)jl_wrap_vararg(ii, NULL);
ii = (jl_value_t*)jl_wrap_vararg(ii, NULL, 1);
JL_GC_POP();
}
return ii;
Expand Down Expand Up @@ -3133,7 +3133,7 @@ static jl_value_t *intersect_varargs(jl_vararg_t *vmx, jl_vararg_t *vmy, ssize_t
else if (yp2 && obviously_egal(yp1, ii) && obviously_egal(yp2, i2))
ii = (jl_value_t*)vmy;
else
ii = (jl_value_t*)jl_wrap_vararg(ii, i2);
ii = (jl_value_t*)jl_wrap_vararg(ii, i2, 1);
JL_GC_POP();
return ii;
}
Expand Down

2 comments on commit e02d3ba

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

Please sign in to comment.