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.

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

Uplift next trait solver to rustc_next_trait_solver #126614

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jun 18, 2024

🎉

There's so many FIXMEs! Sorry! Ideally this merges with the FIXMEs and we track and squash them over the near future.

Also, this still doesn't build on anything other than rustc. I still need to fix feature = "nightly" in rustc_type_ir, and remove and fix all the nightly feature usage in the new trait solver (notably: let-chains).

Also, sorry @lcnr I know you asked for me to separate the commit where we mv rustc_trait_selection/solve/... rustc_next_trait_solver/solve/..., but I had already done all the work by that point. Luckily, git understands the file moves so it should still be relatively reviewable.

If this is still very difficult to review, then I can do some rebasing magic to try to separate this out. Please let me know!

r? lcnr

@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 Jun 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2024

Some changes occurred to the core trait solver

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

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@@ -334,149 +334,6 @@ pub struct InferCtxt<'tcx> {
pub obligation_inspector: Cell<Option<ObligationInspector<'tcx>>>,
}

impl<'tcx> ty::InferCtxtLike for InferCtxt<'tcx> {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was moved to rustc_trait_selection/src/solve/infcx.rs

@@ -830,6 +687,10 @@ impl<'tcx> InferCtxt<'tcx> {
self.tcx.dcx()
}

pub fn defining_opaque_types(&self) -> &'tcx ty::List<LocalDefId> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Since defining_opaque_types is a private field on InferCtxt, and I've chosen to expose only one trait (SolverDelegate) and not two (InferCtxtLike), we gotta make it possible to reference this in the impl in rustc_trait_solver.

@@ -48,7 +42,7 @@ impl<'tcx> EvaluationCache<'tcx> {
if cfg!(debug_assertions) {
drop(map);
let expected = CacheData { result, proof_tree, additional_depth, encountered_overflow };
let actual = self.get(tcx, key, [], Limit(additional_depth));
Copy link
Member Author

Choose a reason for hiding this comment

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

Limit is defined in rustc_session, and I was kinda too lazy to find a good ancestor crate to put Limit in...

Could fix it as a follow-up or fix it in this PR if necessary.

type GenericArgs = ty::GenericArgsRef<'tcx>;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, whenever I use rust-analyzer to sort items by definition, the spacing gets all fucked up.

@@ -301,16 +302,14 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {

let mut candidates = vec![];

self.assemble_non_blanket_impl_candidates(goal, &mut candidates);
self.assemble_impl_candidates(goal, &mut candidates);
Copy link
Member Author

Choose a reason for hiding this comment

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

This consolidates assemble_{blanket,non_blanket}_impl_candidates. See the definition of for_each_relevant_impl that I added to Interner.

/// When we encounter a coinductive cycle, we have to fetch the
/// result of that cycle while we are still computing it. Because
/// of this we continuously recompute the cycle until the result
/// of the previous iteration is equal to the final result, at which
/// point we are done.
fn fixpoint_step_in_task<F>(
fn fixpoint_step_in_task<Infcx, F>(
Copy link
Member Author

Choose a reason for hiding this comment

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

Could make the search graph generic over Infcx and use a PhantomData just like we do with ProofTreeBuilder?

};

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub struct SolverLimit(usize);
Copy link
Member Author

@compiler-errors compiler-errors Jun 18, 2024

Choose a reason for hiding this comment

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

Ad-hoc reintroduced a Limit-like newtype. We should probably consolidate this with the normal Limit. Probably best left for a follow-up.

let eligible = if node_item.is_final() {
// Non-specializable items are always projectable.
true
Ok(if target_container_def_id == impl_trait_ref.def_id {
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic was rewritten to avoid LeafDef and Node.

let tcx = ecx.interner();

let goal_trait_ref = goal.predicate.alias.trait_ref(tcx);
let impl_trait_header = tcx.impl_trait_header(impl_def_id).unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

I split impl_trait_header back into impl_trait_ref and impl_polarity b/c I couldn't be bothered to uplift ImplHeader. I can leave a FIXME to add it back if it matters that much.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

nice nice

some nits, then r=me

good work

@bors rollup=never

compiler/rustc_middle/src/ty/context.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/generic_args.rs Show resolved Hide resolved
compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved
@@ -288,7 +293,10 @@ fn fulfillment_error_for_stalled<'tcx>(
root_obligation: PredicateObligation<'tcx>,
) -> FulfillmentError<'tcx> {
let (code, refine_obligation) = infcx.probe(|_| {
match infcx.evaluate_root_goal(root_obligation.clone().into(), GenerateProofTree::No).0 {
match <&SolverDelegate<'tcx>>::from(infcx)
.evaluate_root_goal(root_obligation.clone().into(), GenerateProofTree::No)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very meh, I would prefer to implement evaluate_root_goal and evaluate_root_goal_raw via an extension trait on InferCtxt. Why do we need the SolverDelegate super trait on that one?

Copy link
Member Author

Choose a reason for hiding this comment

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

because EvalCtxt::enter_root requires a SolverDelegate

Copy link
Contributor

Choose a reason for hiding this comment

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

but enter_root is only used inside of evaluate_root_goal, so can't we wrap the InferCtxt inside of that function instead?

compiler/rustc_type_ir/src/lang_items.rs Show resolved Hide resolved
@compiler-errors
Copy link
Member Author

@bors r=lcnr rollup=never p=1 (ever so slightly bitrotty)

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit 6609501 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 Jun 18, 2024
@bors
Copy link
Contributor

bors commented Jun 18, 2024

⌛ Testing commit 6609501 with merge 8fcd4dd...

@bors
Copy link
Contributor

bors commented Jun 18, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 8fcd4dd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2024
@bors bors merged commit 8fcd4dd into rust-lang:master Jun 18, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 18, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8fcd4dd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.8% [3.8%, 3.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.1% [-2.1%, -2.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [-2.1%, 3.8%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.341s -> 691.815s (3.05%)
Artifact size: 320.42 MiB -> 323.69 MiB (1.02%)

@lcnr lcnr mentioned this pull request Sep 4, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

6 participants