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

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 3 commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lints/inherent_associated_pub_const_missing.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ SemverQuery(
public_api @filter(op: "=", value: ["$true"])
}

inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){
impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){
associated_constant {
name @filter(op: "=", value: ["%associated_constant"])
arpity22 marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
8 changes: 8 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
Expand Up @@ -54,3 +54,11 @@ impl DocHiddenStruct {
impl DocHiddenStruct {
pub const PublicConstantO: i32 = 0;
}

pub trait Trait {
const N: i64 = 0;
}

pub struct Example;

impl Trait for Example {}
10 changes: 10 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 @@ -78,3 +78,13 @@ impl DocHiddenStruct {
pub const PublicConstantN: i32 = 0;
pub const PublicConstantO: i32 = 0;
}

pub trait Trait {}

pub struct Example;

impl Example {
pub const N: i64 = 0;
}

impl Trait for Example {}
11 changes: 11 additions & 0 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 @@ -33,5 +33,16 @@
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
{
"associated_constant": String("N"),
"name": String("Example"),
"path": List([
String("inherent_associated_pub_const_missing"),
String("Example"),
]),
"span_begin_line": Uint64(87),
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
Copy link
Owner

Choose a reason for hiding this comment

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

Oooh interesting, this is a false-positive — we wanted this to not get flagged.

Perhaps because the N constant isn't defined in the impl block and is instead a default item on the trait? 🤔

Sorry this is turning into a bit of a rabbit hole. I see two options to move forward here.

If this PR isn't something you find that interesting and you don't want to go down the rabbit hole (which is totally fine!), we can merge it as-is and I can open a separate issue for resolving this false-positive.

If you find this PR interesting and you'd like to continue polishing it, then it would be great to do the following:

  • Add a new test case, similar to the trait+struct one we have already, but where the const doesn't have a default value and is instead given a value in the impl block for the trait.
  • Add comments on both the old and new test cases, to explain what they test, how they are different, and why it's important to have both of them and not just one or the other.
  • Depending on how this new test case interacts with the query (whether it gets flagged or not), we'd continue tweaking the query -- for example, by checking two separate cases explicitly: "inherent impl must not have the constant" and "impl of a trait, where that trait must not have a const by that name either."

Your call! Let me know how you'd prefer to move forward.

Copy link
Contributor Author

@arpity22 arpity22 Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I would keep working on it but if you think this is a rare case and it would be more beneficial to have the lint as it is currently than to not have it at all you can merge it. I don't have a problem either way.

BTW, I added the test case, it didn't trigger the lint.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice! There's no rush to merge this if you are happy to keep working on it. I was just offering it in case you felt frustrated by the back-and-forth on it, or if you just wanted a change of pace to something else.

It's great that the new test case didn't trigger the lint. Let's try tweaking the lint then.

We probably want to go back to inherent_impl like we had before. Then we probably want to add another block that also says "no implemented trait has a const by that name either." Here's a short snippet in the playground to help you get started: link

Lmk if you get stuck or would like another pair of eyes on something!

],
}
Loading