Rollup merge of #138176 - compiler-errors:rigid-sized-obl, r=lcnr
Prefer built-in sized impls (and only sized impls) for rigid types always
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`
2b4694a698/compiler/rustc_trait_selection/src/traits/select/mod.rs (L1804-L1866)
Preferring param-env candidates when the builtin candidate has nested obligations *still* ends up leading to detrimental inference guidance, like:
```rust
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](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=30377da5b8a88f654884ab4ebc72f52b)
- 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. https://github.com/rust-lang/rust/issues/133044
- not an issue for `Sized` as it doesn't have associated types.
r? lcnr
This commit is contained in:
Matthias Krüger
GitHub
commit
e15161d528
12 changed files with 214 additions and 16 deletions
|
|
@ -86,10 +86,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
|
|||
// `Pointee` is automatically implemented for every type.
|
||||
candidates.vec.push(BuiltinCandidate { has_nested: false });
|
||||
} else if tcx.is_lang_item(def_id, LangItem::Sized) {
|
||||
// Sized is never implementable by end-users, it is
|
||||
// always automatically computed.
|
||||
let sized_conditions = self.sized_conditions(obligation);
|
||||
self.assemble_builtin_bound_candidates(sized_conditions, &mut candidates);
|
||||
self.assemble_builtin_sized_candidate(obligation, &mut candidates);
|
||||
} else if tcx.is_lang_item(def_id, LangItem::Unsize) {
|
||||
self.assemble_candidates_for_unsizing(obligation, &mut candidates);
|
||||
} else if tcx.is_lang_item(def_id, LangItem::Destruct) {
|
||||
|
|
@ -1061,6 +1058,27 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
|
|||
/// Assembles the trait which are built-in to the language itself:
|
||||
/// `Copy`, `Clone` and `Sized`.
|
||||
#[instrument(level = "debug", skip(self, candidates))]
|
||||
fn assemble_builtin_sized_candidate(
|
||||
&mut self,
|
||||
obligation: &PolyTraitObligation<'tcx>,
|
||||
candidates: &mut SelectionCandidateSet<'tcx>,
|
||||
) {
|
||||
match self.sized_conditions(obligation) {
|
||||
BuiltinImplConditions::Where(nested) => {
|
||||
candidates
|
||||
.vec
|
||||
.push(SizedCandidate { has_nested: !nested.skip_binder().is_empty() });
|
||||
}
|
||||
BuiltinImplConditions::None => {}
|
||||
BuiltinImplConditions::Ambiguous => {
|
||||
candidates.ambiguous = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Assembles the trait which are built-in to the language itself:
|
||||
/// e.g. `Copy` and `Clone`.
|
||||
#[instrument(level = "debug", skip(self, candidates))]
|
||||
fn assemble_builtin_bound_candidates(
|
||||
&mut self,
|
||||
conditions: BuiltinImplConditions<'tcx>,
|
||||
|
|
|
|||
|
|
@ -40,6 +40,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
|
|||
candidate: SelectionCandidate<'tcx>,
|
||||
) -> Result<Selection<'tcx>, SelectionError<'tcx>> {
|
||||
let mut impl_src = match candidate {
|
||||
SizedCandidate { has_nested } => {
|
||||
let data = self.confirm_builtin_candidate(obligation, has_nested);
|
||||
ImplSource::Builtin(BuiltinImplSource::Misc, data)
|
||||
}
|
||||
|
||||
BuiltinCandidate { has_nested } => {
|
||||
let data = self.confirm_builtin_candidate(obligation, has_nested);
|
||||
ImplSource::Builtin(BuiltinImplSource::Misc, data)
|
||||
|
|
|
|||
|
|
@ -1801,17 +1801,21 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
|
|||
return Some(candidates.pop().unwrap().candidate);
|
||||
}
|
||||
|
||||
// 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
|
||||
// We prefer `Sized` candidates over everything.
|
||||
let mut sized_candidates =
|
||||
candidates.iter().filter(|c| matches!(c.candidate, SizedCandidate { has_nested: _ }));
|
||||
if let Some(sized_candidate) = sized_candidates.next() {
|
||||
// There should only ever be a single sized candidate
|
||||
// as they would otherwise overlap.
|
||||
debug_assert_eq!(trivial_builtin.next(), None);
|
||||
return Some(BuiltinCandidate { has_nested: false });
|
||||
debug_assert_eq!(sized_candidates.next(), None);
|
||||
// Only prefer the built-in `Sized` candidate if its nested goals are certain.
|
||||
// Otherwise, we may encounter failure later on if inference causes this candidate
|
||||
// to not hold, but a where clause would've applied instead.
|
||||
if sized_candidate.evaluation.must_apply_modulo_regions() {
|
||||
return Some(sized_candidate.candidate.clone());
|
||||
} else {
|
||||
return None;
|
||||
}
|
||||
}
|
||||
|
||||
// Before we consider where-bounds, we have to deduplicate them here and also
|
||||
|
|
@ -1940,7 +1944,8 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
|
|||
// Don't use impl candidates which overlap with other candidates.
|
||||
// This should pretty much only ever happen with malformed impls.
|
||||
if candidates.iter().all(|c| match c.candidate {
|
||||
BuiltinCandidate { has_nested: _ }
|
||||
SizedCandidate { has_nested: _ }
|
||||
| BuiltinCandidate { has_nested: _ }
|
||||
| TransmutabilityCandidate
|
||||
| AutoImplCandidate
|
||||
| ClosureCandidate { .. }
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue