feature: add new lint `pub_underscore_fields` #10283

ParkMyCar · 2023-02-03T15:24:43Z

This PR introduces a new lint pub_underscore_fields that lints when a user has marked a field of a struct as public, but also prefixed it with an underscore (_). This is something users should avoid because the two ideas are contradictory. Prefixing a field with an _ is inferred as the field being unused, but making a field public infers that it will be used.

Followed [lint naming conventions][lint_naming]
- I believe I followed the naming conventions, more than happy to update the naming if I did not :)
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

changelog: new lint: [pub_underscore_fields]
#10283

rustbot · 2023-02-03T15:24:49Z

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

Please see the contribution instructions for more information.

bors · 2023-02-23T16:10:12Z

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

ParkMyCar · 2023-03-14T02:54:56Z

Hey @flip1995, if you wouldn't mind taking a look at this when you get the chance, it would be much appreciated! No rush though :)

bors · 2023-03-16T10:49:15Z

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

blyxyas · 2023-10-07T13:48:11Z

@ParkMyCar Philipp stopped being on the reviewer rotation a few months ago, are you still interested in continuing with this development? ฅ^•ﻌ•^ฅ

r? @blyxyas

ParkMyCar · 2023-10-07T16:43:05Z

@ParkMyCar Philipp stopped being on the reviewer rotation a few months ago, are you still interested in continuing with this development? ฅ^•ﻌ•^ฅ

r? @blyxyas

For sure! I just rebased off of master and updated the lint version, whenever you have time let me know if there is any other feedback :)

blyxyas

Very good for a first review! Just a couple minor changes and I think this is it ❤️ |ΦωΦ|

blyxyas · 2023-10-08T08:28:35Z

clippy_lints/src/pub_underscore_fields.rs

+            return;
+        };
+
+        let msg = "field marked as public but also inferred as unused because it's prefixed with `_`";


I'd just inline the message, but if you want to have it as a separate variable, put it later in the checks (just before span_lint_and_help). We don't want to allocate innecesarily.

Inlined it is :)

blyxyas · 2023-10-08T08:29:09Z

clippy_lints/src/pub_underscore_fields.rs

+        };
+
+        let msg = "field marked as public but also inferred as unused because it's prefixed with `_`";
+        for field in st.fields().iter() {


Suggested change

for field in st.fields().iter() {

for field in st.fields() {

clippy_lints/src/pub_underscore_fields.rs

Alexendoo · 2023-10-08T13:16:27Z

It would be good if this ignored PhantomData fields (it's common to underscore prefix them) and fields that are #[doc(hidden)]

ParkMyCar · 2023-10-11T03:34:59Z

It would be good if this ignored PhantomData fields (it's common to underscore prefix them) and fields that are #[doc(hidden)]

Thanks for the feedback @Alexendoo! I'm not sure I agree though. AFAIU prefixing a field name with an underscore generally indicates that field should be ignored, if some part of your code needs to use the PhantomData marker, then it should not be prefixed, same with #[doc_hidden]. I'd be more than happy to make this change, but it's not totally clear to me why these would need to be special cased, could you explain more or provide an example? Thanks :)

blyxyas · 2023-10-11T08:24:00Z

PhantomData<T> is a data type used for enforcing type rules. It isn't meant to be used because it holds literally 0 data, so using _phantom is really common [~84K occurrences].

On the topic of #[doc(hidden)], I'm not sure of any strong reasons of ignoring them, some more information would be appreciated.

blyxyas · 2023-10-11T11:49:45Z

clippy_lints/src/pub_underscore_fields.rs

@@ -20,9 +20,15 @@ declare_clippy_lint! {
    /// ```
    /// Use instead:
    /// ```rust
-    /// struct FileHandle {
+    /// struct FileHandle_Foo {


Mixing PascalCase with underscores hehe 🐈
For future reference, you can use attributes like should_panic on your code blocks so that these name collisions don't matter.

You can make it two separate code blocks

```rust struct FileHandle { .. } ``` or ```rust struct FileHandle { .. } ```

🤦‍♂️ yep totally missed this, updated it to be two code blocks!

Alexendoo · 2023-10-11T12:23:33Z

doc(hidden) + underscore prefix is used to drive home the "please don't use this" message, that the field is only public for some implementation reason such as being used in a macro expansion

e.g. https://docs.rs/enumset/1.1.2/src/enumset/set.rs.html#125-130

ParkMyCar · 2023-10-13T00:51:44Z

@blyxyas @Alexendoo thanks for the examples! I'm thoroughly convinced, I updated the lint to be a LatePass so we can get type info. Then skip fields that are #[doc(hidden)] or use PhantomData`.

I struggled a bit to get the visibility settings right, if there's an easier to detect the range of "pub" visibilities we want to guard against, please let me know!

ParkMyCar · 2023-10-13T00:52:16Z

Whoops, just realized I forgot to add tests for the new #[doc(hidden)] and PhantomData checks, let me do that!

blyxyas

Just some polishing and I think this is ready! Thanks for the patience! ❤️ (=^･ω･^=)

clippy_lints/src/pub_underscore_fields.rs

Alexendoo · 2023-10-13T11:28:44Z

clippy_lints/src/pub_underscore_fields.rs

+        let is_visible = |field: &FieldDef<'_>| {
+            let parent = cx.tcx.parent_module_from_def_id(field.def_id);
+            let grandparent = cx.tcx.parent_module_from_def_id(parent.into());
+            let visibility = cx.tcx.visibility(field.def_id);
+
+            let case_1 = parent == grandparent && !field.vis_span.is_empty();
+            let case_2 = visibility != Visibility::Restricted(parent.to_def_id());
+
+            case_1 || case_2
+        };


Can try cx.effective_visibilities.is_exported(field.def_id)) for this

I tried that, unfortunately it didn't work :/

I think the issue is the test file, none of the fields in it are actually publicly reachable. You'd have to move the definitions outside of fn main() {} and make the inner/inner2 modules public

is_exported doesn't lint for pub(...), and it seems like it has some problems with fields of type Option<T> for some reason?

I don't see it as a requirement.

The effective visibility is what matters for the lint though

Fields prefixed with an _ are inferred as unused, which suggests it should not be marked as pub, because marking it as pub infers it will be used.

pub(crate), or pub where the field is not publicly reachable does not mark the field as used

What's the issue with Option fields?

Hmm, now I'm not sure.
We could lint for any field that is public and its name starts with _, which is what the name of lint implies. Or we could analyze the description a little bit more and just lint when the pub is actually useful.

ParkMyCar · 2023-10-13T16:55:03Z

Just some polishing and I think this is ready! Thanks for the patience! ❤️ (=^･ω･^=)

Sweet! Made all of your suggested changes!

blyxyas

Just the version upgrade and that's all, thanks ❤️!

clippy_lints/src/pub_underscore_fields.rs

blyxyas · 2023-10-16T12:40:08Z

Could you please rebase and squash these commits into 1 or 2 relevant ones?

blyxyas

And also, could you run cargo collect-metadata

ParkMyCar · 2023-12-27T21:11:34Z

And also, could you run cargo collect-metadata

Updated to 1.77, ran cargo collect-metadata, and squashed all of the commits into one!

blyxyas · 2023-12-28T22:42:22Z

book/src/lint_configuration.md

+---
+**Affected lints:**
+* [`pub_underscore_fields`](https://rust-lang.github.io/rust-clippy/master/index.html#pub_underscore_fields)
+* [`or`](https://rust-lang.github.io/rust-clippy/master/index.html#or)


I'm not really sure why this is here

Me either! It's what cargo collect-metadata generated, other lints seem to have similar "Affected lints" sections :)

I meant the or lint, that doesn't exist xD

blyxyas · 2023-12-29T09:19:36Z

@bors try

bors · 2023-12-29T09:20:44Z

⌛ Trying commit 73a3636 with merge 493b5a3...

feature: add new lint `pub_underscore_fields` fixes: #10282 This PR introduces a new lint `pub_underscore_fields` that lints when a user has marked a field of a struct as public, but also prefixed it with an underscore (`_`). This is something users should avoid because the two ideas are contradictory. Prefixing a field with an `_` is inferred as the field being unused, but making a field public infers that it will be used. - \[x] Followed [lint naming conventions][lint_naming] - I believe I followed the naming conventions, more than happy to update the naming if I did not :) - \[x] Added passing UI tests (including committed `.stderr` file) - \[x] `cargo test` passes locally - \[x] Executed `cargo dev update_lints` - \[x] Added lint documentation - \[x] Run `cargo dev fmt` --- changelog: new lint: [`pub_underscore_fields`] [#10283](#10283)

bors · 2023-12-29T09:30:34Z

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 493b5a3 (493b5a31558ca205f5fe5c3a13bca6652baf16bc)

- add a new late pass lint, with config options - add ui tests for both variations of config option - update CHANGELOG.md github feedback bump version to 1.77 and run cargo collect-metadata Change `,` to `;` in `conf.rs`

blyxyas · 2023-12-29T10:48:31Z

I changed that , to a ;, this is a bug in our metadata collector, it seems that it just gets any word after a comma. I already pushed this to your fork.

Let's hope that everything goes well this time (=^･^=)

blyxyas · 2023-12-29T10:50:50Z

@bors r+

bors · 2023-12-29T10:50:52Z

📌 Commit fa7dd1c has been approved by blyxyas

It is now in the queue for this repository.

bors · 2023-12-29T10:52:03Z

⌛ Testing commit fa7dd1c with merge 174a0d7...

bors · 2023-12-29T11:00:30Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 174a0d7 to master...

ParkMyCar · 2023-12-29T14:01:35Z

Woo! Thank you @blyxyas for reviewing this PR and getting it merged, I appreciate it! 😊

GrantGryczan · 2024-01-13T17:25:29Z

It's spelled "publicly", not "publically". I recommend fixing that in the PublicallyExported value. (If needed, I can submit a separate issue for this.)

Furthermore, the pub-underscore-fields-behavior option has no description in the documentation, so I've got no clue what it does. I don't even know what values it can be set to other than its default.

blyxyas · 2024-01-13T23:47:35Z

I'll fix this myself. Thanks for the report.

Add . to end of lint lists in configuration + Fix typo in pub_underscore_fields_behavior Fixes #10283 (comment) In the "/// Lint: " list on each configuration option, you have to end with a dot. If the lint list doesn't have a dot, the configuration won't have documentation. This PR adds those missing dots in some of the configuration, thus also adding their documentation. changelog: Fix bug where a lot of config documentation wasn't showing. changelog: Fix typo in `pub_underscore_fields_behavior` (`PublicallyExported` -> `PubliclyExported`)

rustbot assigned flip1995 Feb 3, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 3, 2023

This was referenced Feb 3, 2023

test: e2e testing for OpenTelemetry Tracing MaterializeInc/materialize#17458

Merged

test: small visibility cleanup MaterializeInc/materialize#17516

Merged

ParkMyCar force-pushed the lint/pub_underscore_fields branch from b134fe6 to 4313a52 Compare March 14, 2023 02:54

rustbot assigned blyxyas and unassigned flip1995 Oct 7, 2023

ParkMyCar force-pushed the lint/pub_underscore_fields branch from 4313a52 to f53c6be Compare October 7, 2023 16:42

ParkMyCar force-pushed the lint/pub_underscore_fields branch from f53c6be to da69c5e Compare October 7, 2023 16:46

blyxyas requested changes Oct 8, 2023

View reviewed changes

ParkMyCar force-pushed the lint/pub_underscore_fields branch from c283906 to 3d16f47 Compare October 11, 2023 03:37

blyxyas reviewed Oct 11, 2023

View reviewed changes

blyxyas approved these changes Oct 13, 2023

View reviewed changes

Alexendoo reviewed Oct 13, 2023

View reviewed changes

blyxyas approved these changes Oct 16, 2023

View reviewed changes

clippy_lints/src/pub_underscore_fields.rs Outdated Show resolved Hide resolved

ParkMyCar force-pushed the lint/pub_underscore_fields branch from cb98e49 to c9a2555 Compare October 16, 2023 13:22

blyxyas requested changes Dec 27, 2023

View reviewed changes

ParkMyCar force-pushed the lint/pub_underscore_fields branch from ea82130 to 73a3636 Compare December 27, 2023 21:10

blyxyas reviewed Dec 28, 2023

View reviewed changes

add new lint, pub_underscore_fields

fa7dd1c

- add a new late pass lint, with config options - add ui tests for both variations of config option - update CHANGELOG.md github feedback bump version to 1.77 and run cargo collect-metadata Change `,` to `;` in `conf.rs`

blyxyas force-pushed the lint/pub_underscore_fields branch from 7df1b40 to fa7dd1c Compare December 29, 2023 10:44

blyxyas mentioned this pull request Dec 29, 2023

[Easy] cargo collect-metadata interprets anything behind a comma as a new lint #12042

Closed

bors merged commit 174a0d7 into rust-lang:master Dec 29, 2023
8 checks passed

blyxyas mentioned this pull request Jan 14, 2024

Add . to end of lint lists in configuration + Fix typo in pub_underscore_fields_behavior #12144

Merged

feature: add new lint pub_underscore_fields #10283

feature: add new lint pub_underscore_fields #10283

Conversation

ParkMyCar commented Feb 3, 2023 • edited by xFrednet Loading

rustbot commented Feb 3, 2023

bors commented Feb 23, 2023

ParkMyCar commented Mar 14, 2023

bors commented Mar 16, 2023

blyxyas commented Oct 7, 2023 • edited Loading

ParkMyCar commented Oct 7, 2023

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo commented Oct 8, 2023

ParkMyCar commented Oct 11, 2023

blyxyas commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo commented Oct 11, 2023

ParkMyCar commented Oct 13, 2023

ParkMyCar commented Oct 13, 2023

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkMyCar commented Oct 13, 2023

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas commented Oct 16, 2023 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

ParkMyCar commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

blyxyas commented Dec 29, 2023

blyxyas commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

bors commented Dec 29, 2023

ParkMyCar commented Dec 29, 2023

GrantGryczan commented Jan 13, 2024 • edited Loading

blyxyas commented Jan 13, 2024

feature: add new lint `pub_underscore_fields` #10283

feature: add new lint `pub_underscore_fields` #10283

ParkMyCar commented Feb 3, 2023 •

edited by xFrednet

Loading

blyxyas commented Oct 7, 2023 •

edited

Loading

Alexendoo Oct 16, 2023 •

edited

Loading

blyxyas commented Oct 16, 2023 •

edited

Loading

GrantGryczan commented Jan 13, 2024 •

edited

Loading