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

New lint: non-sealed pub trait method default implementation removed #294

Closed
1 of 2 tasks
SmolSir opened this issue Jan 17, 2023 · 4 comments · Fixed by #880
Closed
1 of 2 tasks

New lint: non-sealed pub trait method default implementation removed #294

SmolSir opened this issue Jan 17, 2023 · 4 comments · Fixed by #880
Labels
A-lint Area: new or existing lint

Comments

@SmolSir
Copy link
Collaborator

SmolSir commented Jan 17, 2023

When the pub trait method's default implementation is removed, the method is no more provided and turns into a declared method. This forces the new version to implement the now-declared method in all of the impl blocks which were deriving that trait with its previously-provided method in the old version.

Required new version: Major

This requires some schema additions:

  • adding the edge from Trait to its Methods [linked PR]
  • enabling the has_body field for Method [linked PR]
@SmolSir
Copy link
Collaborator Author

SmolSir commented Jan 17, 2023

Should we rename this issue to #293 which was meant to be this but I accidentally duplicated when linking in #5? (sigh)

@SmolSir SmolSir added the A-lint Area: new or existing lint label Jan 17, 2023
@obi1kenobi
Copy link
Owner

Sure!

@obi1kenobi obi1kenobi changed the title non-sealed trait removed method default implementation New lint: non-sealed pub trait method default implementation removed Jan 17, 2023
@obi1kenobi
Copy link
Owner

The "non-sealed" part here is very important: removing default implementations for sealed traits is not a breaking change -- I think it isn't even semver-minor.

Removing the default implementation is breaking if it can force other crates to provide implementations whereas previously they relied on the existence of a default. Sealed traits cannot be implemented by other crates -- only the crate that defines the sealed trait can implement it. So no other crate could possibly have had implementations of the trait which might be broken.

Sealing a previously non-sealed trait, however, is a very breaking change, and I just added it to the list in #5.

@obi1kenobi
Copy link
Owner

We still need the has_body property on Method, otherwise this is now implementable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants