`evaluate_goal` avoid unnecessary step #142774

lcnr · 2025-06-20T07:54:18Z

This does not mess with the debug logging for the trait solver and is a very nice cleanup for #142735. E.g. for

#[derive(Clone)]
struct Wrapper<T>(T);
#[derive(Clone)]
struct Nested; // using a separate type to avoid the fast paths
fn is_clone<T: Clone>() {}
fn main() {
    is_clone::<Wrapper<Nested>>();
}

We get the following proof tree with RUSTC_LOG=rustc_type_ir::search_graph=debug,rustc_next_trait_solver=debug

 rustc_next_trait_solver::solve::eval_ctxt::evaluate_root_goal goal=Goal { param_env: ParamEnv { caller_bounds: [] }, predicate: Binder { value: TraitPredicate(<Wrapper<Nested> as std::clone::Clone>, polarity:Positive), bound_vars: [] } }, generate_proof_tree=No, span=src/main.rs:7:5: 7:34 (#0), stalled_on=None
   rustc_type_ir::search_graph::evaluate_goal input=CanonicalQueryInput { canonical: Canonical { value: QueryInput { goal: Goal { param_env: ParamEnv { caller_bounds: [] }, predicate: Binder { value: TraitPredicate(<Wrapper<Nested> as std::clone::Clone>, polarity:Positive), bound_vars: [] } }, predefined_opaques_in_body: PredefinedOpaques(PredefinedOpaquesData { opaque_types: [] }) }, max_universe: U0, variables: [] }, typing_mode: Analysis { defining_opaque_types_and_generators: [] } }, step_kind_from_parent=Unknown
     rustc_next_trait_solver::solve::eval_ctxt::probe::enter source=Impl(DefId(0:10 ~ main[21d2]::{impl#0}))
       rustc_next_trait_solver::solve::eval_ctxt::add_goal source=ImplWhereBound, goal=Goal { param_env: ParamEnv { caller_bounds: [] }, predicate: Binder { value: TraitPredicate(<_ as std::marker::Sized>, polarity:Positive), bound_vars: [] } }
       rustc_next_trait_solver::solve::eval_ctxt::add_goal source=ImplWhereBound, goal=Goal { param_env: ParamEnv { caller_bounds: [] }, predicate: Binder { value: TraitPredicate(<_ as std::clone::Clone>, polarity:Positive), bound_vars: [] } }
       rustc_type_ir::search_graph::evaluate_goal input=CanonicalQueryInput { canonical: Canonical { value: QueryInput { goal: Goal { param_env: ParamEnv { caller_bounds: [] }, predicate: Binder { value: TraitPredicate(<Nested as std::clone::Clone>, polarity:Positive), bound_vars: [] } }, predefined_opaques_in_body: PredefinedOpaques(PredefinedOpaquesData { opaque_types: [] }) }, max_universe: U0, variables: [] }, typing_mode: Analysis { defining_opaque_types_and_generators: [] } }, step_kind_from_parent=Unknown
         0ms DEBUG rustc_type_ir::search_graph global cache hit, required_depth=0
         0ms DEBUG rustc_type_ir::search_graph return=Ok(Canonical { value: Response { certainty: Yes, var_values: CanonicalVarValues { var_values: [] }, external_constraints: ExternalConstraints(ExternalConstraintsData { region_constraints: [], opaque_types: [], normalization_nested_goals: NestedNormalizationGoals([]) }) }, max_universe: U0, variables: [] })
     rustc_next_trait_solver::solve::eval_ctxt::probe::enter source=BuiltinImpl(Misc)
     rustc_next_trait_solver::solve::trait_goals::merge_trait_candidates candidates=[Candidate { source: Impl(DefId(0:10 ~ main[21d2]::{impl#0})), result: Canonical { value: Response { certainty: Yes, var_values: CanonicalVarValues { var_values: [] }, external_constraints: ExternalConstraints(ExternalConstraintsData { region_constraints: [], opaque_types: [], normalization_nested_goals: NestedNormalizationGoals([]) }) }, max_universe: U0, variables: [] } }]
       0ms DEBUG rustc_next_trait_solver::solve::trait_goals return=Ok((Canonical { value: Response { certainty: Yes, var_values: CanonicalVarValues { var_values: [] }, external_constraints: ExternalConstraints(ExternalConstraintsData { region_constraints: [], opaque_types: [], normalization_nested_goals: NestedNormalizationGoals([]) }) }, max_universe: U0, variables: [] }, Some(Misc)))
     0ms DEBUG rustc_type_ir::search_graph insert global cache, evaluation_result=EvaluationResult { encountered_overflow: false, required_depth: 1, heads: CycleHeads { heads: {} }, nested_goals: NestedGoals { nested_goals: {} }, result: Ok(Canonical { value: Response { certainty: Yes, var_values: CanonicalVarValues { var_values: [] }, external_constraints: ExternalConstraints(ExternalConstraintsData { region_constraints: [], opaque_types: [], normalization_nested_goals: NestedNormalizationGoals([]) }) }, max_universe: U0, variables: [] }) }
     0ms DEBUG rustc_type_ir::search_graph return=Ok(Canonical { value: Response { certainty: Yes, var_values: CanonicalVarValues { var_values: [] }, external_constraints: ExternalConstraints(ExternalConstraintsData { region_constraints: [], opaque_types: [], normalization_nested_goals: NestedNormalizationGoals([]) }) }, max_universe: U0, variables: [] })

also return `EvaluationResult` instead of the final `StackEntry` to make sure we correctly track information between reruns.

rustbot · 2025-06-20T07:54:22Z

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-06-20T08:11:00Z

Some changes occurred to the core trait solver

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

oli-obk · 2025-06-20T08:20:52Z

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

        );
+        canonical_goal_evaluation.query_result(canonical_response);
+        goal_evaluation.canonical_goal_evaluation(canonical_goal_evaluation);


hmm, now that I understand the various new_foo/foo APIs on ProofTreeBuilder I have questions 😆

could these be one function taking a closure and thus being more of a transaction? The current use cases seem to say so, but I'm guessing there will be more uses and you want to keep a more general API?

some doc comments would have helped (at least linking functions together that belong together)

some functions take &mut self but do not need it

canonical_goal_evalution mutating self seems surprising. I would have expected something with apply or finish in the name

could these be one function taking a closure and thus being more of a transaction? The current use cases seem to say so, but I'm guessing there will be more uses and you want to keep a more general API?

this concrete case yes, ignoring the issue of this potentially being a perf hit 🤔 I think it should be fine... I personally feel slightly iffy about having proof tree stuff be "in control of the control flow", but expect that using closures hare is easier to read

some functions take &mut self but do not need it

unsurprising :3

canonical_goal_evalution mutating self seems surprising. I would have expected something with apply or finish in the name

I agree, ideally we just have WipGoalEvaluation be the Root and not have a separate root state.

Mind pushing these cleanups until after #142735/to not do them in this PR? I am changing the control flow inside of the search graph and don't fully know how this will impact stuff.

Absolutely. I just wanted to understand things xD

oli-obk · 2025-06-20T08:22:20Z

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

+        let response = match canonical_result {
+            Err(e) => return Err(e),
            Ok(response) => response,
        };


Suggested change

let response = match canonical_result {

Err(e) => return Err(e),

Ok(response) => response,

};

let response = canonical_result?

considered it, i actually kinda feel like explicitly matching is better than using ? on a local 🤔 but fine to apply this if u think it's significantly better :3

I do think it is, but I understand verbosity as a signal that something unusual is going on can be useful. I just see this getting changed as a drive by in the future without a comment, and if you add a comment to the one liner you have as much signal as the expanded version

oli-obk

r=me once base PR lands

rust-log-analyzer · 2025-06-20T09:01:36Z

The job mingw-check-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 Documenting rustc_next_trait_solver v0.0.0 (/checkout/compiler/rustc_next_trait_solver)
[RUSTC-TIMING] rustc_middle test:false 15.697
    Checking rustc_infer v0.0.0 (/checkout/compiler/rustc_infer)
    Checking rustc_transmute v0.0.0 (/checkout/compiler/rustc_transmute)
error: unresolved link to `SearchGraph::with_new_goal`
   --> compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs:342:58
    |
342 |     /// has had the nested goal recorded on its stack ([`SearchGraph::with_new_goal`]),
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^ the type alias `SearchGraph` has no associated item named `with_new_goal`
    |
    = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(rustdoc::broken_intra_doc_links)]`

error: could not document `rustc_next_trait_solver`
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] rustc_transmute test:false 0.440
[RUSTC-TIMING] rustc_infer test:false 1.741
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:00:38
  local time: Fri Jun 20 09:01:24 UTC 2025
  network time: Fri, 20 Jun 2025 09:01:25 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

lcnr added 4 commits June 18, 2025 17:14

actually use a doc comment

2e57845

update comment

8710c1f

reevaluate: reset encountered_overflow

75da5c4

also return `EvaluationResult` instead of the final `StackEntry` to make sure we correctly track information between reruns.

evaluate_goal: accept different inputs

ecd65f8

rustbot assigned oli-obk Jun 20, 2025

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 20, 2025

This comment has been minimized.

Sign in to view

evaluate_goal: avoid unnecessary step

2a45990

lcnr force-pushed the search_graph-2 branch from 9904d31 to 3f8cf47 Compare June 20, 2025 08:10

inspect: merge [Canonical]GoalEvaluation

571a1f7

lcnr force-pushed the search_graph-2 branch from 3f8cf47 to 571a1f7 Compare June 20, 2025 08:13

oli-obk reviewed Jun 20, 2025

View reviewed changes

oli-obk approved these changes Jun 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`evaluate_goal` avoid unnecessary step #142774

`evaluate_goal` avoid unnecessary step #142774

lcnr commented Jun 20, 2025 •

edited

Loading

Uh oh!

rustbot commented Jun 20, 2025

Uh oh!

This comment has been minimized.

rustbot commented Jun 20, 2025

Uh oh!

oli-obk Jun 20, 2025

Uh oh!

lcnr Jun 20, 2025

Uh oh!

oli-obk Jun 20, 2025

Uh oh!

oli-obk Jun 20, 2025

Uh oh!

lcnr Jun 20, 2025 •

edited

Loading

Uh oh!

oli-obk Jun 20, 2025

Uh oh!

oli-obk left a comment

Uh oh!

rust-log-analyzer commented Jun 20, 2025

Uh oh!

Uh oh!

evaluate_goal avoid unnecessary step #142774

Are you sure you want to change the base?

evaluate_goal avoid unnecessary step #142774

Conversation

lcnr commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 20, 2025

Uh oh!

This comment has been minimized.

rustbot commented Jun 20, 2025

Uh oh!

oli-obk Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

lcnr Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

oli-obk Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

oli-obk Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

lcnr Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

rust-log-analyzer commented Jun 20, 2025

Uh oh!

Uh oh!

`evaluate_goal` avoid unnecessary step #142774

`evaluate_goal` avoid unnecessary step #142774

lcnr commented Jun 20, 2025 •

edited

Loading

lcnr Jun 20, 2025 •

edited

Loading