Be a bit more careful around exotic cycles in in the inliner #143704

compiler-errors · 2025-07-09T20:02:40Z

Copied from the comment here: #143700 (comment)

#![feature(fn_traits)]

#[inline]
pub fn a() {
    FnOnce::call_once(a, ());
    FnOnce::call_once(b, ());
}

#[inline]
pub fn b() {
    FnOnce::call_once(b, ());
    FnOnce::call_once(a, ());
}

This should demonstrate the issue. For ease of discussion, I'm gonna call the two fn-def types {a} and {b}.

When collecting the cyclic local callees in mir_callgraph_cyclic for a, we first check the first call terminator in a. We end up calling process on <{a} as FnOnce>::call_once, which ends up visiting a's instance again. This is cyclical. However, we don't end up marking FnOnce::call_once as a cyclical def id because it's a foreign item. That's fine.

When visiting the second call terminator in a, which is <{b} as FnOnce>::call_once, we end up recursing into b. We check the first terminator, which is <{b} as FnOnce>::call_once, but although that is its own mini cycle, it doesn't consider itself a cycle for the purpose of this query because it doesn't involve the root. However, when we visit the second terminator in b, which is <{a} as FnOnce>::call_once, we end up erroneously not considering that call to be cyclical since we've already inserted it into our set of seen instances, and as a consequence we don't recurse into it. This means that we never collect b as recursive.

Do this in the flipped case too, and we end up having two functions which mututally do not consider each other to be recursive participants. This leads to a query cycle.

I ended up also renaming some variables so I could more clearly understand their responsibilities in this code. Let me know if the renames are not welcome.

Fixes #143700
r? @cjgillot

rustbot · 2025-07-09T20:02:47Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2025-07-09T20:02:53Z

compiler/rustc_mir_transform/src/inline/cycle.rs

    involved: &mut FxHashSet<LocalDefId>,
    recursion_limiter: &mut FxHashMap<DefId, usize>,
    recursion_limit: Limit,
 ) -> bool {
    trace!(%caller);
-    let mut cycle_found = false;
+    let mut reaches_root = false;


This is more like "may reach root" given that we bail out with a possibly false-positive when hitting the recursion limit, but that was a mouthful.

compiler-errors · 2025-07-09T20:03:08Z

compiler/rustc_mir_transform/src/inline/cycle.rs


-    for &(callee, args) in tcx.mir_inliner_callees(caller.def) {
+    for &(callee_def_id, args) in tcx.mir_inliner_callees(caller.def) {


with the name callee, DefId ends up shadowing Instance ends up shadowing DefId again 😆

compiler-errors · 2025-07-09T20:04:39Z

compiler/rustc_mir_transform/src/inline/cycle.rs

+            // end up reaching that same recursive callee through some *other* cycle.
+            c
+        } else {
+            seen.insert(callee, false);


I think it's safe to consider this to be false initially.

compiler-errors · 2025-07-09T20:04:57Z

compiler/rustc_mir_transform/src/inline/cycle.rs

-            cycle_found = true;
+            reaches_root = true;
+            seen.insert(callee, true);
+            continue;


I don't think it's necessary to recurse on callees here, so I just continued.

compiler-errors · 2025-07-09T20:07:30Z

tests/mir-opt/inline_double_cycle.b.Inline.panic-unwind.diff

+          StorageLive(_1);
+          StorageLive(_2);
+          _2 = ();
+-         _1 = <fn() {b} as FnOnce<()>>::call_once(b, move _2) -> [return: bb1, unwind continue];


It's nice to see we didn't break inlining at least for the FnOnce call. It's kind of a shame we can't inline a into b or vice versa, but it's obviously not clear which one to inline into the other without adding arbitrary ordering which is bad.

cjgillot · 2025-07-12T10:24:49Z

Thanks a lot for fixing this!
@bors r+

bors · 2025-07-12T10:24:52Z

📌 Commit c39a187 has been approved by cjgillot

It is now in the queue for this repository.

…gillot Be a bit more careful around exotic cycles in in the inliner Copied from the comment here: rust-lang#143700 (comment) --- ```rust #![feature(fn_traits)] #[inline] pub fn a() { FnOnce::call_once(a, ()); FnOnce::call_once(b, ()); } #[inline] pub fn b() { FnOnce::call_once(b, ()); FnOnce::call_once(a, ()); } ``` This should demonstrate the issue. For ease of discussion, I'm gonna call the two fn-def types `{a}` and `{b}`. When collecting the cyclic local callees in `mir_callgraph_cyclic` for `a`, we first check the first call terminator in `a`. We end up calling process on `<{a} as FnOnce>::call_once`, which ends up visiting `a`'s instance again. This is cyclical. However, we don't end up marking `FnOnce::call_once` as a cyclical def id because it's a foreign item. That's fine. When visiting the second call terminator in `a`, which is `<{b} as FnOnce>::call_once`, we end up recursing into `b`. We check the first terminator, which is `<{b} as FnOnce>::call_once`, but although that is its own mini cycle, it doesn't consider itself a cycle for the purpose of this query because it doesn't involve the *root*. However, when we visit the *second* terminator in `b`, which is `<{a} as FnOnce>::call_once`, we end up **erroneously** *not* considering that call to be cyclical since we've already inserted it into our set of seen instances, and as a consequence we don't recurse into it. This means that we never collect `b` as recursive. Do this in the flipped case too, and we end up having two functions which mututally do not consider each other to be recursive participants. This leads to a query cycle. --- I ended up also renaming some variables so I could more clearly understand their responsibilities in this code. Let me know if the renames are not welcome. Fixes rust-lang#143700 r? `@cjgillot`

Rollup of 9 pull requests Successful merges: - #143213 (de-duplicate condition scoping logic between AST→HIR lowering and `ScopeTree` construction) - #143461 (make `cfg_select` a builtin macro) - #143519 (Check assoc consts and tys later like assoc fns) - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const) - #143704 (Be a bit more careful around exotic cycles in in the inliner) - #143774 (constify `From` and `Into`) - #143786 (Fix fallback for CI_JOB_NAME) - #143796 (Fix ICE for parsed attributes with longer path not handled by CheckAttribute) - #143798 (Remove format short command trait) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Be a bit more careful around exotic cycles in in the inliner Copied from the comment here: rust-lang#143700 (comment) --- ```rust #![feature(fn_traits)] #[inline] pub fn a() { FnOnce::call_once(a, ()); FnOnce::call_once(b, ()); } #[inline] pub fn b() { FnOnce::call_once(b, ()); FnOnce::call_once(a, ()); } ``` This should demonstrate the issue. For ease of discussion, I'm gonna call the two fn-def types `{a}` and `{b}`. When collecting the cyclic local callees in `mir_callgraph_cyclic` for `a`, we first check the first call terminator in `a`. We end up calling process on `<{a} as FnOnce>::call_once`, which ends up visiting `a`'s instance again. This is cyclical. However, we don't end up marking `FnOnce::call_once` as a cyclical def id because it's a foreign item. That's fine. When visiting the second call terminator in `a`, which is `<{b} as FnOnce>::call_once`, we end up recursing into `b`. We check the first terminator, which is `<{b} as FnOnce>::call_once`, but although that is its own mini cycle, it doesn't consider itself a cycle for the purpose of this query because it doesn't involve the *root*. However, when we visit the *second* terminator in `b`, which is `<{a} as FnOnce>::call_once`, we end up **erroneously** *not* considering that call to be cyclical since we've already inserted it into our set of seen instances, and as a consequence we don't recurse into it. This means that we never collect `b` as recursive. Do this in the flipped case too, and we end up having two functions which mututally do not consider each other to be recursive participants. This leads to a query cycle. --- I ended up also renaming some variables so I could more clearly understand their responsibilities in this code. Let me know if the renames are not welcome. Fixes rust-lang#143700 r? ``@cjgillot``

Rollup of 14 pull requests Successful merges: - #143301 (`tests/ui`: A New Order [26/N]) - #143461 (make `cfg_select` a builtin macro) - #143519 (Check assoc consts and tys later like assoc fns) - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const) - #143634 (interpret/allocation: expose init + write_wildcards on a range) - #143679 (Preserve the .debug_gdb_scripts section) - #143685 (Resolve: merge `source_bindings` and `target_bindings` into `bindings`) - #143704 (Be a bit more careful around exotic cycles in in the inliner) - #143734 (Refactor resolve resolution bindings) - #143774 (constify `From` and `Into`) - #143785 (Add --compile-time-deps argument for x check) - #143786 (Fix fallback for CI_JOB_NAME) - #143825 (clippy: fix test filtering when TESTNAME is empty) - #143826 (Fix command trace) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2025-07-13T08:24:22Z

@bors r-
#143866 (comment)

rustbot assigned cjgillot Jul 9, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 9, 2025

compiler-errors force-pushed the cycle-exotic branch from dd239d4 to abbadb1 Compare July 9, 2025 20:04

compiler-errors commented Jul 9, 2025

View reviewed changes

Be a bit more careful around exotic cycles in in the inliner

c39a187

compiler-errors force-pushed the cycle-exotic branch from abbadb1 to c39a187 Compare July 9, 2025 20:06

compiler-errors commented Jul 9, 2025

View reviewed changes

estebank approved these changes Jul 10, 2025

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2025

matthiaskrgr mentioned this pull request Jul 12, 2025

Rollup of 9 pull requests #143840

Closed

fmease mentioned this pull request Jul 13, 2025

Rollup of 14 pull requests #143866

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Be a bit more careful around exotic cycles in in the inliner #143704

Be a bit more careful around exotic cycles in in the inliner #143704

compiler-errors commented Jul 9, 2025 •

edited

Loading

Uh oh!

rustbot commented Jul 9, 2025

Uh oh!

compiler-errors Jul 9, 2025 •

edited

Loading

Uh oh!

compiler-errors Jul 9, 2025

Uh oh!

compiler-errors Jul 9, 2025

Uh oh!

compiler-errors Jul 9, 2025

Uh oh!

compiler-errors Jul 9, 2025

Uh oh!

cjgillot commented Jul 12, 2025

Uh oh!

bors commented Jul 12, 2025

Uh oh!

matthiaskrgr commented Jul 13, 2025

Uh oh!

Uh oh!


		for &(callee, args) in tcx.mir_inliner_callees(caller.def) {
		for &(callee_def_id, args) in tcx.mir_inliner_callees(caller.def) {

Be a bit more careful around exotic cycles in in the inliner #143704

Are you sure you want to change the base?

Be a bit more careful around exotic cycles in in the inliner #143704

Conversation

compiler-errors commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 9, 2025

Uh oh!

compiler-errors Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Jul 12, 2025

Uh oh!

bors commented Jul 12, 2025

Uh oh!

matthiaskrgr commented Jul 13, 2025

Uh oh!

Uh oh!

compiler-errors commented Jul 9, 2025 •

edited

Loading

compiler-errors Jul 9, 2025 •

edited

Loading