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 lints: an enum variant changes from unit/tuple/struct to another kind. #884

Open
suaviloquence opened this issue Aug 24, 2024 · 1 comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@suaviloquence
Copy link
Contributor

suaviloquence commented Aug 24, 2024

Describe your use case

We have lints for when a struct changes "kind":

However, currently cargo-semver-checks does not check when an enum variant changes kind like this.

from:

pub enum Enum { 
    WasTuple(),
    WasStruct {},
    WasUnit,
    TupleItem (()),
    StructItem { a: (), },
    Unit,
}

to:

pub enum Enum {
    WasTuple {},
    WasStruct,
    WasUnit(),
    TupleItem { a: () },
    StructItem,
    Unit(()),
}

the first three variants would break with:

let _ = Enum::WasTuple;
// since it's now a tuple variant, this still gives us a value,
// but it's a `fn(()) -> Enum` tuple constructor, not an `Enum` instance.
let _: Enum = Enum::WasStruct;
let _ = Enum::WasUnit;

the second three also have breakage in a similar fashion, and cargo-semver-checks currently does not catch the
fields_missing/removed lints because these variants changed kind

Describe the solution you'd like

Thus, it would be helpful to have lint(s) that detect this change for constructible enum variants - if a tuple/struct variant is marked #[non_exhaustive] in the baseline version, this is not a breaking change because it was not possible to construct these variants in the baseline version.

Alternatives, if applicable

No response

Additional Context

No response

@suaviloquence suaviloquence added the C-enhancement Category: raise the bar on expectations label Aug 24, 2024
@obi1kenobi obi1kenobi added A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Aug 25, 2024
@obi1kenobi
Copy link
Owner

obi1kenobi commented Aug 25, 2024

Nice catch!

Similarly to the #[non_exhaustive] case, if the variant was #[doc(hidden)] we probably don't want to lint on it since the variant isn't meant to be constructed by downstream code.

For a piece of code like:

pub enum Example {
    Plain,
    Tuple(),
    Struct {},
}

Note that #[non_exhaustive] tuple variants can't be constructed by an external crate, so witness (proof-of-breakage) code can't rely on constructing the variant.

We want the following lints:

  • public API unit variant that isn't #[non_exhaustive] changes to another kind: Enum unit variant changed kind #894
    • witness: let _: Example = Example::Plain only works for unit variants
  • public API tuple variant that isn't #[non_exhaustive] changes to another kind: Enum tuple variant changed kind #906
    • witness let _: Example = Example::Tuple(...) only works for tuple variants, nothing else can use parens
  • public API #[non_exhaustive] tuple variant that has at least one public API field changes to another kind
    • witness: accessing the public API field, since struct variants can't declare fields with numeric keys to make .0 work
    • this covers the edge case of the previous bullet point; we add #[non_exhaustive] to this lint to ensure the two lints are completely disjoint
  • public API struct variant with at least one public API field changes to another kind
    • witness: accessing the public API field, since it must have been named non-numerically (not .0) so plain and tuple structs can't have the same field
    • changing a struct variant with no fields is not breaking since {}-like instantiation is allowed for all variant kinds

Remaining questions:

  • Is it a breaking change if a #[non_exhaustive] unit variant changes to another kind?
    • I think the answer is "no" since I can't come up with a witness. Is this correct?

@obi1kenobi obi1kenobi changed the title Add lints for when an enum variant changes from unit/tuple/struct to another kind. New lints: an enum variant changes from unit/tuple/struct to another kind. Sep 2, 2024
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 C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

2 participants