-
-
Notifications
You must be signed in to change notification settings - Fork 130
Add lint inherent_associated_pub_const_missing #714
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
Changes from 23 commits
d1ba5a7
37702f4
9ba6760
0c677cc
79adc6d
6ff5d23
bd81dc1
99b523d
3995496
8042812
938db7d
ea1f008
f11a39f
76b2b57
90c3172
49eb155
4cf6944
4081a40
c4062cf
11c7098
afb330a
60b2ed3
353d8a8
c42b06c
7da4ce5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| SemverQuery( | ||
| id: "inherent_associated_pub_const_missing", | ||
| human_readable_name: "inherent impl's associated pub const removed", | ||
| description: "An inherent impl's associated public const removed or renamed", | ||
| required_update: Major, | ||
| reference_link: Some("https://doc.rust-lang.org/cargo/reference/semver.html#item-remove"), | ||
| query: r#" | ||
| { | ||
| CrateDiff { | ||
| baseline { | ||
| item { | ||
| ... on ImplOwner { | ||
| visibility_limit @filter(op: "=", value: ["$public"]) @output | ||
|
|
||
| importable_path { | ||
| path @output @tag | ||
| public_api @filter(op: "=", value: ["$true"]) | ||
| } | ||
|
|
||
| inherent_impl { | ||
| public_api_eligible @filter(op: "=", value: ["$true"]) | ||
|
|
||
| associated_constant { | ||
| associated_constant: name @output @tag | ||
| public_api_eligible @filter(op: "=", value: ["$true"]) | ||
|
|
||
| span_: span @optional { | ||
| filename @output | ||
| begin_line @output | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| current { | ||
| item { | ||
| ... on ImplOwner { | ||
| visibility_limit @filter(op: "=", value: ["$public"]) | ||
| name @output | ||
|
|
||
| importable_path { | ||
| path @filter(op: "=", value: ["%path"]) | ||
| public_api @filter(op: "=", value: ["$true"]) | ||
| } | ||
|
|
||
| inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { | ||
| associated_constant { | ||
| name @filter(op: "=", value: ["%associated_constant"]) | ||
| } | ||
| } | ||
|
|
||
| or: | ||
|
obi1kenobi marked this conversation as resolved.
Outdated
|
||
| impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { | ||
| implemented_trait { | ||
| trait { | ||
| associated_constant { | ||
| name @filter(op: "=", value: ["%associated_constant"]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }"#, | ||
| arguments: { | ||
| "public": "public", | ||
| "true": true, | ||
| "zero": 0, | ||
| }, | ||
| error_message: "An inherent impl's associated public constant is removed or renamed", | ||
| per_result_error_template: Some("{{name}}::{{associated_constant}}, previously at {{span_filename}}:{{span_begin_line}}"), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [package] | ||
| publish = false | ||
| name = "inherent_associated_pub_const_missing" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [dependencies] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| // Test Cases where #[doc(hidden)] is neither added or removed | ||
| pub struct StructA {} | ||
|
|
||
| impl StructA { | ||
| pub const PublicConstantB: i32 = 0; | ||
| // Should Be caught on renaming | ||
| pub const PublicConstantRenamedA: i32 = 0; | ||
| } | ||
|
|
||
| struct StructB {} | ||
|
|
||
| impl StructB { | ||
| // Should not be caught since StructB is not pub | ||
| pub const PublicConstantRenamedB: i32 = 0; | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| pub struct StructC {} | ||
|
|
||
| impl StructC { | ||
| // Should not be caught on removing or renaming since the struct is #[doc(hidden)] | ||
| pub const PublicConstantRenamedC: i32 = 0; | ||
| } | ||
|
|
||
| pub struct StructD {} | ||
|
|
||
| #[doc(hidden)] | ||
| impl StructD { | ||
| pub const PublicConstantE: i32 = 0; | ||
| } | ||
|
|
||
| // Test Cases where #[doc(hidden)] is added | ||
| pub struct StructE {} | ||
|
|
||
| #[doc(hidden)] | ||
| impl StructE { | ||
| pub const PublicConstantH: i32 = 0; | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| pub struct StructF {} | ||
|
|
||
| impl StructF { | ||
| pub const PublicConstantJ: i32 = 0; | ||
| } | ||
|
|
||
| // Test cases where #[doc(hidden)] is removed | ||
| pub struct DocHiddenStruct {} | ||
|
|
||
| impl DocHiddenStruct { | ||
| pub const PublicConstantL: i32 = 0; | ||
| } | ||
|
|
||
| impl DocHiddenStruct { | ||
| pub const PublicConstantO: i32 = 0; | ||
| } | ||
|
|
||
| // Test for when an inherent const is implemented as trait's const | ||
| pub trait TraitA { | ||
| const N_A: i64 = 0; | ||
| } | ||
|
|
||
| pub struct ExampleA; | ||
|
|
||
| impl TraitA for ExampleA {} | ||
|
|
||
| // Test for when an inherent const is implemented as trait's const but given a value in impl block | ||
| pub trait TraitB { | ||
| const N_B: i64; | ||
| } | ||
|
|
||
| pub struct ExampleB; | ||
|
|
||
| impl TraitB for ExampleB { | ||
| const N_B: i64 = 0; | ||
| } | ||
|
Comment on lines
+14
to
+76
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add these kinds of variants to the baseline as well, so that we also test whether removals of Right now, I think these tests still don't target the Also, as a bit of a nitpick: please consider adding blank lines between different items, such as between structs and impl blocks etc. Code this tight is quite difficult to read, even if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the misunderstanding, so we want test cases for when we remove the const from impl block/struct that were #[doc(hidden)] or we remove the const that was #[doc(hidden)] and the lint should be triggered in such cases
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want both kinds of test cases. At a high level, our tests serve three purposes:
So we want to check Currently, you have the first kind of test covered well. Removing Making the const, its impl block, or the owning type become
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Summary of added test cases:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent, nicely done! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [package] | ||
| publish = false | ||
| name = "inherent_associated_pub_const_missing" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [dependencies] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| // Test Cases where #[doc(hidden)] is neither added or removed | ||
| pub struct StructA {} | ||
|
|
||
| impl StructA { | ||
| // Basic Test case should be caught | ||
| pub const PublicConstantA: i32 = 0; | ||
| pub const PublicConstantB: i32 = 0; | ||
| // Const that was #[doc(hidden)] will be removed | ||
| #[doc(hidden)] | ||
| pub const DocHiddenConstantA: i32 = 0; | ||
| // Should Be caught on renaming | ||
| pub const PublicConstantRenameA: i32 = 0; | ||
| // Should not be caught on removing since its not pub | ||
| const ConstantA: i32 = 0; | ||
| } | ||
|
|
||
| struct StructB {} | ||
|
|
||
| impl StructB { | ||
| // Should not be caught since StructB is not pub | ||
| pub const PublicConstantC: i32 = 0; | ||
| pub const PublicConstantRenameB: i32 = 0; | ||
| const ConstantB: i32 = 0; | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| pub struct StructC {} | ||
|
|
||
| impl StructC { | ||
| // Should not be caught on removing or renaming since the struct #[doc(hidden)] | ||
| pub const PublicConstantD: i32 = 0; | ||
| pub const PublicConstantRenameC: i32 = 0; | ||
| } | ||
|
|
||
| pub struct StructD {} | ||
|
obi1kenobi marked this conversation as resolved.
|
||
|
|
||
| #[doc(hidden)] | ||
| impl StructD { | ||
| pub const PublicConstantE: i32 = 0; | ||
| pub const PublicConstantF: i32 = 0; | ||
| } | ||
|
obi1kenobi marked this conversation as resolved.
|
||
|
|
||
| // Test Cases where #[doc(hidden)] is added | ||
| pub struct StructE {} | ||
|
|
||
| // This impl block will be #[doc(hidden)] | ||
| impl StructE { | ||
| pub const PublicConstantH: i32 = 0; | ||
| // This will be removed | ||
| pub const PublicConstantI: i32 = 0; | ||
| } | ||
|
|
||
| // This struct will be #[doc(hidden)] | ||
| pub struct StructF {} | ||
|
|
||
| impl StructF { | ||
| pub const PublicConstantJ: i32 = 0; | ||
| // This const will be removed | ||
| pub const PublicConstantK: i32 = 0; | ||
| } | ||
|
|
||
| // Test cases where #[doc(hidden)] is removed | ||
| #[doc(hidden)] | ||
| pub struct DocHiddenStruct {} | ||
|
|
||
| impl DocHiddenStruct { | ||
| // This const will be removed | ||
| #[doc(hidden)] | ||
| pub const DocHiddenConstantB: i32 = 0; | ||
| pub const PublicConstantL: i32 = 0; | ||
| // This const will be removed | ||
| pub const PublicConstantM: i32 = 0; | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| impl DocHiddenStruct { | ||
| // This const will be removed | ||
| pub const PublicConstantN: i32 = 0; | ||
| pub const PublicConstantO: i32 = 0; | ||
| } | ||
|
|
||
| // Test for when an inherent const is implemented as trait's const | ||
| pub trait TraitA {} | ||
|
|
||
| pub struct ExampleA; | ||
|
|
||
| impl ExampleA { | ||
| pub const N_A: i64 = 0; | ||
| } | ||
|
|
||
| impl TraitA for ExampleA {} | ||
|
|
||
| // Test for when an inherent const is implemented as trait's const but given a value in impl | ||
| pub trait TraitB {} | ||
|
|
||
| pub struct ExampleB; | ||
|
|
||
| impl ExampleB { | ||
| pub const N_B: i64 = 0; | ||
| } | ||
|
|
||
| impl TraitB for ExampleB {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| { | ||
| "./test_crates/inherent_associated_pub_const_missing/": [ | ||
| { | ||
| "associated_constant": String("PublicConstantA"), | ||
| "name": String("StructA"), | ||
| "path": List([ | ||
| String("inherent_associated_pub_const_missing"), | ||
| String("StructA"), | ||
| ]), | ||
| "span_begin_line": Uint64(6), | ||
| "span_filename": String("src/lib.rs"), | ||
| "visibility_limit": String("public"), | ||
| }, | ||
| { | ||
| "associated_constant": String("PublicConstantRenameA"), | ||
| "name": String("StructA"), | ||
| "path": List([ | ||
| String("inherent_associated_pub_const_missing"), | ||
| String("StructA"), | ||
| ]), | ||
| "span_begin_line": Uint64(12), | ||
| "span_filename": String("src/lib.rs"), | ||
| "visibility_limit": String("public"), | ||
| }, | ||
| { | ||
| "associated_constant": String("PublicConstantI"), | ||
| "name": String("StructE"), | ||
| "path": List([ | ||
| String("inherent_associated_pub_const_missing"), | ||
| String("StructE"), | ||
| ]), | ||
| "span_begin_line": Uint64(50), | ||
|
obi1kenobi marked this conversation as resolved.
Outdated
|
||
| "span_filename": String("src/lib.rs"), | ||
| "visibility_limit": String("public"), | ||
| }, | ||
| ], | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.