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

Tracking issue for `overlapping_inherent_impls` compatibility lint #36889

Closed
petrochenkov opened this Issue Oct 1, 2016 · 18 comments

Comments

Projects
None yet
8 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 1, 2016

UPDATE: We've decided to convert this warning into a hard error, but are still waiting for a PR to make that a reality. If you're interested in contributing, there are directions for writing such a PR available here.

This tracking issue pertains to a bug fix for #22889. The problem was that the compiler was accepting two inherent impls that both defined the same method, which creates an obvious ambiguity:

struct Foo;

impl Foo {
    fn id() {}
}

impl Foo {
    fn id() {}
}

The most obvious fix here is to give one method a distinct name, though sometimes there are other possible rewrites (e.g., one could rewrite the impls to use a double dispatch pattern).

Current status

  • #31925 introduces the overlapping_inherent_impls lint as warn-by-default
  • #36894 makes the overlapping_inherent_impls lint deny-by-default
  • PR ? makes the overlapping_inherent_impls lint a hard error
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 1, 2016

@aturon , could you write a description for this?

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 10, 2017

#38967 could be related to this.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 24, 2017

@rfcbot fcp merge

This warning has been out for a while. I propose we move it to deny-by-default.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 24, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 24, 2017

@nikomatsakis

I propose we move it to deny-by-default.

There's a catch.
It's already deny-by-default for almost 4 months.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 25, 2017

@petrochenkov heh =) Um, d'oh! Then I propose we kill it. I'll start again. :)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 25, 2017

@rfcbot fcp cancel

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 25, 2017

@nikomatsakis proposal cancelled.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 25, 2017

@rfcbot fcp merge

This warning has been deny-by-default for a while. I propose we move it to a hard error.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 25, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 30, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 9, 2017

The final comment period is now complete.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 15, 2017

cc @petrochenkov @rust-lang/compiler -- anyone want to make a quick stabilization PR that we can backport to 1.17 beta?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 16, 2017

This seems like a good first bug. I wrote up some directions and posted them to forge for converting compatibility lints into hard errors. The descriptions probably haven't landed yet, but here is the PR:

rust-lang/rust-forge#48

Marking a E-easy and E-mentor, the directions are in that PR. =)

@topecongiro

This comment has been minimized.

Copy link
Contributor

topecongiro commented Mar 19, 2017

Hello, I would like to work on this if no one is interested.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 21, 2017

@topecongiro go for it:)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 21, 2017

@topecongiro please feel free to ping me on irc (nmatsakis) if you encounter any difficulty...

@topecongiro

This comment has been minimized.

Copy link
Contributor

topecongiro commented Mar 22, 2017

@nikomatsakis Thank you!

bors added a commit that referenced this issue Mar 30, 2017

Auto merge of #40728 - topecongiro:stabilize, r=arielb1
Make overlapping_inherent_impls lint a hard error

Closes #36889.

bors added a commit that referenced this issue Mar 31, 2017

Auto merge of #40728 - topecongiro:stabilize, r=arielb1
Make overlapping_inherent_impls lint a hard error

Closes #36889.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 31, 2017

Rollup merge of rust-lang#40728 - topecongiro:stabilize, r=arielb1
Make overlapping_inherent_impls lint a hard error

Closes rust-lang#36889.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 31, 2017

Rollup merge of rust-lang#40728 - topecongiro:stabilize, r=arielb1
Make overlapping_inherent_impls lint a hard error

Closes rust-lang#36889.

@bors bors closed this in #40728 Apr 1, 2017

@joshlf joshlf referenced this issue May 2, 2017

Merged

Allocators, take III #1398

12 of 25 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.