Avoid stack overflow when checking struct/enum representability#11839
Avoid stack overflow when checking struct/enum representability#11839bors merged 3 commits intorust-lang:masterfrom
Conversation
src/librustc/middle/ty.rs
Outdated
There was a problem hiding this comment.
This could be return if i == 0 { SelfRecursive } else { ContainsRecursive }. (Also, the ; at the end of the if here is not required.)
|
Looks reasonable to me; this is a cool first contribution! I'd guess this fixes (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".) |
|
Oh! Didn't realize #3779 existed. I thought |
|
I'm actually still able to trigger a stack overflow in fun cases like: 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? |
|
Woo! Nice 1.0 fix. |
|
This should be OK now. @huonw what next? |
There was a problem hiding this comment.
Do you reckon you could put a comment above this like check_instantiable has below?
There was a problem hiding this comment.
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`.
|
(thanks for all the feedback!) |
|
@huonw I just realized that Right now, |
|
Hm, good catch... I'm not sure the semantics of that corner case are decided. I'd recommend removing the |
|
For the struct and enum both, the debug output loops on " |
|
OK, will do. |
|
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. :) |
|
@huonw Removed the |
…s non-representable until rust-lang#11924 is resolved
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.
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.