Skip to content

Conversation

@adwinwhite
Copy link
Contributor

And also split assembly_and_merge_candidates for HostEffect and NormalizesTo goals.

r? @lcnr

also split the `assembly_and_merge_candidates` for `HostEffect` and `NormalizesTo`
@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 Dec 2, 2025
Copy link
Contributor Author

@adwinwhite adwinwhite Dec 2, 2025

Choose a reason for hiding this comment

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

Not sure whether candidate preference matters to effect goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's whatever :> please add a FIXME(const_traits) to assemble_and_merge_candidates that this was copied from the old projection normalization behavior and might not be what we actually want here

//
// We currently intentionally do not guide inference this way.
// Since we skip proving the trait goal in normalizes-to goal now, the normalizes-to
// goal can successfully resolve the infer var via param env.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems unavoidable since we don't know whether the trait goal will succeed or not at this time.

@rust-log-analyzer

This comment has been minimized.

@adwinwhite adwinwhite marked this pull request as draft December 2, 2025 08:44
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2025
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.

fun, this isn't quite the behavior of the old trait solver either :3

the old solver only skips trying to prove the trait goal of we've got a builtin trait object impls. I think this ended up not really being necessary after your previous work?

idk... this sucks xd

let's see if the change impacts perf in a major way. If so, we can still avoid assembling impls in some cases at least :3

View changes since this review

D: SolverDelegate<Interner = I>,
I: Interner,
{
pub(super) fn normalize_trait_associated_term(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub(super) fn normalize_trait_associated_term(
pub(super) fn normalize_projection_term(

that's what we use in AliasTermKind 🤔

// We currently intentionally do not guide inference this way.
// Since we skip proving the trait goal in normalizes-to goal now, the normalizes-to
// goal can successfully resolve the infer var via param env.
// This causes the stalled ambiguous trait goal to succeed as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

this also doesn't compile with the old solver right now 🤔 why?

ah, because the old solver always uses assemble_candidates_from_impls and forces ambiguity unless assembly resulted in a builtin trait object candidate :<

that's wild xx

Comment on lines 461 to 463
// We don't care about overflow. If proving the trait goal overflowed, then
// it's enough to report an overflow error for that, we don't also have to
// overflow during normalization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// We don't care about overflow. If proving the trait goal overflowed, then
// it's enough to report an overflow error for that, we don't also have to
// overflow during normalization.
// We don't care about overflow. If proving the trait goal overflowed, then
// it's enough to report an overflow error for that, we don't also have to
// overflow for the const bound.

or sth

Copy link
Contributor

Choose a reason for hiding this comment

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

it's whatever :> please add a FIXME(const_traits) to assemble_and_merge_candidates that this was copied from the old projection normalization behavior and might not be what we actually want here

Comment on lines 106 to 108
// If we're normalizing an GAT, we bail if using a where-bound would constrain
// its generic arguments.
// FIXME(generic_associated_types): Addresses aggressive inference in #92917.
Copy link
Contributor

Choose a reason for hiding this comment

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

flip these two statements. I want the comment to start with the FIXME. Right now it seems like the first line is separate from the content of the FIXME

&mut self,
goal: Goal<I, ty::NormalizesTo<I>>,
) -> QueryResult<I> {
let (mut candidates, _) =
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: something something we already looked for param_env and alias-bound candidates so we don't have to assemble them again

@lcnr
Copy link
Contributor

lcnr commented Dec 2, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Dec 2, 2025
skip proving the trait goal if possible in `NormalizesTo` goal
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 2, 2025
@rust-bors

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Dec 2, 2025

☀️ Try build successful (CI)
Build commit: 405e127 (405e127a5ed88fdce13f91d651e2346944cc27f1, parent: 47cd7120d9b4e1b64eb27c87522a07888197fae8)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (405e127): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-12.9% [-40.3%, -0.3%] 9
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-1.0%, -0.9%] 2
All ❌✅ (primary) - - 0

Cycles

Results (secondary -21.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-21.1% [-25.0%, -17.0%] 3
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 469.029s -> 471.041s (0.43%)
Artifact size: 386.77 MiB -> 386.75 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 2, 2025
// ambiguous fallback candidate. It happens to be this object assembly call
// here.
let (candidates, _) =
self.assemble_and_evaluate_candidates(goal, AssembleCandidatesFrom::Object);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved object candidate out of the fast path to avoid coherence issues.

@adwinwhite adwinwhite marked this pull request as ready for review December 12, 2025 04:59
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants