Fix rustdoc ICE caused by mishandling of ambiguity errors - #162782
Conversation
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor These commits modify Please ensure that if you've changed the output:
cc @obi1kenobi |
|
Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path:
This change is correct. Please separately do a PR to fix the type-outlives fastpath 😊 @bors r+ rollup |
Fix rustdoc ICE caused by mishandling of ambiguity errors Fixes rust-lang#162557 ```rust pub trait Service<Request> { type Future; } pub trait ZebraService<Request>: Service<Request> {} impl<MaybeVerify, Request> ZebraService<Request> for MaybeVerify where MaybeVerify: Service<Request, Future: 'static> { } pub struct Verifier; impl Service<()> for Verifier { type Future = &'static (); } ``` In the above minimization of the issue, when we try to check whether the blanket impl can be applied to `Verifier`, https://github.com/rust-lang/rust/blob/a8a1e6fd9df2e094d6f09c0d57991508680acc1c/src/librustdoc/clean/blanket_impl.rs#L42-L67 We make fresh args for the where-clause and skip normalization for it. So, we have `<?MaybeVerify as Service<?Request>>::Future: 'static` bound to check. But it immediately evaluated into ambiguity due to stalled on infer vars in the fast path as the obligation contains the infer vars. But due rust-lang#162182 we evaluated it directly in the solver without `stalled_on` and this time it succeeded because we can normalize the alias into a concrete type `&'static ()` and it outlives the static region. So, I think it's very iffy to call `InferCtxt::evaluate_obligation` on a non-rigid alias and we should eagerly normalize it in `L67` from the above rustdoc code. But it may break something in rustdoc as normalizations inside it is pretty messy in general 🫠 So, I guess in another PR with crater runs. r? lcnr
…uwer Rollup of 9 pull requests Successful merges: - #163483 (Bump bootstrap compiler to 1.100.0 beta) - #161380 (only rerun const eval in next-solver if the const actually references opaques) - #162900 (Some refactorings around metadata encoding) - #163580 (Provide better doc code example for `UnixDatagram::bind_addr` and `UnixListener::bind_addr`) - #163584 ([triagebot] Ping me for debugger visualizer changes) - #162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - #163314 (move `#[macro_export]` on declarative macro check to `rustc_attr_parsing`) - #163405 (Remove some #[linkage] options) - #163581 (do not suggest precise capturing when the opaque span is in a macro expansion)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #163595 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#163595), which was unapproved. |
Sadage. It looks like one of the changed test in the rollup was affected by this again |
621507f to
d85a18b
Compare
|
@bors r+ |
Fix rustdoc ICE caused by mishandling of ambiguity errors Fixes rust-lang#162557 ```rust pub trait Service<Request> { type Future; } pub trait ZebraService<Request>: Service<Request> {} impl<MaybeVerify, Request> ZebraService<Request> for MaybeVerify where MaybeVerify: Service<Request, Future: 'static> { } pub struct Verifier; impl Service<()> for Verifier { type Future = &'static (); } ``` In the above minimization of the issue, when we try to check whether the blanket impl can be applied to `Verifier`, https://github.com/rust-lang/rust/blob/a8a1e6fd9df2e094d6f09c0d57991508680acc1c/src/librustdoc/clean/blanket_impl.rs#L42-L67 We make fresh args for the where-clause and skip normalization for it. So, we have `<?MaybeVerify as Service<?Request>>::Future: 'static` bound to check. But it immediately evaluated into ambiguity due to stalled on infer vars in the fast path as the obligation contains the infer vars. But due rust-lang#162182 we evaluated it directly in the solver without `stalled_on` and this time it succeeded because we can normalize the alias into a concrete type `&'static ()` and it outlives the static region. So, I think it's very iffy to call `InferCtxt::evaluate_obligation` on a non-rigid alias and we should eagerly normalize it in `L67` from the above rustdoc code. But it may break something in rustdoc as normalizations inside it is pretty messy in general 🫠 So, I guess in another PR with crater runs. r? lcnr
…=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in rust-lang#162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in rust-lang#162782 😅 r? lcnr
Fix rustdoc ICE caused by mishandling of ambiguity errors Fixes rust-lang#162557 ```rust pub trait Service<Request> { type Future; } pub trait ZebraService<Request>: Service<Request> {} impl<MaybeVerify, Request> ZebraService<Request> for MaybeVerify where MaybeVerify: Service<Request, Future: 'static> { } pub struct Verifier; impl Service<()> for Verifier { type Future = &'static (); } ``` In the above minimization of the issue, when we try to check whether the blanket impl can be applied to `Verifier`, https://github.com/rust-lang/rust/blob/a8a1e6fd9df2e094d6f09c0d57991508680acc1c/src/librustdoc/clean/blanket_impl.rs#L42-L67 We make fresh args for the where-clause and skip normalization for it. So, we have `<?MaybeVerify as Service<?Request>>::Future: 'static` bound to check. But it immediately evaluated into ambiguity due to stalled on infer vars in the fast path as the obligation contains the infer vars. But due rust-lang#162182 we evaluated it directly in the solver without `stalled_on` and this time it succeeded because we can normalize the alias into a concrete type `&'static ()` and it outlives the static region. So, I think it's very iffy to call `InferCtxt::evaluate_obligation` on a non-rigid alias and we should eagerly normalize it in `L67` from the above rustdoc code. But it may break something in rustdoc as normalizations inside it is pretty messy in general 🫠 So, I guess in another PR with crater runs. r? lcnr
…=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in rust-lang#162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in rust-lang#162782 😅 r? lcnr
…uwer Rollup of 13 pull requests Successful merges: - #163317 (Fix incremental compilation for fat LTO) - #163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - #151793 (Add mul_add_relaxed methods for floating-point types) - #162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - #163010 (Miri can do dirfd now) - #163535 (Improve `DocStrings` perf) - #163576 (Fix `TypeOutlives` fast-path) - #163587 (Several small span improvements) - #163612 (fix `ValidateBoundVars`) - #163632 (bump rustc-build-sysroot) - #163635 (Revert note about signum of NaN) - #163644 (Add mailmap entry) - #163651 (Remove variants from `feature-gate-autodiff-use` test)
…=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in rust-lang#162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in rust-lang#162782 😅 r? lcnr
…uwer Rollup of 14 pull requests Successful merges: - #163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - #151793 (Add mul_add_relaxed methods for floating-point types) - #162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - #163010 (Miri can do dirfd now) - #163535 (Improve `DocStrings` perf) - #163576 (Fix `TypeOutlives` fast-path) - #163587 (Several small span improvements) - #163603 (Reorganise reflection intrinsics) - #163612 (fix `ValidateBoundVars`) - #163632 (bump rustc-build-sysroot) - #163635 (Revert note about signum of NaN) - #163644 (Add mailmap entry) - #163647 (Rename `rustc_driver::run_compiler` to `compiler_entrypoint`) - #163651 (Remove variants from `feature-gate-autodiff-use` test)
Rollup merge of #162782 - ShoyuVanilla:issue-162557, r=lcnr Fix rustdoc ICE caused by mishandling of ambiguity errors Fixes #162557 ```rust pub trait Service<Request> { type Future; } pub trait ZebraService<Request>: Service<Request> {} impl<MaybeVerify, Request> ZebraService<Request> for MaybeVerify where MaybeVerify: Service<Request, Future: 'static> { } pub struct Verifier; impl Service<()> for Verifier { type Future = &'static (); } ``` In the above minimization of the issue, when we try to check whether the blanket impl can be applied to `Verifier`, https://github.com/rust-lang/rust/blob/a8a1e6fd9df2e094d6f09c0d57991508680acc1c/src/librustdoc/clean/blanket_impl.rs#L42-L67 We make fresh args for the where-clause and skip normalization for it. So, we have `<?MaybeVerify as Service<?Request>>::Future: 'static` bound to check. But it immediately evaluated into ambiguity due to stalled on infer vars in the fast path as the obligation contains the infer vars. But due #162182 we evaluated it directly in the solver without `stalled_on` and this time it succeeded because we can normalize the alias into a concrete type `&'static ()` and it outlives the static region. So, I think it's very iffy to call `InferCtxt::evaluate_obligation` on a non-rigid alias and we should eagerly normalize it in `L67` from the above rustdoc code. But it may break something in rustdoc as normalizations inside it is pretty messy in general 🫠 So, I guess in another PR with crater runs. r? lcnr
Rollup merge of #163576 - ShoyuVanilla:outlives-fast-path, r=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in #162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in #162782 😅 r? lcnr
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (4f8baea): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +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)This perf run didn't have relevant results for this metric. CyclesResults (primary 2.3%, secondary 4.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: missing data |
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - rust-lang/rust#151793 (Add mul_add_relaxed methods for floating-point types) - rust-lang/rust#162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - rust-lang/rust#163010 (Miri can do dirfd now) - rust-lang/rust#163535 (Improve `DocStrings` perf) - rust-lang/rust#163576 (Fix `TypeOutlives` fast-path) - rust-lang/rust#163587 (Several small span improvements) - rust-lang/rust#163603 (Reorganise reflection intrinsics) - rust-lang/rust#163612 (fix `ValidateBoundVars`) - rust-lang/rust#163632 (bump rustc-build-sysroot) - rust-lang/rust#163635 (Revert note about signum of NaN) - rust-lang/rust#163644 (Add mailmap entry) - rust-lang/rust#163647 (Rename `rustc_driver::run_compiler` to `compiler_entrypoint`) - rust-lang/rust#163651 (Remove variants from `feature-gate-autodiff-use` test)
Fixes #162557
In the above minimization of the issue, when we try to check whether the blanket impl can be applied to
Verifier,rust/src/librustdoc/clean/blanket_impl.rs
Lines 42 to 67 in a8a1e6f
We make fresh args for the where-clause and skip normalization for it.
So, we have
<?MaybeVerify as Service<?Request>>::Future: 'staticbound to check. But it immediately evaluated into ambiguity due to stalled on infer vars in the fast path as the obligation contains the infer vars.But due #162182 we evaluated it directly in the solver without
stalled_onand this time it succeeded because we can normalize the alias into a concrete type&'static ()and it outlives the static region.So, I think it's very iffy to call
InferCtxt::evaluate_obligationon a non-rigid alias and we should eagerly normalize it inL67from the above rustdoc code.But it may break something in rustdoc as normalizations inside it is pretty messy in general 🫠 So, I guess in another PR with crater runs.
r? lcnr