Fix stack overflow when checking for structural recursion #85012

FabianWolff · 2021-05-06T21:11:12Z

This pull request aims to fix #74224 and fix #84611. The current logic for detecting ADTs with structural recursion is flawed because it only looks at the root type, and then for exact matches. What I mean by this is that for examples such as:

struct A<T> {
    x: T,
    y: A<A<T>>,
}

struct B {
    z: A<usize>
}

fn main() {}

When checking A, the compiler correctly determines that it has an infinite size (because the "root" type is A, and A occurs, albeit with different type arguments, as a nested type in A).

However, when checking B, it also recurses into A, but now B is the root type, and it only checks for exact matches of A, but since A never precisely contains itself (only A<A<T>>, A<A<A<T>>>, etc.), an endless recursion ensues until the stack overflows.

In this PR, I have attempted to fix this behavior by implementing a two-phase checking: When checking B, my code first checks A separately and stops if A already turns out to be infinite. If not (such as for Option<T>), the second phase checks whether the root type (B) is ever nested inside itself, e.g.:

struct Foo { x: Option<Option<Foo>> }

Special care needs to be taken for mutually recursive types, e.g.:

struct A<T> {
    z: T,
    x: B<T>,
}

struct B<T> {
    y: A<T>
}

Here, both A and B both are SelfRecursive and contain a recursive type. The current behavior, which I have maintained, is to treat both A and B as SelfRecursive, and accordingly report errors for both.

rust-highfive · 2021-05-06T21:11:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

tmiasko · 2021-05-08T17:51:07Z

compiler/rustc_ty_utils/src/representability.rs

+                    // If we have encountered an ADT definition that we have not seen
+                    // before (no need to check them twice), recurse to see whether that
+                    // definition is SelfRecursive. If so, we must be ContainsRecursive.
+                    if shadow_seen.iter().len() > 1


iter() is redundant.

tmiasko · 2021-05-08T17:53:36Z

compiler/rustc_ty_utils/src/representability.rs

+                        Some(_) => {
+                            bug!("shadow_seen stack contains non-ADT type: {:?}", ty);
+                        }


Maybe shadow stack could store already extracted &'tcx ty::AdtDef?

tmiasko · 2021-05-08T17:55:10Z

compiler/rustc_ty_utils/src/representability.rs

@@ -28,8 +28,18 @@ pub fn ty_is_representable<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, sp: Span) -> R
    // 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: Vec<Ty<'_>> = Vec::new();
+    let mut shadow_seen: Vec<Ty<'_>> = Vec::new();


Could you update the comment above and describe what does it mean for a type to be present on seen stack and shadow_seen stack.

tmiasko · 2021-05-08T18:16:36Z

src/test/ui/issues/issue-74224.rs

@@ -0,0 +1,11 @@
+struct A<T> {


There has been an effort to organize tests by the area of compiler they exercise, and we try to avoid additional tests into issues directory.

Maybe we could have a new directory for structural recursion check, if no other directory is appropriate?

davidtwco

LGTM, some minor nits (in addition to the existing comments)

compiler/rustc_ty_utils/src/representability.rs

FabianWolff · 2021-05-10T13:49:06Z

@davidtwco Thanks for your suggestions! I have implemented them now.

davidtwco · 2021-05-10T18:52:18Z

@bors r+

bors · 2021-05-10T18:52:19Z

📌 Commit 98728c2 has been approved by davidtwco

tmiasko · 2021-05-10T22:50:43Z

compiler/rustc_ty_utils/src/representability.rs

+                        // mentioned above).
+                        let mut nested_seen: Vec<Ty<'_>> = vec![];
+                        result = Some(
+                            match is_type_structurally_recursive(


This independent check for structural recursion belongs somewhere outside this fold, since it not specific to a field. Outside this function most likely, since the check is not strictly about the inner types like the name are_inner_types_recursive would suggest. Maybe in is_type_structurally_recursive_inner? The same is the case for shadow_seen.first() == Some(def) a few lines earlier.

Fix stack overflow when checking for structural recursion This pull request aims to fix rust-lang#74224 and fix rust-lang#84611. The current logic for detecting ADTs with structural recursion is flawed because it only looks at the root type, and then for exact matches. What I mean by this is that for examples such as: ```rust struct A<T> { x: T, y: A<A<T>>, } struct B { z: A<usize> } fn main() {} ``` When checking `A`, the compiler correctly determines that it has an infinite size (because the "root" type is `A`, and `A` occurs, albeit with different type arguments, as a nested type in `A`). However, when checking `B`, it also recurses into `A`, but now `B` is the root type, and it only checks for _exact_ matches of `A`, but since `A` never precisely contains itself (only `A<A<T>>`, `A<A<A<T>>>`, etc.), an endless recursion ensues until the stack overflows. In this PR, I have attempted to fix this behavior by implementing a two-phase checking: When checking `B`, my code first checks `A` _separately_ and stops if `A` already turns out to be infinite. If not (such as for `Option<T>`), the second phase checks whether the root type (`B`) is ever nested inside itself, e.g.: ```rust struct Foo { x: Option<Option<Foo>> } ``` Special care needs to be taken for mutually recursive types, e.g.: ```rust struct A<T> { z: T, x: B<T>, } struct B<T> { y: A<T> } ``` Here, both `A` and `B` both _are_ `SelfRecursive` and _contain_ a recursive type. The current behavior, which I have maintained, is to treat both `A` and `B` as `SelfRecursive`, and accordingly report errors for both.

bors · 2021-05-11T00:00:57Z

⌛ Testing commit 98728c2 with merge d4d129d...

bors · 2021-05-11T02:41:33Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing d4d129d to master...

FabianWolff added 2 commits May 6, 2021 22:31

Fix stack overflow when checking for structural recursion

309710d

Add test cases for detecting structural recursion

ee882b3

rust-highfive assigned davidtwco May 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2021

tmiasko reviewed May 8, 2021

View reviewed changes

davidtwco reviewed May 10, 2021

View reviewed changes

compiler/rustc_ty_utils/src/representability.rs Outdated Show resolved Hide resolved

compiler/rustc_ty_utils/src/representability.rs Outdated Show resolved Hide resolved

davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2021

Implement changes suggested by tmiasko and davidtwco

98728c2

FabianWolff force-pushed the struct-rec branch from 6d1f6fb to 98728c2 Compare May 10, 2021 16:03

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 10, 2021

tmiasko reviewed May 10, 2021

View reviewed changes

Dylan-DPC-zz mentioned this pull request May 10, 2021

Rollup of 7 pull requests #85163

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2021

bors merged commit d4d129d into rust-lang:master May 11, 2021

rustbot added this to the 1.54.0 milestone May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stack overflow when checking for structural recursion #85012

Fix stack overflow when checking for structural recursion #85012

FabianWolff commented May 6, 2021 •

edited

Loading

rust-highfive commented May 6, 2021

tmiasko May 8, 2021

tmiasko May 8, 2021

tmiasko May 8, 2021

tmiasko May 8, 2021

davidtwco left a comment •

edited

Loading

FabianWolff commented May 10, 2021

davidtwco commented May 10, 2021

bors commented May 10, 2021

tmiasko May 10, 2021

bors commented May 11, 2021

bors commented May 11, 2021

Fix stack overflow when checking for structural recursion #85012

Fix stack overflow when checking for structural recursion #85012

Conversation

FabianWolff commented May 6, 2021 • edited Loading

rust-highfive commented May 6, 2021

tmiasko May 8, 2021

Choose a reason for hiding this comment

tmiasko May 8, 2021

Choose a reason for hiding this comment

tmiasko May 8, 2021

Choose a reason for hiding this comment

tmiasko May 8, 2021

Choose a reason for hiding this comment

davidtwco left a comment • edited Loading

Choose a reason for hiding this comment

FabianWolff commented May 10, 2021

davidtwco commented May 10, 2021

bors commented May 10, 2021

tmiasko May 10, 2021

Choose a reason for hiding this comment

bors commented May 11, 2021

bors commented May 11, 2021

FabianWolff commented May 6, 2021 •

edited

Loading

davidtwco left a comment •

edited

Loading