Skip to content

Commit

Permalink
Rollup merge of #135639 - lqd:trivial-builtin-impls, r=lcnr
Browse files Browse the repository at this point in the history
new solver: prefer trivial builtin impls

As discussed [on zulip](https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/needs_help.3A.20trivial.20builtin.20impls), this PR:
- adds a new `BuiltinImplSource::Trivial` source, and marks the `Sized` builtin impls as trivial
- prefers these trivial builtin impls in `merge_trait_candidates`

The comments can likely be wordsmithed a bit better, and I ~stole~ was inspired by the old solver ones. Let me know how you want them improved.

When enabling the new solver for tests, 3 UI tests now pass:
- `regions/issue-26448-1.rs` and its sibling `regions/issue-26448-2.rs` were rejected by the new solver but accepted by the old one
- and `issues/issue-42796.rs` where the old solver emitted some overflow errors in addition to the expected error

(For some reason one of these tests is run-pass, but I can take care of that another day)

r? lcnr
  • Loading branch information
matthiaskrgr authored Jan 18, 2025
2 parents 80fcf15 + 00844be commit 862a17c
Show file tree
Hide file tree
Showing 8 changed files with 41 additions and 6 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4259,6 +4259,7 @@ dependencies = [
"rustc_serialize",
"rustc_type_ir",
"rustc_type_ir_macros",
"smallvec",
"tracing",
]

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_next_trait_solver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ rustc_macros = { path = "../rustc_macros", optional = true }
rustc_serialize = { path = "../rustc_serialize", optional = true }
rustc_type_ir = { path = "../rustc_type_ir", default-features = false }
rustc_type_ir_macros = { path = "../rustc_type_ir_macros" }
smallvec = "1.8.1"
tracing = "0.1"
# tidy-alphabetical-end

Expand Down
28 changes: 26 additions & 2 deletions compiler/rustc_next_trait_solver/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_type_ir::lang_items::TraitSolverLangItem;
use rustc_type_ir::solve::CanonicalResponse;
use rustc_type_ir::visit::TypeVisitableExt as _;
use rustc_type_ir::{self as ty, Interner, TraitPredicate, TypingMode, Upcast as _, elaborate};
use smallvec::SmallVec;
use tracing::{instrument, trace};

use crate::delegate::SolverDelegate;
Expand Down Expand Up @@ -225,7 +226,7 @@ where
}

ecx.probe_and_evaluate_goal_for_constituent_tys(
CandidateSource::BuiltinImpl(BuiltinImplSource::Misc),
CandidateSource::BuiltinImpl(BuiltinImplSource::Trivial),
goal,
structural_traits::instantiate_constituent_tys_for_sized_trait,
)
Expand Down Expand Up @@ -1194,7 +1195,30 @@ where
};
}

// FIXME: prefer trivial builtin impls
// We prefer trivial builtin candidates, i.e. builtin impls without any
// nested requirements, over all others. This is a fix for #53123 and
// prevents where-bounds from accidentally extending the lifetime of a
// variable.
if candidates
.iter()
.any(|c| matches!(c.source, CandidateSource::BuiltinImpl(BuiltinImplSource::Trivial)))
{
let trivial_builtin_impls: SmallVec<[_; 1]> = candidates
.iter()
.filter(|c| {
matches!(c.source, CandidateSource::BuiltinImpl(BuiltinImplSource::Trivial))
})
.map(|c| c.result)
.collect();
// There should only ever be a single trivial builtin candidate
// as they would otherwise overlap.
assert_eq!(trivial_builtin_impls.len(), 1);
return if let Some(response) = self.try_merge_responses(&trivial_builtin_impls) {
Ok((response, Some(TraitGoalProvenVia::Misc)))
} else {
Ok((self.bail_with_ambiguity(&trivial_builtin_impls), None))
};
}

// If there are non-global where-bounds, prefer where-bounds
// (including global ones) over everything else.
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
Err(ErrorGuaranteed { .. }) => true,
}
}
ImplSource::Builtin(BuiltinImplSource::Misc, _) => {
ImplSource::Builtin(BuiltinImplSource::Misc | BuiltinImplSource::Trivial, _) => {
// While a builtin impl may be known to exist, the associated type may not yet
// be known. Any type with multiple potential associated types is therefore
// not eligible.
Expand Down Expand Up @@ -1296,7 +1296,7 @@ fn confirm_select_candidate<'cx, 'tcx>(
) -> Progress<'tcx> {
match impl_source {
ImplSource::UserDefined(data) => confirm_impl_candidate(selcx, obligation, data),
ImplSource::Builtin(BuiltinImplSource::Misc, data) => {
ImplSource::Builtin(BuiltinImplSource::Misc | BuiltinImplSource::Trivial, data) => {
let tcx = selcx.tcx();
let trait_def_id = obligation.predicate.trait_def_id(tcx);
if tcx.is_lang_item(trait_def_id, LangItem::Coroutine) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ty_utils/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ fn resolve_associated_item<'tcx>(
})
}
}
traits::ImplSource::Builtin(BuiltinImplSource::Misc, _) => {
traits::ImplSource::Builtin(BuiltinImplSource::Misc | BuiltinImplSource::Trivial, _) => {
if tcx.is_lang_item(trait_ref.def_id, LangItem::Clone) {
// FIXME(eddyb) use lang items for methods instead of names.
let name = tcx.item_name(trait_item_id);
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_type_ir/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ pub enum CandidateSource<I: Interner> {
#[derive(Clone, Copy, Hash, PartialEq, Eq, Debug)]
#[cfg_attr(feature = "nightly", derive(HashStable_NoContext, TyEncodable, TyDecodable))]
pub enum BuiltinImplSource {
/// A built-in impl that is considered trivial, without any nested requirements. They
/// are preferred over where-clauses, and we want to track them explicitly.
Trivial,
/// Some built-in impl we don't need to differentiate. This should be used
/// unless more specific information is necessary.
Misc,
Expand Down
5 changes: 4 additions & 1 deletion tests/ui/regions/issue-26448-1.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@ run-pass
//@ revisions: current next
//@ [next] compile-flags: -Znext-solver
//@ ignore-compare-mode-next-solver (explicit revisions)
//@ check-pass

pub trait Foo<T> {
fn foo(self) -> T;
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/regions/issue-26448-2.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
//@ revisions: current next
//@ [next] compile-flags: -Znext-solver
//@ ignore-compare-mode-next-solver (explicit revisions)
//@ check-pass

pub struct Bar<T> {
Expand Down

0 comments on commit 862a17c

Please sign in to comment.