Bug fixes for struct+enum representability check #17815
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. |
| ContainsRecursive, | ||
| SelfRecursive, |
There was a problem hiding this comment.
I take it this ordering is now important; could you add a comment describing this? (Or at least noting it.)
There was a problem hiding this comment.
Good point, will do
There was a problem hiding this comment.
Do you know whether #[deriving(Ord)] is guaranteed to order the variants so the first listed is the 'least' and the last is the 'greatest'? Or is it better if I don't rely on this?
There was a problem hiding this comment.
Yes, I think we do have that guarantee.
I'm confused by this sentence; is it meant to say "identified as legally recursive"? |
| mut iter: It) -> Representability { | ||
| let mut r = Representable; | ||
|
|
||
| for ty in iter { |
There was a problem hiding this comment.
Theoretically this whole function could be: iter.fold(Representable, |r, ty| cmp::max(r, type_structurally_recursive(cx, sp, seen, ty)). Especially since the short-circuit case is the error case, and hence compilation will soon stop anyway.
|
I don't see any tests for SelfRecursive enums; could you add some, just to ensure the right thing is happening for them? (e.g. the first test case you give has infinite recursion with enums too.) Thanks so much for really focusing on this @typelist! |
No. |
|
cc me |
isabelmu
force-pushed
the
recursive-structs
branch
from
d6e51e2
to
53ddf2e
Compare
|
I think I've addressed the comments so far. (Thanks especially for the suggestions for terser use of iterators, @huonw!) Maybe this code will have to change again at some point to take associated types/items into account? (I haven't really looked into it yet.) |
The representability-checking routine ```is_type_representable``` failed to detect structural recursion in some cases, leading to stack overflow later on. The first problem was in the loop in the ```find_nonrepresentable``` function. We were improperly terminating the iteration if we saw a ```ContainsRecursive``` condition. We should have kept going in case a later member of the struct (or enum, etc) being examined was ```SelfRecursive```. The example from #17431 triggered this issue: ```rust use std::sync::Mutex; struct Foo { foo: Mutex<Option<Foo>> } impl Foo { fn bar(self) {} } fn main() {} ``` I'm not 100% sure, but I think the ```ty_enum``` case of ```fn type_structurally_recursive``` had a similar problem, since it could ```break``` on ```ContainsRecursive``` before looking at all variants. I've replaced this with a ```flat_map``` call. The second problem was that we were failing to identify code like ```struct Foo { foo: Option<Option<Foo>> }``` as SelfRecursive, even though we correctly identified ```struct Foo { foo: Option<Foo> }```. This was caused by using DefId's for the ```ContainsRecursive``` check, which meant the nested ```Option```s were identified as illegally recursive (because ```ContainsRecursive``` is not an error, we would then keep compiling and eventually hit a stack overflow). In order to make sure that we can recurse through the different ```Option``` invocations, I've changed the type of ```seen``` from ```Vec<DefId>``` to ```Vec<t>``` and added a separate ```same_type``` function to check whether two types are the same when generics are taken into account. Now we only return ```ContainsRecursive``` when this stricter check is satisfied. (There's probably a better way to do this, and I'm not sure my code is entirely correct--but my knowledge of rustc internals is pretty limited, so any help here would be appreciated!) Note that the ```SelfRecursive``` check is still comparing ```DefId```s--this is necessary to prevent code like this from being allowed: ```rust struct Foo { x: Bar<Foo> } struct Bar<T> { x: Bar<Foo> } ``` All four of the new ```issue-17431``` tests cause infinite recursion on master, and errors with this pull request. I wrote the extra ```issue-3008-4.rs``` test to make sure I wasn't introducing a regression. Fixes #17431.
…p, r=Veykril feat: Allow declaring cfg groups in rust-project.json, to help sharing common cfgs Closes rust-lang#17815.
The representability-checking routine
is_type_representablefailed to detect structural recursion in some cases, leading to stack overflow later on.The first problem was in the loop in the
find_nonrepresentablefunction. We were improperly terminating the iteration if we saw aContainsRecursivecondition. We should have kept going in case a later member of the struct (or enum, etc) being examined wasSelfRecursive. The example from #17431 triggered this issue:I'm not 100% sure, but I think the
ty_enumcase offn type_structurally_recursivehad a similar problem, since it couldbreakonContainsRecursivebefore looking at all variants. I've replaced this with aflat_mapcall.The second problem was that we were failing to identify code like
struct Foo { foo: Option<Option<Foo>> }as SelfRecursive, even though we correctly identifiedstruct Foo { foo: Option<Foo> }. This was caused by using DefId's for theContainsRecursivecheck, which meant the nestedOptions were identified as illegally recursive (becauseContainsRecursiveis not an error, we would then keep compiling and eventually hit a stack overflow).In order to make sure that we can recurse through the different
Optioninvocations, I've changed the type ofseenfromVec<DefId>toVec<t>and added a separatesame_typefunction to check whether two types are the same when generics are taken into account. Now we only returnContainsRecursivewhen this stricter check is satisfied. (There's probably a better way to do this, and I'm not sure my code is entirely correct--but my knowledge of rustc internals is pretty limited, so any help here would be appreciated!)Note that the
SelfRecursivecheck is still comparingDefIds--this is necessary to prevent code like this from being allowed:All four of the new
issue-17431tests cause infinite recursion on master, and errors with this pull request. I wrote the extraissue-3008-4.rstest to make sure I wasn't introducing a regression.Fixes #17431.