implement `intercrate_ambiguity_causes` in the new solver #115996

lcnr · 2023-09-20T08:50:56Z

I added some comments but this is still somewhat of a mess. I think we should for the most part be able to treat all of this as a black box, so I can accept that this code isn't too great.

I also believe that some of the weirdness here is unavoidable, as proof trees - and their visitor - hide semantically relevant information, so they cannot perfectly represent the actual solver behavior.

There are some known bugs here when testing with ./x.py test tests/ui --bless -- --target-rustcflags -Ztrait-solver=next-coherence. While I haven't diagnosed them all in detail I believe we are able to handle them all separately

structurally_normalize currently does not normalize opaque types, resulting in divergence between the solver internal trait_ref_is_knowable and the one when computing intercrate ambiguity causes.
we don't add an intercrate_ambiguity_cause for reserved impls
we should deeply_normalize the trait ref before printing it, that requires a "best effort" version of deeply_normalize

r? @compiler-errors

rustbot · 2023-09-20T08:51:06Z

Some changes occurred to the core trait solver

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

lcnr · 2023-09-20T18:57:09Z

compiler/rustc_trait_selection/src/solve/eval_ctxt.rs

@@ -568,7 +569,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
                GoalEvaluationKind::Nested { is_normalizes_to_hack: IsNormalizesToHack::Yes },
                unconstrained_goal,
            )?;
-            self.add_goals(instantiate_goals);
+            self.nested_goals.goals.extend(instantiate_goals);


I stopped using add_goals here because the added goals aren't really nested goals and it ended up breaking the proof tree builder

Can you be more specific how it breaks things? Like I understand how we probably don't want to include these goals, and I especially understand not wanting to add ambiguous goal repeatedly over and over below, but does this affect tests detrimentally?

the issue was that we add goals while the state is DebugBuilder::AddedGoalsEvaluation.

For us to sensibly handle these nested goals we'd have to either add AddGoal as an action to AddedGoalsEvaluation or somehow take the added goals during the evaluation and "move them back" into the WipProbe once the added goals evaluation is done. Both of these feel somewhat ugly and slightly annoying to implement

lcnr · 2023-09-20T19:49:55Z

tests/ui/traits/new-solver/coherence/trait_ref_is_knowable-norm-overflow.stderr

+WARN rustc_trait_selection::traits::coherence expected an unknowable trait ref: <<LocalTy as Overflow>::Assoc as std::marker::Sized>
+WARN rustc_trait_selection::traits::coherence expected an unknowable trait ref: <<LocalTy as Overflow>::Assoc as std::marker::Sized>
+WARN rustc_trait_selection::traits::coherence expected an unknowable trait ref: <<LocalTy as Overflow>::Assoc as std::marker::Sized>
+WARN rustc_trait_selection::traits::coherence expected an unknowable trait ref: <<LocalTy as Overflow>::Assoc as std::marker::Sized>


this happens because I only changed structurally_normalize to silently fail on overflow, added a fixme there to correctly handle it.

compiler-errors · 2023-09-20T19:24:40Z

compiler/rustc_trait_selection/src/solve/eval_ctxt.rs

@@ -568,7 +569,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
                GoalEvaluationKind::Nested { is_normalizes_to_hack: IsNormalizesToHack::Yes },
                unconstrained_goal,
            )?;
-            self.add_goals(instantiate_goals);
+            self.nested_goals.goals.extend(instantiate_goals);


Can you be more specific how it breaks things? Like I understand how we probably don't want to include these goals, and I especially understand not wanting to add ambiguous goal repeatedly over and over below, but does this affect tests detrimentally?

compiler-errors · 2023-09-20T19:31:57Z

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs

@@ -29,6 +33,22 @@ use rustc_span::DUMMY_SP;
 use std::iter;
 use std::ops::Deref;

+trait ResponseT<'tcx> {


This name is kinda bad, but I can change it in a follow-up.

compiler-errors · 2023-09-20T19:34:08Z

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs

@@ -206,21 +228,21 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
    /// This returns the substitutions to instantiate the bound variables of
    /// the canonical response. This depends on the `original_values` for the
    /// bound variables.
-    fn compute_query_response_substitution(
-        &self,
+    fn compute_query_response_substitution<T: ResponseT<'tcx>>(


I don't know why this is a method on EvalCtxt anymore in this case. Should probably just be free and live in some public submodule, or even just in solve. You don't have to change this though.

compiler-errors · 2023-09-20T19:34:17Z

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs

@@ -287,9 +309,9 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
        CanonicalVarValues { var_values }
    }

-    #[instrument(level = "debug", skip(self, param_env), ret)]
+    #[instrument(level = "debug", skip(infcx, param_env), ret)]
    fn unify_query_var_values(


Same as above.

compiler-errors · 2023-09-20T19:37:52Z

compiler/rustc_trait_selection/src/solve/inspect/analyse.rs

+    /// need it.
+    pub fn visit_nested<V: ProofTreeVisitor<'tcx>>(
+        &self,
+        visitor: &mut V,


Like type folders, I feel like it makes more sense for the visitor to provide a fn infcx(&self) -> &InferCtxt<'tcx> rather than carrying it in the candidate itself. You don't have to change this though.

hmm, I had the InferCtxt as part of the context instead of the visitor as the context is responsible for calling infcx.probe which in some sense requires "ownership" of the infcx to me 🤷

compiler-errors · 2023-09-20T19:44:32Z

compiler/rustc_trait_selection/src/solve/inspect/analyse.rs

+        self.candidates_recur(&mut candidates, &mut nested_goals, &last_eval_step.evaluation);
+
+        if candidates.is_empty() {
+            candidates.push(InspectCandidate {


Hm, does this matter? If we fail to assemble any candidates, do the nested obligations that we accumulate in probes actually matter?

added a comment and slightly restructured the code. It is slightly broken rn and I am still a bit unsure how to best handle this 🤷

compiler-errors · 2023-09-20T19:48:51Z

compiler/rustc_trait_selection/src/traits/coherence.rs

-                        },
-                    );
-                }
+    if !overlap_mode.use_implicit_negative() {


What's up with the control flow changes here? Doesn't this mean we're no longer doing leak check below?

yeah, wanted to avoid computing intercrate ambiguity causes in case we didn't actually prove the obligations as we now don't collect them while proving them. Changed this back.

compiler-errors · 2023-09-20T19:51:44Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+    infcx: &InferCtxt<'tcx>,
+    obligations: &[PredicateObligation<'tcx>],
+) -> FxIndexSet<IntercrateAmbiguityCause> {
+    let mut causes: FxIndexSet<IntercrateAmbiguityCause> = Default::default();


nit: inference should guide this? You don't need to fix this.

not sure what you mean with that

Why did you need to ascribe the type here lol

compiler-errors · 2023-09-20T19:54:09Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+
+        let Goal { param_env, predicate } = goal.goal();
+
+        let trait_ref = match predicate.kind().no_bound_vars() {


Do we ever have bound preds? I'm not exactly sure how we track that extra step of instantiation with placeholders in the proof tree. Is it just a goal with 1 nested goal?

yeah, for these goals we instantiate the bound vars with placeholders in the ProbeKind::Root and then prove it as a nested goal. Added commenmt

compiler-errors · 2023-09-20T19:59:22Z

compiler/rustc_trait_selection/src/traits/coherence.rs

+                // FIXME: this isn't quite right. Changing a goal from YES with
+                // inference contraints to AMBIGUOUS can also cause a goal to not
+                // fail.
+                Ok(Certainty::Yes) => {


Changing a goal from YES with inference contraints to AMBIGUOUS can also cause a goal to not fail.

Not certain what this means.

This should be checking that the goal is YES + no inference constraints, right? We essentially don't want to report ambiguity if we would've just disregarded that branch of the tree because of a better one?

I think it really only cares about "if the coherence ambiguity candidate did not exist, would this goal be an error", changed it and improved the FIXME

This should be checking that the goal is YES + no inference constraints, right?

this is already handled in line 929

compiler-errors · 2023-09-21T01:16:57Z

r=me after addressing this one comment: #115996 (comment)

The rest can be clarified asynchronously I guess, since they're just miscellaneous thoughts.

lcnr · 2023-09-21T07:35:25Z

@bors r=compiler-errors rollup (new solver)

bors · 2023-09-21T07:35:27Z

📌 Commit 614760f has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-09-21T07:58:31Z

⌛ Testing commit 614760f with merge e4a361a...

bors · 2023-09-21T09:51:17Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing e4a361a to master...

rust-timer · 2023-09-21T11:13:14Z

Finished benchmarking commit (e4a361a): 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)

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

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: 630.991s -> 633.931s (0.47%)
Artifact size: 317.95 MiB -> 317.95 MiB (0.00%)

rustbot assigned compiler-errors Sep 20, 2023

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 Sep 20, 2023

This comment has been minimized.

Sign in to view

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from 052b3bc to 609a6fb Compare September 20, 2023 08:59

This comment has been minimized.

Sign in to view

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from a6b3582 to 30f314b Compare September 20, 2023 18:50

lcnr commented Sep 20, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch 2 times, most recently from 9e7cc9e to f0bffce Compare September 20, 2023 19:27

This comment has been minimized.

Sign in to view

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from 5fee5f7 to ceb890a Compare September 20, 2023 19:41

This comment has been minimized.

Sign in to view

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from ceb890a to ed7f5ce Compare September 20, 2023 19:47

lcnr commented Sep 20, 2023

View reviewed changes

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch 2 times, most recently from 2c1ea63 to 4bc1228 Compare September 20, 2023 19:59

compiler-errors reviewed Sep 20, 2023

View reviewed changes

lcnr added 3 commits September 21, 2023 08:17

proof trees: use for intercrate_ambiguity_causes

de53877

update tests

8eade3a

HACK: avoid hang in structurally_normalize

8024c69

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from 4bc1228 to 8024c69 Compare September 21, 2023 06:18

lcnr added 2 commits September 21, 2023 08:40

slight refactor, add comment

8e139ee

w

8167a25

This comment has been minimized.

Sign in to view

review

614760f

lcnr force-pushed the intercrate_ambiguity_causes-uwu branch from c1ff61a to 614760f Compare September 21, 2023 06:57

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 Sep 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 21, 2023

bors merged commit e4a361a into rust-lang:master Sep 21, 2023

rustbot added this to the 1.74.0 milestone Sep 21, 2023

lcnr deleted the intercrate_ambiguity_causes-uwu branch September 21, 2023 11:00


		let Goal { param_env, predicate } = goal.goal();

		let trait_ref = match predicate.kind().no_bound_vars() {

implement intercrate_ambiguity_causes in the new solver #115996

implement intercrate_ambiguity_causes in the new solver #115996

Conversation

lcnr commented Sep 20, 2023 • edited Loading

rustbot commented Sep 20, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lcnr Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Sep 21, 2023 • edited Loading

This comment has been minimized.

lcnr commented Sep 21, 2023

bors commented Sep 21, 2023

bors commented Sep 21, 2023

bors commented Sep 21, 2023

rust-timer commented Sep 21, 2023

Overall result: no relevant changes - no action needed

implement `intercrate_ambiguity_causes` in the new solver #115996

implement `intercrate_ambiguity_causes` in the new solver #115996

lcnr commented Sep 20, 2023 •

edited

Loading

lcnr Sep 20, 2023 •

edited

Loading

compiler-errors Sep 20, 2023 •

edited

Loading

compiler-errors Sep 20, 2023 •

edited

Loading

lcnr Sep 21, 2023 •

edited

Loading

compiler-errors Sep 20, 2023 •

edited

Loading

compiler-errors Sep 20, 2023 •

edited

Loading

compiler-errors commented Sep 21, 2023 •

edited

Loading