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

coherence: fix is_knowable logic #46192

Merged
merged 6 commits into from
Dec 6, 2017
Merged
Changes from all commits
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
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
@@ -204,6 +204,12 @@ declare_lint! {
"detects generic lifetime arguments in path segments with late bound lifetime parameters"
}

declare_lint! {
pub INCOHERENT_FUNDAMENTAL_IMPLS,
Warn,
"potentially-conflicting impls were erroneously allowed"
}

declare_lint! {
pub DEPRECATED,
Warn,
@@ -267,6 +273,7 @@ impl LintPass for HardwiredLints {
MISSING_FRAGMENT_SPECIFIER,
PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,
LATE_BOUND_LIFETIME_ARGUMENTS,
INCOHERENT_FUNDAMENTAL_IMPLS,
DEPRECATED,
UNUSED_UNSAFE,
UNUSED_MUT,
249 changes: 192 additions & 57 deletions src/librustc/traits/coherence.rs

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
@@ -60,6 +60,13 @@ mod structural_impls;
pub mod trans;
mod util;

// Whether to enable bug compatibility with issue #43355
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum IntercrateMode {
Issue43355,
Fixed
}

/// An `Obligation` represents some trait reference (e.g. `int:Eq`) for
/// which the vtable must be found. The process of finding a vtable is
/// called "resolving" the `Obligation`. This process consists of
99 changes: 60 additions & 39 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
@@ -13,8 +13,9 @@
use self::SelectionCandidate::*;
use self::EvaluationResult::*;

use super::coherence;
use super::coherence::{self, Conflict};
use super::DerivedObligationCause;
use super::IntercrateMode;
use super::project;
use super::project::{normalize_with_depth, Normalized, ProjectionCacheKey};
use super::{PredicateObligation, TraitObligation, ObligationCause};
@@ -87,7 +88,7 @@ pub struct SelectionContext<'cx, 'gcx: 'cx+'tcx, 'tcx: 'cx> {
/// other words, we consider `$0 : Bar` to be unimplemented if
/// there is no type that the user could *actually name* that
/// would satisfy it. This avoids crippling inference, basically.
intercrate: bool,
intercrate: Option<IntercrateMode>,

inferred_obligations: SnapshotVec<InferredObligationsSnapshotVecDelegate<'tcx>>,

@@ -111,21 +112,24 @@ impl IntercrateAmbiguityCause {
/// See #23980 for details.
pub fn add_intercrate_ambiguity_hint<'a, 'tcx>(&self,
err: &mut ::errors::DiagnosticBuilder) {
err.note(&self.intercrate_ambiguity_hint());
}

pub fn intercrate_ambiguity_hint(&self) -> String {
match self {
&IntercrateAmbiguityCause::DownstreamCrate { ref trait_desc, ref self_desc } => {
let self_desc = if let &Some(ref ty) = self_desc {
format!(" for type `{}`", ty)
} else { "".to_string() };
err.note(&format!("downstream crates may implement trait `{}`{}",
trait_desc, self_desc));
format!("downstream crates may implement trait `{}`{}", trait_desc, self_desc)
}
&IntercrateAmbiguityCause::UpstreamCrateUpdate { ref trait_desc, ref self_desc } => {
let self_desc = if let &Some(ref ty) = self_desc {
format!(" for type `{}`", ty)
} else { "".to_string() };
err.note(&format!("upstream crates may add new impl of trait `{}`{} \
in future versions",
trait_desc, self_desc));
format!("upstream crates may add new impl of trait `{}`{} \
in future versions",
trait_desc, self_desc)
}
}
}
@@ -417,17 +421,19 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: false,
intercrate: None,
inferred_obligations: SnapshotVec::new(),
intercrate_ambiguity_causes: Vec::new(),
}
}

pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'gcx, 'tcx>) -> SelectionContext<'cx, 'gcx, 'tcx> {
pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'gcx, 'tcx>,
mode: IntercrateMode) -> SelectionContext<'cx, 'gcx, 'tcx> {
debug!("intercrate({:?})", mode);
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: true,
intercrate: Some(mode),
inferred_obligations: SnapshotVec::new(),
intercrate_ambiguity_causes: Vec::new(),
}
@@ -758,7 +764,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
debug!("evaluate_trait_predicate_recursively({:?})",
obligation);

if !self.intercrate && obligation.is_global() {
if !self.intercrate.is_some() && obligation.is_global() {
// If a param env is consistent, global obligations do not depend on its particular
// value in order to work, so we can clear out the param env and get better
// caching. (If the current param env is inconsistent, we don't care what happens).
@@ -814,7 +820,11 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// terms of `Fn` etc, but we could probably make this more
// precise still.
let unbound_input_types = stack.fresh_trait_ref.input_types().any(|ty| ty.is_fresh());
if unbound_input_types && self.intercrate {
// this check was an imperfect workaround for a bug n the old
// intercrate mode, it should be removed when that goes away.
if unbound_input_types &&
self.intercrate == Some(IntercrateMode::Issue43355)
{
debug!("evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous",
stack.fresh_trait_ref);
// Heuristics: show the diagnostics when there are no candidates in crate.
@@ -1077,28 +1087,32 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
return Ok(None);
}

if !self.is_knowable(stack) {
debug!("coherence stage: not knowable");
// Heuristics: show the diagnostics when there are no candidates in crate.
let candidate_set = self.assemble_candidates(stack)?;
if !candidate_set.ambiguous && candidate_set.vec.is_empty() {
let trait_ref = stack.obligation.predicate.skip_binder().trait_ref;
let self_ty = trait_ref.self_ty();
let trait_desc = trait_ref.to_string();
let self_desc = if self_ty.has_concrete_skeleton() {
Some(self_ty.to_string())
} else {
None
};
let cause = if !coherence::trait_ref_is_local_or_fundamental(self.tcx(),
trait_ref) {
IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_desc, self_desc }
} else {
IntercrateAmbiguityCause::DownstreamCrate { trait_desc, self_desc }
};
self.intercrate_ambiguity_causes.push(cause);
match self.is_knowable(stack) {
None => {}
Some(conflict) => {
debug!("coherence stage: not knowable");
// Heuristics: show the diagnostics when there are no candidates in crate.
let candidate_set = self.assemble_candidates(stack)?;
if !candidate_set.ambiguous && candidate_set.vec.iter().all(|c| {
!self.evaluate_candidate(stack, &c).may_apply()
}) {
let trait_ref = stack.obligation.predicate.skip_binder().trait_ref;
let self_ty = trait_ref.self_ty();
let trait_desc = trait_ref.to_string();
let self_desc = if self_ty.has_concrete_skeleton() {
Some(self_ty.to_string())
} else {
None
};
let cause = if let Conflict::Upstream = conflict {
IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_desc, self_desc }
} else {
IntercrateAmbiguityCause::DownstreamCrate { trait_desc, self_desc }
};
self.intercrate_ambiguity_causes.push(cause);
}
return Ok(None);
}
return Ok(None);
}

let candidate_set = self.assemble_candidates(stack)?;
@@ -1205,12 +1219,12 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {

fn is_knowable<'o>(&mut self,
stack: &TraitObligationStack<'o, 'tcx>)
-> bool
-> Option<Conflict>
{
debug!("is_knowable(intercrate={})", self.intercrate);
debug!("is_knowable(intercrate={:?})", self.intercrate);

if !self.intercrate {
return true;
if !self.intercrate.is_some() {
return None;
}

let obligation = &stack.obligation;
@@ -1221,7 +1235,14 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// bound regions
let trait_ref = predicate.skip_binder().trait_ref;

coherence::trait_ref_is_knowable(self.tcx(), trait_ref)
let result = coherence::trait_ref_is_knowable(self.tcx(), trait_ref);
if let (Some(Conflict::Downstream { used_to_be_broken: true }),
Some(IntercrateMode::Issue43355)) = (result, self.intercrate) {
debug!("is_knowable: IGNORING conflict to be bug-compatible with #43355");
None
} else {
result
}
}

/// Returns true if the global caches can be used.
@@ -1246,7 +1267,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// the master cache. Since coherence executes pretty quickly,
// it's not worth going to more trouble to increase the
// hit-rate I don't think.
if self.intercrate {
if self.intercrate.is_some() {
return false;
}

39 changes: 29 additions & 10 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
@@ -30,6 +30,8 @@ use ty::{self, TyCtxt, TypeFoldable};
use syntax_pos::DUMMY_SP;
use std::rc::Rc;

use lint;

pub mod specialization_graph;

/// Information pertinent to an overlapping impl error.
@@ -325,16 +327,33 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx
// This is where impl overlap checking happens:
let insert_result = sg.insert(tcx, impl_def_id);
// Report error if there was one.
if let Err(overlap) = insert_result {
let mut err = struct_span_err!(tcx.sess,
tcx.span_of_impl(impl_def_id).unwrap(),
E0119,
"conflicting implementations of trait `{}`{}:",
overlap.trait_desc,
overlap.self_desc.clone().map_or(String::new(),
|ty| {
format!(" for type `{}`", ty)
}));
let (overlap, used_to_be_allowed) = match insert_result {
Err(overlap) => (Some(overlap), false),
Ok(opt_overlap) => (opt_overlap, true)
};

if let Some(overlap) = overlap {
let msg = format!("conflicting implementations of trait `{}`{}:{}",
overlap.trait_desc,
overlap.self_desc.clone().map_or(
String::new(), |ty| {
format!(" for type `{}`", ty)
}),
if used_to_be_allowed { " (E0119)" } else { "" }
);
let mut err = if used_to_be_allowed {
tcx.struct_span_lint_node(
lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,
tcx.hir.as_local_node_id(impl_def_id).unwrap(),
tcx.span_of_impl(impl_def_id).unwrap(),
&msg)
} else {
struct_span_err!(tcx.sess,
tcx.span_of_impl(impl_def_id).unwrap(),
E0119,
"{}",
msg)
};

match tcx.span_of_impl(overlap.with_impl) {
Ok(span) => {
68 changes: 45 additions & 23 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
@@ -68,7 +68,7 @@ struct Children {
/// The result of attempting to insert an impl into a group of children.
enum Inserted {
/// The impl was inserted as a new child in this group of children.
BecameNewSibling,
BecameNewSibling(Option<OverlapError>),

/// The impl replaced an existing impl that specializes it.
Replaced(DefId),
@@ -105,17 +105,39 @@ impl<'a, 'gcx, 'tcx> Children {
simplified_self: Option<SimplifiedType>)
-> Result<Inserted, OverlapError>
{
let mut last_lint = None;

for slot in match simplified_self {
Some(sty) => self.filtered_mut(sty),
None => self.iter_mut(),
} {
let possible_sibling = *slot;

let overlap_error = |overlap: traits::coherence::OverlapResult| {
// overlap, but no specialization; error out
let trait_ref = overlap.impl_header.trait_ref.unwrap();
let self_ty = trait_ref.self_ty();
OverlapError {
with_impl: possible_sibling,
trait_desc: trait_ref.to_string(),
// only report the Self type if it has at least
// some outer concrete shell; otherwise, it's
// not adding much information.
self_desc: if self_ty.has_concrete_skeleton() {
Some(self_ty.to_string())
} else {
None
},
intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes,
}
};

let tcx = tcx.global_tcx();
let (le, ge) = tcx.infer_ctxt().enter(|infcx| {
let overlap = traits::overlapping_impls(&infcx,
possible_sibling,
impl_def_id);
impl_def_id,
traits::IntercrateMode::Issue43355);
if let Some(overlap) = overlap {
if tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) {
return Ok((false, false));
@@ -125,22 +147,7 @@ impl<'a, 'gcx, 'tcx> Children {
let ge = tcx.specializes((possible_sibling, impl_def_id));

if le == ge {
// overlap, but no specialization; error out
let trait_ref = overlap.impl_header.trait_ref.unwrap();
let self_ty = trait_ref.self_ty();
Err(OverlapError {
with_impl: possible_sibling,
trait_desc: trait_ref.to_string(),
// only report the Self type if it has at least
// some outer concrete shell; otherwise, it's
// not adding much information.
self_desc: if self_ty.has_concrete_skeleton() {
Some(self_ty.to_string())
} else {
None
},
intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes,
})
Err(overlap_error(overlap))
} else {
Ok((le, ge))
}
@@ -163,14 +170,27 @@ impl<'a, 'gcx, 'tcx> Children {
*slot = impl_def_id;
return Ok(Inserted::Replaced(possible_sibling));
} else {
if !tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) {
tcx.infer_ctxt().enter(|infcx| {
if let Some(overlap) = traits::overlapping_impls(
&infcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Fixed)
{
last_lint = Some(overlap_error(overlap));
}
});
}

// no overlap (error bailed already via ?)
}
}

// no overlap with any potential siblings, so add as a new sibling
debug!("placing as new sibling");
self.insert_blindly(tcx, impl_def_id);
Ok(Inserted::BecameNewSibling)
Ok(Inserted::BecameNewSibling(last_lint))
}

fn iter_mut(&'a mut self) -> Box<Iterator<Item = &'a mut DefId> + 'a> {
@@ -199,7 +219,7 @@ impl<'a, 'gcx, 'tcx> Graph {
pub fn insert(&mut self,
tcx: TyCtxt<'a, 'gcx, 'tcx>,
impl_def_id: DefId)
-> Result<(), OverlapError> {
-> Result<Option<OverlapError>, OverlapError> {
assert!(impl_def_id.is_local());

let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap();
@@ -220,10 +240,11 @@ impl<'a, 'gcx, 'tcx> Graph {
self.parent.insert(impl_def_id, trait_def_id);
self.children.entry(trait_def_id).or_insert(Children::new())
.insert_blindly(tcx, impl_def_id);
return Ok(());
return Ok(None);
}

let mut parent = trait_def_id;
let mut last_lint = None;
let simplified = fast_reject::simplify_type(tcx, trait_ref.self_ty(), false);

// Descend the specialization tree, where `parent` is the current parent node
@@ -234,7 +255,8 @@ impl<'a, 'gcx, 'tcx> Graph {
.insert(tcx, impl_def_id, simplified)?;

match insert_result {
BecameNewSibling => {
BecameNewSibling(opt_lint) => {
last_lint = opt_lint;
break;
}
Replaced(new_child) => {
@@ -251,7 +273,7 @@ impl<'a, 'gcx, 'tcx> Graph {
}

self.parent.insert(impl_def_id, parent);
Ok(())
Ok(last_lint)
}

/// Insert cached metadata mapping from a child impl back to its parent.
5 changes: 4 additions & 1 deletion src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
@@ -247,11 +247,14 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
id: LintId::of(SAFE_PACKED_BORROWS),
reference: "issue #46043 <https://github.com/rust-lang/rust/issues/46043>",
},
FutureIncompatibleInfo {
id: LintId::of(INCOHERENT_FUNDAMENTAL_IMPLS),
reference: "issue #46205 <https://github.com/rust-lang/rust/issues/46205>",
},
FutureIncompatibleInfo {
id: LintId::of(COERCE_NEVER),
reference: "issue #46325 <https://github.com/rust-lang/rust/issues/46325>",
},

]);

// Register renamed and removed lints
52 changes: 41 additions & 11 deletions src/librustc_typeck/coherence/inherent_impls_overlap.rs
Original file line number Diff line number Diff line change
@@ -12,9 +12,11 @@ use namespace::Namespace;
use rustc::hir::def_id::{CrateNum, DefId, LOCAL_CRATE};
use rustc::hir;
use rustc::hir::itemlikevisit::ItemLikeVisitor;
use rustc::traits;
use rustc::traits::{self, IntercrateMode};
use rustc::ty::TyCtxt;

use lint;

pub fn crate_inherent_impls_overlap_check<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
crate_num: CrateNum) {
assert_eq!(crate_num, LOCAL_CRATE);
@@ -28,7 +30,8 @@ struct InherentOverlapChecker<'a, 'tcx: 'a> {

impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> {
fn check_for_common_items_in_impls(&self, impl1: DefId, impl2: DefId,
overlap: traits::OverlapResult) {
overlap: traits::OverlapResult,
used_to_be_allowed: bool) {

let name_and_namespace = |def_id| {
let item = self.tcx.associated_item(def_id);
@@ -43,11 +46,21 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> {

for &item2 in &impl_items2[..] {
if (name, namespace) == name_and_namespace(item2) {
let mut err = struct_span_err!(self.tcx.sess,
self.tcx.span_of_impl(item1).unwrap(),
E0592,
"duplicate definitions with name `{}`",
name);
let node_id = self.tcx.hir.as_local_node_id(impl1);
let mut err = if used_to_be_allowed && node_id.is_some() {
self.tcx.struct_span_lint_node(
lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,
node_id.unwrap(),
self.tcx.span_of_impl(item1).unwrap(),
&format!("duplicate definitions with name `{}` (E0592)", name)
)
} else {
struct_span_err!(self.tcx.sess,
self.tcx.span_of_impl(item1).unwrap(),
E0592,
"duplicate definitions with name `{}`",
name)
};

err.span_label(self.tcx.span_of_impl(item1).unwrap(),
format!("duplicate definitions for `{}`", name));
@@ -69,12 +82,30 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> {

for (i, &impl1_def_id) in impls.iter().enumerate() {
for &impl2_def_id in &impls[(i + 1)..] {
self.tcx.infer_ctxt().enter(|infcx| {
let used_to_be_allowed = self.tcx.infer_ctxt().enter(|infcx| {
if let Some(overlap) =
traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) {
self.check_for_common_items_in_impls(impl1_def_id, impl2_def_id, overlap)
traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id,
IntercrateMode::Issue43355)
{
self.check_for_common_items_in_impls(
impl1_def_id, impl2_def_id, overlap, false);
false
} else {
true
}
});

if used_to_be_allowed {
self.tcx.infer_ctxt().enter(|infcx| {
if let Some(overlap) =
traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id,
IntercrateMode::Fixed)
{
self.check_for_common_items_in_impls(
impl1_def_id, impl2_def_id, overlap, true);
}
});
}
}
}
}
@@ -100,4 +131,3 @@ impl<'a, 'tcx, 'v> ItemLikeVisitor<'v> for InherentOverlapChecker<'a, 'tcx> {
fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem) {
}
}

32 changes: 32 additions & 0 deletions src/test/compile-fail/issue-43355.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![deny(incoherent_fundamental_impls)]

pub trait Trait1<X> {
type Output;
}

pub trait Trait2<X> {}

pub struct A;

impl<X, T> Trait1<X> for T where T: Trait2<X> {
type Output = ();
}

impl<X> Trait1<Box<X>> for A {
//~^ ERROR conflicting implementations of trait
//~| hard error
//~| downstream crates may implement trait `Trait2<std::boxed::Box<_>>` for type `A`
type Output = i32;
}

fn main() {}
36 changes: 36 additions & 0 deletions src/test/run-pass/issue-43355.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Check that the code for issue #43355 can run without an ICE, please remove
// this test when it becomes an hard error.

pub trait Trait1<X> {
type Output;
}
pub trait Trait2<X> {}

impl<X, T> Trait1<X> for T where T: Trait2<X> {
type Output = ();
}
impl<X> Trait1<Box<X>> for A {
type Output = i32;
}

pub struct A;

fn f<X, T: Trait1<Box<X>>>() {
println!("k: {}", ::std::mem::size_of::<<T as Trait1<Box<X>>>::Output>());
}

pub fn g<X, T: Trait2<Box<X>>>() {
f::<X, T>();
}

fn main() {}