Move coercion hack from coerce_unsized to check_cast

[compiler-errors]

In #136127, an if statement was added to cause us to not unconditionally commit to an unsize coercion in the case we have a coercion that looks like *const W<dyn Trait> -> *const dyn Trait, which can't succeed b/c the pointee of the LHS is not sized. Instead, we want to fall through in the cast check into a ptr-to-ptr cast, which equates the metadata of the pointees (in this case, they both have dyn Trait's metadata).

rust/compiler/rustc_hir_typeck/src/coercion.rs

Lines 602 to 651 in 4d30011

	// If the root `Source: CoerceUnsized<Target>` obligation can't possibly hold,
	// we don't have to assume that this is unsizing coercion (it will always lead to an error)
	//
	// However, we don't want to bail early all the time, since the unholdable obligations
	// may be interesting for diagnostics (such as trying to coerce `&T` to `&dyn Id<This = U>`),
	// so we only bail if there (likely) is another way to convert the types.
	if !self.infcx.predicate_may_hold(&root_obligation) {
	if let Some(dyn_metadata_adt_def_id) = self.tcx.lang_items().get(LangItem::DynMetadata)
	&& let Some(metadata_type_def_id) = self.tcx.lang_items().get(LangItem::Metadata)
	{
	self.probe(|_| {
	let ocx = ObligationCtxt::new(&self.infcx);
	// returns `true` if `<ty as Pointee>::Metadata` is `DynMetadata<_>`
	let has_dyn_trait_metadata = |ty| {
	let metadata_ty: Result<_, _> = ocx.structurally_normalize_ty(
	&ObligationCause::dummy(),
	self.fcx.param_env,
	Ty::new_alias(
	self.tcx,
	ty::AliasTyKind::Projection,
	AliasTy::new(self.tcx, metadata_type_def_id, [ty]),
	),
	);
	metadata_ty.is_ok_and(|metadata_ty| {
	metadata_ty
	.ty_adt_def()
	.is_some_and(|d| d.did() == dyn_metadata_adt_def_id)
	})
	};
	// If both types are raw pointers to a (wrapper over a) trait object,
	// this might be a cast like `*const W<dyn Trait> -> *const dyn Trait`.
	// So it's better to bail and try that. (even if the cast is not possible, for
	// example due to vtables not matching, cast diagnostic will likely still be better)
	//
	// N.B. use `target`, not `coerce_target` (the latter is a var)
	if let &ty::RawPtr(source_pointee, _) = coerce_source.kind()
	&& let &ty::RawPtr(target_pointee, _) = target.kind()
	&& has_dyn_trait_metadata(source_pointee)
	&& has_dyn_trait_metadata(target_pointee)
	{
	return Err(TypeError::Mismatch);
	}
	Ok(())
	})?;
	}
	}

However, due to how it was implemented it resulted in quite a dramatic performance hit. In order to mitigate this, this PR moves this hack into the cast check. I also made the coerce_unsized code stop emitting errors eagerly and instead bubble up the obligation that failed in the InferOk of the coercion, which is on its own a good change IMO since it was weird that we could be emitting hard errors even in a may_coerce call.

[compiler-errors]

@bors try @rust-timer queue

[compiler-errors]

cc #136127 (comment)

[bors]

⌛ Trying commit 1a7c923 with merge 5883d54...

[bors]

☀️ Try build successful - checks-actions
Build commit: 5883d54 (5883d54c552de16e31084556df0bec1ef4695e72)

[rust-timer]

Finished benchmarking commit (5883d54): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-0.7%	[-2.1%, -0.1%]	132
Improvements ✅ (secondary)	-3.4%	[-11.4%, -0.2%]	94
All ❌✅ (primary)	-0.7%	[-2.1%, -0.1%]	132

Max RSS (memory usage)

Results (primary -2.1%, secondary -2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.8%	[1.5%, 6.1%]	2
Improvements ✅ (primary)	-2.1%	[-4.1%, -0.7%]	11
Improvements ✅ (secondary)	-4.1%	[-7.2%, -2.3%]	6
All ❌✅ (primary)	-2.1%	[-4.1%, -0.7%]	11

Cycles

Results (primary -1.5%, secondary -5.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.2%]	2
Improvements ✅ (primary)	-1.5%	[-2.7%, -0.7%]	55
Improvements ✅ (secondary)	-5.4%	[-11.7%, -1.4%]	40
All ❌✅ (primary)	-1.5%	[-2.7%, -0.7%]	55

Binary size

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

Bootstrap: 773.519s -> 771.65s (-0.24%)
Artifact size: 365.06 MiB -> 365.13 MiB (0.02%)

[compiler-errors]

Gamer. I like this approach a lot better than #138438 since it's far less invasive, and this should be sufficient to fix the perf regression in #136127 (comment) since it moves the regression totally into the cast check rather than on all coercions.

r? lcnr (I am happy to explain the approach more if you have no context, or reassign it if you don't want to review this)

[lcnr]

this feels iffy to me 🤔

both because this is the first use of may_coerce in the happy path and because the code layout seems a bit odd.

If we were to keep this, please move the may_coerce into try_coercion_cast 🤔

[lcnr]

afaict we call try_coercion_cast before do_check to lint trivial casts 🤔

What would break if we just always use do_check for raw pointers and never attempt to coerce them?

[compiler-errors]

If we were to keep this, please move the may_coerce into try_coercion_cast

We can't do that. That would negatively affect diagnostics in other coercion casts (e.g. for refs) which currently rely on coercion bubbling up a more specific error. We intentionally only want to check may_coerce when casting only raw pointers, which is the point of this hack in the first place.

The purpose of calling may_coerce is to check that the nested obligations may hold. This could be written more explicitly, as a probe + coerce + check nested obligations, but it's code duplication.

What would break if we just always use do_check for raw pointers and never attempt to coerce them?

All casts that don't involve identical fat pointees. Like, *mut dyn Trait + 'long -> *mut dyn Trait + 'short relies on coercion. We also allow *mut dyn Trait + Send -> *mut dyn Trait, and raw pointer upcasting.