unneeded pub(crate) lint #6199

emberian · 2020-10-20T22:17:36Z

this lint will tell you if an item is pub(crate) but the crate would compile just fine if it weren't. it does this check by looking at all uses of each pub(crate) item (call it W). if the module the usage of W occurs within is a submodule (or the same module) of the parent of W, this usage would have succeeded if the pub(crate) were absent.

open question: should the redundant_pub_crate lint (which is a... subset of this one?) be folded into this lint pass? does the check redundant_pub_crate does ever fire when this lint wouldn't?

h/t @eddyb who helped me finagle the visitor infrastructure.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Please keep the line below
changelog: Add unneeded_pub_crate lint

this lint will tell you if an item is `pub(crate)` but the crate would compile just fine if it weren't.

rust-highfive · 2020-10-20T22:17:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

this includes struct fields, structs/enums/unions, and methods. a bit overzealous right now, actually.

emberian · 2020-10-21T14:44:35Z

this lint doesn't check positional fields. it needs to, i think?
there's an ICE in type_dependent_def that i don't understand.
the latest commit broke the test way more than just not fixing the positional fields. why?
more test cases?

bors · 2020-10-25T09:57:41Z

☔ The latest upstream changes (presumably #6109) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

phansch · 2020-11-04T14:23:56Z

Thanks for you PR! I'm currently taking a break from reviews so I'm going to re-assign this - r? @llogiq

llogiq

I like this lint, and I very much want to use it on my code. r=me once the ICE is fixed.

llogiq · 2020-11-04T22:38:01Z

clippy_lints/src/unneeded_pub_crate.rs

+            },
+            hir::QPath::TypeRelative(..) | hir::QPath::LangItem(..) => self
+                .maybe_typeck_results
+                .and_then(|typeck_results| typeck_results.type_dependent_def(hir_id)),


This is the line where the ICE appears to happen. I'm not deep enough into typeck_results to know where the problem stems from, but I think I've seen something similar in the past. I'll need a bit of time to check, though.

Edit: It's likely because you call typeck_results from a visitor – usually you want to do this directly in the lint pass (which as you know is a visitor of sorts by itself).

@llogiq i run this visitor in the context of the lint pass, down in check_crate_post. is that what you mean?

my best guess is that there are qpaths that i'm not supposed to look up, maybe that LangItem case?

I'm not completely sure, but I'd hope rewriting this into a single pass (where you'd collect refs to items and references to them, and finally linting all non-publicly ref'd ones) might fix it.

llogiq · 2020-11-07T15:52:35Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-11-17T20:52:44Z

☔ The latest upstream changes (presumably #6070) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

llogiq · 2020-11-28T21:45:47Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

emberian · 2020-12-19T21:06:41Z

i plan on coming back to this next week!

giraffate · 2021-01-04T00:34:47Z

ping from triage @emberian. Could you have any update on this?

giraffate · 2021-01-26T00:08:25Z

ping from triage @emberian. According to triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen.

WIP: unnecessary pub(crate) lint

ed51ea7

this lint will tell you if an item is `pub(crate)` but the crate would compile just fine if it weren't.

rust-highfive assigned phansch Oct 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 20, 2020

emberian added 3 commits October 20, 2020 22:58

check more things for unneeded

b9dc175

this includes struct fields, structs/enums/unions, and methods. a bit overzealous right now, actually.

check fields and variants more thoroughly

7eb9ded

fix ICE

4ca9c1c

emberian changed the title ~~[WIP] unneeded pub(crate) lint~~ unneeded pub(crate) lint Oct 21, 2020

try testing positional fields

6c6dd76

rust-highfive assigned llogiq and unassigned phansch Nov 4, 2020

llogiq reviewed Nov 4, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into unnecessary_pub_crate

867ab54

giraffate closed this Jan 26, 2021

giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unneeded pub(crate) lint #6199

unneeded pub(crate) lint #6199

emberian commented Oct 20, 2020 •

edited

Loading

rust-highfive commented Oct 20, 2020

emberian commented Oct 21, 2020

bors commented Oct 25, 2020

phansch commented Nov 4, 2020

llogiq left a comment

llogiq Nov 4, 2020

emberian Nov 16, 2020

emberian Nov 16, 2020 •

edited

Loading

llogiq Nov 21, 2020

llogiq commented Nov 7, 2020

bors commented Nov 17, 2020

llogiq commented Nov 28, 2020

emberian commented Dec 19, 2020

giraffate commented Jan 4, 2021

giraffate commented Jan 26, 2021

unneeded pub(crate) lint #6199

unneeded pub(crate) lint #6199

Conversation

emberian commented Oct 20, 2020 • edited Loading

rust-highfive commented Oct 20, 2020

emberian commented Oct 21, 2020

bors commented Oct 25, 2020

phansch commented Nov 4, 2020

llogiq left a comment

Choose a reason for hiding this comment

llogiq Nov 4, 2020

Choose a reason for hiding this comment

emberian Nov 16, 2020

Choose a reason for hiding this comment

emberian Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

llogiq Nov 21, 2020

Choose a reason for hiding this comment

llogiq commented Nov 7, 2020

bors commented Nov 17, 2020

llogiq commented Nov 28, 2020

emberian commented Dec 19, 2020

giraffate commented Jan 4, 2021

giraffate commented Jan 26, 2021

emberian commented Oct 20, 2020 •

edited

Loading

emberian Nov 16, 2020 •

edited

Loading