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

also fixes #21816

Author: JeffBezanson

base/compiler/tfuncs.jl

@@ -568,7 +568,7 @@ is_dt_const_field(fld::Int) = (
 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)
+    elseif is_dt_const_field(fld) && isdefined(sv, fld)
         return Const(getfield(sv, fld))
     end
     return nothing

src/builtins.c

@@ -1250,6 +1250,27 @@ JL_CALLABLE(jl_f__setsuper)
 
 void jl_reinstantiate_inner_types(jl_datatype_t *t);
 
+static int equiv_field_types(jl_value_t *old, jl_value_t *ft)
+{
+    size_t nf = jl_svec_len(ft);
+    if (jl_svec_len(old) != nf)
+        return 0;
+    size_t i;
+    for (i = 0; i < nf; i++) {
+        jl_value_t *ta = jl_svecref(old, i);
+        jl_value_t *tb = jl_svecref(ft, i);
+        if (jl_has_free_typevars(ta)) {
+            if (!jl_has_free_typevars(tb) || !jl_egal(ta, tb))
+                return 0;
+        }
+        else if (jl_has_free_typevars(tb) || jl_typeof(ta) != jl_typeof(tb) ||
+                 !jl_types_equal(ta, tb)) {
+            return 0;
+        }
+    }
+    return 1;
+}
+
 JL_CALLABLE(jl_f__typebody)
 {
     JL_NARGS(_typebody!, 1, 2);
@@ -1258,16 +1279,23 @@ JL_CALLABLE(jl_f__typebody)
     if (nargs == 2) {
         jl_value_t *ft = args[1];
         JL_TYPECHK(_typebody!, simplevector, ft);
-        dt->types = (jl_svec_t*)ft;
-        jl_gc_wb(dt, ft);
-        for (size_t i = 0; i < jl_svec_len(dt->types); i++) {
-            jl_value_t *elt = jl_svecref(dt->types, i);
+        size_t nf = jl_svec_len(ft);
+        for (size_t i = 0; i < nf; i++) {
+            jl_value_t *elt = jl_svecref(ft, i);
             if ((!jl_is_type(elt) && !jl_is_typevar(elt)) || jl_is_vararg_type(elt)) {
                 jl_type_error_rt(jl_symbol_name(dt->name->name),
                                  "type definition",
                                  (jl_value_t*)jl_type_type, elt);
             }
         }
+        if (dt->types != NULL) {
+            if (!equiv_field_types((jl_value_t*)dt->types, ft))
+                jl_errorf("invalid redefinition of type %s", jl_symbol_name(dt->name->name));
+        }
+        else {
+            dt->types = (jl_svec_t*)ft;
+            jl_gc_wb(dt, ft);
+        }
     }
 
     JL_TRY {
@@ -1294,15 +1322,13 @@ static int equiv_type(jl_value_t *ta, jl_value_t *tb)
           dta->name->name == dtb->name->name &&
           dta->abstract == dtb->abstract &&
           dta->mutabl == dtb->mutabl &&
-          dta->size == dtb->size &&
+          (jl_svec_len(jl_field_names(dta)) != 0 || dta->size == dtb->size) &&
           dta->ninitialized == dtb->ninitialized &&
           jl_egal((jl_value_t*)jl_field_names(dta), (jl_value_t*)jl_field_names(dtb)) &&
-          jl_nparams(dta) == jl_nparams(dtb) &&
-          jl_svec_len(dta->types) == jl_svec_len(dtb->types)))
+          jl_nparams(dta) == jl_nparams(dtb)))
         return 0;
     jl_value_t *a=NULL, *b=NULL;
     int ok = 1;
-    size_t i, nf = jl_svec_len(dta->types);
     JL_GC_PUSH2(&a, &b);
     a = jl_rewrap_unionall((jl_value_t*)dta->super, dta->name->wrapper);
     b = jl_rewrap_unionall((jl_value_t*)dtb->super, dtb->name->wrapper);
@@ -1328,21 +1354,6 @@ static int equiv_type(jl_value_t *ta, jl_value_t *tb)
         a = jl_instantiate_unionall(ua, (jl_value_t*)ub->var);
         b = ub->body;
     }
-    assert(jl_is_datatype(a) && jl_is_datatype(b));
-    a = (jl_value_t*)jl_get_fieldtypes((jl_datatype_t*)a);
-    b = (jl_value_t*)jl_get_fieldtypes((jl_datatype_t*)b);
-    for (i = 0; i < nf; i++) {
-        jl_value_t *ta = jl_svecref(a, i);
-        jl_value_t *tb = jl_svecref(b, i);
-        if (jl_has_free_typevars(ta)) {
-            if (!jl_has_free_typevars(tb) || !jl_egal(ta, tb))
-                goto no;
-        }
-        else if (jl_has_free_typevars(tb) || jl_typeof(ta) != jl_typeof(tb) ||
-                 !jl_types_equal(ta, tb)) {
-            goto no;
-        }
-    }
     JL_GC_POP();
     return 1;
  no:

src/jltypes.c

@@ -1773,6 +1773,9 @@ JL_DLLEXPORT jl_svec_t *jl_compute_fieldtypes(jl_datatype_t *st JL_PROPAGATES_RO
     assert(n > 0 && "expected empty case to be handled during construction");
     //if (n == 0)
     //    return ((st->types = jl_emptysvec));
+    if (wt->types == NULL)
+        jl_errorf("cannot determine field types of incomplete type %s",
+                  jl_symbol_name(st->name->name));
     jl_typeenv_t *env = (jl_typeenv_t*)alloca(n * sizeof(jl_typeenv_t));
     for (i = 0; i < n; i++) {
         env[i].var = (jl_tvar_t*)jl_svecref(wt->parameters, i);

src/julia-syntax.scm

@@ -884,7 +884,8 @@
           (defs2 (if (null? defs)
                      (default-inner-ctors name field-names field-types params bounds locs)
                      defs))
-          (min-initialized (min (ctors-min-initialized defs) (length fields))))
+          (min-initialized (min (ctors-min-initialized defs) (length fields)))
+          (prev (make-ssavalue)))
      (let ((dups (has-dups field-names)))
        (if dups (error (string "duplicate field name: \"" (car dups) "\" is not unique"))))
      (for-each (lambda (v)
@@ -898,16 +899,29 @@
          (local-def ,name)
          ,@(map (lambda (v) `(local ,v)) params)
          ,@(map (lambda (n v) (make-assignment n (bounds-to-TypeVar v #t))) params bounds)
-         (toplevel-only struct)
+         (toplevel-only struct (outerref ,name))
          (= ,name (call (core _structtype) (thismodule) (inert ,name) (call (core svec) ,@params)
                         (call (core svec) ,@(map quotify field-names))
                         ,mut ,min-initialized))
          (call (core _setsuper!) ,name ,super)
-         (call (core _typebody!) ,name (call (core svec) ,@field-types))
-         (if (&& (isdefined (outerref ,name))
-                 (call (core _equiv_typedef) (outerref ,name) ,name))
-             (null)
+         (if (isdefined (outerref ,name))
+             (block
+              (= ,prev (outerref ,name))
+              (if (call (core _equiv_typedef) ,prev ,name)
+                  ;; if this is compatible with an old definition, use the existing type object
+                  ;; and its parameters
+                  (block (= ,name ,prev)
+                         ,@(if (pair? params)
+                               `((= (tuple ,@params) (|.|
+                                                      ,(foldl (lambda (_ x) `(|.| ,x (quote body)))
+                                                              prev
+                                                              params)
+                                                      (quote parameters))))
+                               '()))
+                  ;; otherwise do an assignment to trigger an error
+                  (= (outerref ,name) ,name)))
              (= (outerref ,name) ,name))
+         (call (core _typebody!) ,name (call (core svec) ,@field-types))
          (null)))
        ;; "inner" constructors
        (scope-block
@@ -3351,7 +3365,12 @@ f(x) = yt(x)
        ((atom? e) e)
        (else
         (case (car e)
-          ((quote top core globalref outerref thismodule toplevel-only line break inert module toplevel null true false meta) e)
+          ((quote top core globalref outerref thismodule line break inert module toplevel null true false meta) e)
+          ((toplevel-only)
+           ;; hack to avoid generating a (method x) expr for struct types
+           (if (eq? (cadr e) 'struct)
+               (put! defined (caddr e) #t))
+           e)
           ((=)
            (let ((var (cadr e))
                  (rhs (cl-convert (caddr e) fname lam namemap defined toplevel interp)))

test/core.jl

@@ -7221,3 +7221,20 @@ struct AVL35416{K,V}
     avl:: Union{Nothing,Node35416{AVL35416{K,V},<:K,<:V}}
 end
 @test AVL35416(Node35416{AVL35416{Integer,AbstractString},Int,String}()) isa AVL35416{Integer,AbstractString}
+
+# issue #36104
+module M36104
+struct T36104
+    v::Vector{M36104.T36104}
+end
+struct T36104   # check that redefining it works, issue #21816
+    v::Vector{T36104}
+end
+end
+@test fieldtypes(M36104.T36104) == (Vector{M36104.T36104},)
+@test_throws ErrorException("expected") @eval(struct X36104; x::error("expected"); end)
+@test @isdefined(X36104)
+struct X36104; x::Int; end
+@test fieldtypes(X36104) == (Int,)
+primitive type P36104 8 end
+@test_throws ErrorException("invalid redefinition of constant P36104") @eval(primitive type P36104 16 end)