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

Handle cycles in overlap with negative impls #109673

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
42 changes: 42 additions & 0 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,48 @@ pub fn overlapping_impls(
Some(overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id, overlap_mode).unwrap())
}

/// Given an impl_def_id that "positively" implement a trait, check if the "negative" holds.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/implement/implements/

pub fn negative_impl_holds(tcx: TyCtxt<'_>, impl_def_id: DefId, overlap_mode: OverlapMode) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/negative_impl_holds/negative_impl_may_hold/

debug!("negative_impl_holds(impl1_header={:?}, overlap_mode={:?})", impl_def_id, overlap_mode);
// `for<T> (Vec<u32>, T): Trait`
let header = tcx.impl_trait_ref(impl_def_id).unwrap();

let infcx = tcx
.infer_ctxt()
.with_opaque_type_inference(DefiningAnchor::Bubble)
.intercrate(true)
.build();

// `[?t]`
let infer_substs = infcx.fresh_substs_for_item(DUMMY_SP, impl_def_id);

// `(Vec<u32>, ?t): Trait`
let trait_ref = header.subst(tcx, infer_substs);

// `(Vec<u32>, ?t): !Trait`
let trait_pred = tcx.mk_predicate(ty::Binder::dummy(ty::PredicateKind::Clause(
ty::Clause::Trait(ty::TraitPredicate {
trait_ref,
constness: ty::BoundConstness::NotConst,
polarity: ty::ImplPolarity::Negative,
}),
)));

// Ideally we would use param_env(impl_def_id) but that's unsound today.
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
let param_env = ty::ParamEnv::empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

To do this properly, what we ought to do is get the parameter environment and then apply the infer_substs above. The idea is that for some impl...

impl<A..Z> SomeTrait<T1..Tn> for T0
where WC

we want to test if...

  • there exists types A..Z such that...
  • assuming WC is true (ie., with the param env in scope)...
  • T0: !SomeTrait<T1..Tn>

this would be an error, because that means that for at least some instance of this positive impl, there exists a negative impl.

Problem is: applying infer_substs to param-env will mean that we have inference variables in the environment, and I think our code doesn't like that.

It should be sound to not assume WC is true, because proving A => X is harder than proving X on its own, but I'm worried we may be overly conservative.

Copy link
Contributor

@lcnr lcnr Mar 31, 2023

Choose a reason for hiding this comment

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

inference variables in the param env generally cause issues, especially in the old solver. Don't remember exactly what breaks but iirc @BoxyUwU encountered a bunch of issues recently when she tried it.

I think a bigger issue is that the way candidate_should_be_dropped_in_favor_of handles param env candidates is incomplete. This means that coherence in the old solver with trait goals in the param env is unsound because inference may incorrectly cause another nested goal to fail, resulting in an incorrect error. My current approach for the new solver is to disable this behavior if we're in coherence: #109724


let selcx = &mut SelectionContext::new(&infcx);
selcx
.evaluate_root_obligation(&Obligation::new(
tcx,
ObligationCause::dummy(),
param_env,
trait_pred,
))
.expect("Overflow should be caught earlier in standard query mode")
.must_apply_modulo_regions()
Copy link
Member

Choose a reason for hiding this comment

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

Why use this over predicate_may_hold? Don't we want to treat treat ambiguity as "yes" instead of "no" for coherence purposes?

Sorry if this question is misunderstanding what this PR is trying to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea if I got everything correct is that we want to check that there's not a negative impl that applies. This is why I wanted to check for a must. But this is one of the things that I have doubts if using must_apply_module_regions is correct here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be may apply modulo regions, not must apply.

}

fn with_fresh_ty_vars<'cx, 'tcx>(
selcx: &mut SelectionContext<'cx, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ pub use self::ImplSource::*;
pub use self::ObligationCauseCode::*;
pub use self::SelectionError::*;

pub use self::coherence::{add_placeholder_note, orphan_check, overlapping_impls};
pub use self::coherence::{
add_placeholder_note, negative_impl_holds, orphan_check, overlapping_impls,
};
pub use self::coherence::{OrphanCheckErr, OverlapResult};
pub use self::engine::{ObligationCtxt, TraitEngineExt};
pub use self::fulfill::{FulfillmentContext, PendingPredicateObligation};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,22 @@ fn overlap_check_considering_specialization<'tcx>(
return Ok(OverlapResult::SpecializeAll(replace_children));
Copy link
Contributor

Choose a reason for hiding this comment

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

Block: early returns

}

if overlap_mode.use_negative_impl()
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this if, but before you do, let's try to figure out why the src/test/ui/traits/negative-impls/pin-unsound-issue-66544-derefmut.rs test gets an error (assuming it does...)

Copy link
Contributor

Choose a reason for hiding this comment

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

probably because we didn't remove the old coherence logic

&& tcx.impl_polarity(impl_def_id) == ty::ImplPolarity::Positive
&& traits::negative_impl_holds(tcx, impl_def_id, overlap_mode)
{
let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap().skip_binder();
let self_ty = trait_ref.self_ty();

return Err(OverlapError {
with_impl: impl_def_id,
trait_ref,
self_ty: self_ty.has_concrete_skeleton().then_some(self_ty),
intercrate_ambiguity_causes: Default::default(),
involves_placeholder: false,
});
}

Ok(OverlapResult::NoOverlap(last_lint))
}

Expand Down
17 changes: 17 additions & 0 deletions tests/ui/coherence/coherence-overlap-negative-cycles.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![feature(trivial_bounds)]
#![feature(negative_impls)]
#![feature(rustc_attrs)]
#![feature(with_negative_coherence)]
#![allow(trivial_bounds)]

#[rustc_strict_coherence]
trait MyTrait {}

struct Foo {}

impl !MyTrait for Foo {}

impl MyTrait for Foo where Foo: MyTrait {}
//~^ ERROR: conflicting implementations of trait `MyTrait`

fn main() {}
12 changes: 12 additions & 0 deletions tests/ui/coherence/coherence-overlap-negative-cycles.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0119]: conflicting implementations of trait `MyTrait` for type `Foo`
--> $DIR/coherence-overlap-negative-cycles.rs:14:1
|
LL | impl MyTrait for Foo where Foo: MyTrait {}
| ^^^^^^^^^^^^^^^^^^^^
| |
| first implementation here
| conflicting implementation for `Foo`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.