Don't call type_of on TAIT in defining scope in new solver
#112825
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
rust-cloud-vms
bot
force-pushed
the
tait-defining-cycle
branch
from
0ee850f
to
5cc5b09
Compare
lcnr
reviewed
Jun 27, 2023
compiler/rustc_trait_selection/src/solve/assembly/structural_traits.rs
Outdated
Show resolved
Hide resolved
rust-cloud-vms
bot
force-pushed
the
tait-defining-cycle
branch
from
5cc5b09
to
950ad25
Compare
lcnr
reviewed
Jun 29, 2023
rust-cloud-vms
bot
force-pushed
the
tait-defining-cycle
branch
from
950ad25
to
31cdf94
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
reviewed
Jun 30, 2023
rust-cloud-vms
bot
force-pushed
the
tait-defining-cycle
branch
2 times, most recently
from
1946723
to
8725c5d
Compare
lcnr
reviewed
Jun 30, 2023
Comment on lines
+124
to
+136
| // Don't call `type_of` on a local TAIT that's in the defining scope, | ||
| // since that may require calling `typeck` on the same item we're | ||
| // currently type checking, which will result in a fatal cycle that | ||
| // ideally we want to avoid, since we can make progress on this goal | ||
| // via an alias bound or a locally-inferred hidden type instead. | ||
| // | ||
| // Also, don't call `type_of` on a TAIT in `Reveal::All` mode, since | ||
| // we already normalize the self type in | ||
| // `assemble_candidates_after_normalizing_self_ty`, and we'd | ||
| // just be registering an identical candidate here. | ||
| // | ||
| // Returning `Err(NoSolution)` here is ok in `SolverMode::Coherence` | ||
| // since we'll always be registering an ambiguous candidate in | ||
| // `assemble_candidates_after_normalizing_self_ty` due to normalizing | ||
| // the TAIT. |
There was a problem hiding this comment.
I personally don't like this perspective on why we do this. Imo the query cycles are a side effect of something more fundamental:
instantiate_constituent_tys_for_auto_traitfor opaque types is a hacky, pseudo normalization-routine which does not check item bounds of the opaque- if we're able to normalize the opaque in a different way, either because we're able to define it or because we're in
Reveal::All, using regular normalization is strictly preferable to auto trait leakage
I would like you to rewrite this comment to reflect that, but also fine to wait and chat about it in sync a bit if you don't consider this perspective to be helpful or maybe even correct
There was a problem hiding this comment.
I'm not particularly attached to any specific comment here, though I think the best way to settle this is for you to just write what you'd like for this comment to say as an inline suggestion.
I'm not sure if we share the exact mental model for why this "hack" is not as much of a hack as it seems, but what I wrote above is how I see it at least from an implementation perspective.
There was a problem hiding this comment.
the comment I would like to write would also require some general code changes, put that in my notes and will look at it at some later point and open a separate PR
|
@bors r+ rollup (new solver) |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 6, 2023
…e, r=lcnr Don't call `type_of` on TAIT in defining scope in new solver It's *never* productive to call `consider_auto_trait_candidate` on a TAIT in the defining scope, since it will always lead to a query cycle since we call `type_of` on the TAIT. So let's just don't. I've reserved this behavior just to `SolverMode::Normal` just to avoid any future problems, since this is *technically* incomplete since we're discarding a candidate that could *theoretically* apply. But given such candidate assembly *always* leads to a query cycle, I think it's relatively low risk, and I could be convinced otherwise and make this apply to both solver mode. I assume it's far less likely to be encountered in coherence, though. This is much more likely to encounter in the new solver, though it can also be encountered in the old solver too, so I'm happy to discuss whether this new behavior we even want in the first place... I encountered this in a couple of failing UI tests: * `tests/ui/type-alias-impl-trait/issue-62000-associate-impl-trait-lifetimes.rs` * `tests/ui/type-alias-impl-trait/issue-93411.rs` r? `@lcnr`
|
@bors r- |
bors
added
S-waiting-on-author
rust-cloud-vms
bot
force-pushed
the
tait-defining-cycle
branch
from
8725c5d
to
388c230
Compare
|
@bors r=lcnr blessed |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 7, 2023
…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#111917 (Simplify duplicate checks for mir validator) - rust-lang#112008 (Fix incorrect documented default bufsize in bufreader/writer) - rust-lang#112825 (Don't call `type_of` on TAIT in defining scope in new solver) - rust-lang#113164 (Add a regression test for rust-lang#109054) - rust-lang#113318 (Revert "alloc: Allow comparing Boxs over different allocators", add regression test) - rust-lang#113397 (Prefer object candidates in new selection) - rust-lang#113419 (Avoid calling item_name for RPITIT) - rust-lang#113421 (Do not assert >1 RPITITs on collect_return_position_impl_trait_in_trait_tys) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's never productive to call
consider_auto_trait_candidateon a TAIT in the defining scope, since it will always lead to a query cycle since we calltype_ofon the TAIT. So let's just don't.I've reserved this behavior just to
SolverMode::Normaljust to avoid any future problems, since this is technically incomplete since we're discarding a candidate that could theoretically apply. But given such candidate assembly always leads to a query cycle, I think it's relatively low risk, and I could be convinced otherwise and make this apply to both solver mode. I assume it's far less likely to be encountered in coherence, though.This is much more likely to encounter in the new solver, though it can also be encountered in the old solver too, so I'm happy to discuss whether this new behavior we even want in the first place...
I encountered this in a couple of failing UI tests:
tests/ui/type-alias-impl-trait/issue-62000-associate-impl-trait-lifetimes.rstests/ui/type-alias-impl-trait/issue-93411.rsr? @lcnr