Project to TyKind::Error when there are unconstrained non-lifetime (ty/const) impl params
#135057
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…d, r=<try> Project to `TyKind::Error` when there are unconstrained non-lifetime (ty/const) impl params I think this is a bit less invasive of an approach compared to rust-lang#127973. It splits the `enforce_impl_params_are_constrained` function into lifetime/non-lifetime, and queryfies the latter. We can then use the result of the latter query (`Result<(), ErrorGuaranteed>`) to intercept projection and constrain the projected type to `TyKind::Error`, which ensures that we leak no ty or const vars to places that don't expect them, like `normalize_erasing_regions`. The reason we split `enforce_impl_params_are_constrained` into two parts is because we only error for *lifetimes* if the lifetime ends up showing up in any of the associated types of the impl (e.g. we allow `impl<'a> Foo { type Assoc = (); }`). However, in order to compute the `type_of` query for the anonymous associated type of an RPITIT, we need to do trait solving (in `query collect_return_position_impl_trait_in_trait_tys`). That would induce cycles. Luckily, it turns out for lifetimes we don't even care about if they're unconstrained, since they're erased in all contexts that we are trying to fix ICEs. So it's sufficient to keep this check separated out of the query. Fixes rust-lang#123141 Fixes rust-lang#125874 Fixes rust-lang#126942 Fixes rust-lang#127804 Fixes rust-lang#130967 r? oli-obk
|
☀️ Try build successful - checks-actions |
|
@rust-timer build 8f14949 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8f14949): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.4%, secondary -0.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 763.678s -> 764.083s (0.05%) |
rustbot
added
perf-regression
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…d, r=<try>
Project to `TyKind::Error` when there are unconstrained non-lifetime (ty/const) impl params
It splits the `enforce_impl_params_are_constrained` function into lifetime/non-lifetime, and queryfies the latter. We can then use the result of the latter query (`Result<(), ErrorGuaranteed>`) to intercept projection and constrain the projected type to `TyKind::Error`, which ensures that we leak no ty or const vars to places that don't expect them, like `normalize_erasing_regions`.
The reason we split `enforce_impl_params_are_constrained` into two parts is because we only error for *lifetimes* if the lifetime ends up showing up in any of the associated types of the impl (e.g. we allow `impl<'a> Foo { type Assoc = (); }`). However, in order to compute the `type_of` query for the anonymous associated type of an RPITIT, we need to do trait solving (in `query collect_return_position_impl_trait_in_trait_tys`). That would induce cycles. Luckily, it turns out for lifetimes we don't even care about if they're unconstrained, since they're erased in all contexts that we are trying to fix ICEs. So it's sufficient to keep this check separated out of the query.
I think this is a bit less invasive of an approach compared to rust-lang#127973. The major difference between this PR and that PR is that we queryify the check instead of merging it into the `explicit_predicates_of` query, and we use the result to taint just projection goals, rather than trait goals too. This doesn't require a lot of new tracking in `ItemCtxt` and `GenericPredicates`, and it also seems to not require any other changes to typeck like that PR did.
Fixes rust-lang#123141
Fixes rust-lang#125874
Fixes rust-lang#126942
Fixes rust-lang#127804
Fixes rust-lang#130967
r? oli-obk
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (16025ae): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.1%, secondary -0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.4%, secondary -1.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 764.353s -> 763.847s (-0.07%) |
rustbot
removed
the
S-waiting-on-perf
| @@ -1719,6 +1719,12 @@ rustc_queries! { | |||
| ensure_forwards_result_if_red | |||
| } | |||
|
|
|||
| query enforce_impl_non_lifetime_params_are_constrained(key: LocalDefId) -> Result<(), ErrorGuaranteed> { | |||
| desc { |tcx| "checking that `{}`'s generics are constrained by the impl header", tcx.def_path_str(key) } | |||
| cache_on_disk_if { true } | |||
There was a problem hiding this comment.
It may be worth trying not to cache the value (unsure how we handle such query cache entries in general, en/decoding them is useless after all), but I think the regression is simply due to the extra nodes in the incremental graph.
There was a problem hiding this comment.
ok ill remove the last commit; i think we should just eat the perf
There was a problem hiding this comment.
see previous perf run for difference
compiler-errors
force-pushed
the
project-unconstrained
branch
from
7ba5e3b to
7143ef6
Compare
|
Dropped that last commit which added the @bors r=oli-obk rollup=never |
bors
added
S-waiting-on-bors
…d, r=oli-obk
Project to `TyKind::Error` when there are unconstrained non-lifetime (ty/const) impl params
It splits the `enforce_impl_params_are_constrained` function into lifetime/non-lifetime, and queryfies the latter. We can then use the result of the latter query (`Result<(), ErrorGuaranteed>`) to intercept projection and constrain the projected type to `TyKind::Error`, which ensures that we leak no ty or const vars to places that don't expect them, like `normalize_erasing_regions`.
The reason we split `enforce_impl_params_are_constrained` into two parts is because we only error for *lifetimes* if the lifetime ends up showing up in any of the associated types of the impl (e.g. we allow `impl<'a> Foo { type Assoc = (); }`). However, in order to compute the `type_of` query for the anonymous associated type of an RPITIT, we need to do trait solving (in `query collect_return_position_impl_trait_in_trait_tys`). That would induce cycles. Luckily, it turns out for lifetimes we don't even care about if they're unconstrained, since they're erased in all contexts that we are trying to fix ICEs. So it's sufficient to keep this check separated out of the query.
I think this is a bit less invasive of an approach compared to rust-lang#127973. The major difference between this PR and that PR is that we queryify the check instead of merging it into the `explicit_predicates_of` query, and we use the result to taint just projection goals, rather than trait goals too. This doesn't require a lot of new tracking in `ItemCtxt` and `GenericPredicates`, and it also seems to not require any other changes to typeck like that PR did.
Fixes rust-lang#123141
Fixes rust-lang#125874
Fixes rust-lang#126942
Fixes rust-lang#127804
Fixes rust-lang#130967
r? oli-obk
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
That looks very spurious. The test doesn't even have any associated types that are defined within the test file. @bors retry |
bors
added
S-waiting-on-bors
compiler-errors
added
the
CI-spurious-fail-mingw
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (7349f6b): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.6%, secondary -0.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -3.3%, secondary -3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 764.09s -> 762.547s (-0.20%) |
…oli-obk move footnote to ordinary comment cc rust-lang#135057
Rollup merge of rust-lang#135150 - lcnr:unconstrained-lts-comment, r=oli-obk move footnote to ordinary comment cc rust-lang#135057
|
It seems like the post-merge run had an effect on more benchmarks than the previous perf. runs, but the overall effect is still low, and the perf. costs were deemed justifiable, as this is a bugfix, so marking as triaged. @rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
It splits the
enforce_impl_params_are_constrainedfunction into lifetime/non-lifetime, and queryfies the latter. We can then use the result of the latter query (Result<(), ErrorGuaranteed>) to intercept projection and constrain the projected type toTyKind::Error, which ensures that we leak no ty or const vars to places that don't expect them, likenormalize_erasing_regions.The reason we split
enforce_impl_params_are_constrainedinto two parts is because we only error for lifetimes if the lifetime ends up showing up in any of the associated types of the impl (e.g. we allowimpl<'a> Foo { type Assoc = (); }). However, in order to compute thetype_ofquery for the anonymous associated type of an RPITIT, we need to do trait solving (inquery collect_return_position_impl_trait_in_trait_tys). That would induce cycles. Luckily, it turns out for lifetimes we don't even care about if they're unconstrained, since they're erased in all contexts that we are trying to fix ICEs. So it's sufficient to keep this check separated out of the query.I think this is a bit less invasive of an approach compared to #127973. The major difference between this PR and that PR is that we queryify the check instead of merging it into the
explicit_predicates_ofquery, and we use the result to taint just projection goals, rather than trait goals too. This doesn't require a lot of new tracking inItemCtxtandGenericPredicates, and it also seems to not require any other changes to typeck like that PR did.Fixes #123141
Fixes #125874
Fixes #126942
Fixes #127804
Fixes #130967
r? oli-obk