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

Add enum variant discriminant changed lint #912

Merged
8 changes: 4 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
84 changes: 84 additions & 0 deletions src/lints/enum_no_repr_variant_discriminant_changed.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
SemverQuery(
id: "enum_no_repr_variant_discriminant_changed",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard. This is not the best name, and not the worst. Nothing better comes to my mind, but if you have other suggestions, it would be good to consider them.

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}}"),
)
1 change: 1 addition & 0 deletions src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "enum_no_repr_variant_discriminant_changed"
version = "0.1.0"
edition = "2021"

[dependencies]
Original file line number Diff line number Diff line change
@@ -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]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case where another variant is #[non_exhaustive], not the one that changes discriminant values?

I think there's a bug in the lint query related to it: the lint is checking for "is the variant with the changed discriminant marked #[non_exhaustive]" when it should be checking for "does the enum have any #[non_exhaustive] variants" — not quite the same thing.

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,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "enum_no_repr_variant_discriminant_changed"
version = "0.1.0"
edition = "2021"

[dependencies]
Original file line number Diff line number Diff line change
@@ -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,
}
56 changes: 56 additions & 0 deletions test_outputs/enum_no_repr_variant_discriminant_changed.output.ron
Original file line number Diff line number Diff line change
@@ -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"),
},
]
}
13 changes: 13 additions & 0 deletions test_outputs/enum_struct_variant_field_added.output.ron
Original file line number Diff line number Diff line change
@@ -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"),
Expand Down
Loading
Loading