fix: Use the same recursion limit as rustc

[Wilfred]

Previously we had a recursion limit of 50 in the next_solver logic, which didn't match rustc. This prevents us generating SCIP files for some crates, and presumably would also stop r-a accepting code that rustc accepted.

For example, the element-hq/matrix-authentication-service GitHub project at commit 8a6751398d7e9ea5cf28d48992bae99541431cd6 previously crashed during SCIP file creation.

AI disclosure: Code was partly written by Opus 5, but comments, commit message and review by me.

[matthiaskrgr]

does this fix #23091 ?

[ChayimFriedman2]

See #t-compiler/rust-analyzer > autoderef / solver recursion limits. This is not something to be done lightly.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Wilfred]

Regarding performance impact of these changes:

rustc runs twice, once with a recursion limit of 128 (unless overridden), and then again with 2x the recursion limit to emit a future compatibility warning (added in rust-lang/rust#159224). So this felt relatively conservative.

I've built SCIP for a ton of internal crates at work, external crates they depend on, and large number of GitHub Rust repos for other projects, and only seen the old limit hit in three cases (the element-hq/matrix-authentication-service, iotaledger-archive/bee and renegade-fi/ark-mpc GitHub repositories). So I think it's very rare to hit this limit in practice, although of course pathological cases could occur for malformed WIP code.

The two tests needing a bigger stack nearly fit in the libtest default (2 MiB I think), it's only around recursion limit ~100 that they start hitting the stack limit (I don't have the original terminal session, I can measure exactly if it's useful). So maybe a value of 128 isn't too crazy?

I don't see any benchmarks mentioned on #20329 when the value of 50 was chosen, do you have any thoughts on how to collect more performance data? Alternatively, considering this now brings us to parity with rustc, how much of a slowdown would we tolerate?

[lnicola]

I understand the performance concern (and wanting to know the impact with broken macros), but I think not matching rustc is a lot worse.

[ChayimFriedman2]

Impact on correct code is important but impact on incorrect code is more problematic. See the linked Zulip where I described the possible problems.

[Wilfred]

OK, I've done a bit of benchmarking. The code snippet in the Zulip thread is specifically about autoderef, which seems to have its own recursion limit and isn't affected by the change here. I took the code snippet in recursive_vars, patched r-a to let me override the recursion limit and measured LSP response times.

    fn recursion_limit(self) -> usize {
        static RECURSION_LIMIT: OnceLock<usize> = OnceLock::new();
        *RECURSION_LIMIT.get_or_init(|| {
            std::env::var("RA_NEXT_SOLVER_RECURSION_LIMIT")
                .ok()
                .and_then(|it| it.parse().ok())
                .unwrap_or(128)
        })
    }

The LSP timings were:

 Limit    Diagnostics    Highlighting      Hover 
━━━━━━━  ━━━━━━━━━━━━━  ━━━━━━━━━━━━━━  ━━━━━━━━━
    10        1274 ms         1370 ms    1384 ms 
───────  ─────────────  ──────────────  ─────────
    20        1253 ms         1390 ms    1410 ms 
───────  ─────────────  ──────────────  ─────────
    50        1261 ms         1376 ms    1392 ms 
───────  ─────────────  ──────────────  ─────────
    80        1248 ms         1396 ms    1402 ms 
───────  ─────────────  ──────────────  ─────────
   128        1277 ms         1406 ms    1429 ms 
───────  ─────────────  ──────────────  ─────────
   256        1351 ms         1479 ms    1479 ms 
───────  ─────────────  ──────────────  ─────────
   512        1641 ms         1804 ms    1795 ms 
───────  ─────────────  ──────────────  ─────────
  1024        3080 ms         3402 ms    3291 ms 

This isn't a perfect benchmark, because the hover information gets progressively more &'? &'? ... long as I increase the the recursion limit, so arguably it's doing more work just printing the type as the recursion limit increases (i.e. measuring rendering time).

I ran this on my M4 Max mac laptop. It does look like 50 -> 128 is a negligible performance impact in this pathological code sample at least.

[ShoyuVanilla]

rustc runs twice, once with a recursion limit of 128 (unless overridden), and then again with 2x the recursion limit to emit a future compatibility warning (added in rust-lang/rust#159224). So this felt relatively conservative.

That FCW shouldn't be considered here as it's just a temporary band-aid to break less things when we stabilize the new solver.

---

But I feel inclined to increasing our recursion limit to match with rustc in general, due to the following reason:

Normally the greater recursion limits result in perf regressions against erroneous codes (and even in some correct ones❗ Some crates rely on refusing some candidates by hitting recursion limits. One of the most famous ones is typenum) and I see Chayim is worrying about them.

But I think the worse case is the perf regressions on the correct codes that cannot be inferred correctly with the lower recursion limit. You can see an example for this in rust-lang/rust#162648, which reports 2.62 s, 6 errors with recursion_limit = 128 while 0.77 s, no error with recursion_limit = 256.

I guess it's because of the reevaluations(or other general handlings) of overflowed goals which could be evaluated with non-ambiguous answer and thus avoidable in the bigger recursion limits and I think this might be pretty common in codes which require bigger recursion limits.