move implicit Sized predicate to end of list

[tlyu]

In Bounds::predicates(), move the implicit Sized predicate to the
end of the generated list. This means that if there is an explicit
Sized bound, it will be checked first, and any resulting
diagnostics will have a more useful span.

Fixes #85998, at least partially. Based on #85979, but only the last 2 commits are new for this pull request. (edit: rebased) A full fix would need to deal with where-clauses, and that seems difficult. Basically, predicates are being collected in multiple stages, and there are two places where implicit Sized predicates can be inserted: once for generic parameters, and once for where-clauses. I think this insertion is happening too early, and we should actually do it only at points where we collect all of the relevant trait bounds for a type parameter.

I could use some help interpreting the changes to the stderr output. It looks like reordering the predicates changed some diagnostics that don't obviously have anything to do with Sized bounds. Possibly some error reporting code is making assumptions about ordering of predicates? The diagnostics for src/test/ui/derives/derives-span-Hash-*.rs seem to have improved, no longer pointing at the type parameter identifier, but src/test/ui/type-alias-impl-trait/generic_duplicate_param_use9.rs became less verbose for some reason.

I also ran into an instance of #84970 while working on this, but I kind of expected that could happen, because I'm reordering predicates. I can open a separate issue on that if it would be helpful.

@estebank this seems likely to conflict (slightly?) with your work on #85947; how would you like to resolve that?

[rust-highfive]

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

[estebank]

This is arguably a regression.

[estebank]

A full fix would need to deal with where-clauses, and that seems difficult. Basically, predicates are being collected in multiple stages, and there are two places where implicit Sized predicates can be inserted: once for generic parameters, and once for where-clauses. I think this insertion is happening too early, and we should actually do it only at points where we collect all of the relevant trait bounds for a type parameter.

I made these two changes in order to have the right spans available. I'm not sure if these changes would be enough to address what you're mentioning here.

29c8a9f#diff-0a61b538a3cec072c76fecae4635af6a12ec3256860029ac70549c2aa53ab394R1480-R1485

29c8a9f#diff-ac1e6e554bac799e1e2d3886bea8fdc0d0bf04b20d33df59f5a8e92c41987ac6R2137-R2139

It looks like reordering the predicates changed some diagnostics that don't obviously have anything to do with Sized bounds. Possibly some error reporting code is making assumptions about ordering of predicates?

It some what does. I can point you at where that is happening, but if you search for where E0282 and E0284 are emitted, you'll find them. It's fine if there are small changes, we just might have to tweak the logic there to keep whatever checks we currently have.

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use9.rs seemed to not get more verbose, things just moved around, which make sense.

I also ran into an instance of #84970 while working on this, but I kind of expected that could happen, because I'm reordering predicates. I can open a separate issue on that if it would be helpful.

Did this happen on tests or while working on rustc? Did it happen with --keep-stage 1? If the problem is one of "we might hit this in CI" it's less concerning to me than "users will hit a new fingerprint issue in stable in 12 weeks".

[estebank]

r? @estebank
r=me after potentially addressing the src/test/ui/issues/issue-16966.stderr case, but if you find it difficult to do (I would just remove the note in every case where it is _: Something) or lack the time, we can land as is.

[tlyu]

Thanks for the review! I'll be taking a closer look at the things you pointed out and will probably reply in more detail soon.

I also ran into an instance of #84970 while working on this, but I kind of expected that could happen, because I'm reordering predicates. I can open a separate issue on that if it would be helpful.

Did this happen on tests or while working on rustc? Did it happen with --keep-stage 1? If the problem is one of "we might hit this in CI" it's less concerning to me than "users will hit a new fingerprint issue in stable in 12 weeks".

I opened issue #86013 with more details. Basically, when I either applied or reverted the patches in this PR, the fingerprints changed and I got the ICE on a specific UI test case. For what it's worth, I built the stage1 compiler and libstd using ./x.py build -i, and did not encounter ICEs while building the compiler itself.

As for whether this will cause fingerprint errors when this lands in stable? I'm not sure I know enough about the details of incremental compilation. Do we have some way of invalidating incremental compilation caches when there are internal changes to the compiler? (I hope so.) If so, how is this done? Is it part of the release process? Automatically according to the commit-hash stored during build?

[tlyu]

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use9.rs seemed to not get more verbose, things just moved around, which make sense.

if you compare
https://github.com/rust-lang/rust/pull/86011/files#diff-06109f234a5a60a647998669551ba82a0a549f34b4741bd5da4a449afbbe14a6L13
with
https://github.com/rust-lang/rust/pull/86011/files#diff-06109f234a5a60a647998669551ba82a0a549f34b4741bd5da4a449afbbe14a6R37
you'll notice (A, B, <A as Foo>::Bar) got deleted from the error message.

For issue-16966, I found this area to be somewhat suspicious:

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Lines 1539 to 1543 in 6c2dd25

	if self.tcx.lang_items().sized_trait() == Some(trait_ref.def_id()) {
	self.emit_inference_failure_err(body_id, span, subst, vec![], ErrorCode::E0282)
	.emit();
	return;
	}

and I'm attempting to investigate. It maybe that the type inference ambiguity causes a bunch of simultaneous errors and we previously happened to get the Sized trait predicate first, outputting the more terse error E0282 instead of E0283.

[tlyu]

What I thought was the "obvious" approach to quieting the new E0283 in issue-16966 has some nontrivial fallout. Maybe the smallest change to the diagnostics is what we already have in this pull request? Do you think we need to make additional changes in this pull request?

I tried to see what would happen if I made this block in maybe_report_ambiguity() either always or never execute:

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Lines 1539 to 1543 in 6c2dd25

	if self.tcx.lang_items().sized_trait() == Some(trait_ref.def_id()) {
	self.emit_inference_failure_err(body_id, span, subst, vec![], ErrorCode::E0282)
	.emit();
	return;
	}

I wound up with these outputs from the UI tests:
always:
always-E0282.txt
never:
never-E0282.txt

The "never" approach generates a lot more diffs in the stderr output than the "always" approach. (51 failures with "never", vs 19 failures with "always")

A quick glance at the output from the "always" approach shows that the diffs are largely making some vague verbose diagnostics less verbose.

My guess (partially confirmed by disabling some of the verbosity suppression) is that in the case of issue-16966, the type inference ambiguity was causing every trait bound to not be satisfied, and prior to my changes, we were luckily hitting Sized first and ending up with E0282, but are now getting E0283 because the first unsatisfied bound is no longer Sized.

Any opinions on whether the deleted verbosity with "always" was at all helpful? It sounds like you might be concerned that the E0283 now appearing with issue-16966 is too verbose? If so, how do you feel about the existing instances of E0283 verbosity that are deleted in the "always" approach?

[estebank]

At a very quick glance, it seems like the always case removes a bunch of useful suggestions, while the never case is just more verbose and perhaps slightly misleading...

I'll try to take a look at this PR with fresh eyes tomorrow, but I think we might want to merge as is.

[tlyu]

@estebank

Did this happen on tests or while working on rustc? Did it happen with --keep-stage 1? If the problem is one of "we might hit this in CI" it's less concerning to me than "users will hit a new fingerprint issue in stable in 12 weeks".

I wrote an update in #86013 (comment)

It seems unlikely to affect users of released versions because invalidation of the incremental caches seems to honor changes in the commit hash.

[tlyu]

Opened #86371 with some discouraging findings. Maybe the better approach is to detect whether we are dealing with violating an implicit Sized bound, and adjust the span to point at any explicit Sized bound that happens to be present, instead of to the type parameter name.

[bors]

☔ The latest upstream changes (presumably #84623) made this pull request unmergeable. Please resolve the merge conflicts.

[estebank]

@tlyu can you rebase? After that r=me, but I would still like to somehow fix src/test/ui/issues/issue-16966.stderr.

[tlyu]

@estebank Rebase done. I looked at #16966 and the change in the stderr in this PR isn't a necessarily a regression of the substance of that issue, which is about duplication of error messages. (The change in src/test/ui/issues/issue-16966.stderr is more (potentially-useless) verbosity than there used to be, but there's no repetition.)

I think there's a big rabbit hole here of various error reporting heuristics that use the failure to satisfy the implicit Sized constraint as a way of suppressing excess verbosity. I'm not sure what to do about them, because in theory, the ordering of trait bounds should be irrelevant to type checking results, right?

[bors]

☔ The latest upstream changes (presumably #87540) made this pull request unmergeable. Please resolve the merge conflicts.

[tlyu]

Rebased again.

[bors]

☔ The latest upstream changes (presumably #87628) made this pull request unmergeable. Please resolve the merge conflicts.

[camelid]

triage: @tlyu could you rebase again? Then perhaps ping estebank to let them know the PR is ready to be approved. Thanks!

[tlyu]

Rebased again. A UI test is currently failing because of #89639, but I confirmed that also occurs on master.

@rustbot label -S-waiting-on-author +S-waiting-on-review

[estebank]

Looking at the output after the rebase, I am much more comfortable landing this as is. Apologies for the delay in landing this!

@bors r+

[bors]

📌 Commit c07f5c4 has been approved by estebank