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

Trait bounds on struct type parameters aren't always enforced (depending on the order of impls) #21837

Closed
SSheldon opened this issue Feb 1, 2015 · 15 comments
Labels
A-type-system Area: Type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority

Comments

@SSheldon
Copy link
Contributor

SSheldon commented Feb 1, 2015

The following compiles without issue, even though the impl of Trait2 is missing a trait bound that T implements Bound:

pub trait Bound { }
pub struct Foo<T: Bound>;

pub trait Trait1 { }
impl<T: Bound> Trait1 for Foo<T> { }

pub trait Trait2 { }
impl<T> Trait2 for Foo<T> { }

fn main() { }

However, if you switch the order of the impls...

pub trait Bound { }
pub struct Foo<T: Bound>;

pub trait Trait2 { }
impl<T> Trait2 for Foo<T> { }

pub trait Trait1 { }
impl<T: Bound> Trait1 for Foo<T> { }

fn main() { }

You suddenly start getting this error:

error: the trait `Bound` is not implemented for the type `T` [E0277]
impl<T> Trait2 for Foo<T> { }
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

I did not expect the ordering of the impls to matter here. I'm using rustc 1.0.0-nightly (1d00c545e 2015-01-30 19:56:34 +0000).

@pnkfelix
Copy link
Member

pnkfelix commented Feb 5, 2015

P-high, not 1.0. (But definitely a weird bug that we would like to fix.)

@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Feb 5, 2015
@nikomatsakis
Copy link
Contributor

cc me

@SSheldon
Copy link
Contributor Author

SSheldon commented Apr 4, 2015

@nikomatsakis, @pnkfelix: should this be P-backcompat-lang since the compiler accepts code that it should not?

@pnkfelix
Copy link
Member

pnkfelix commented Apr 4, 2015

@SSheldon I think we decided this is one of those edge cases where the amount of breakage (that fixing this is expected to incur) was deemed acceptable for fixing post 1.0.

At the time this was last triaged, there was a general rule that if something was tagged P-backcompat-lang, then it must also be set for the 1.0 milestone (at the latest).

However, since then during recent triage meetings, we have been more willing to tag things as P-backcompat-lang even if they were not attached to the 1.0 milestone.

So, with that in mind:

triage: P-backcompat-lang, not P-high

@rust-highfive rust-highfive added P-backcompat-lang and removed P-medium Medium priority labels Apr 4, 2015
@brson brson added P-high High priority and removed P-backcompat-lang labels Apr 29, 2015
@pnkfelix pnkfelix self-assigned this Apr 30, 2015
@arielb1
Copy link
Contributor

arielb1 commented Oct 1, 2015

This causes an RFC1214 warning in 1.4+.

@nikomatsakis
Copy link
Contributor

triage: P-medium

Looks like this is already fixed by RFC 1214, as @arielb1 reports.

@rust-highfive rust-highfive added P-medium Medium priority and removed P-high High priority labels Oct 29, 2015
@nikomatsakis nikomatsakis added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 29, 2015
@nikomatsakis
Copy link
Contributor

Tagging as E-needstest, since we should be able to add a test with WARN messages (and perhaps a #[rustc_error] annotation on main)

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Oct 29, 2015
@nikomatsakis
Copy link
Contributor

Tagging as mentor just because I can give advice on how to craft a suitable test (nmatsakis on IRC)

@bltavares
Copy link
Contributor

Triaging:

Both examples causes error on nightly rustc 1.11.0-nightly (5c2a5d449 2016-06-11), but this seems to be the expected behaviour.

<anon>:2:16: 2:17 error: parameter `T` is never used [E0392]
<anon>:2 pub struct Foo<T: Bound>;
                        ^
<anon>:2:16: 2:17 help: see the detailed explanation for E0392
<anon>:2:16: 2:17 help: consider removing `T` or using a marker such as `core::marker::PhantomData`
<anon>:8:9: 8:15 error: the trait `Bound` is not implemented for the type `T` [E0277]
<anon>:8 impl<T> Trait2 for Foo<T> { }
                 ^~~~~~
<anon>:8:9: 8:15 help: see the detailed explanation for E0277
<anon>:8:9: 8:15 note: required by `Foo`
error: aborting due to 2 previous errors

@nikomatsakis I would like to help with the test if you don't mind giving initial directions.

@SSheldon
Copy link
Contributor Author

@bltavares, that repro case was from pre-1.0, here's the updated repro case:

pub trait Bound { }
pub struct Foo<T: Bound>(T);

pub trait Trait1 { }
impl<T: Bound> Trait1 for Foo<T> { }

pub trait Trait2 { }
impl<T> Trait2 for Foo<T> { }

fn main() { }

That code will compile with rustc 1.0.0 (a59de37e9 2015-05-13), but fails to compile with rustc 1.9.0 (e4e8b6668 2016-05-18).

@brson
Copy link
Contributor

brson commented Jul 19, 2016

@arielb1 This seems to have been fixed by RFC 1214. Can it be closed?

@pnkfelix
Copy link
Member

@brson I think it needs a test. I probably should not be assigned to it since I don't plan to spend time making a test.

@pnkfelix pnkfelix removed their assignment Jul 20, 2016
@brson brson added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jul 30, 2016
@martinhath
Copy link
Contributor

So all there is to do is add a test which checks that Ssheldons code does not compile?

@nikomatsakis
Copy link
Contributor

@martinhath yes :)

martinhath pushed a commit to martinhath/rust that referenced this issue Oct 3, 2016
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2016
Add regression test for Issue rust-lang#21837

This PR adds a regression test for Issue rust-lang#21837, as explained in the comments of the issue.
@SSheldon
Copy link
Contributor Author

SSheldon commented Oct 8, 2016

Fixed now with a test merged in #36941

@SSheldon SSheldon closed this as completed Oct 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

9 participants