Prefer built-in sized impls (and only sized impls) for rigid types always

[compiler-errors]

This PR changes the confirmation of Sized obligations to unconditionally prefer the built-in impl, even if it has nested obligations. This also changes all other built-in impls (namely, Copy/Clone/DiscriminantKind/Pointee) to not prefer built-in impls over param-env impls. This aligns the old solver with the behavior of the new solver.

---

In the old solver, we register many builtin candidates with the BuiltinCandidate { has_nested: bool } candidate kind. The precedence this candidate takes over other candidates is based on the has_nested field. We only prefer builtin impls over param-env candidates if has_nested is false

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

Lines 1804 to 1866 in 2b4694a

	// We prefer trivial builtin candidates, i.e. builtin impls without any nested
	// requirements, over all others. This is a fix for #53123 and prevents winnowing
	// from accidentally extending the lifetime of a variable.
	let mut trivial_builtin = candidates
	.iter()
	.filter(|c| matches!(c.candidate, BuiltinCandidate { has_nested: false }));
	if let Some(_trivial) = trivial_builtin.next() {
	// There should only ever be a single trivial builtin candidate
	// as they would otherwise overlap.
	debug_assert_eq!(trivial_builtin.next(), None);
	return Some(BuiltinCandidate { has_nested: false });
	}
	// Before we consider where-bounds, we have to deduplicate them here and also
	// drop where-bounds in case the same where-bound exists without bound vars.
	// This is necessary as elaborating super-trait bounds may result in duplicates.
	'search_victim: loop {
	for (i, this) in candidates.iter().enumerate() {
	let ParamCandidate(this) = this.candidate else { continue };
	for (j, other) in candidates.iter().enumerate() {
	if i == j {
	continue;
	}
	let ParamCandidate(other) = other.candidate else { continue };
	if this == other {
	candidates.remove(j);
	continue 'search_victim;
	}
	if this.skip_binder().trait_ref == other.skip_binder().trait_ref
	&& this.skip_binder().polarity == other.skip_binder().polarity
	&& !this.skip_binder().trait_ref.has_escaping_bound_vars()
	{
	candidates.remove(j);
	continue 'search_victim;
	}
	}
	}
	break;
	}
	// The next highest priority is for non-global where-bounds. However, while we don't
	// prefer global where-clauses here, we do bail with ambiguity when encountering both
	// a global and a non-global where-clause.
	//
	// Our handling of where-bounds is generally fairly messy but necessary for backwards
	// compatibility, see #50825 for why we need to handle global where-bounds like this.
	let is_global = |c: ty::PolyTraitPredicate<'tcx>| c.is_global() && !c.has_bound_vars();
	let param_candidates = candidates
	.iter()
	.filter_map(|c| if let ParamCandidate(p) = c.candidate { Some(p) } else { None });
	let mut has_global_bounds = false;
	let mut param_candidate = None;
	for c in param_candidates {
	if is_global(c) {
	has_global_bounds = true;
	} else if param_candidate.replace(c).is_some() {
	// Ambiguity, two potentially different where-clauses
	return None;
	}
	}

Preferring param-env candidates when the builtin candidate has nested obligations still ends up leading to detrimental inference guidance, like:

fn hello<T>() where (T,): Sized {
    let x: (_,) = Default::default();
    // ^^ The `Sized` obligation on the variable infers `_ = T`.
    let x: (i32,) = x;
    // We error here, both a type mismatch and also b/c `T: Default` doesn't hold.
}

Therefore this PR adjusts the candidate precedence of Sized obligations by making them a distinct candidate kind and unconditionally preferring them over all other candidate kinds.

Special-casing Sized this way is necessary as there are a lot of traits with a Sized super-trait bound, so a &'a str: From<T> where-bound results in an elaborated &'a str: Sized bound. People tend to not add explicit where-clauses which overlap with builtin impls, so this tends to not be an issue for other traits.

We don't know of any tests/crates which need preference for other builtin traits. As this causes builtin impls to diverge from user-written impls we would like to minimize the affected traits. Otherwise e.g. moving impls for tuples to std by using variadic generics would be a breaking change. For other builtin impls it's also easier for the preference of builtin impls over where-bounds to result in issues.

---

There are two ways preferring builtin impls over where-bounds can be incorrect and undesirable:

• applying the builtin impl results in undesirable region constraints. E.g. if only MyType<'static> implements Copy then a goal like (MyType<'a>,): Copy would require 'a == 'static so we must not prefer it over a (MyType<'a>,): Copy where-bound
  • this is mostly not an issue for Sized as all Sized impls are builtin and don't add any region constraints not already required for the type to be well-formed
  • however, even with Sized this is still an issue if a nested goal also gets proven via a where-bound: playground
• if the builtin impl has associated types, we should not prefer it over where-bounds when normalizing that associated type. This can result in normalization adding more region constraints than just proving trait bounds. candidate selection for normalization and trait goals disagree #133044
  • not an issue for Sized as it doesn't have associated types.

r? lcnr

[compiler-errors]

@bors try @rust-timer queue

(and crater)

[bors]

⌛ Trying commit 49e4fbc with merge 51b1964...

[bors]

☀️ Try build successful - checks-actions
Build commit: 51b1964 (51b1964e35e5e26f8cd938940f0da6f7b5cc54fc)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-138176 created and queued.
🤖 Automatically detected try build 51b1964
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-138176 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (51b1964): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.3%	[0.1%, 0.4%]	6
Regressions ❌ (secondary)	0.4%	[0.2%, 0.9%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.3%	[0.1%, 0.4%]	6

Max RSS (memory usage)

Results (primary 1.0%, secondary -1.5%)

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)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	4.1%	[3.3%, 4.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-6.1%, -2.7%]	5
All ❌✅ (primary)	1.0%	[1.0%, 1.0%]	1

Cycles

Results (primary 2.7%, secondary 1.3%)

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)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	2.6%	[1.4%, 3.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Binary size

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

Bootstrap: 766.551s -> 766.651s (0.01%)
Artifact size: 362.09 MiB -> 362.13 MiB (0.01%)

[craterbot]

🎉 Experiment pr-138176 is completed!
📊 4 regressed and 2 fixed (593768 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

can you also try to remove the precedence for Builtin { nested: false }, i.e. exclusively prefer Sized. This would then match the new solver, if it causes breakage, we should instead hcange the list of "trivial builtin traits/impls"

[compiler-errors]

i.e. exclusively prefer Sized

Do you mean stop preferring other trivial built-in impls, like Copy and Clone?

[compiler-errors]

Yeah, let me re-crater that then.

[lcnr]

feeling conflicted here. Preferring trivial builtin impls for traits with assoc types results in #133044. This also leaks whether impls are builtin or not. E.g. going from a builtin impl for fn-ptrs to an blanket impl for F: FnPtr is a subtle breaking change.

In general:

• we prefer trivial builtin impls over where-bounds as the impl can never add any undesirable constraints. Where-bounds may add undesirable constraints, especially if the where-bound gets added by elaborating trait Sub: Trait.
• if the builtin impl defines an associated type which references either regions or some param, we should use the where-bound as it's otherwise observable that we ignore the bound in case it's assoc type differs 🤔
• if the builtin impl has nested obligations, using it may result in undesirable errors, e.g. if you check (T, T): Copy and the environment contains (T, T): Copy using the builtin impl for tuples causes this to error.
  • winnowing first evaluates all nested obligations of each candidate, so this is only an issue if proving the nested obligations is still ambig (due to inference vars instead of T), or if proving the nested obligations only cause unsatisfied region constraints

For Sized we don't need to worry about region constraints from the builtin impl, even if it has nested obligations as Sized impls never have region constraints not already required by well-formedness.

We do have the issue when proving Sized for types still involving infer vars, i.e. the following should error with this PR

struct W<T: ?Sized>(T);

fn is_sized<T: Sized>(x: *const T) {}
fn dummy<T: ?Sized>() -> *const T { todo!() }
fn non_param_where_bound<T: ?Sized>()
where
    W<T>: Sized,
{
    let x: *const W<_> = dummy();
    is_sized::<W<_>>(x);
    let _: *const W<T> = x;
}

proving W<?x>: Sized prefers the builtin impl over the where-bound as the nested obligations of that impl are still ambig. We then constrain ?x to T in which case proving the nested obligations fails.

Can you change the preference for Sized to bail with ambiguity if the self type still has non-region infer vars?

[compiler-errors]

@bors try

[bors]

⌛ Trying commit ee3359a with merge 8a84d47...

[compiler-errors]

@bors try

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[lcnr]

@bors r+ rollup

[bors]

📌 Commit c0230f4 has been approved by lcnr

It is now in the queue for this repository.

[Zalathar]

Bors thinks this is still in the queue, so:

@bors r-