Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits - #162275
Conversation
| ty::Alias( | ||
| ty::IsRigid::Yes, | ||
| ty::AliasTy { kind: ty::Opaque { .. }, .. }, | ||
| ) => LowerAvailableDepth::No, |
There was a problem hiding this comment.
hmm, this means we get
Vec<opaque>: Send7opaque: Send7hidden_ty: Send6
instead of
Vec<opaque>: Send7opaque: Send6hidden_ty: Send6
This feels slightly iffy as you can also prove opaque: Send using its item bounds 🤔
does just the coroutine witness stuff fix some of the fcws?
There was a problem hiding this comment.
Oh, I forgot about alias bound candidate.
The coroutine witness change alone fixes triagebot but not bors, unfortunately.
There was a problem hiding this comment.
hmm, we could moe the step_kind computation into this function and then use a separate goal source for the opaque type auto trait leakage?
There was a problem hiding this comment.
alternatively, hmm. want to merge just the coroutine witness change for now. I do just feel generally less confident about doing this for auto trait leakage 🤔
There was a problem hiding this comment.
I really don't know what I want here :> I guess doing it for opaque auto trait leakage is also fine 🤷 it also doesn#t feel too relevant or principled, no matter what we do 😅
There was a problem hiding this comment.
It's awkward to pass the auto trait leakage info to search graph since the goal source is set by probe_and_evaluate_goal_for_constituent_tys which is used by more than auto traits.
Is it okay to check whether we're gonna pick alias bound candidate in this helper?
We'll still have
- Vec: Send 7
- opaque: Send 7
- hidden: Send 6
but only when it's proved via auto trait leakage.
855ea9e to
cf1f8ac
Compare
|
This PR was rebased onto a different main 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. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3a50673): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.6%, secondary 6.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 478.978s -> 478.032s (-0.20%) |
Current nightly fails without this. This change is undesirable for a number of reasons - see arti#2715. But it is required for now to allow building with nightly. We should revert it as soon as possible. (I am watching the upstream ticket and the MR rust-lang/rust/pull/162275 which may fix it.) Also, this is a very noisy change which affects many many crates, so we hope to revert it before the next release! THIS IS NOT THE ONLY COMMIT TO REVERT. When reverting, use git log -G 'recursion.*limit' to find appropriate commits an d git grep 'recursion.*limit' to check you're done.
| ) => { | ||
| // We only want to skip lowering depth when the proving is done via auto | ||
| // trait leakage. If the goal can be proved via item bounds, we | ||
| // should lower depth faithfully. |
There was a problem hiding this comment.
can you add a "FIXME: Ideally we'd instead lower the depth for the nested goal instead. Implementing this is a bit harder"
because you could have impl Sized: Send bound as a where-clause with TAIT/rtn. getting the recursion depth wrong isn't a big issue, but would still like to to not be slightly scuffed long-term
with this r=me
cf1f8ac to
caadd68
Compare
|
@bors r+ rollup |
Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits With deeply nested async calls, we can easily overflow evaluating auto trait goals as shown in rust-lang#159228. Users have no choice but increase the per-crate `recursion_limit` which is bad for compilation time/RSS. And downstream users may encounter the same warnings when calling library async functions. We mitigate that by no longer increasing recursion depth for coroutine witness and rigid opaques when proving auto traits. See the comments for why it's okay to do so. This fix actually makes the required depth a third of what it was before for async calls. I've checked locally that `bors` and `triagebot` no longer have FCWs. r? lcnr
Rollup of 9 pull requests Successful merges: - #162228 (tests: Run more pauth tests in CI and make them pass) - #162493 (Add support for -Zsanitizer-cfi-minimal-runtime) - #163271 (add a leak check test) - #163282 (cleanup: clean up more dependencies that are unused) - #157562 (Avoid computing layout of enums with non-int discriminants) - #162275 (Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits) - #163037 (fix and test `va_arg` on `f128` on `x86`) - #163114 (Add regression test for hang on mutually recursive trait impls) - #163255 (Check the entire library and cg_clif workspaces for permitted deps in tidy)
Rollup of 8 pull requests Successful merges: - #162228 (tests: Run more pauth tests in CI and make them pass) - #163271 (add a leak check test) - #163282 (cleanup: clean up more dependencies that are unused) - #157562 (Avoid computing layout of enums with non-int discriminants) - #162275 (Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits) - #163037 (fix and test `va_arg` on `f128` on `x86`) - #163114 (Add regression test for hang on mutually recursive trait impls) - #163255 (Check the entire library and cg_clif workspaces for permitted deps in tidy)
Rollup merge of #162275 - adwinwhite:half-depth, r=lcnr Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits With deeply nested async calls, we can easily overflow evaluating auto trait goals as shown in #159228. Users have no choice but increase the per-crate `recursion_limit` which is bad for compilation time/RSS. And downstream users may encounter the same warnings when calling library async functions. We mitigate that by no longer increasing recursion depth for coroutine witness and rigid opaques when proving auto traits. See the comments for why it's okay to do so. This fix actually makes the required depth a third of what it was before for async calls. I've checked locally that `bors` and `triagebot` no longer have FCWs. r? lcnr
Current nightly fails without this. This change is undesirable for a number of reasons - see arti#2715. But it is required for now to allow building with nightly. We should revert it as soon as possible. (I am watching the upstream ticket and the MR rust-lang/rust/pull/162275 which may fix it.) Also, this is a very noisy change which affects many many crates, so we hope to revert it before the next release! THIS IS NOT THE ONLY COMMIT TO REVERT. When reverting, use git log -G 'recursion.*limit' to find appropriate commits an d git grep 'recursion.*limit' to check you're done.
With deeply nested async calls, we can easily overflow evaluating auto trait goals as shown in #159228.
Users have no choice but increase the per-crate
recursion_limitwhich is bad for compilation time/RSS. And downstream users may encounter the same warnings when calling library async functions.We mitigate that by no longer increasing recursion depth for coroutine witness and rigid opaques when proving auto traits. See the comments for why it's okay to do so.
This fix actually makes the required depth a third of what it was before for async calls.
I've checked locally that
borsandtriagebotno longer have FCWs.r? lcnr