small refactor to new projection code#107348
Conversation
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
lcnr
force-pushed
the
project-solve-new
branch
from
b898d12 to
a3b053c
Compare
lcnr
force-pushed
the
project-solve-new
branch
from
a3b053c to
9c3fe58
Compare
| ty::Binder::dummy(goal.predicate.with_self_ty(tcx, self_ty)), | ||
| ); | ||
| return ecx.evaluate_all_and_make_canonical_response(vec![new_goal]); | ||
| let (_, certainty) = ecx.evaluate_goal(new_goal)?; |
There was a problem hiding this comment.
Why do we throw away changed here, but evaluate_all runs in a loop until changed is false?
There was a problem hiding this comment.
because evaluate_all has multiple goals which can influence each other.
added a second commit which checks that the result of evaluate_goal is stable and we don't get any additional information by rerunning a goal after applying its substitutions.
Will have to change the is_identity check to ignore regions once they are implemented but that doesn't yet matter.
lcnr
force-pushed
the
project-solve-new
branch
from
92ab944 to
faa07d8
Compare
lcnr
force-pushed
the
project-solve-new
branch
from
faa07d8 to
85e6f38
Compare
|
I don't know enough of the new solver to approve. |
compiler-errors
left a comment
compiler-errors
left a comment
There was a problem hiding this comment.
r=me with nit or not
|
|
||
| /// This field is used by a debug assertion in [`EvalCtxt::evaluate_goal`], | ||
| /// see the comment in that method for more details. | ||
| in_projection_eq_hack: bool, |
There was a problem hiding this comment.
Thiis hack would be also useful for other let (_, certainty) = evaluate_goal() calls in candidate assembly, maybe generalize the name and put it in a helper like evaluate_goal_expect_stable?
There was a problem hiding this comment.
why would it be useful there? All other evaluate_goal shouldn't result in cycles when rerunning with substitutions applied (that's at least what I believe and why I've added the debug assert in the first place)
There was a problem hiding this comment.
I guess you meant for evaluate_goal added in the future? We only really hit this issue if we branch on the inference state where we actually drop info, I don't think this will happen for anything apart from hacks for better caching
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
|
@bors r+ i don't think we want it to say that you approved it 😅 |
|
💡 This pull request was already approved, no need to approve it again.
|
…-errors small refactor to new projection code extract `eq_term_and_make_canonical_response` into a helper function which also is another guarantee that the expected term does not influence candidate selection for projections. also change `evaluate_all(vec![single_goal])` to use `evaluate_goal`. the second commit now also adds a `debug_assert!` to `evaluate_goal`.
…llaumeGomez Rollup of 12 pull requests Successful merges: - rust-lang#106898 (Include both md and yaml ICE ticket templates) - rust-lang#107331 (Clean up eslint annotations and remove unused JS function) - rust-lang#107348 (small refactor to new projection code) - rust-lang#107354 (rustdoc: update Source Serif 4 from 4.004 to 4.005) - rust-lang#107412 (avoid needless checks) - rust-lang#107467 (Improve enum checks) - rust-lang#107486 (Track bound types like bound regions) - rust-lang#107491 (rustdoc: remove unused CSS from `.setting-check`) - rust-lang#107508 (`Edition` micro refactor) - rust-lang#107525 (PointeeInfo is advisory only) - rust-lang#107527 (rustdoc: stop making unstable items transparent) - rust-lang#107535 (Replace unwrap with ? in TcpListener doc) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
5 participants
extract
eq_term_and_make_canonical_responseinto a helper function which also is another guarantee that the expected term does not influence candidate selection for projections.also change
evaluate_all(vec![single_goal])to useevaluate_goal.the second commit now also adds a
debug_assert!toevaluate_goal.