Add lint inherent_associated_pub_const_missing #714
Conversation
There was a problem hiding this comment.
Good point on the name consistency question. I think you chose a reasonable name for this lint. We may change up all the lint names in the future to improve our consistency. Would you like to open an issue for that and include your consistency suggestion there?
The lint query looks correct and I'd be happy to merge it. If you get a chance, I'd love to get a bit more test coverage first, since there are some tricky edge cases here that we currently aren't testing. I made a concrete suggestion for this in the PR itself.
| "visibility_limit": String("public"), | ||
| }, | ||
| ], | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
Consider making your editor add these trailing newlines automatically.
| #[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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Summary of added test cases:
- Removing const for doc hidden struct - not triggered
- Removing const for doc hidden impl - not triggered
- Adding doc hidden for impl block but not removing const - not triggered
- Adding doc hidden for impl block but removing const - triggered
- Adding doc hidden for struct - only
struct_now_doc_hiddentriggered - Removing doc hidden for struct, impl block and/or removing the const - not triggered
| public_api @filter(op: "=", value: ["$true"]) | ||
| } | ||
|
|
||
| inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){ |
There was a problem hiding this comment.
Ah one last test case is needed here.
The following might not always be a breaking change, and should be covered by a separate lint:
pub trait Trait {}
pub struct Example;
impl Example {
pub const N: i64 = 0;
}
impl Trait for Example {}to
pub trait Trait {
const N: i64 = 0;
}
pub struct Example;
impl Trait for Example {}The reason this might not be breaking is that if both Example and Trait are in a prelude, or if the Trait trait is already otherwise in scope, Example::N will continue to work. In the original code that refers to an inherent const, but in the new code it's a trait's const, equivalent to <Example as Trait>::N in explicit form.
To fix this, we allow the query to look at all impl blocks (including impl Trait for Example ones) and not just inherent impl (only impl Example) blocks:
| inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){ | |
| impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){ |
It would be great if you could add the above code as a test case here.
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
| { | ||
| "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"), | ||
| }, |
There was a problem hiding this comment.
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
constdoesn't have a default value and is instead given a value in theimplblock 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
| item { | ||
| ... on ImplOwner { | ||
| visibility_limit @filter(op: "=", value: ["$public"]) | ||
| name @output | ||
|
|
||
| importable_path { | ||
| path @filter(op: "=", value: ["%path"]) | ||
| public_api @filter(op: "=", value: ["$true"]) | ||
| } | ||
|
|
||
| impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){ | ||
| implemented_trait { | ||
| trait { | ||
| associated_constant { | ||
| name @filter(op: "=", value: ["%associated_constant"]) | ||
| } |
There was a problem hiding this comment.
Block to check if there is an implemented trait with the const.
False-negative was not triggered with the new lint.
There was a problem hiding this comment.
You could actually take this impl block and move it into the other item block, side by side with its existing inherent_impl block.
The query engine will still optimize this query to essentially the same thing as written, but it's still unnecessarily long and confusing for human readers so probably a good idea to apply this change.
Also, a nitpick: try to polish up the small details, like indentation and formatting. Right now, the query has some unexpected indentation, inconsistency in spacing (e.g. no space between ){), and other elements that seem possibly accidental. While no individual thing like this is a big deal on its own, as a whole they do get in the way and make maintainability harder over time. Sometimes they even cause bugs, and even if they don't, they still make code harder to read and understand for maintainers and new contributors.
|
Marked for merge! Thanks again for putting this together! 🚀 |
Lint for removed associated pub constant inside
ImplOwner's inherent implIssue mentioned in #366 (comment)
P.S : I named it missing inspired from lints like
enum_missing,trait_missing,pub_module_level_const_missingetc but should it be removed like lintstrait_removed_associated_type,trait_removed_supertrait?