From 2ae4bedd859b2a42a03e9513e40e4829309af4c6 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 25 Jul 2023 01:17:58 +0000 Subject: [PATCH 1/5] more span info --- compiler/rustc_middle/src/ty/mod.rs | 2 +- .../src/traits/coherence.rs | 16 +++++++++------- tests/ui/traits/issue-105231.rs | 2 +- tests/ui/traits/issue-105231.stderr | 8 ++++++-- .../cycle-via-builtin-auto-trait-impl.rs | 2 +- .../cycle-via-builtin-auto-trait-impl.stderr | 8 ++++++-- 6 files changed, 24 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index 48aa25dba6dc7..377dd82a00e44 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -238,7 +238,7 @@ pub struct ImplHeader<'tcx> { pub impl_def_id: DefId, pub self_ty: Ty<'tcx>, pub trait_ref: Option>, - pub predicates: Vec>, + pub predicates: Vec<(Predicate<'tcx>, Span)>, } #[derive(Copy, Clone, PartialEq, Eq, Debug, TypeFoldable, TypeVisitable)] diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index e56af586ed875..dd15f1a5492ee 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -151,14 +151,16 @@ fn with_fresh_ty_vars<'cx, 'tcx>( .predicates_of(impl_def_id) .instantiate(tcx, impl_args) .iter() - .map(|(c, _)| c.as_predicate()) + .map(|(c, s)| (c.as_predicate(), s)) .collect(), }; - let InferOk { value: mut header, obligations } = - selcx.infcx.at(&ObligationCause::dummy(), param_env).normalize(header); + let InferOk { value: mut header, obligations } = selcx + .infcx + .at(&ObligationCause::dummy_with_span(tcx.def_span(impl_def_id)), param_env) + .normalize(header); - header.predicates.extend(obligations.into_iter().map(|o| o.predicate)); + header.predicates.extend(obligations.into_iter().map(|o| (o.predicate, o.cause.span))); header } @@ -289,7 +291,7 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( ) -> bool { let infcx = selcx.infcx; - let obligation_guaranteed_to_fail = move |obligation: &PredicateObligation<'tcx>| { + let obligation_guaranteed_to_fail = |obligation: &PredicateObligation<'tcx>| { if infcx.next_trait_solver() { infcx.evaluate_obligation(obligation).map_or(false, |result| !result.may_apply()) } else { @@ -306,8 +308,8 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( let opt_failing_obligation = [&impl1_header.predicates, &impl2_header.predicates] .into_iter() .flatten() - .map(|&predicate| { - Obligation::new(infcx.tcx, ObligationCause::dummy(), param_env, predicate) + .map(|&(predicate, span)| { + Obligation::new(infcx.tcx, ObligationCause::dummy_with_span(span), param_env, predicate) }) .chain(obligations) .find(obligation_guaranteed_to_fail); diff --git a/tests/ui/traits/issue-105231.rs b/tests/ui/traits/issue-105231.rs index 74c7afd6b9edb..bb2b13664ba26 100644 --- a/tests/ui/traits/issue-105231.rs +++ b/tests/ui/traits/issue-105231.rs @@ -1,9 +1,9 @@ -//~ ERROR overflow evaluating the requirement `A>>>>>>: Send` struct A(B); //~^ ERROR recursive types `A` and `B` have infinite size struct B(A>); trait Foo {} impl Foo for T where T: Send {} +//~^ ERROR overflow evaluating the requirement `A>>>>>>: Send` impl Foo for B {} fn main() {} diff --git a/tests/ui/traits/issue-105231.stderr b/tests/ui/traits/issue-105231.stderr index fe20c47c57a80..76a71067353f1 100644 --- a/tests/ui/traits/issue-105231.stderr +++ b/tests/ui/traits/issue-105231.stderr @@ -1,5 +1,5 @@ error[E0072]: recursive types `A` and `B` have infinite size - --> $DIR/issue-105231.rs:2:1 + --> $DIR/issue-105231.rs:1:1 | LL | struct A(B); | ^^^^^^^^^^^ ---- recursive without indirection @@ -15,10 +15,14 @@ LL ~ struct B(Box>>); | error[E0275]: overflow evaluating the requirement `A>>>>>>: Send` + --> $DIR/issue-105231.rs:5:28 + | +LL | impl Foo for T where T: Send {} + | ^^^^ | = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`issue_105231`) note: required because it appears within the type `B>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` - --> $DIR/issue-105231.rs:4:8 + --> $DIR/issue-105231.rs:3:8 | LL | struct B(A>); | ^ diff --git a/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.rs b/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.rs index d37943b929a1f..206ab07898b61 100644 --- a/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.rs +++ b/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.rs @@ -1,4 +1,3 @@ -//~ ERROR overflow // A regression test for #111729 checking that we correctly // track recursion depth for obligations returned by confirmation. use std::panic::RefUnwindSafe; @@ -15,6 +14,7 @@ struct RootDatabase { } impl Database for T { + //~^ ERROR overflow type Storage = SalsaStorage; } impl Database for RootDatabase { diff --git a/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.stderr b/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.stderr index 8f9ce3ef1e99c..4123a8199a071 100644 --- a/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.stderr +++ b/tests/ui/traits/solver-cycles/cycle-via-builtin-auto-trait-impl.stderr @@ -1,13 +1,17 @@ error[E0275]: overflow evaluating the requirement `Runtime: RefUnwindSafe` + --> $DIR/cycle-via-builtin-auto-trait-impl.rs:16:9 + | +LL | impl Database for T { + | ^^^^^^^^^^^^^ | = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`cycle_via_builtin_auto_trait_impl`) note: required because it appears within the type `RootDatabase` - --> $DIR/cycle-via-builtin-auto-trait-impl.rs:13:8 + --> $DIR/cycle-via-builtin-auto-trait-impl.rs:12:8 | LL | struct RootDatabase { | ^^^^^^^^^^^^ note: required for `RootDatabase` to implement `Database` - --> $DIR/cycle-via-builtin-auto-trait-impl.rs:17:24 + --> $DIR/cycle-via-builtin-auto-trait-impl.rs:16:24 | LL | impl Database for T { | ------------- ^^^^^^^^ ^ From 56f5704ff843256912260678433778f27bb6f6c8 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 25 Jul 2023 01:49:29 +0000 Subject: [PATCH 2/5] Implement lint against coinductive impl overlap --- compiler/rustc_lint_defs/src/builtin.rs | 51 +++++++++++++++++++ .../src/traits/coherence.rs | 45 ++++++++++++++-- .../src/traits/select/mod.rs | 36 +++++++++++-- .../warn-when-cycle-is-error-in-coherence.rs | 35 +++++++++++++ ...rn-when-cycle-is-error-in-coherence.stderr | 22 ++++++++ 5 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs create mode 100644 tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 3747e562cec58..b3e5fee4185ae 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -3366,6 +3366,7 @@ declare_lint_pass! { BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE, CENUM_IMPL_DROP_CAST, COHERENCE_LEAK_CHECK, + COINDUCTIVE_OVERLAP_IN_COHERENCE, CONFLICTING_REPR_HINTS, CONST_EVALUATABLE_UNCHECKED, CONST_ITEM_MUTATION, @@ -4422,6 +4423,56 @@ declare_lint! { @feature_gate = sym::type_privacy_lints; } +declare_lint! { + /// The `coinductive_overlap_in_coherence` lint detects impls which are currently + /// considered not overlapping, but may be considered to overlap if support for + /// coinduction is added to the trait solver. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// use std::borrow::Borrow; + /// use std::cmp::Ordering; + /// use std::marker::PhantomData; + /// + /// #[derive(PartialEq, Default)] + /// pub(crate) struct Interval(T); + /// + /// impl PartialEq for Interval + /// where + /// Q: PartialOrd, + /// { + /// fn eq(&self, other: &Q) -> bool { + /// todo!() + /// } + /// } + /// + /// impl PartialOrd for Interval + /// where + /// Q: PartialOrd, + /// { + /// fn partial_cmp(&self, other: &Q) -> Option { + /// todo!() + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// The manual impl of `PartialEq` impl overlaps with the `derive`, since + /// if we replace `Q = Interval`, then the second impl leads to a cycle: + /// `PartialOrd for Interval where Interval: Partial`. + pub COINDUCTIVE_OVERLAP_IN_COHERENCE, + Warn, + "impls that are not considered to overlap may be considered to \ + overlap in the future", + @future_incompatible = FutureIncompatibleInfo { + reference: "issue #114040 ", + }; +} + declare_lint! { /// The `unknown_diagnostic_attributes` lint detects unrecognized diagnostic attributes. /// diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index dd15f1a5492ee..2cc0b6548769a 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -24,6 +24,7 @@ use rustc_middle::traits::DefiningAnchor; use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams}; use rustc_middle::ty::visit::{TypeVisitable, TypeVisitableExt}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitor}; +use rustc_session::lint::builtin::COINDUCTIVE_OVERLAP_IN_COHERENCE; use rustc_span::symbol::sym; use rustc_span::DUMMY_SP; use std::fmt::Debug; @@ -295,9 +296,8 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( if infcx.next_trait_solver() { infcx.evaluate_obligation(obligation).map_or(false, |result| !result.may_apply()) } else { - // We use `evaluate_root_obligation` to correctly track - // intercrate ambiguity clauses. We do not need this in the - // new solver. + // We use `evaluate_root_obligation` to correctly track intercrate + // ambiguity clauses. We cannot use this in the new solver. selcx.evaluate_root_obligation(obligation).map_or( false, // Overflow has occurred, and treat the obligation as possibly holding. |result| !result.may_apply(), @@ -315,6 +315,45 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( .find(obligation_guaranteed_to_fail); if let Some(failing_obligation) = opt_failing_obligation { + // Check the failing obligation once again, treating inductive cycles as + // ambiguous instead of error. + if !infcx.next_trait_solver() + && SelectionContext::with_treat_inductive_cycle_as_ambiguous(infcx) + .evaluate_root_obligation(&failing_obligation) + .map_or(true, |result| result.may_apply()) + { + let first_local_impl = impl1_header + .impl_def_id + .as_local() + .or(impl2_header.impl_def_id.as_local()) + .expect("expected one of the impls to be local"); + infcx.tcx.struct_span_lint_hir( + COINDUCTIVE_OVERLAP_IN_COHERENCE, + infcx.tcx.local_def_id_to_hir_id(first_local_impl), + infcx.tcx.def_span(first_local_impl), + "impls that are not considered to overlap may be considered to \ + overlap in the future", + |lint| { + lint.span_label( + infcx.tcx.def_span(impl1_header.impl_def_id), + "the first impl is here", + ) + .span_label( + infcx.tcx.def_span(impl2_header.impl_def_id), + "the second impl is here", + ); + if !failing_obligation.cause.span.is_dummy() { + lint.span_label( + failing_obligation.cause.span, + "this where-clause causes a cycle, but it may be treated \ + as possibly holding in the future, causing the impls to overlap", + ); + } + lint + }, + ); + } + debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); true } else { diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 2bd2b08115784..0a8aa1bee26ac 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -118,6 +118,8 @@ pub struct SelectionContext<'cx, 'tcx> { /// policy. In essence, canonicalized queries need their errors propagated /// rather than immediately reported because we do not have accurate spans. query_mode: TraitQueryMode, + + treat_inductive_cycle: TreatInductiveCycleAs, } // A stack that walks back up the stack frame. @@ -198,6 +200,11 @@ enum BuiltinImplConditions<'tcx> { Ambiguous, } +enum TreatInductiveCycleAs { + Recur, + Ambig, +} + impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { @@ -205,6 +212,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { freshener: infcx.freshener(), intercrate_ambiguity_causes: None, query_mode: TraitQueryMode::Standard, + treat_inductive_cycle: TreatInductiveCycleAs::Recur, + } + } + + pub fn with_treat_inductive_cycle_as_ambiguous( + infcx: &'cx InferCtxt<'tcx>, + ) -> SelectionContext<'cx, 'tcx> { + assert!(infcx.intercrate, "this doesn't do caching yet, so don't use it out of intercrate"); + SelectionContext { + infcx, + freshener: infcx.freshener(), + intercrate_ambiguity_causes: None, + query_mode: TraitQueryMode::Standard, + treat_inductive_cycle: TreatInductiveCycleAs::Ambig, } } @@ -719,7 +740,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { stack.update_reached_depth(stack_arg.1); return Ok(EvaluatedToOk); } else { - return Ok(EvaluatedToRecur); + match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), + } } } return Ok(EvaluatedToOk); @@ -837,7 +861,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig), - ProjectAndUnifyResult::Recursive => Ok(EvaluatedToRecur), + ProjectAndUnifyResult::Recursive => match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), + }, ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr), } } @@ -1151,7 +1178,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { Some(EvaluatedToOk) } else { debug!("evaluate_stack --> recursive, inductive"); - Some(EvaluatedToRecur) + match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => Some(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => Some(EvaluatedToRecur), + } } } else { None diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs new file mode 100644 index 0000000000000..80530acd2aada --- /dev/null +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs @@ -0,0 +1,35 @@ +#![deny(coinductive_overlap_in_coherence)] + +use std::borrow::Borrow; +use std::cmp::Ordering; +use std::marker::PhantomData; + +#[derive(PartialEq, Default)] +pub(crate) struct Interval(PhantomData); + +// This impl overlaps with the `derive` unless we reject the nested +// `Interval: PartialOrd>` candidate which results +// in an inductive cycle right now. +impl PartialEq for Interval +//~^ ERROR impls that are not considered to overlap may be considered to overlap in the future +//~| WARN this was previously accepted by the compiler but is being phased out +where + T: Borrow, + Q: ?Sized + PartialOrd, +{ + fn eq(&self, _: &Q) -> bool { + true + } +} + +impl PartialOrd for Interval +where + T: Borrow, + Q: ?Sized + PartialOrd, +{ + fn partial_cmp(&self, _: &Q) -> Option { + None + } +} + +fn main() {} diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr new file mode 100644 index 0000000000000..fd6193f4b7424 --- /dev/null +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr @@ -0,0 +1,22 @@ +error: impls that are not considered to overlap may be considered to overlap in the future + --> $DIR/warn-when-cycle-is-error-in-coherence.rs:13:1 + | +LL | #[derive(PartialEq, Default)] + | --------- the second impl is here +... +LL | impl PartialEq for Interval + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the first impl is here +... +LL | Q: ?Sized + PartialOrd, + | ---------- this where-clause causes a cycle, but it may be treated as possibly holding in the future, causing the impls to overlap + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #114040 +note: the lint level is defined here + --> $DIR/warn-when-cycle-is-error-in-coherence.rs:1:9 + | +LL | #![deny(coinductive_overlap_in_coherence)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From d2a14df70e359660c86e79f20aaa5df1e71f4bb1 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 25 Jul 2023 09:19:42 -0700 Subject: [PATCH 3/5] nits Co-authored-by: lcnr --- compiler/rustc_lint_defs/src/builtin.rs | 5 ++++- compiler/rustc_trait_selection/src/traits/select/mod.rs | 6 +++--- tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index b3e5fee4185ae..815eadbcb91b2 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -4463,7 +4463,10 @@ declare_lint! { /// /// The manual impl of `PartialEq` impl overlaps with the `derive`, since /// if we replace `Q = Interval`, then the second impl leads to a cycle: - /// `PartialOrd for Interval where Interval: Partial`. + /// `PartialOrd for Interval where Interval: PartialOrd`. This cycle + /// currently causes the compiler to consider `Interval: PartialOrd` to not + /// hold, causing the two implementations to be disjoint. This will change in + /// a future release. pub COINDUCTIVE_OVERLAP_IN_COHERENCE, Warn, "impls that are not considered to overlap may be considered to \ diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 0a8aa1bee26ac..85f4df28bdf01 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -741,7 +741,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { return Ok(EvaluatedToOk); } else { match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToUnknown), TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), } } @@ -862,7 +862,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig), ProjectAndUnifyResult::Recursive => match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToUnknown), TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), }, ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr), @@ -1179,7 +1179,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } else { debug!("evaluate_stack --> recursive, inductive"); match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => Some(EvaluatedToAmbig), + TreatInductiveCycleAs::Ambig => Some(EvaluatedToUnknown), TreatInductiveCycleAs::Recur => Some(EvaluatedToRecur), } } diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs index 80530acd2aada..268fe56368cc6 100644 --- a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs @@ -9,7 +9,7 @@ pub(crate) struct Interval(PhantomData); // This impl overlaps with the `derive` unless we reject the nested // `Interval: PartialOrd>` candidate which results -// in an inductive cycle right now. +// in a - currently inductive - cycle. impl PartialEq for Interval //~^ ERROR impls that are not considered to overlap may be considered to overlap in the future //~| WARN this was previously accepted by the compiler but is being phased out From ca49a37390ed4fdead6432c322ab246b63306705 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 25 Jul 2023 16:49:17 +0000 Subject: [PATCH 4/5] Reuse the selection context, compute failing obligations first in ambig mode --- compiler/rustc_lint_defs/src/builtin.rs | 2 + .../src/traits/coherence.rs | 141 ++++++++---------- .../src/traits/select/mod.rs | 25 ++-- 3 files changed, 80 insertions(+), 88 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 815eadbcb91b2..cd5269dab9b96 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -4431,6 +4431,8 @@ declare_lint! { /// ### Example /// /// ```rust,compile_fail + /// #![deny(coinductive_overlap_in_coherence)] + /// /// use std::borrow::Borrow; /// use std::cmp::Ordering; /// use std::marker::PhantomData; diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 2cc0b6548769a..19a122e8b909d 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -7,7 +7,7 @@ use crate::infer::outlives::env::OutlivesEnvironment; use crate::infer::InferOk; use crate::traits::outlives_bounds::InferCtxtExt as _; -use crate::traits::select::IntercrateAmbiguityCause; +use crate::traits::select::{IntercrateAmbiguityCause, TreatInductiveCycleAs}; use crate::traits::util::impl_subject_and_oblig; use crate::traits::SkipLeakCheck; use crate::traits::{ @@ -210,16 +210,53 @@ fn overlap<'tcx>( let equate_obligations = equate_impl_headers(selcx.infcx, &impl1_header, &impl2_header)?; debug!("overlap: unification check succeeded"); - if overlap_mode.use_implicit_negative() - && impl_intersection_has_impossible_obligation( - selcx, - param_env, - &impl1_header, - impl2_header, - equate_obligations, - ) - { - return None; + if overlap_mode.use_implicit_negative() { + for mode in [TreatInductiveCycleAs::Ambig, TreatInductiveCycleAs::Recur] { + if let Some(failing_obligation) = selcx.with_treat_inductive_cycle_as(mode, |selcx| { + impl_intersection_has_impossible_obligation( + selcx, + param_env, + &impl1_header, + &impl2_header, + &equate_obligations, + ) + }) { + if matches!(mode, TreatInductiveCycleAs::Recur) { + let first_local_impl = impl1_header + .impl_def_id + .as_local() + .or(impl2_header.impl_def_id.as_local()) + .expect("expected one of the impls to be local"); + infcx.tcx.struct_span_lint_hir( + COINDUCTIVE_OVERLAP_IN_COHERENCE, + infcx.tcx.local_def_id_to_hir_id(first_local_impl), + infcx.tcx.def_span(first_local_impl), + "impls that are not considered to overlap may be considered to \ + overlap in the future", + |lint| { + lint.span_label( + infcx.tcx.def_span(impl1_header.impl_def_id), + "the first impl is here", + ) + .span_label( + infcx.tcx.def_span(impl2_header.impl_def_id), + "the second impl is here", + ); + if !failing_obligation.cause.span.is_dummy() { + lint.span_label( + failing_obligation.cause.span, + "this where-clause causes a cycle, but it may be treated \ + as possibly holding in the future, causing the impls to overlap", + ); + } + lint + }, + ); + } + + return None; + } + } } // We toggle the `leak_check` by using `skip_leak_check` when constructing the @@ -287,78 +324,30 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, param_env: ty::ParamEnv<'tcx>, impl1_header: &ty::ImplHeader<'tcx>, - impl2_header: ty::ImplHeader<'tcx>, - obligations: PredicateObligations<'tcx>, -) -> bool { + impl2_header: &ty::ImplHeader<'tcx>, + obligations: &PredicateObligations<'tcx>, +) -> Option> { let infcx = selcx.infcx; - let obligation_guaranteed_to_fail = |obligation: &PredicateObligation<'tcx>| { - if infcx.next_trait_solver() { - infcx.evaluate_obligation(obligation).map_or(false, |result| !result.may_apply()) - } else { - // We use `evaluate_root_obligation` to correctly track intercrate - // ambiguity clauses. We cannot use this in the new solver. - selcx.evaluate_root_obligation(obligation).map_or( - false, // Overflow has occurred, and treat the obligation as possibly holding. - |result| !result.may_apply(), - ) - } - }; - - let opt_failing_obligation = [&impl1_header.predicates, &impl2_header.predicates] + [&impl1_header.predicates, &impl2_header.predicates] .into_iter() .flatten() .map(|&(predicate, span)| { Obligation::new(infcx.tcx, ObligationCause::dummy_with_span(span), param_env, predicate) }) - .chain(obligations) - .find(obligation_guaranteed_to_fail); - - if let Some(failing_obligation) = opt_failing_obligation { - // Check the failing obligation once again, treating inductive cycles as - // ambiguous instead of error. - if !infcx.next_trait_solver() - && SelectionContext::with_treat_inductive_cycle_as_ambiguous(infcx) - .evaluate_root_obligation(&failing_obligation) - .map_or(true, |result| result.may_apply()) - { - let first_local_impl = impl1_header - .impl_def_id - .as_local() - .or(impl2_header.impl_def_id.as_local()) - .expect("expected one of the impls to be local"); - infcx.tcx.struct_span_lint_hir( - COINDUCTIVE_OVERLAP_IN_COHERENCE, - infcx.tcx.local_def_id_to_hir_id(first_local_impl), - infcx.tcx.def_span(first_local_impl), - "impls that are not considered to overlap may be considered to \ - overlap in the future", - |lint| { - lint.span_label( - infcx.tcx.def_span(impl1_header.impl_def_id), - "the first impl is here", - ) - .span_label( - infcx.tcx.def_span(impl2_header.impl_def_id), - "the second impl is here", - ); - if !failing_obligation.cause.span.is_dummy() { - lint.span_label( - failing_obligation.cause.span, - "this where-clause causes a cycle, but it may be treated \ - as possibly holding in the future, causing the impls to overlap", - ); - } - lint - }, - ); - } - - debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); - true - } else { - false - } + .chain(obligations.into_iter().cloned()) + .find(|obligation: &PredicateObligation<'tcx>| { + if infcx.next_trait_solver() { + infcx.evaluate_obligation(obligation).map_or(false, |result| !result.may_apply()) + } else { + // We use `evaluate_root_obligation` to correctly track intercrate + // ambiguity clauses. We cannot use this in the new solver. + selcx.evaluate_root_obligation(obligation).map_or( + false, // Overflow has occurred, and treat the obligation as possibly holding. + |result| !result.may_apply(), + ) + } + }) } /// Check if both impls can be satisfied by a common type by considering whether diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 85f4df28bdf01..6ca818b79cf65 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -200,7 +200,8 @@ enum BuiltinImplConditions<'tcx> { Ambiguous, } -enum TreatInductiveCycleAs { +#[derive(Copy, Clone)] +pub enum TreatInductiveCycleAs { Recur, Ambig, } @@ -216,17 +217,17 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } - pub fn with_treat_inductive_cycle_as_ambiguous( - infcx: &'cx InferCtxt<'tcx>, - ) -> SelectionContext<'cx, 'tcx> { - assert!(infcx.intercrate, "this doesn't do caching yet, so don't use it out of intercrate"); - SelectionContext { - infcx, - freshener: infcx.freshener(), - intercrate_ambiguity_causes: None, - query_mode: TraitQueryMode::Standard, - treat_inductive_cycle: TreatInductiveCycleAs::Ambig, - } + // Sets the `TreatInductiveCycleAs` mode temporarily in the selection context + pub fn with_treat_inductive_cycle_as( + &mut self, + treat_inductive_cycle: TreatInductiveCycleAs, + f: impl FnOnce(&mut Self) -> T, + ) -> T { + let treat_inductive_cycle = + std::mem::replace(&mut self.treat_inductive_cycle, treat_inductive_cycle); + let value = f(self); + self.treat_inductive_cycle = treat_inductive_cycle; + value } pub fn with_query_mode( From 0e20155662e5513e0bcdb1300c81458c16b7cca9 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 30 Jul 2023 21:46:04 +0000 Subject: [PATCH 5/5] more nits --- compiler/rustc_lint_defs/src/builtin.rs | 41 ++++++------------- .../src/traits/coherence.rs | 33 ++++++++++++--- .../src/traits/select/mod.rs | 33 +++++++++------ .../warn-when-cycle-is-error-in-coherence.rs | 2 +- ...rn-when-cycle-is-error-in-coherence.stderr | 5 ++- 5 files changed, 65 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index cd5269dab9b96..96c31a90da865 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -4433,42 +4433,25 @@ declare_lint! { /// ```rust,compile_fail /// #![deny(coinductive_overlap_in_coherence)] /// - /// use std::borrow::Borrow; - /// use std::cmp::Ordering; - /// use std::marker::PhantomData; + /// trait CyclicTrait {} + /// impl CyclicTrait for T {} /// - /// #[derive(PartialEq, Default)] - /// pub(crate) struct Interval(T); - /// - /// impl PartialEq for Interval - /// where - /// Q: PartialOrd, - /// { - /// fn eq(&self, other: &Q) -> bool { - /// todo!() - /// } - /// } - /// - /// impl PartialOrd for Interval - /// where - /// Q: PartialOrd, - /// { - /// fn partial_cmp(&self, other: &Q) -> Option { - /// todo!() - /// } - /// } + /// trait Trait {} + /// impl Trait for T {} + /// // conflicting impl with the above + /// impl Trait for u8 {} /// ``` /// /// {{produces}} /// /// ### Explanation /// - /// The manual impl of `PartialEq` impl overlaps with the `derive`, since - /// if we replace `Q = Interval`, then the second impl leads to a cycle: - /// `PartialOrd for Interval where Interval: PartialOrd`. This cycle - /// currently causes the compiler to consider `Interval: PartialOrd` to not - /// hold, causing the two implementations to be disjoint. This will change in - /// a future release. + /// We have two choices for impl which satisfy `u8: Trait`: the blanket impl + /// for generic `T`, and the direct impl for `u8`. These two impls nominally + /// overlap, since we can infer `T = u8` in the former impl, but since the where + /// clause `u8: CyclicTrait` would end up resulting in a cycle (since it depends + /// on itself), the blanket impl is not considered to hold for `u8`. This will + /// change in a future release. pub COINDUCTIVE_OVERLAP_IN_COHERENCE, Warn, "impls that are not considered to overlap may be considered to \ diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 19a122e8b909d..5746781ae35d7 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -231,10 +231,29 @@ fn overlap<'tcx>( COINDUCTIVE_OVERLAP_IN_COHERENCE, infcx.tcx.local_def_id_to_hir_id(first_local_impl), infcx.tcx.def_span(first_local_impl), - "impls that are not considered to overlap may be considered to \ - overlap in the future", + format!( + "implementations {} will conflict in the future", + match impl1_header.trait_ref { + Some(trait_ref) => { + let trait_ref = infcx.resolve_vars_if_possible(trait_ref); + format!( + "of `{}` for `{}`", + trait_ref.print_only_trait_path(), + trait_ref.self_ty() + ) + } + None => format!( + "for `{}`", + infcx.resolve_vars_if_possible(impl1_header.self_ty) + ), + }, + ), |lint| { - lint.span_label( + lint.note( + "impls that are not considered to overlap may be considered to \ + overlap in the future", + ) + .span_label( infcx.tcx.def_span(impl1_header.impl_def_id), "the first impl is here", ) @@ -245,8 +264,12 @@ fn overlap<'tcx>( if !failing_obligation.cause.span.is_dummy() { lint.span_label( failing_obligation.cause.span, - "this where-clause causes a cycle, but it may be treated \ - as possibly holding in the future, causing the impls to overlap", + format!( + "`{}` may be considered to hold in future releases, \ + causing the impls to overlap", + infcx + .resolve_vars_if_possible(failing_obligation.predicate) + ), ); } lint diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 6ca818b79cf65..19385e2d7f260 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -202,10 +202,25 @@ enum BuiltinImplConditions<'tcx> { #[derive(Copy, Clone)] pub enum TreatInductiveCycleAs { + /// This is the previous behavior, where `Recur` represents an inductive + /// cycle that is known not to hold. This is not forwards-compatible with + /// coinduction, and will be deprecated. This is the default behavior + /// of the old trait solver due to back-compat reasons. Recur, + /// This is the behavior of the new trait solver, where inductive cycles + /// are treated as ambiguous and possibly holding. Ambig, } +impl From for EvaluationResult { + fn from(treat: TreatInductiveCycleAs) -> EvaluationResult { + match treat { + TreatInductiveCycleAs::Ambig => EvaluatedToUnknown, + TreatInductiveCycleAs::Recur => EvaluatedToRecur, + } + } +} + impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { @@ -223,6 +238,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { treat_inductive_cycle: TreatInductiveCycleAs, f: impl FnOnce(&mut Self) -> T, ) -> T { + // Should be executed in a context where caching is disabled, + // otherwise the cache is poisoned with the temporary result. + assert!(self.is_intercrate()); let treat_inductive_cycle = std::mem::replace(&mut self.treat_inductive_cycle, treat_inductive_cycle); let value = f(self); @@ -741,10 +759,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { stack.update_reached_depth(stack_arg.1); return Ok(EvaluatedToOk); } else { - match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToUnknown), - TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), - } + return Ok(self.treat_inductive_cycle.into()); } } return Ok(EvaluatedToOk); @@ -862,10 +877,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig), - ProjectAndUnifyResult::Recursive => match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToUnknown), - TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), - }, + ProjectAndUnifyResult::Recursive => Ok(self.treat_inductive_cycle.into()), ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr), } } @@ -1179,10 +1191,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { Some(EvaluatedToOk) } else { debug!("evaluate_stack --> recursive, inductive"); - match self.treat_inductive_cycle { - TreatInductiveCycleAs::Ambig => Some(EvaluatedToUnknown), - TreatInductiveCycleAs::Recur => Some(EvaluatedToRecur), - } + Some(self.treat_inductive_cycle.into()) } } else { None diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs index 268fe56368cc6..01f7d6ce90101 100644 --- a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs @@ -11,7 +11,7 @@ pub(crate) struct Interval(PhantomData); // `Interval: PartialOrd>` candidate which results // in a - currently inductive - cycle. impl PartialEq for Interval -//~^ ERROR impls that are not considered to overlap may be considered to overlap in the future +//~^ ERROR implementations of `PartialEq>` for `Interval<_>` will conflict in the future //~| WARN this was previously accepted by the compiler but is being phased out where T: Borrow, diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr index fd6193f4b7424..f315ba8213029 100644 --- a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr @@ -1,4 +1,4 @@ -error: impls that are not considered to overlap may be considered to overlap in the future +error: implementations of `PartialEq>` for `Interval<_>` will conflict in the future --> $DIR/warn-when-cycle-is-error-in-coherence.rs:13:1 | LL | #[derive(PartialEq, Default)] @@ -8,10 +8,11 @@ LL | impl PartialEq for Interval | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the first impl is here ... LL | Q: ?Sized + PartialOrd, - | ---------- this where-clause causes a cycle, but it may be treated as possibly holding in the future, causing the impls to overlap + | ---------- `Interval<_>: PartialOrd` may be considered to hold in future releases, causing the impls to overlap | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #114040 + = note: impls that are not considered to overlap may be considered to overlap in the future note: the lint level is defined here --> $DIR/warn-when-cycle-is-error-in-coherence.rs:1:9 |