Stop using translate_args in the new solver
#125776
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 |
compiler-errors
force-pushed
the
translate-args
branch
from
dd11a84
to
20f7544
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
reviewed
May 30, 2024
| impl<T> Spec for [T; 0] {} | ||
|
|
||
| fn main() { | ||
| let x: <[u32; 0] as Spec>::Assoc = 1; |
There was a problem hiding this comment.
you're not testing the [_; 0] ice?
There was a problem hiding this comment.
No, because it's not fixed in the old solver. Could add one for the new solver if you want?
There was a problem hiding this comment.
Added a test for the ambiguity case, and known-bug:unknown in the old solver
compiler-errors
force-pushed
the
translate-args
branch
from
20f7544
to
c3cb059
Compare
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
translate-args
branch
from
c3cb059
to
b411352
Compare
lcnr
reviewed
May 30, 2024
Comment on lines
237
to
268
| let args = match assoc_def.defining_node { | ||
| Node::Trait(_) => goal.predicate.alias.args, | ||
| Node::Impl(target_impl_def_id) => { | ||
| let impl_args_with_gat = goal.predicate.alias.args.rebase_onto( | ||
| tcx, | ||
| goal_trait_ref.def_id, | ||
| impl_args, | ||
| ); | ||
| if target_impl_def_id == impl_def_id { | ||
| // Same impl, no need to rebase. | ||
| impl_args_with_gat | ||
| } else { | ||
| let target_args = ecx.fresh_args_for_item(target_impl_def_id); | ||
| let target_trait_ref = tcx | ||
| .impl_trait_ref(target_impl_def_id) | ||
| .unwrap() | ||
| .instantiate(tcx, target_args); | ||
| // Relate source impl to target impl by equating trait refs. | ||
| ecx.eq(goal.param_env, impl_trait_ref, target_trait_ref)?; | ||
| // Also add predicates since they may be needed to constrain the | ||
| // target impl's params. | ||
| ecx.add_goals( | ||
| GoalSource::Misc, | ||
| tcx.predicates_of(target_impl_def_id) | ||
| .instantiate(tcx, target_args) | ||
| .into_iter() | ||
| .map(|(pred, _)| goal.with(tcx, pred)), | ||
| ); | ||
| impl_args_with_gat.rebase_onto(tcx, impl_def_id, target_args) | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
can you move this into a seperate method? 🤔 it's fairly involved and feels like it's also conceptionally self-contained.
compiler-errors
force-pushed
the
translate-args
branch
from
b411352
to
6bdc1bf
Compare
lcnr
reviewed
May 31, 2024
Comment on lines
885
to
886
| fn translate_args<'tcx>( | ||
| ecx: &mut EvalCtxt<'_, InferCtxt<'tcx>>, |
There was a problem hiding this comment.
why is that not a method on EvalCtxt? 😅
There was a problem hiding this comment.
🤷 fetch_eligible_assoc_item_def isn't one either
There was a problem hiding this comment.
i feel like both should be, r=me with or without that change
compiler-errors
force-pushed
the
translate-args
branch
from
6bdc1bf
to
20699fe
Compare
|
@bors r=lcnr rollup |
bors
added
S-waiting-on-bors
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this pull request
May 31, 2024
…lcnr Stop using `translate_args` in the new solver It was unnecessary and also sketchy, since it was doing an out-of-search-graph fulfillment loop. Added a test for the only really minor subtlety of translating args, though not sure if it was being tested before, though I wouldn't be surprised if it wasn't. r? lcnr
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 31, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#125652 (Revert propagation of drop-live information from Polonius) - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc) - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators) - rust-lang#125776 (Stop using `translate_args` in the new solver) - rust-lang#125796 (Also InstSimplify `&raw*`) - rust-lang#125807 (Also resolve the type of constants, even if we already turned it into an error constant) - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 31, 2024
Rollup merge of rust-lang#125776 - compiler-errors:translate-args, r=lcnr Stop using `translate_args` in the new solver It was unnecessary and also sketchy, since it was doing an out-of-search-graph fulfillment loop. Added a test for the only really minor subtlety of translating args, though not sure if it was being tested before, though I wouldn't be surprised if it wasn't. r? lcnr
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 was unnecessary and also sketchy, since it was doing an out-of-search-graph fulfillment loop. Added a test for the only really minor subtlety of translating args, though not sure if it was being tested before, though I wouldn't be surprised if it wasn't.
r? lcnr