Skip to content

Commit

Permalink
Auto merge of #54906 - qnighy:fix-issue-50452, r=nikomatsakis
Browse files Browse the repository at this point in the history
Reattach all grandchildren when constructing specialization graph.

Specialization graphs are constructed by incrementally adding impls in the order of declaration. If the impl being added has its specializations in the graph already, they should be reattached under the impl. However, the current implementation only reattaches the one found first. Therefore, in the following specialization graph,

```
  Tr1
   |
   I3
  /  \
 I1  I2
```

If `I1`, `I2`, and `I3` are declared in this order, the compiler mistakenly constructs the following graph:

```
  Tr1
  /  \
 I3  I2
  |
 I1
```

This patch fixes the reattach procedure to include all specializing grandchildren-to-be.

Fixes #50452.
  • Loading branch information
bors committed Nov 15, 2018
2 parents 4ec0ba9 + 3a8e0af commit 99e3fca
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 8 deletions.
27 changes: 19 additions & 8 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ enum Inserted {
/// The impl was inserted as a new child in this group of children.
BecameNewSibling(Option<OverlapError>),

/// The impl should replace an existing impl X, because the impl specializes X.
ReplaceChild(DefId),
/// The impl should replace existing impls [X1, ..], because the impl specializes X1, X2, etc.
ReplaceChildren(Vec<DefId>),

/// The impl is a specialization of an existing child.
ShouldRecurseOn(DefId),
Expand Down Expand Up @@ -124,6 +124,7 @@ impl<'a, 'gcx, 'tcx> Children {
-> Result<Inserted, OverlapError>
{
let mut last_lint = None;
let mut replace_children = Vec::new();

debug!(
"insert(impl_def_id={:?}, simplified_self={:?})",
Expand Down Expand Up @@ -194,7 +195,7 @@ impl<'a, 'gcx, 'tcx> Children {
debug!("placing as parent of TraitRef {:?}",
tcx.impl_trait_ref(possible_sibling).unwrap());

return Ok(Inserted::ReplaceChild(possible_sibling));
replace_children.push(possible_sibling);
} else {
if !tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) {
traits::overlapping_impls(
Expand All @@ -211,6 +212,10 @@ impl<'a, 'gcx, 'tcx> Children {
}
}

if !replace_children.is_empty() {
return Ok(Inserted::ReplaceChildren(replace_children));
}

// no overlap with any potential siblings, so add as a new sibling
debug!("placing as new sibling");
self.insert_blindly(tcx, impl_def_id);
Expand Down Expand Up @@ -282,7 +287,7 @@ impl<'a, 'gcx, 'tcx> Graph {
last_lint = opt_lint;
break;
}
ReplaceChild(grand_child_to_be) => {
ReplaceChildren(grand_children_to_be) => {
// We currently have
//
// P
Expand All @@ -302,17 +307,23 @@ impl<'a, 'gcx, 'tcx> Graph {
let siblings = self.children
.get_mut(&parent)
.unwrap();
siblings.remove_existing(tcx, grand_child_to_be);
for &grand_child_to_be in &grand_children_to_be {
siblings.remove_existing(tcx, grand_child_to_be);
}
siblings.insert_blindly(tcx, impl_def_id);
}

// Set G's parent to N and N's parent to P
self.parent.insert(grand_child_to_be, impl_def_id);
for &grand_child_to_be in &grand_children_to_be {
self.parent.insert(grand_child_to_be, impl_def_id);
}
self.parent.insert(impl_def_id, parent);

// Add G as N's child.
self.children.entry(impl_def_id).or_default()
.insert_blindly(tcx, grand_child_to_be);
for &grand_child_to_be in &grand_children_to_be {
self.children.entry(impl_def_id).or_default()
.insert_blindly(tcx, grand_child_to_be);
}
break;
}
ShouldRecurseOn(new_parent) => {
Expand Down
32 changes: 32 additions & 0 deletions src/test/compile-fail/specialization/issue-50452.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2018 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.

// compile-fail

#![feature(specialization)]

pub trait Foo {
fn foo();
}

impl Foo for i32 {}
impl Foo for i64 {
fn foo() {}
//~^ERROR `foo` specializes an item from a parent `impl`
}
impl<T> Foo for T {
fn foo() {}
}

fn main() {
i32::foo();
i64::foo();
u8::foo();
}
29 changes: 29 additions & 0 deletions src/test/run-pass/specialization/issue-50452.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2018 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.

// run-pass

#![feature(specialization)]

pub trait Foo {
fn foo();
}

impl Foo for i32 {}
impl Foo for i64 {}
impl<T> Foo for T {
fn foo() {}
}

fn main() {
i32::foo();
i64::foo();
u8::foo();
}

0 comments on commit 99e3fca

Please sign in to comment.