Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discard default auto trait impls if explicit ones exist (rebase of #85048) #113312

Merged
merged 4 commits into from Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions compiler/rustc_lint_defs/src/builtin.rs
Expand Up @@ -4052,12 +4052,12 @@ declare_lint! {
///
/// The compiler disables the automatic implementation if an explicit one
/// exists for given type constructor. The exact rules governing this
/// are currently unsound, quite subtle, and will be modified in the future.
/// This change will cause the automatic implementation to be disabled in more
/// were previously unsound, quite subtle, and have been recently modified.
/// This change caused the automatic implementation to be disabled in more
/// cases, potentially breaking some code.
pub SUSPICIOUS_AUTO_TRAIT_IMPLS,
Warn,
"the rules governing auto traits will change in the future",
"the rules governing auto traits have recently changed resulting in potential breakage",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseSemanticsChange,
reference: "issue #93367 <https://github.com/rust-lang/rust/issues/93367>",
Expand Down
Expand Up @@ -124,11 +124,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

self.assemble_candidates_from_projected_tys(obligation, &mut candidates);
self.assemble_candidates_from_caller_bounds(stack, &mut candidates)?;
// Auto implementations have lower priority, so we only
// consider triggering a default if there is no other impl that can apply.
if candidates.vec.is_empty() {
self.assemble_candidates_from_auto_impls(obligation, &mut candidates);
}
self.assemble_candidates_from_auto_impls(obligation, &mut candidates);
}
debug!("candidate list size: {}", candidates.vec.len());
Ok(candidates)
Expand Down Expand Up @@ -516,7 +512,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// for an example of a test case that exercises
// this path.
}
ty::Infer(ty::TyVar(_)) => {
ty::Infer(ty::TyVar(_) | ty::IntVar(_) | ty::FloatVar(_)) => {
// The auto impl might apply; we don't know.
candidates.ambiguous = true;
}
Expand All @@ -536,7 +532,63 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
}

_ => candidates.vec.push(AutoImplCandidate),
ty::Infer(ty::FreshTy(_) | ty::FreshIntTy(_) | ty::FreshFloatTy(_)) => {
bug!(
"asked to assemble auto trait candidates of unexpected type: {:?}",
self_ty
);
}

ty::Alias(_, _)
if candidates.vec.iter().any(|c| matches!(c, ProjectionCandidate(..))) =>
{
// We do not generate an auto impl candidate for `impl Trait`s which already
// reference our auto trait.
//
// For example during candidate assembly for `impl Send: Send`, we don't have
// to look at the constituent types for this opaque types to figure out that this
// trivially holds.
//
// Note that this is only sound as projection candidates of opaque types
// are always applicable for auto traits.
}
ty::Alias(_, _) => candidates.vec.push(AutoImplCandidate),

ty::Bool
| ty::Char
| ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::Str
| ty::Array(_, _)
| ty::Slice(_)
| ty::Adt(..)
| ty::RawPtr(_)
| ty::Ref(..)
| ty::FnDef(..)
| ty::FnPtr(_)
| ty::Closure(_, _)
| ty::Generator(..)
| ty::Never
| ty::Tuple(_)
| ty::GeneratorWitness(_)
| ty::GeneratorWitnessMIR(..) => {
// Only consider auto impls if there are no manual impls for the root of `self_ty`.
//
// For example, we only consider auto candidates for `&i32: Auto` if no explicit impl
// for `&SomeType: Auto` exists. Due to E0321 the only crate where impls
// for `&SomeType: Auto` can be defined is the crate where `Auto` has been defined.
//
// Generally, we have to guarantee that for all `SimplifiedType`s the only crate
// which may define impls for that type is either the crate defining the type
// or the trait. This should be guaranteed by the orphan check.
let mut has_impl = false;
self.tcx().for_each_relevant_impl(def_id, self_ty, |_| has_impl = true);
if !has_impl {
candidates.vec.push(AutoImplCandidate)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this is wrong for opaque types 😅 we need to always emit consider AutoImplCandidates for them if they don't also have a ProjectionCandidate. This is a bit of a mess in the old solver.

The issue is that we consider all impls to potentially apply for opaque types, but we should instead actually leak their underlying type and check whether it implements the auto trait

cc @compiler-errors given that we've recently talked about auto traits and opaques :>

ty::Error(_) => {} // do not add an auto trait impl for `ty::Error` for now.
}
}
}
Expand Down
25 changes: 18 additions & 7 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Expand Up @@ -1814,6 +1814,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
/// candidates and prefer where-clause candidates.
///
/// See the comment for "SelectionCandidate" for more details.
#[instrument(level = "debug", skip(self))]
fn candidate_should_be_dropped_in_favor_of(
&mut self,
victim: &EvaluatedCandidate<'tcx>,
Expand All @@ -1837,13 +1838,6 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
// This is a fix for #53123 and prevents winnowing from accidentally extending the
// lifetime of a variable.
match (&other.candidate, &victim.candidate) {
(_, AutoImplCandidate) | (AutoImplCandidate, _) => {
bug!(
"default implementations shouldn't be recorded \
when there are other valid candidates"
);
}

// FIXME(@jswrenn): this should probably be more sophisticated
(TransmutabilityCandidate, _) | (_, TransmutabilityCandidate) => DropVictim::No,

Expand Down Expand Up @@ -1885,6 +1879,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
(
ParamCandidate(ref other_cand),
ImplCandidate(..)
| AutoImplCandidate
| ClosureCandidate { .. }
| GeneratorCandidate
| FutureCandidate
Expand Down Expand Up @@ -1912,6 +1907,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
}
(
ImplCandidate(_)
| AutoImplCandidate
| ClosureCandidate { .. }
| GeneratorCandidate
| FutureCandidate
Expand Down Expand Up @@ -1945,6 +1941,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
(
ObjectCandidate(_) | ProjectionCandidate(..),
ImplCandidate(..)
| AutoImplCandidate
| ClosureCandidate { .. }
| GeneratorCandidate
| FutureCandidate
Expand All @@ -1958,6 +1955,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {

(
ImplCandidate(..)
| AutoImplCandidate
| ClosureCandidate { .. }
| GeneratorCandidate
| FutureCandidate
Expand Down Expand Up @@ -2048,6 +2046,19 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
}
}

(AutoImplCandidate, ImplCandidate(_)) | (ImplCandidate(_), AutoImplCandidate) => {
DropVictim::No
}

(AutoImplCandidate, _) | (_, AutoImplCandidate) => {
bug!(
"default implementations shouldn't be recorded \
when there are other global candidates: {:?} {:?}",
other,
victim
);
}

// Everything else is ambiguous
(
ImplCandidate(_)
Expand Down
31 changes: 31 additions & 0 deletions tests/ui/auto-traits/issue-83857-ub.rs
@@ -0,0 +1,31 @@
#![allow(suspicious_auto_trait_impls)]

struct Always<T, U>(T, U);
unsafe impl<T, U> Send for Always<T, U> {}
struct Foo<T, U>(Always<T, U>);

trait False {}
unsafe impl<U: False> Send for Foo<u32, U> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be worth a comment explaining what is being tested: that the manual impl Send for Foo should "block" the implicit impl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how much explanation makes sense here, though a "Tests that we don't incorrectly allow overlap between a builtin auto trait impl and a user written one. See #83857 for more details." would be good here.


trait WithAssoc {
type Output;
}
impl<T: Send> WithAssoc for T {
type Output = Self;
}
impl WithAssoc for Foo<u32, ()> {
type Output = Box<i32>;
}

fn generic<T, U>(v: Foo<T, U>, f: fn(<Foo<T, U> as WithAssoc>::Output) -> i32) {
//~^ ERROR `Foo<T, U>` cannot be sent between threads safely
f(foo(v));
}

fn foo<T: Send>(x: T) -> <T as WithAssoc>::Output {
x
}

fn main() {
generic(Foo(Always(0, ())), |b| *b);
}
22 changes: 22 additions & 0 deletions tests/ui/auto-traits/issue-83857-ub.stderr
@@ -0,0 +1,22 @@
error[E0277]: `Foo<T, U>` cannot be sent between threads safely
--> $DIR/issue-83857-ub.rs:20:38
|
LL | fn generic<T, U>(v: Foo<T, U>, f: fn(<Foo<T, U> as WithAssoc>::Output) -> i32) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Foo<T, U>` cannot be sent between threads safely
|
= help: the trait `Send` is not implemented for `Foo<T, U>`
note: required for `Foo<T, U>` to implement `WithAssoc`
--> $DIR/issue-83857-ub.rs:13:15
|
LL | impl<T: Send> WithAssoc for T {
| ---- ^^^^^^^^^ ^
| |
| unsatisfied trait bound introduced here
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
|
LL | fn generic<T, U>(v: Foo<T, U>, f: fn(<Foo<T, U> as WithAssoc>::Output) -> i32) where Foo<T, U>: Send {
| +++++++++++++++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
6 changes: 3 additions & 3 deletions tests/ui/generator/auto-trait-regions.rs
Expand Up @@ -26,7 +26,7 @@ fn assert_foo<T: Foo>(f: T) {}
fn main() {
// Make sure 'static is erased for generator interiors so we can't match it in trait selection
let x: &'static _ = &OnlyFooIfStaticRef(No);
let gen = || {
let gen = move || {
let x = x;
yield;
assert_foo(x);
Expand All @@ -36,15 +36,15 @@ fn main() {

// Allow impls which matches any lifetime
let x = &OnlyFooIfRef(No);
let gen = || {
let gen = move || {
let x = x;
yield;
assert_foo(x);
};
assert_foo(gen); // ok

// Disallow impls which relates lifetimes in the generator interior
let gen = || {
let gen = move || {
let a = A(&mut true, &mut true, No);
//~^ temporary value dropped while borrowed
//~| temporary value dropped while borrowed
Expand Down
Empty file.
10 changes: 10 additions & 0 deletions tests/ui/impl-trait/recursive-auto-trait.rs
@@ -0,0 +1,10 @@
// check-pass
fn is_send<T: Send>(_: T) {}
fn foo() -> impl Send {
if false {
is_send(foo());
}
()
}

fn main() {}