From 6e95110b2456e5cd6720ed0802747fc4c577354d Mon Sep 17 00:00:00 2001 From: David Matos Date: Thu, 19 Sep 2024 02:01:06 +0200 Subject: [PATCH] Add enum variant discriminant changed lint (#912) * Add enum variant discriminant changed lint * Change human_readable name * Change CI test to allow extra failure for this lint * Change behavior of lint to lint API specific breakage * Make modifications to lint and add extra test cases * Add two test cases to cover `non_exhaustive` Typos and grammar * spacing on lint file --- .github/workflows/ci.yml | 8 +- ...m_no_repr_variant_discriminant_changed.ron | 84 ++++++++++++++++ src/query.rs | 1 + .../new/Cargo.toml | 7 ++ .../new/src/lib.rs | 95 +++++++++++++++++++ .../old/Cargo.toml | 7 ++ .../old/src/lib.rs | 93 ++++++++++++++++++ ...pr_variant_discriminant_changed.output.ron | 56 +++++++++++ ...enum_struct_variant_field_added.output.ron | 13 +++ ...m_variant_marked_non_exhaustive.output.ron | 13 +++ 10 files changed, 373 insertions(+), 4 deletions(-) create mode 100644 src/lints/enum_no_repr_variant_discriminant_changed.ron create mode 100644 test_crates/enum_no_repr_variant_discriminant_changed/new/Cargo.toml create mode 100644 test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs create mode 100644 test_crates/enum_no_repr_variant_discriminant_changed/old/Cargo.toml create mode 100644 test_crates/enum_no_repr_variant_discriminant_changed/old/src/lib.rs create mode 100644 test_outputs/enum_no_repr_variant_discriminant_changed.output.ron diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ca7d480c..c7718ad5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1329,8 +1329,8 @@ jobs: - name: Check output run: | cd semver - EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---")" - RESULT="$(cat output | grep failure)" + EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")" + RESULT="$(cat output | grep failure | sort)" diff <(echo "$RESULT") <(echo "$EXPECTED") - name: Cleanup @@ -1361,8 +1361,8 @@ jobs: - name: Check output run: | cd semver - EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---")" - RESULT="$(cat output | grep failure)" + EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")" + RESULT="$(cat output | grep failure | sort)" diff <(echo "$RESULT") <(echo "$EXPECTED") - name: Save rustdoc diff --git a/src/lints/enum_no_repr_variant_discriminant_changed.ron b/src/lints/enum_no_repr_variant_discriminant_changed.ron new file mode 100644 index 00000000..e17f14ae --- /dev/null +++ b/src/lints/enum_no_repr_variant_discriminant_changed.ron @@ -0,0 +1,84 @@ +SemverQuery( + id: "enum_no_repr_variant_discriminant_changed", + human_readable_name: "enum variant had its discriminant change value", + description: "A public enum's variant had its discriminant changed from its previous value.", + reference: Some("A public enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`."), + required_update: Major, + lint_level: Deny, + reference_link: Some("https://doc.rust-lang.org/reference/items/enumerations.html#assigning-discriminant-values"), + query: r#" + { + CrateDiff { + baseline { + item { + ... on Enum { + visibility_limit @filter(op: "=", value: ["$public"]) @output + enum_name: name @output @tag + + attribute @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { + content { + base @filter(op: "=", value: ["$repr"]) + } + } + + importable_path { + path @output @tag + public_api @filter(op: "=", value: ["$true"]) + } + + variant { + variant_name: name @output @tag + + discriminant { + old_value: value @output @tag + } + } + + variant @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { + attrs @filter(op: "contains", value: ["$non_exhaustive"]) + } + } + } + } + current { + item { + ... on Enum { + visibility_limit @filter(op: "=", value: ["$public"]) + name @filter(op: "=", value: ["%enum_name"]) + + importable_path { + path @filter(op: "=", value: ["%path"]) + public_api @filter(op: "=", value: ["$true"]) + } + + variant { + name @filter(op: "=", value: ["%variant_name"]) + + discriminant { + new_value: value @output @filter(op: "!=", value: ["%old_value"]) + } + + span_: span @optional { + filename @output + begin_line @output + } + } + + variant @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { + attrs @filter(op: "contains", value: ["$non_exhaustive"]) + } + } + } + } + } + }"#, + arguments: { + "public": "public", + "repr": "repr", + "non_exhaustive": "#[non_exhaustive]", + "zero": 0, + "true": true, + }, + error_message: "The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`.", + per_result_error_template: Some("variant {{enum_name}}::{{variant_name}} {{old_value}} -> {{new_value}} in {{span_filename}}:{{span_begin_line}}"), +) diff --git a/src/query.rs b/src/query.rs index 28c5787b..e6179896 100644 --- a/src/query.rs +++ b/src/query.rs @@ -1044,6 +1044,7 @@ add_lints!( enum_marked_non_exhaustive, enum_missing, enum_must_use_added, + enum_no_repr_variant_discriminant_changed, enum_now_doc_hidden, enum_repr_int_changed, enum_repr_int_removed, diff --git a/test_crates/enum_no_repr_variant_discriminant_changed/new/Cargo.toml b/test_crates/enum_no_repr_variant_discriminant_changed/new/Cargo.toml new file mode 100644 index 00000000..fcfd8659 --- /dev/null +++ b/test_crates/enum_no_repr_variant_discriminant_changed/new/Cargo.toml @@ -0,0 +1,7 @@ +[package] +publish = false +name = "enum_no_repr_variant_discriminant_changed" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs b/test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs new file mode 100644 index 00000000..84545953 --- /dev/null +++ b/test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs @@ -0,0 +1,95 @@ +// Explicit discriminant changed values. By doing so, it changed the implicit +// discriminant's value as well. Should be reported. +pub enum ExplicitAndImplicitDiscriminantsAreChanged { + First = 2, + Second, + Third = 5, +} + +// Discriminant changed values. Having #[non_exhaustive] on the enum should not have any effect +// on the *API* breakage. Should be reported. +#[non_exhaustive] +pub enum DiscriminantIsChanged { + First = 1, +} + +// Discriminant changed values and the variant is also marked as `non_exhaustive`. +// This now implies that the discriminant is no longer `well-defined`, which means that a numeric +// cast is no longer possible on the enum. Should not be reported. +// https://github.com/rust-lang/reference/pull/1249#issuecomment-2339003824 +#[non_exhaustive] +pub enum DiscriminantIsChangedAndMarkedNonExhaustive { + First, + #[non_exhaustive] + Second = 2, +} + +// Discriminant changed values, but the variant is already `non_exhaustive`. +// This means that the discriminant is already not `well-defined`, and the numeric cast +// was never possible. Should not be reported. +#[non_exhaustive] +pub enum DiscriminantIsChangedButAlreadyNonExhaustive { + First, + #[non_exhaustive] + Second = 2, +} + +// Variant became doc(hidden) while variant became explicit. Being doc(hidden) is not relevant +// since it's still part of the public API. Should be reported. +pub enum DiscriminantBecomesDocHiddenAndExplicit { + First, + #[doc(hidden)] + Second = 2, +} + +// Explicit discriminants changed values, but being private dominates. Should not be +// reported. +enum PrivateEnum { + First = 10, + Second = 11, +} + +// Discriminant changed values, but the enum has a `repr` attr. Should not be reported as it +// involves ABI breakage as well, and this is linted at *API* only breakage. +#[repr(u8)] +pub enum FieldlessWithDiscrimants { + First = 10, + Tuple(), + Struct {}, + Unit, + Last = 21, +} + +// Implicit discriminant value changed by becoming explicit. Should not be reported as it involves +// ABI breakage as well, and this is linted at *API* only breakage. +#[repr(u8)] +pub enum FieldlessChangesToExplicitDescriminant { + Tuple(), + Struct {}, + Unit = 5, +} + +// Variant `Struct` acquires a field. By doing so, it makes the discriminant not `well-defined`, +// meaning it is not possible to do a numeric cast anymore on the enum. Should not be reported. +pub enum UnitOnlyBecomesUndefined { + First, + Second, + Struct { a: i64 }, +} + +// Discriminant changed values, but the other variant is marked `non_exhaustive`. +// This means that a numeric cast is not possible on the enum. Should not be reported. +#[non_exhaustive] +pub enum DiscriminantIsChangedButAVariantIsNonExhaustive { + #[non_exhaustive] + First, + Second = 2, +} + +// Discriminant changed values, and the variant is no longer `non_exhaustive`. +// The fact that the variant is no longer `non_exhaustive` is not relevant, since a numeric cast +// was impossible to begin with. Should not be reported. +pub enum DiscriminantIsChangedAndVariantNoLongerNonExhaustive { + First, + Second = 2, +} diff --git a/test_crates/enum_no_repr_variant_discriminant_changed/old/Cargo.toml b/test_crates/enum_no_repr_variant_discriminant_changed/old/Cargo.toml new file mode 100644 index 00000000..fcfd8659 --- /dev/null +++ b/test_crates/enum_no_repr_variant_discriminant_changed/old/Cargo.toml @@ -0,0 +1,7 @@ +[package] +publish = false +name = "enum_no_repr_variant_discriminant_changed" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/enum_no_repr_variant_discriminant_changed/old/src/lib.rs b/test_crates/enum_no_repr_variant_discriminant_changed/old/src/lib.rs new file mode 100644 index 00000000..ad935b9f --- /dev/null +++ b/test_crates/enum_no_repr_variant_discriminant_changed/old/src/lib.rs @@ -0,0 +1,93 @@ +// Explicit discriminant changed values. By doing so, it changed the implicit +// discriminant's value as well. Should be reported. +pub enum ExplicitAndImplicitDiscriminantsAreChanged { + First = 1, + Second, + Third = 5, +} + +// Discriminant changed values. Having #[non_exhaustive] on the enum should not have any effect +// on the *API* breakage. Should be reported. +#[non_exhaustive] +pub enum DiscriminantIsChanged { + First, +} + +// Discriminant changed values and the variant is also marked as `non_exhaustive`. +// This now implies that the discriminant is no longer `well-defined`, which means that a numeric +// cast is no longer possible on the enum. Should not be reported. +// https://github.com/rust-lang/reference/pull/1249#issuecomment-2339003824 +#[non_exhaustive] +pub enum DiscriminantIsChangedAndMarkedNonExhaustive { + First, + Second, +} + +// Discriminant changed values, but the variant is already `non_exhaustive`. +// This means that the discriminant is already not `well-defined`, and the numeric cast +// was never possible. Should not be reported. +#[non_exhaustive] +pub enum DiscriminantIsChangedButAlreadyNonExhaustive { + First, + #[non_exhaustive] + Second, +} + +// Variant became doc(hidden) while variant became explicit. Being doc(hidden) is not relevant +// since it's still part of the public API. Should be reported. +pub enum DiscriminantBecomesDocHiddenAndExplicit { + First, + Second, +} + +// Explicit discriminants changed values, but being private dominates. Should not be +// reported. +enum PrivateEnum { + First = 1, + Second = 2, +} + +// Discriminant changed values, but the enum has a `repr` attr. Should not be reported as it +// involves ABI breakage as well, and this is linted at *API* only breakage. +#[repr(u8)] +pub enum FieldlessWithDiscrimants { + First = 10, + Tuple(), + Struct {}, + Unit, + Last = 20, +} + +// Implicit discriminant value changed by becoming explicit. Should not be reported as it involves +// ABI breakage as well, and this is linted at *API* only breakage. +pub enum FieldlessChangesToExplicitDescriminant { + Tuple(), + Struct {}, + Unit, +} + +// Variant `Struct` acquires a field. By doing so, it makes the discriminant not `well-defined`, +// meaning it is not possible to do a numeric cast anymore on the enum. Should not be reported. +pub enum UnitOnlyBecomesUndefined { + First, + Second, + Struct {}, +} + +// Discriminant changed values, but the other variant is marked `non_exhaustive`. +// This means that a numeric cast is not possible on the enum. Should not be reported. +#[non_exhaustive] +pub enum DiscriminantIsChangedButAVariantIsNonExhaustive { + #[non_exhaustive] + First, + Second, +} + +// Discriminant changed values, and the variant is no longer `non_exhaustive`. +// The fact that the variant is no longer `non_exhaustive` is not relevant, since a numeric cast +// was impossible to begin with. Should not be reported. +pub enum DiscriminantIsChangedAndVariantNoLongerNonExhaustive { + #[non_exhaustive] + First, + Second, +} diff --git a/test_outputs/enum_no_repr_variant_discriminant_changed.output.ron b/test_outputs/enum_no_repr_variant_discriminant_changed.output.ron new file mode 100644 index 00000000..88c6dac0 --- /dev/null +++ b/test_outputs/enum_no_repr_variant_discriminant_changed.output.ron @@ -0,0 +1,56 @@ +{ + "./test_crates/enum_no_repr_variant_discriminant_changed/": [ + { + "enum_name": String("ExplicitAndImplicitDiscriminantsAreChanged"), + "new_value": String("2"), + "old_value": String("1"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("ExplicitAndImplicitDiscriminantsAreChanged"), + ]), + "span_begin_line": Uint64(4), + "span_filename": String("src/lib.rs"), + "variant_name": String("First"), + "visibility_limit": String("public"), + }, + { + "enum_name": String("ExplicitAndImplicitDiscriminantsAreChanged"), + "new_value": String("3"), + "old_value": String("2"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("ExplicitAndImplicitDiscriminantsAreChanged"), + ]), + "span_begin_line": Uint64(5), + "span_filename": String("src/lib.rs"), + "variant_name": String("Second"), + "visibility_limit": String("public"), + }, + { + "enum_name": String("DiscriminantIsChanged"), + "new_value": String("1"), + "old_value": String("0"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("DiscriminantIsChanged"), + ]), + "span_begin_line": Uint64(13), + "span_filename": String("src/lib.rs"), + "variant_name": String("First"), + "visibility_limit": String("public"), + }, + { + "enum_name": String("DiscriminantBecomesDocHiddenAndExplicit"), + "new_value": String("2"), + "old_value": String("1"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("DiscriminantBecomesDocHiddenAndExplicit"), + ]), + "span_begin_line": Uint64(42), + "span_filename": String("src/lib.rs"), + "variant_name": String("Second"), + "visibility_limit": String("public"), + }, + ] +} diff --git a/test_outputs/enum_struct_variant_field_added.output.ron b/test_outputs/enum_struct_variant_field_added.output.ron index 1aa468c5..f83db451 100644 --- a/test_outputs/enum_struct_variant_field_added.output.ron +++ b/test_outputs/enum_struct_variant_field_added.output.ron @@ -1,4 +1,17 @@ { + "./test_crates/enum_no_repr_variant_discriminant_changed/": [ + { + "enum_name": String("UnitOnlyBecomesUndefined"), + "field_name": String("a"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("UnitOnlyBecomesUndefined"), + ]), + "span_begin_line": Uint64(77), + "span_filename": String("src/lib.rs"), + "variant_name": String("Struct"), + }, + ], "./test_crates/enum_struct_field_hidden_from_public_api/": [ { "enum_name": String("AddedVariantField"), diff --git a/test_outputs/enum_variant_marked_non_exhaustive.output.ron b/test_outputs/enum_variant_marked_non_exhaustive.output.ron index fbd58f68..d6be7414 100644 --- a/test_outputs/enum_variant_marked_non_exhaustive.output.ron +++ b/test_outputs/enum_variant_marked_non_exhaustive.output.ron @@ -1,4 +1,17 @@ { + "./test_crates/enum_no_repr_variant_discriminant_changed/": [ + { + "name": String("DiscriminantIsChangedAndMarkedNonExhaustive"), + "path": List([ + String("enum_no_repr_variant_discriminant_changed"), + String("DiscriminantIsChangedAndMarkedNonExhaustive"), + ]), + "span_begin_line": Uint64(24), + "span_filename": String("src/lib.rs"), + "variant_name": String("Second"), + "visibility_limit": String("public"), + }, + ], "./test_crates/enum_struct_variant_field_added/": [ { "name": String("EnumWithExhaustiveVariant"),