Make sure to insert Sized bound first into clauses list

[compiler-errors]

#120323 made it so that we don't insert an implicit Sized bound whenever we see an explicit Sized bound. However, since the code that inserts implicit sized bounds puts the bound as the first in the list, that means that it had the side-effect of possibly meaning we check Sized after checking other trait bounds.

If those trait bounds result in ambiguity or overflow or something, it may change how we winnow candidates. (edit: SEE #123303) This is likely the cause for the regression in #123279 (comment), since the impl...

impl<T: Job + Sized> AsJob for T { // <----- changing this to `Sized + Job` or just `Job` (which turns into `Sized + Job`) will FIX the issue.
}

...looks incredibly suspicious.

Fixes [after beta-backport] #123279.

Alternative is to revert #120323. I don't have a strong opinion about this, but think it may be nice to keep the diagnostic changes around.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
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

[compiler-errors]

So the Sized thing is actually a red herring and this is actually due to #123303. Still think we should fix this on beta, first, and this seems relatively harmless.

[fmease]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

You probably need to do sth. similar to #120323 (comment) / https://github.com/rust-lang/rust/pull/120323/files#diff-bdb9592e6609d61bb876c9d8a8da8b0acf3d1516ee7f35eaca144d74b96afdf0 to fix the incremental issue.

[compiler-errors]

ya i'll fix the test later -- too interested in the root cause rn lol

[estebank]

I'm ok with reverting the change that triggered this at least in beta to get some additional time for the fix for the underlying issue. Am I right to understand that even though my PR caused a user visible change, conceptually it shouldn't have? If I'm wrong on that, then my mental model of the language of how trait bounds are meant to work is incorrect and needs a refresher.

[estebank]

The failing test is one that changed on the original PR but isn't now. The reason for it is that by removing an explicit Sized bound the implicit one is added, and in the incr compilation mode where we ignore spans, that means adding or removing an explicit Sized bound is a no-op. With this change instead just reordering the bounds list, it goes back to triggering recompilation of that item. You can look at the original PR for what the test change needs to be.

r=me.

[estebank]

Idle thought: I wonder what happens if we always sorted trait bound predicates in a stable way, independent of source placement 🤔

[lcnr]

Am I right to understand that even though my PR caused a user visible change, conceptually it shouldn't have?

yes, caching in the old trait solver is broken.

Idle thought: I wonder what happens if we always sorted trait bound predicates in a stable way, independent of source placement

Breakage :3 this affects both incomplete inference guides from ParamEnv candidates and causes crates to now hit overflow for T: Impossible + Overflow bounds (which then results in fatal errors in the old solver)

[estebank]

r? @estebank
@bors r+

[bors]

📌 Commit f2fd2d8 has been approved by estebank

It is now in the queue for this repository.

[apiraino]

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted