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

Changing module order causes specialization to fail #50452

Closed
tuxmark5 opened this issue May 4, 2018 · 2 comments · Fixed by #54906
Closed

Changing module order causes specialization to fail #50452

tuxmark5 opened this issue May 4, 2018 · 2 comments · Fixed by #54906
Labels
A-specialization Area: Trait impl specialization C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tuxmark5
Copy link

tuxmark5 commented May 4, 2018

The below example fails to compile with error "not all trait items implemented, missing: trait_1".
Swapping modules one and two causes the code to compile correctly.
The same issue happens when one and two are defined in external files.

Version: rustc 1.27.0-nightly (e82261d 2018-05-03)

#![feature(specialization)]

mod two {
    use one::{Foo, Trait};

    impl Trait for Foo<i32> {
        fn trait_2(&self) {}
    }
    
    impl Trait for Foo<i64> { // not all trait items implemented, missing: `trait_1`
        fn trait_2(&self) {}
    }
}

mod one {
    pub struct Foo<A> { data: A }
    
    pub trait Trait {
        fn trait_1(&self);
        fn trait_2(&self);
    }
    
    impl<A> Trait for Foo<A> {
        fn trait_1(&self) {}
        default fn trait_2(&self) {}
    }
}

fn main() {
}
@sfackler sfackler added the A-specialization Area: Trait impl specialization label May 4, 2018
@XAMPPRocky XAMPPRocky added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Oct 2, 2018
@qnighy
Copy link
Contributor

qnighy commented Oct 8, 2018

Minimized:

#![feature(specialization)]

pub trait Foo {
    fn foo();
}

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

In the current nightly, changing the order of the items or uncommenting the line will make it compile. The latter behavior is also strange.

@qnighy
Copy link
Contributor

qnighy commented Oct 8, 2018

Ah, this is likely caused by a bug in specialization graph construction. Consider the following specialization graph:

  Tr1
   |
   I3
  /  \
 I1  I2

When we try to construct the graph by adding I1, I2, and I3 in this order, the intermediate graph will look like:

  Tr1
  /  \
 I1  I2

Then the compiler compares I1 and I3. The problem is, after the compiler find out that I1 specializes I3, it will immediately insert I3 between Tr1 and I1, resulting in the following graph:

  Tr1
  /  \
 I3  I2
  |
 I1

I will prepare a fix for this.

qnighy added a commit to qnighy/rust that referenced this issue Oct 8, 2018
kennytm added a commit to kennytm/rust that referenced this issue Nov 15, 2018
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 rust-lang#50452.
bors added a commit that referenced this issue Nov 15, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-specialization Area: Trait impl specialization C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants