Avoid stack overflow when checking struct/enum representability

[isabelmu]

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.

[huonw]

This could be return if i == 0 { SelfRecursive } else { ContainsRecursive }. (Also, the ; at the end of the if here is not required.)

[huonw]

Looks reasonable to me; this is a cool first contribution!

I'd guess this fixes struct Foo { x: Option<Foo> } too? In which case, you can add that test case and "fixes #3779" to a commit message or the PR text.

(Btw you had "fixes issue #3008" before, but github doesn't recognise that as a special command. It has to be "fixes #3008" without "issue".)

[isabelmu]

Oh! Didn't realize #3779 existed. I thought structs were somehow OK (and say as much in a comment somewhere) already. Clearly they aren't. I will add an is_type_representable call for structs, and a test case.

[isabelmu]

I'm actually still able to trigger a stack overflow in fun cases like:

enum E1 { V1(E2<E1>), }
enum E2<T> { V2(E2<E1>), }

I'll try to work out what's going on later today. (This stack overflow happens later, like the one in #3779).

I suppose #[deriving(Eq)] is totally wrong for ReprItem?

[brson]

Woo! Nice 1.0 fix.

[isabelmu]

This should be OK now. @huonw what next?

[huonw]

Do you reckon you could put a comment above this like check_instantiable has below?

[isabelmu]

Would this be OK?

/// 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`.

[huonw]

Yes

[isabelmu]

(thanks for all the feedback!)

[isabelmu]

@huonw I just realized that struct Foo { x: [Foo, .. 0] } and enum Foo { A([Foo, .. 0]) } still present a problem after this patch. The struct gives a stack overflow on use (let y = Foo { x: [] };) and the enum gives a stack overflow on declaration (not sure where in the code the stack overflow happens).

Right now, is_type_representable considers fixed-length-zero vectors as representable--is this OK? Are these Foo definitions supposed to be legal and usable (a new issue)?

[huonw]

Hm, good catch... I'm not sure the semantics of that corner case are decided.

I'd recommend removing the len == 0 special case (so that those Foo you've given there are not representable), filing an issue about it and leaving a FIXME #<issue number> at the relevant place in the code.

[isabelmu]

For the struct and enum both, the debug output loops on "Representing: Foo", printed from trans::adt::represent_type--maybe a zero-length check is all that is needed?

[isabelmu]

OK, will do.

[huonw]

Fixing the downstream code would be acceptable too. However, I do think filling an issue so that the precise semantics are decided upon (rather than just "this is what the compiler happens to do") would be good. :)

[isabelmu]

@huonw Removed the if len != 0 check as discussed.