auto merge of #11839 : typelist/rust/issue3008, r=huonw

It was possible to trigger a stack overflow in rustc because the routine used to verify enum representability,
type_structurally_contains, would recurse on inner types until hitting the original type. The overflow condition was when a different structurally recursive type (enum or struct) was contained in the type being checked.

I suspect my solution isn't as efficient as it could be. I pondered adding a cache of previously-seen types to avoid duplicating work (if enums A and B both contain type C, my code goes through C twice), but I didn't want to do anything that may not be necessary.

I'm a new contributor, so please pay particular attention to any unidiomatic code, misuse of terminology, bad naming of tests, or similar horribleness :)

Updated to verify struct representability as well.

Fixes #3008.
Fixes #3779.

Author: bors

src/librustc/middle/ty.rs

@@ -2348,53 +2348,104 @@ pub fn is_instantiable(cx: ctxt, r_ty: t) -> bool {
     !subtypes_require(cx, &mut seen, r_ty, r_ty)
 }
 
-pub fn type_structurally_contains(cx: ctxt, ty: t, test: |x: &sty| -> bool)
-                                  -> bool {
-    let sty = &get(ty).sty;
-    debug!("type_structurally_contains: {}",
-           ::util::ppaux::ty_to_str(cx, ty));
-    if test(sty) { return true; }
-    match *sty {
-      ty_enum(did, ref substs) => {
-        for variant in (*enum_variants(cx, did)).iter() {
-            for aty in variant.args.iter() {
-                let sty = subst(cx, substs, *aty);
-                if type_structurally_contains(cx, sty, |x| test(x)) { return true; }
+/// Describes whether a type is representable. For types that are not
+/// representable, 'SelfRecursive' and 'ContainsRecursive' are used to
+/// distinguish between types that are recursive with themselves and types that
+/// contain a different recursive type. These cases can therefore be treated
+/// differently when reporting errors.
+#[deriving(Eq)]
+pub enum Representability {
+    Representable,
+    SelfRecursive,
+    ContainsRecursive,
+}
+
+/// Check whether a type is representable. This means it cannot contain unboxed
+/// structural recursion. This check is needed for structs and enums.
+pub fn is_type_representable(cx: ctxt, ty: t) -> Representability {
+
+    // Iterate until something non-representable is found
+    fn find_nonrepresentable<It: Iterator<t>>(cx: ctxt, seen: &mut ~[DefId],
+                                              mut iter: It) -> Representability {
+        for ty in iter {
+            let r = type_structurally_recursive(cx, seen, ty);
+            if r != Representable {
+                 return r
             }
         }
-        return false;
-      }
-      ty_struct(did, ref substs) => {
-        let r = lookup_struct_fields(cx, did);
-        for field in r.iter() {
-            let ft = lookup_field_type(cx, did, field.id, substs);
-            if type_structurally_contains(cx, ft, |x| test(x)) { return true; }
+        Representable
+    }
+
+    // Does the type `ty` directly (without indirection through a pointer)
+    // contain any types on stack `seen`?
+    fn type_structurally_recursive(cx: ctxt, seen: &mut ~[DefId],
+                                   ty: t) -> Representability {
+        debug!("type_structurally_recursive: {}",
+               ::util::ppaux::ty_to_str(cx, ty));
+
+        // Compare current type to previously seen types
+        match get(ty).sty {
+            ty_struct(did, _) |
+            ty_enum(did, _) => {
+                for (i, &seen_did) in seen.iter().enumerate() {
+                    if did == seen_did {
+                        return if i == 0 { SelfRecursive }
+                               else { ContainsRecursive }
+                    }
+                }
+            }
+            _ => (),
         }
-        return false;
-      }
 
-      ty_tup(ref ts) => {
-        for tt in ts.iter() {
-            if type_structurally_contains(cx, *tt, |x| test(x)) { return true; }
+        // Check inner types
+        match get(ty).sty {
+            // Tuples
+            ty_tup(ref ts) => {
+                find_nonrepresentable(cx, seen, ts.iter().map(|t| *t))
+            }
+            // Fixed-length vectors.
+            // FIXME(#11924) Behavior undecided for zero-length vectors.
+            ty_vec(mt, vstore_fixed(_)) => {
+                type_structurally_recursive(cx, seen, mt.ty)
+            }
+
+            // Push struct and enum def-ids onto `seen` before recursing.
+            ty_struct(did, ref substs) => {
+                seen.push(did);
+                let fields = struct_fields(cx, did, substs);
+                let r = find_nonrepresentable(cx, seen,
+                                              fields.iter().map(|f| f.mt.ty));
+                seen.pop();
+                r
+            }
+            ty_enum(did, ref substs) => {
+                seen.push(did);
+                let vs = enum_variants(cx, did);
+
+                let mut r = Representable;
+                for variant in vs.iter() {
+                    let iter = variant.args.iter().map(|aty| subst(cx, substs, *aty));
+                    r = find_nonrepresentable(cx, seen, iter);
+
+                    if r != Representable { break }
+                }
+
+                seen.pop();
+                r
+            }
+
+            _ => Representable,
         }
-        return false;
-      }
-      ty_vec(ref mt, vstore_fixed(_)) => {
-        return type_structurally_contains(cx, mt.ty, test);
-      }
-      _ => return false
     }
-}
 
-pub fn type_structurally_contains_uniques(cx: ctxt, ty: t) -> bool {
-    return type_structurally_contains(cx, ty, |sty| {
-        match *sty {
-          ty_uniq(_) |
-          ty_vec(_, vstore_uniq) |
-          ty_str(vstore_uniq) => true,
-          _ => false,
-        }
-    });
+    debug!("is_type_representable: {}",
+           ::util::ppaux::ty_to_str(cx, ty));
+
+    // To avoid a stack overflow when checking an enum variant or struct that
+    // contains a different, structurally recursive type, maintain a stack
+    // of seen types and check recursion for each of them (issues #3008, #3779).
+    let mut seen: ~[DefId] = ~[];
+    type_structurally_recursive(cx, &mut seen, ty)
 }
 
 pub fn type_is_trait(ty: t) -> bool {

src/librustc/middle/typeck/check/mod.rs

@@ -531,7 +531,10 @@ pub fn check_no_duplicate_fields(tcx: ty::ctxt,
 pub fn check_struct(ccx: @CrateCtxt, id: ast::NodeId, span: Span) {
     let tcx = ccx.tcx;
 
-    // Check that the class is instantiable
+    // Check that the struct is representable
+    check_representable(tcx, span, id, "struct");
+
+    // Check that the struct is instantiable
     check_instantiable(tcx, span, id);
 
     if ty::lookup_simd(tcx, local_def(id)) {
@@ -3410,6 +3413,33 @@ pub fn check_const_with_ty(fcx: @FnCtxt,
     writeback::resolve_type_vars_in_expr(fcx, e);
 }
 
+/// Checks whether a type can be represented in memory. In particular, it
+/// identifies types that contain themselves without indirection through a
+/// pointer, which would mean their size is unbounded. This is different from
+/// the question of whether a type can be instantiated. See the definition of
+/// `check_instantiable`.
+pub fn check_representable(tcx: ty::ctxt,
+                           sp: Span,
+                           item_id: ast::NodeId,
+                           designation: &str) {
+    let rty = ty::node_id_to_type(tcx, item_id);
+
+    // Check that it is possible to represent this type. This call identifies
+    // (1) types that contain themselves and (2) types that contain a different
+    // recursive type. It is only necessary to throw an error on those that
+    // contain themselves. For case 2, there must be an inner type that will be
+    // caught by case 1.
+    match ty::is_type_representable(tcx, rty) {
+      ty::SelfRecursive => {
+        tcx.sess.span_err(
+          sp, format!("illegal recursive {} type; \
+                       wrap the inner value in a box to make it representable",
+                      designation));
+      }
+      ty::Representable | ty::ContainsRecursive => (),
+    }
+}
+
 /// Checks whether a type can be created without an instance of itself.
 /// This is similar but different from the question of whether a type
 /// can be represented.  For example, the following type:
@@ -3565,7 +3595,6 @@ pub fn check_enum_variants(ccx: @CrateCtxt,
         return variants;
     }
 
-    let rty = ty::node_id_to_type(ccx.tcx, id);
     let hint = ty::lookup_repr_hint(ccx.tcx, ast::DefId { crate: ast::LOCAL_CRATE, node: id });
     if hint != attr::ReprAny && vs.len() <= 1 {
         ccx.tcx.sess.span_err(sp, format!("unsupported representation for {}variant enum",
@@ -3580,22 +3609,8 @@ pub fn check_enum_variants(ccx: @CrateCtxt,
         enum_var_cache.get().insert(local_def(id), @variants);
     }
 
-    // Check that it is possible to represent this enum:
-    let mut outer = true;
-    let did = local_def(id);
-    if ty::type_structurally_contains(ccx.tcx, rty, |sty| {
-        match *sty {
-          ty::ty_enum(id, _) if id == did => {
-            if outer { outer = false; false }
-            else { true }
-          }
-          _ => false
-        }
-    }) {
-        ccx.tcx.sess.span_err(sp,
-                              "illegal recursive enum type; \
-                               wrap the inner value in a box to make it representable");
-    }
+    // Check that it is possible to represent this enum.
+    check_representable(ccx.tcx, sp, id, "enum");
 
     // Check that it is possible to instantiate this enum:
     //

src/test/compile-fail/issue-3008-1.rs

@@ -0,0 +1,15 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+enum foo { foo(bar) }
+enum bar { bar_none, bar_some(bar) } //~ ERROR illegal recursive enum type; wrap the inner value in a box to make it representable
+
+fn main() {
+}

src/test/compile-fail/issue-3008-2.rs

@@ -0,0 +1,17 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+enum foo { foo(bar) }
+struct bar { x: bar } //~ ERROR illegal recursive struct type; wrap the inner value in a box to make it representable
+//~^ ERROR this type cannot be instantiated without an instance of itself
+
+fn main() {
+}
+

src/test/compile-fail/issue-3008-3.rs

@@ -0,0 +1,15 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+enum E1 { V1(E2<E1>), }
+enum E2<T> { V2(E2<E1>), } //~ ERROR illegal recursive enum type; wrap the inner value in a box to make it representable
+
+fn main() {
+}

src/test/compile-fail/issue-3779.rs

@@ -0,0 +1,17 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+struct S { //~ ERROR illegal recursive struct type; wrap the inner value in a box to make it representable
+    element: Option<S>
+}
+
+fn main() {
+    let x = S { element: None };
+}