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.

Sign up for GitHub

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

Jump to bottom

Add lint inherent_associated_pub_const_missing #714

Merged
merged 25 commits into from
Apr 14, 2024
Merged

Add lint inherent_associated_pub_const_missing #714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d1ba5a7
Added lint inherent_associated_pub_const_missing
arpity22 Mar 22, 2024
37702f4
fixes
arpity22 Mar 22, 2024
9ba6760
Modifications
arpity22 Mar 22, 2024
0c677cc
Modifications
arpity22 Mar 23, 2024
79adc6d
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 25, 2024
6ff5d23
Adding new test cases
arpity22 Mar 29, 2024
bd81dc1
editing test output
arpity22 Mar 29, 2024
99b523d
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 29, 2024
3995496
Adding new test cases
arpity22 Mar 29, 2024
8042812
Adding new test cases
arpity22 Mar 29, 2024
938db7d
Update src/lints/inherent_associated_pub_const_missing.ron
arpity22 Mar 30, 2024
ea1f008
adding test case
arpity22 Mar 30, 2024
f11a39f
Merge branch 'main' into main
arpity22 Mar 30, 2024
76b2b57
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 30, 2024
90c3172
adding test case
arpity22 Mar 30, 2024
49eb155
Merge branch 'main' of github.com:arpity22/cargo-semver-checks
arpity22 Mar 30, 2024
4cf6944
Update src/lints/inherent_associated_pub_const_missing.ron
arpity22 Mar 31, 2024
4081a40
Merge branch 'main' into main
arpity22 Mar 31, 2024
c4062cf
Merge branch 'main' into main
obi1kenobi Apr 1, 2024
11c7098
Merge branch 'main' into main
obi1kenobi Apr 3, 2024
afb330a
Query Modifications
arpity22 Apr 4, 2024
60b2ed3
Formatting
arpity22 Apr 4, 2024
353d8a8
Merge branch 'main' into main
arpity22 Apr 14, 2024
c42b06c
Apply suggestions from code review
obi1kenobi Apr 14, 2024
7da4ce5
Merge branch 'main' into main
obi1kenobi Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions test_crates/inherent_associated_pub_const_missing/new/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
pub struct StructA {}
impl StructA {
pub const PublicConstantB: i32 = 0;
#[doc(hidden)]
pub const PublicConstantE: i32 = 0;
pub const PublicConstantRenamedA: i32 = 0;
}
struct StructB {}
impl StructB {
#[doc(hidden)]
pub const PublicConstantD: i32 = 0;
pub const PublicConstantRenamedB: i32 = 0;
}
#[doc(hidden)]
pub struct StructC {}
impl StructC {}
#[doc(hidden)]
pub struct StructD {}
impl StructD {
pub const PublicConstantG: i32 = 0;
}
pub struct StructE {}
#[doc(hidden)]
impl StructE {
pub const PublicConstantI: i32 = 0;
}
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 these kinds of variants to the baseline as well, so that we also test whether removals of doc(hidden) in all these places results in the lint being triggered?

Right now, I think these tests still don't target the public_api_eligible lines in the query I pointed out, because those lines were in the baseline arm of the query, whereas this code is only evaluated in the current arm since it's in the new directory in the test crate.

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 rustfmt lets it go through.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Owner

Choose a reason for hiding this comment

The 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:

  • True-positive vs false-negative: we expected a lint, and got a lint.
  • False-positive vs true-negative: we didn't expect a lint, and we didn't get one.
  • One lint per problem: we don't want to flag the same issue more than once, e.g. if a struct gets deleted, we don't also want to trigger lints for each of its methods and associated constants that got deleted too.

So we want to check #[doc(hidden)] in all the places where it can appear, both where it's added in new and where it's removed in new, as well as where it remains unchanged between old and new.

Currently, you have the first kind of test covered well.

Removing #[doc(hidden)] from any of these places while also removing the constant is not a major change, and shouldn't trigger the lint. This is the second kind of test, and is not currently covered.

Making the const, its impl block, or the owning type become #[doc(hidden)] is a major change, but one for a different lint. (Same if the owning type got deleted altogether.) Even if the const itself is removed altogether, this lint shouldn't trigger here. This is the third kind of test, and is partially covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summary of added test cases:

  1. Removing const for doc hidden struct - not triggered
  2. Removing const for doc hidden impl - not triggered
  3. Adding doc hidden for impl block but not removing const - not triggered
  4. Adding doc hidden for impl block but removing const - triggered
  5. Adding doc hidden for struct - only struct_now_doc_hidden triggered
  6. Removing doc hidden for struct, impl block and/or removing the const - not triggered

Copy link
Owner

Choose a reason for hiding this comment

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

Excellent, nicely done!

19 changes: 19 additions & 0 deletions test_crates/inherent_associated_pub_const_missing/old/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ impl StructA {
//Basic Test case should be caught
pub const PublicConstantA: i32 = 0;
pub const PublicConstantB: i32 = 0;
//Should not be caught on marking #[doc(hidden)]
pub const PublicConstantE: i32 = 0;
// Should Be caught on renaming
pub const PublicConstantRenameA: i32 = 0;
//Should not be caught on removing since its not pub
Expand All @@ -16,3 +18,20 @@ impl StructB {
pub const PublicConstantRenameB: i32 = 0;
const ConstantB: i32 = 0;
}
#[doc(hidden)]
pub struct StructC {}
impl StructC {
// should not be caught on removing since the struct #[doc(hidden)]
pub const PublicConstantF: i32 = 0;
}
//should not be caught by this lint on marking #[doc(hidden)]
pub struct StructD {}
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved
impl StructD {
pub const PublicConstantG: i32 = 0;
pub const PublicConstantH: i32 = 0;
}
pub struct StructE {}
impl StructE {
pub const PublicConstantI: i32 = 0;
pub const PublicConstantJ: i32 = 0;
}
4 changes: 2 additions & 2 deletions test_outputs/inherent_associated_pub_const_missing.output.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
String("inherent_associated_pub_const_missing"),
String("StructA"),
]),
"span_begin_line": Uint64(7),
"span_begin_line": Uint64(9),
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
],
}
}
11 changes: 11 additions & 0 deletions test_outputs/struct_now_doc_hidden.output.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
{
"./test_crates/inherent_associated_pub_const_missing/": [
{
"path": List([
String("inherent_associated_pub_const_missing"),
String("StructD"),
]),
"span_begin_line": Uint64(18),
"span_filename": String("src/lib.rs"),
"struct_name": String("StructD"),
},
],
"./test_crates/struct_now_doc_hidden/": [
{
"path": List([
Expand Down