Skip to content
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.

Sign up for GitHub

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

Jump to bottom

small refactor to new projection code #107348

Merged
merged 2 commits into from
Feb 1, 2023
Merged

small refactor to new projection code #107348

merged 2 commits into from
Feb 1, 2023

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jan 27, 2023 •
edited
Loading

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.

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 (-Znext-solver) labels Jan 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

compiler-errors
compiler-errors reviewed Jan 27, 2023
View reviewed changes
compiler/rustc_trait_selection/src/solve/project_goals.rs
@@ -453,7 +456,8 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> {
tcx,
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)?;
Copy link
Member

@compiler-errors compiler-errors Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we throw away changed here, but evaluate_all runs in a loop until changed is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cjgillot
Copy link
Contributor

I don't know enough of the new solver to approve.
r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned cjgillot Jan 28, 2023
compiler-errors
compiler-errors approved these changes Jan 28, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with nit or not

compiler/rustc_trait_selection/src/solve/mod.rs

/// 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@compiler-errors compiler-errors mentioned this pull request Jan 29, 2023
Merged
@lcnr
Copy link
Contributor Author

lcnr commented Jan 31, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 31, 2023

📌 Commit 85e6f38 has been approved by lcnr

It is now in the queue for this repository.

@bors 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 Jan 31, 2023
@compiler-errors
Copy link
Member

@bors r+

i don't think we want it to say that you approved it 😅

@bors
Copy link
Contributor

bors commented Jan 31, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jan 31, 2023

📌 Commit 85e6f38 has been approved by compiler-errors

It is now in the queue for this repository.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 31, 2023
@GuillaumeGomez
Rollup merge of rust-lang#107348 - lcnr:project-solve-new, r=compiler…
bf54246 
…-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`.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 31, 2023
Closed
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 31, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2023
@bors
Auto merge of rust-lang#107536 - GuillaumeGomez:rollup-xv7dx2h, r=Gui…
ad8e1dc 
…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
@bors bors merged commit d65f60d into rust-lang:master Feb 1, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 1, 2023
@lcnr lcnr deleted the project-solve-new branch February 1, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

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 (-Znext-solver)
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lcnr @rustbot @cjgillot @bors @compiler-errors