-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new solver: make all goal evaluation able to be automatically rerun #108896
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
Status: Awaiting review from the assignee but also interested parties.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
WG-trait-system-refactor
The Rustc Trait System Refactor Initiative
labels
Mar 8, 2023
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
BoxyUwU
force-pushed
the
new_solver_add_goal_fn
branch
3 times, most recently
from
March 9, 2023 15:43
c2ccd21
to
c95a1a6
Compare
☔ The latest upstream changes (presumably #109019) made this pull request unmergeable. Please resolve the merge conflicts. |
BoxyUwU
force-pushed
the
new_solver_add_goal_fn
branch
from
March 16, 2023 13:45
d0c88bd
to
00e91d4
Compare
This comment has been minimized.
This comment has been minimized.
BoxyUwU
force-pushed
the
new_solver_add_goal_fn
branch
from
March 16, 2023 14:58
1aa44d6
to
ed63201
Compare
lcnr
reviewed
Mar 17, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with this code, some final (hopefully) nits and ideas
Co-authored-by: lcnr <[email protected]>
BoxyUwU
force-pushed
the
new_solver_add_goal_fn
branch
from
March 17, 2023 13:38
e6f21f6
to
ce14a1e
Compare
lcnr
reviewed
Mar 17, 2023
BoxyUwU
force-pushed
the
new_solver_add_goal_fn
branch
from
March 17, 2023 14:40
480ddf0
to
9df35a5
Compare
@bors r+ rollup |
bors
added
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Mar 20, 2023
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Mar 21, 2023
…lcnr new solver: make all goal evaluation able to be automatically rerun It is generally wrong to call `evaluate_goal` multiple times or `evaluate_goal` and `evaluate_all` for the same `QueryResult` without correctly handling rerunning the goals when inference makes progress. Not doing so will result in the assertion in `evaluate_goal` firing because rerunning the goal will lead to a more accurate `QueryResult`. Currently there are lots of places that get this wrong and generally it is complex and error prone to handle correctly everywhere. This PR introduces a way to add goals to the `EvalCtxt` and then run all the added goals in a loop so that `evaluate_goal`/`evaluate_all` is not necessary to call manually. There are a few complications for making everything work "right": 1. the `normalizes-to` hack that replaces the rhs with an unconstrained infer var requires special casing in the new `try_evaluate_added_goals` function similar to how `evaluate_goal`'s assertion special cases that hack. 2. `assemble_candidates_after_normalizing_self_ty`'s normalization step needs to be reran for each candidate otherwise the found candidates will potentially get a more accurate `QueryResult` when rerunning the projection/trait goal which can effect the `QueryResult` of the projection/trait goal. This is implemented via `EvalCtxt::probe`'s closure's `EvalCtxt` inheriting the added goals of the `EvalCtxt` that `probe` is called on, allowing us to add goals in a probe, and then enter a nested probe for each candidate and evaluate added goals which include the normalization step's goals. I made `make_canonical_response` evaluate added goals so that it will be hard to mess up the impl of the solver by forgetting to evaluate added goals. Right now the only way to mess this up would be to call `response_no_constraints` (which from the name is obviously weird). The visibility of `evaluate_goal` means that it can be called from various `compute_x_goal` or candidate assembly functions, this is generally wrong and we should never call `evaluate_goal` manually, instead we should be calling `add_goal`/`add_goals`. This is solved by moving `evaluate_goal` `evaluate_canonical_goal` and `compute_goal` into `eval_ctxt`'s module and making them private so they cannot be called from elsewhere, forcing people to call `add_goal/s` and `evaluate_added_goals_and_make_canonical_resposne`/`try_evaluate_added_goals` --- Other changes: - removed the `&& false` that was introduced to the assertion in `evaluate_goal` in rust-lang#108839 - remove a `!self.did_overflow()` requirement in `search_graph.is_empty()` which causes goals that overflow to ICE - made `EvalCtxt::eq` take `&mut self` and add all the nested goals via `add_goals` instead of returning them as 99% of call sites just immediately called `EvalCtxt::add_goals` manually. r? `@lcnr`
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 21, 2023
…lcnr new solver: make all goal evaluation able to be automatically rerun It is generally wrong to call `evaluate_goal` multiple times or `evaluate_goal` and `evaluate_all` for the same `QueryResult` without correctly handling rerunning the goals when inference makes progress. Not doing so will result in the assertion in `evaluate_goal` firing because rerunning the goal will lead to a more accurate `QueryResult`. Currently there are lots of places that get this wrong and generally it is complex and error prone to handle correctly everywhere. This PR introduces a way to add goals to the `EvalCtxt` and then run all the added goals in a loop so that `evaluate_goal`/`evaluate_all` is not necessary to call manually. There are a few complications for making everything work "right": 1. the `normalizes-to` hack that replaces the rhs with an unconstrained infer var requires special casing in the new `try_evaluate_added_goals` function similar to how `evaluate_goal`'s assertion special cases that hack. 2. `assemble_candidates_after_normalizing_self_ty`'s normalization step needs to be reran for each candidate otherwise the found candidates will potentially get a more accurate `QueryResult` when rerunning the projection/trait goal which can effect the `QueryResult` of the projection/trait goal. This is implemented via `EvalCtxt::probe`'s closure's `EvalCtxt` inheriting the added goals of the `EvalCtxt` that `probe` is called on, allowing us to add goals in a probe, and then enter a nested probe for each candidate and evaluate added goals which include the normalization step's goals. I made `make_canonical_response` evaluate added goals so that it will be hard to mess up the impl of the solver by forgetting to evaluate added goals. Right now the only way to mess this up would be to call `response_no_constraints` (which from the name is obviously weird). The visibility of `evaluate_goal` means that it can be called from various `compute_x_goal` or candidate assembly functions, this is generally wrong and we should never call `evaluate_goal` manually, instead we should be calling `add_goal`/`add_goals`. This is solved by moving `evaluate_goal` `evaluate_canonical_goal` and `compute_goal` into `eval_ctxt`'s module and making them private so they cannot be called from elsewhere, forcing people to call `add_goal/s` and `evaluate_added_goals_and_make_canonical_resposne`/`try_evaluate_added_goals` --- Other changes: - removed the `&& false` that was introduced to the assertion in `evaluate_goal` in rust-lang#108839 - remove a `!self.did_overflow()` requirement in `search_graph.is_empty()` which causes goals that overflow to ICE - made `EvalCtxt::eq` take `&mut self` and add all the nested goals via `add_goals` instead of returning them as 99% of call sites just immediately called `EvalCtxt::add_goals` manually. r? ``@lcnr``
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 21, 2023
…lcnr new solver: make all goal evaluation able to be automatically rerun It is generally wrong to call `evaluate_goal` multiple times or `evaluate_goal` and `evaluate_all` for the same `QueryResult` without correctly handling rerunning the goals when inference makes progress. Not doing so will result in the assertion in `evaluate_goal` firing because rerunning the goal will lead to a more accurate `QueryResult`. Currently there are lots of places that get this wrong and generally it is complex and error prone to handle correctly everywhere. This PR introduces a way to add goals to the `EvalCtxt` and then run all the added goals in a loop so that `evaluate_goal`/`evaluate_all` is not necessary to call manually. There are a few complications for making everything work "right": 1. the `normalizes-to` hack that replaces the rhs with an unconstrained infer var requires special casing in the new `try_evaluate_added_goals` function similar to how `evaluate_goal`'s assertion special cases that hack. 2. `assemble_candidates_after_normalizing_self_ty`'s normalization step needs to be reran for each candidate otherwise the found candidates will potentially get a more accurate `QueryResult` when rerunning the projection/trait goal which can effect the `QueryResult` of the projection/trait goal. This is implemented via `EvalCtxt::probe`'s closure's `EvalCtxt` inheriting the added goals of the `EvalCtxt` that `probe` is called on, allowing us to add goals in a probe, and then enter a nested probe for each candidate and evaluate added goals which include the normalization step's goals. I made `make_canonical_response` evaluate added goals so that it will be hard to mess up the impl of the solver by forgetting to evaluate added goals. Right now the only way to mess this up would be to call `response_no_constraints` (which from the name is obviously weird). The visibility of `evaluate_goal` means that it can be called from various `compute_x_goal` or candidate assembly functions, this is generally wrong and we should never call `evaluate_goal` manually, instead we should be calling `add_goal`/`add_goals`. This is solved by moving `evaluate_goal` `evaluate_canonical_goal` and `compute_goal` into `eval_ctxt`'s module and making them private so they cannot be called from elsewhere, forcing people to call `add_goal/s` and `evaluate_added_goals_and_make_canonical_resposne`/`try_evaluate_added_goals` --- Other changes: - removed the `&& false` that was introduced to the assertion in `evaluate_goal` in rust-lang#108839 - remove a `!self.did_overflow()` requirement in `search_graph.is_empty()` which causes goals that overflow to ICE - made `EvalCtxt::eq` take `&mut self` and add all the nested goals via `add_goals` instead of returning them as 99% of call sites just immediately called `EvalCtxt::add_goals` manually. r? ```@lcnr```
Noratrieb
added a commit
to Noratrieb/rust
that referenced
this pull request
Mar 21, 2023
…lcnr new solver: make all goal evaluation able to be automatically rerun It is generally wrong to call `evaluate_goal` multiple times or `evaluate_goal` and `evaluate_all` for the same `QueryResult` without correctly handling rerunning the goals when inference makes progress. Not doing so will result in the assertion in `evaluate_goal` firing because rerunning the goal will lead to a more accurate `QueryResult`. Currently there are lots of places that get this wrong and generally it is complex and error prone to handle correctly everywhere. This PR introduces a way to add goals to the `EvalCtxt` and then run all the added goals in a loop so that `evaluate_goal`/`evaluate_all` is not necessary to call manually. There are a few complications for making everything work "right": 1. the `normalizes-to` hack that replaces the rhs with an unconstrained infer var requires special casing in the new `try_evaluate_added_goals` function similar to how `evaluate_goal`'s assertion special cases that hack. 2. `assemble_candidates_after_normalizing_self_ty`'s normalization step needs to be reran for each candidate otherwise the found candidates will potentially get a more accurate `QueryResult` when rerunning the projection/trait goal which can effect the `QueryResult` of the projection/trait goal. This is implemented via `EvalCtxt::probe`'s closure's `EvalCtxt` inheriting the added goals of the `EvalCtxt` that `probe` is called on, allowing us to add goals in a probe, and then enter a nested probe for each candidate and evaluate added goals which include the normalization step's goals. I made `make_canonical_response` evaluate added goals so that it will be hard to mess up the impl of the solver by forgetting to evaluate added goals. Right now the only way to mess this up would be to call `response_no_constraints` (which from the name is obviously weird). The visibility of `evaluate_goal` means that it can be called from various `compute_x_goal` or candidate assembly functions, this is generally wrong and we should never call `evaluate_goal` manually, instead we should be calling `add_goal`/`add_goals`. This is solved by moving `evaluate_goal` `evaluate_canonical_goal` and `compute_goal` into `eval_ctxt`'s module and making them private so they cannot be called from elsewhere, forcing people to call `add_goal/s` and `evaluate_added_goals_and_make_canonical_resposne`/`try_evaluate_added_goals` --- Other changes: - removed the `&& false` that was introduced to the assertion in `evaluate_goal` in rust-lang#108839 - remove a `!self.did_overflow()` requirement in `search_graph.is_empty()` which causes goals that overflow to ICE - made `EvalCtxt::eq` take `&mut self` and add all the nested goals via `add_goals` instead of returning them as 99% of call sites just immediately called `EvalCtxt::add_goals` manually. r? ````@lcnr````
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 21, 2023
Rollup of 10 pull requests Successful merges: - rust-lang#106434 (Document `Iterator::sum/product` for Option/Result) - rust-lang#108326 (Implement read_buf for a few more types) - rust-lang#108842 (Enforce non-lifetime-binders in supertrait preds are not object safe) - rust-lang#108896 (new solver: make all goal evaluation able to be automatically rerun ) - rust-lang#109124 (Add `dist.compression-profile` option to control compression speed) - rust-lang#109240 (Walk un-shifted nested `impl Trait` in trait when setting up default trait method assumptions) - rust-lang#109385 (fix typo) - rust-lang#109386 (add myself to mailmap) - rust-lang#109390 (Custom MIR: Support aggregate expressions) - rust-lang#109408 (not *all* retags might be explicit in Runtime MIR) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
WG-trait-system-refactor
The Rustc Trait System Refactor Initiative
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 is generally wrong to call
evaluate_goal
multiple times orevaluate_goal
andevaluate_all
for the sameQueryResult
without correctly handling rerunning the goals when inference makes progress. Not doing so will result in the assertion inevaluate_goal
firing because rerunning the goal will lead to a more accurateQueryResult
.Currently there are lots of places that get this wrong and generally it is complex and error prone to handle correctly everywhere. This PR introduces a way to add goals to the
EvalCtxt
and then run all the added goals in a loop so thatevaluate_goal
/evaluate_all
is not necessary to call manually.There are a few complications for making everything work "right":
the
normalizes-to
hack that replaces the rhs with an unconstrained infer var requires special casing in the newtry_evaluate_added_goals
function similar to howevaluate_goal
's assertion special cases that hack.assemble_candidates_after_normalizing_self_ty
's normalization step needs to be reran for each candidate otherwise the found candidates will potentially get a more accurateQueryResult
when rerunning the projection/trait goal which can effect theQueryResult
of the projection/trait goal.This is implemented via
EvalCtxt::probe
's closure'sEvalCtxt
inheriting the added goals of theEvalCtxt
thatprobe
is called on, allowing us to add goals in a probe, and then enter a nested probe for each candidate and evaluate added goals which include the normalization step's goals.I made
make_canonical_response
evaluate added goals so that it will be hard to mess up the impl of the solver by forgetting to evaluate added goals. Right now the only way to mess this up would be to callresponse_no_constraints
(which from the name is obviously weird).The visibility of
evaluate_goal
means that it can be called from variouscompute_x_goal
or candidate assembly functions, this is generally wrong and we should never callevaluate_goal
manually, instead we should be callingadd_goal
/add_goals
. This is solved by movingevaluate_goal
evaluate_canonical_goal
andcompute_goal
intoeval_ctxt
's module and making them private so they cannot be called from elsewhere, forcing people to calladd_goal/s
andevaluate_added_goals_and_make_canonical_resposne
/try_evaluate_added_goals
Other changes:
&& false
that was introduced to the assertion inevaluate_goal
in Canonicalize root var when making response from new solver #108839!self.did_overflow()
requirement insearch_graph.is_empty()
which causes goals that overflow to ICEEvalCtxt::eq
take&mut self
and add all the nested goals viaadd_goals
instead of returning them as 99% of call sites just immediately calledEvalCtxt::add_goals
manually.r? @lcnr