Skip to content

fix negative impls inference #74525

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

fix negative impls inference #74525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f12ceb1
time to bodge
lcnr Jul 19, 2020
0d8b172
Merge branch 'master' of github.com:rust-lang/rust into negative-impl…
lcnr Jul 19, 2020
e68b9ed
fmt
lcnr Jul 19, 2020
85511d2
add auto traits regression test
lcnr Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions src/librustc_trait_selection/traits/select/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,16 +934,34 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
(result, dep_node)
}

// Treat negative impls as unimplemented, and reservation impls as ambiguity.
/// Deal with negative impls and interpret and reservation impls as ambiguity.
///
/// If the negative impl is an exact fit, this returns `Err(Unimplemented)`. In case
/// the negative impl does not apply to all possible values of `self` it is instead
/// interpreted as ambiguity.
fn filter_negative_and_reservation_impls(
&mut self,
pred: ty::PolyTraitPredicate<'tcx>,
candidate: SelectionCandidate<'tcx>,
) -> SelectionResult<'tcx, SelectionCandidate<'tcx>> {
if let ImplCandidate(def_id) = candidate {
let tcx = self.tcx();
match tcx.impl_polarity(def_id) {
ty::ImplPolarity::Negative if !self.allow_negative_impls => {
Copy link
Contributor Author

@lcnr lcnr Jul 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So we end up here if we have exactly one impl candidate which is negative.

We only have a negative impl for Foo<()> here, while the self type is actually more generic (Foo<_>).
So if Send is an auto trait we should return ambiguous in this case, as Foo can still implement Send, e.g. Foo<u8>.

I don't know how we can detect that the impl is at least as general as the current self type rn though, need some help for this. (I don't think I have to write a new type relation for this 🤔 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

So actually I'm not sure there is a bug here. There has been a long-standing debate about the correct semantics of auto traits in the face of negative impls, but the current semantics (not, perhaps 100% settled) is that

  • If the user writes ANY explicit impl, positive or negative, then the compiler generates NO automatic impl

This was perhaps to be paired with disallowing negative impls that were overly narrow, such as !Foo for Bar<()>.

So in this case, writing impl !Send for Bar<()> (for example) would mean that the compiler does not add an automatic Send for Bar impl at all.

It's debatable what's more intuitive. I personally prefer the current rules -- I would prefer that if you write anything explicit, you must write everything explicit. But also this avoids having a setup (which specialization/coherence currently do not permit) where you have a "base impl" that accepts lots of things and then "negative impls" that cut out "exceptions" where that base impl no longer applies.

In other words, given specialization (and auto traits as currently implemented and specified), if you know that a "base impl" applies, you know that the trait is implemented, and no specialization can make that "untrue".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I think this makes sense 🤔

disallowing negative impls that were overly narrow, such as !Foo for Bar<()>.

While negative impls are still unstable, this seems like a restriction we should probably enforce automatically.

Quite interestingly (or much rather, a logical consequence of how this is implemented rn), we can currently "fix" this inference issue by adding another nonsensical impl:

#![feature(negative_impls)]

struct Foo<T>(T);

struct IDontCare;

impl !Send for Foo<()> {}
unsafe impl Send for Foo<IDontCare> {}

fn test<T>() -> T where Foo<T>: Send { todo!() }

fn main() {
    let _: u8 = test();
}

I would love to use this to forbid calling array_chunks::<0>() in #74373 until we implement const where bounds,
but that may stop us from fixing negative impls (as we now need the broken version in std, even if only for an unstable method).

Copy link
Contributor

Choose a reason for hiding this comment

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

While negative impls are still unstable, this seems like a restriction we should probably enforce automatically.

these has been an open bug on this forever -- well, no, it's the first checkbox on #13231. It'd be easy enough to do, we can use the same code we use for Drop impls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, might look into this in the following weeks 🤔 let's see how much time I have for this.

return Err(Unimplemented);
let trait_ref = tcx.impl_trait_ref(def_id).unwrap();
let self_ty = trait_ref.self_ty();
let string = format!(
"trait_ref: {:?}, self_ty: {:?}, pred: {:?}",
trait_ref, self_ty, pred
);
warn!("trait_ref: {:?}, self_ty: {:?}, pred: {:?}", trait_ref, self_ty, pred);
if string
== "trait_ref: <Foo<()> as std::marker::Send>, self_ty: Foo<()>, pred: Binder(TraitPredicate(<Foo<_> as std::marker::Send>))"
{
return Ok(None);
} else {
return Err(Unimplemented);
}
}
ty::ImplPolarity::Reservation => {
if let Some(intercrate_ambiguity_clauses) =
Expand Down Expand Up @@ -1049,7 +1067,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// Instead, we select the right impl now but report "`Bar` does
// not implement `Clone`".
if candidates.len() == 1 {
return self.filter_negative_and_reservation_impls(candidates.pop().unwrap());
return self.filter_negative_and_reservation_impls(
stack.obligation.predicate,
candidates.pop().unwrap(),
);
}

// Winnow, but record the exact outcome of evaluation, which
Expand Down Expand Up @@ -1122,7 +1143,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

// Just one candidate left.
self.filter_negative_and_reservation_impls(candidates.pop().unwrap().candidate)
self.filter_negative_and_reservation_impls(
stack.obligation.predicate,
candidates.pop().unwrap().candidate,
)
}

fn is_knowable<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>) -> Option<Conflict> {
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/__check/meh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// check-pass
#![feature(negative_impls)]

struct Foo<T>(T);

impl !Send for Foo<()> {}

fn test<T>() -> T where Foo<T>: Send { todo!() }

fn main() {
let _: u8 = test();
}
18 changes: 18 additions & 0 deletions src/test/ui/auto-traits/negative-impl-ambiguity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// check-pass
use std::rc::Rc;

trait A {
fn foo(&self) {}
}

impl<T: Send> A for T {}

fn test<T: A>(rc: &Rc<T>) {
rc.foo()
// `Rc: Send` must not be evaluated as ambiguous
// for this to compile, as we are otherwise
// not allowed to use auto deref here to use
// the `T: A` implementation.
}

fn main() {}