-
-
Notifications
You must be signed in to change notification settings - Fork 15k
Add a new lint UNCONSTRUCTIBLE_PUB_STRUCTS
#146440
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 all commits
e9c11a9
2a5b16d
8ca8ddd
6c254e3
df48a9e
c383118
8bf04d0
0b55de6
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 |
|---|---|---|
|
|
@@ -147,6 +147,7 @@ declare_lint_pass! { | |
| UNUSED_MACRO_RULES, | ||
| UNUSED_MUT, | ||
| UNUSED_QUALIFICATIONS, | ||
| UNUSED_UNCONSTRUCTABLE_PUB_STRUCTS, | ||
| UNUSED_UNSAFE, | ||
| UNUSED_VARIABLES, | ||
| UNUSED_VISIBILITIES, | ||
|
|
@@ -818,6 +819,47 @@ declare_lint! { | |
| crate_level_only | ||
| } | ||
|
|
||
| declare_lint! { | ||
| /// The `unused_unconstructable_pub_structs` lint detects public structs | ||
| /// with private fields that cannot be constructed or otherwise used through | ||
| /// the external API. | ||
| /// | ||
| /// ### Example | ||
| /// | ||
| /// ```rust,compile_fail | ||
| /// #![deny(unused_unconstructable_pub_structs)] | ||
| /// | ||
| /// pub struct Foo(i32); | ||
| /// # fn main() {} | ||
| /// ``` | ||
| /// | ||
| /// {{produces}} | ||
| /// | ||
| /// ### Explanation | ||
| /// | ||
| /// This lint is emitted for a `pub` struct when: | ||
| /// | ||
| /// * It has private fields and no public constructor, so downstream crates | ||
| /// cannot construct a value of the type. | ||
| /// * It is not used locally and does not appear in any reachable path from | ||
| /// external APIs other than the public struct item itself. | ||
| /// * It is not uninhabited, is not composed only of ZST fields, and has no unit field. | ||
|
Member
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. Do we exclude types with repr(C)/repr(transparent) on them as well?
Member
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. Yes, I will add the rule into the description |
||
| /// | ||
| /// Such structs may have been unused for a long time, but are now effectively dead code. | ||
| /// This lint helps find those items. | ||
| /// | ||
| /// To silence the warning for individual items, prefix the name with an | ||
| /// underscore such as `_Foo`. | ||
|
Member
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. If the type is pre-existing, but can't just be deleted, then renaming it is also highly likely not possible (a breaking change as much as deleting it would be). If the type is new then naming it I'd prefer we recommend one of those options over the _ prefixing. I think we do that below ("is intentional"), maybe we just delete this line? |
||
| /// | ||
| /// To indicate that the behavior is intentional, add a field with unit or | ||
| /// never type if the struct is only used at the type level. | ||
| /// | ||
| /// Otherwise, consider removing it if the struct is no longer in use. | ||
| pub UNUSED_UNCONSTRUCTABLE_PUB_STRUCTS, | ||
|
Member
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. With regards to the name and finding alternatives to unconstructable, how would we feel about focusing on the fields being dead code? Essentially something like Alternatively, I think in all cases where we lint on the struct with this lint we're already lint on each individual field with "field is never read", right? So maybe we just extend the As another option, it seems plausible to me that we also want a lint on uncallable functions that reference such a struct -- maybe we can focus on that, something like unreachable_api?
Member
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.
Currently "field is never read" is a part of I still prefer providing a separate lint, and keeping the scope of
I like this idea! Especially since we could implement the extensions mentioned above in theory. Even with current scope of the lint, I think this name (or other variants) would still work, and would leave room for future extensions! |
||
| Deny, | ||
| "detects public structs with private fields that cannot be constructed or otherwise used through the external API" | ||
| } | ||
|
|
||
| declare_lint! { | ||
| /// The `unused_attributes` lint detects attributes that were not used by | ||
| /// the compiler. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think the "reachable path from external APIs" is what is ruling out this lint applying to code like this:
but then this code still lints and it seems very similar to the above -- Bar is mentioned in public APIs, albeit all impls on Bar:
and yet if one of those is an inherent impl, then the lint stops applying:
or takes Bar as an argument:
Can you say more about the reasoning behind why we're applying this lint in some cases but not others? It seems like the "non-constructable" argument ought to apply to all cases here.
View changes since the review
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to apply this to free functions and inherent methods whose signatures depend on that type.
The current implementation is based on methods' signatures have receivers or not. This means only methods like
fn foo(&self)or with other kinds of receivers would be defered to analyze (based on the existing two-phase analysis for impls.).For methods likeThe analysis here is wrong, see below.impl Bar { pub fn foo() {} }, since the signature does not depend onBar, we only make the most conservative assumption: the function body may use that type. Unless we pre-collect the defs that can be reached through that function body, but that would be a big change, and I guess it may cause a significant performance regression.For methods like
impl From<Bar> for u32 { fn from(_: Bar) -> u32 { todo!() } }, this may be easy to extend, becasue we could not only check the method has a receiver, but also check if the self type appear in types of params.For free functions or methods like
pub fn foo(_: Bar) {}, I think this could be also extended. But this requires we extend the two-phase analysis for impls to free functions, and records the defs are depent by such functions, because we could have multi params, e.g.,pub fn foo(_: Foo, _: Bar) {}.I think these cases (partly) could be handled later as extensions to this lint, and implemented in separate PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have to look through the body already? I'd expect that to be needed to determine that the local crate isn't constructing the structure. And indeed just mentioning the type wthin a function does silence this lint, even if the constructor is not actually invoked:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. After thinking about it more, I don’t think we could extend the lint to cases like
impl Bar { pub fn foo() { ... } }.If
Bar::foois public, then it will propagate toBar, becauseBar::foodepends onBar(at type-level at least), regardless of whether its body actually usesBar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but some external crate could also be depending on Bar at a type level. I think it still makes sense that if Bar doesn't look like a special type (heuristics you've put in the conditions to apply this lint under -- ZST/inhabited/etc), then we should tell the user that while the type is in the public API, it's a 'weird' pattern and quite plausibly a bug (e.g., missing API).
I don't see much difference between
impl Bar { pub fn foo() {} }andpub fn foo(_: Option<Bar>) {}, personally. Both are callable without constructing Bar, but only one of them triggers the lint.I'm also seeing that we don't silence the lint in the presence of default trait methods returning the otherwise-hidden struct, e.g. this lints:
and this lints, which is fully incorrect:
I'm guessing it's pretty hard to fix these. In general I think the utility of this lint is not necessarily worth its complexity -- it seems like we need a pretty sophisticated analysis if we want to avoid potential false positives like this. The examples here are a little contrived, but the combination of complicated heuristics and the fact that it's a breaking change to remove the struct if you care about API stability makes me question whether it's a good idea to keep trying to fix the lint.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They both won't trigger the lint because they both will mark
Baras live (for now). But seems what you said is mainly for the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking it over, I agree with your point. I also haven’t come up with a low-complexity way to address the case above.
So the best approach may be to close this PR for now.