Add a new lint UNUSED_UNCONSTRUCTABLE_PUB_STRUCTS

[mu001999]

View all comments

Add a new lint UNUSED_UNCONSTRUCTABLE_PUB_STRUCTS to detect unused unconstructable public structs, based on the following observations:

1. A public struct with private field(s) cannot be directly constructed from external crates.
2. Associated functions with a receiver require an already constructed value of type Self.
3. Therefore, public structs with private fields and their associated functions that take a receiver can be included in the local crate's dead code analysis.
4. If a public struct with private fields cannot be constructed in any reachable code path, it could be considered dead.

And, with the similar approach of the lint dead_code_pub_in_binary, this lint is also independent of dead_code and dead_code_pub_in_binary.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[juntyr]

Would the lint fire on token structs that are public, have private fields, have no public constructor method, but expose a limited number of pre-constructed objects, e.g. through a static that contains an optional token?

[mu001999]

a static that contains an optional token

won't fire like private types used in such places

[davidtwco]

Nominating for t-lang to decide whether we want this lint, then I'll review the implementation.

Also, s/unconstructible/unconstructable.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Darksonn]

Does this trigger on structs that are intended only as marker types in generic parameters?

[mu001999]

Does this trigger on structs that are intended only as marker types in generic parameters?

Such types usually have intended private unit fields and this won't trigger on them. This will only trigger types with trivial fields but not be used or cannot be constructed.

[nikomatsakis]

Are there examples of code in the wild that is affected by this lint?

[scottmcm]

I worry about completeness here. If I have something like pub struct Foo(pub [u8]); it's not "constructible" in a sense, but it might be entirely expected anyway. What if there's something only created via slice_from_raw_parts and pointer casts to get &MyType?

---

Musing: what if this was signature-based, say? Could it be phrased as "why is this pub when it's not in a signature; maybe it should be pub(crate)?" or something?

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting.

Was there any particular motivating use case that led to proposing this? Can you point to some code that motivated this?

We wondered about potential corner cases, notably structs that are only constructed in unsafe code. For instance, something using repr(C) or repr(transparent) that's constructed via transmute, or a dynamically sized type that requires unsafe code to construct. We're hoping those can be handled and won't produce false positives.

Once those are addressed, we'd like to see the (triaged) results of a crater run with this lint marked as deny-by-default, so we can get an idea of 1) how widespread this is and 2) whether this catches any issues.

[traviscross]

Please renominate for lang when these answers are available.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0611f7de057c584b8771102fde7aed6c70c71d3b (0611f7de057c584b8771102fde7aed6c70c71d3b, parent: 507271bc119683008ec719ecee48814e8ac86c65)

[mu001999]

@craterbot check

[craterbot]

👌 Experiment pr-146440 created and queued.
🤖 Automatically detected try build 0611f7de057c584b8771102fde7aed6c70c71d3b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-146440 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-146440 is completed!
📊 2645 regressed and 0 fixed (937893 total)
📊 5208 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-146440/retry-regressed-list.txt ↩

[mu001999]

Sorry for the long delay in getting back to this. I previously hadn’t quite figured out how to make the new lint unused_unconstructable_pub_structs independent from lint dead_code. (Previously it is named unconstructable_pub_struct, I think the new name may be more accurate.)

After the implementation of lint dead_code_pub_in_binary lint (#149509), I realized we can achieve this (independent from lint dead_code) in a similar way.

We saw this as an extension of the existing dead_code lints; whether or not it has an additional named toggle of its own (which it probably should), once ready it should be part of the dead_code lint group, and should integrate with the dead code logic (including transitively marking things dead). Since you mention liveness propagation, you may already be doing that; we just want to make sure this gets treated as part of the dead_code lint group.

First, to clarify: we currently don't have a dead_code lint group. The dead_code lint itself is part of the unused lint group. Also, similar to the dead_code_pub_in_binary lint, I’d like unused_unconstructable_pub_structs to be independent from the dead_code lint, while still being part of the unused lint group. Because I think having the two lints affect each other would be confusing for users. The current implementation achieves this, while sharing most of the dead-code analysis logic.

We'd like to see a clear description of the rules for when a type is "unconstructable".

A type is considered an unconstructable candidate only if:

1. It is a reachable pub struct
2. And downstream crates cannot construct it directly or through public constructors:
   1. Not all fields are public
   2. No public constructor-like method is provided
      • i.e., public methods without a receiver
      • we treat such methods as potentially able to construct the type

Then we also exclude a few intentional/special cases, the following kinds of structs won't be checked:

1. Lang items. Since they may be used directly by compiler
2. With #[repr(C)]/#[repr(transparent)]. Since they may be constructed via FFI
3. Uninhabited, containing only ZST fields, or containing unit fields. Since they're usually intentional type-level markers

@tmandry also had a sample to consider, involving intentionally unconstructable marker types that are used in the type system only

The rules above should cover the sample here, as well as other common cases of intentionally unconstructable types.

Finally, we'd like to review a couple of examples (which could be links to tests you're adding in this PR, if you can highlight some representative ones for us).

1. receiver-methods-do-not-construct.rs
   • This case shows the main kind of types this lint is intended to catch: public types that provide public methods, but those methods with receivers require an already-constructed value.
   • These types do not provide any public way to construct them, and are also never constructed within the current crate, so they are effectively useless.
2. local-construction.rs shows that once the type is actually used, the lint no longer fires.
3. repr-exemptions.rs and intentional-marker-fields.rs show that intentionally unconstructable marker types are skipped.
4. Remove unused FixupError rust-analyzer#22365, this lint had already detected an unused public type in RA.

---

I did a crater run (#146440 (comment)), but it produced too many results, so that I cannot check them all one by one in fact.

I checked some of them (though not many), and did not observe false-positives. The types were either unused in fact, or only used under certain cfgs.

---

After all, I think this lint is valuable for detecting and eliminating unused pub structs. Previously, these types could not be detected by lint dead_code (in fact, they are outside the scope covered by dead_code).

Also, although we already have tools for finding unused pub items in a workspace, such as est31/warnalyzer and cpg314/cargo-workspace-unused-pub, this lint is not limited to workspace scope. It can help eliminate obsolete types exported by ordinary library crates.

[traviscross]

What do you make of the arti case?

https://crater-reports.s3.amazonaws.com/pr-146440/try%230611f7de057c584b8771102fde7aed6c70c71d3b/gh/0xinf0.arti/log.txt

• Types containing a field of unit or never type are not checked, since these are usually marker types that are intentionally unconstructable
• Types whose fields are all PhantomData are not checked, for the same reason to unit/never type

What are your thoughts on whether this should be widened to exempt all uninhabited types (rather than just types containing a field of never type)? What about to all types containing only ZST fields (rather than just all-PhantomData fields)?

[mu001999]

What do you make of the arti case?
https://crater-reports.s3.amazonaws.com/pr-146440/try%230611f7de057c584b8771102fde7aed6c70c71d3b/gh/0xinf0.arti/log.txt

Good catch! I missed it. I believed it is not expected because Void is uninhabited, it should work like !.

What are your thoughts on whether this should be widened to exempt all uninhabited types (rather than just types containing a field of never type)? What about to all types containing only ZST fields (rather than just all-PhantomData fields)?

Widening the exemption to all uninhabited types makes sense to me.

But widening it to all types containing only ZST fields may be too broad. I think a field of ZST is fundamentally different from other types here, while PhantomData is different because it could be used to avoid E0392.

After thinking about it, I think this also makes sense. We may have custom pub Marker<T>(PhantomData<T>); and the use it as the type of other types' fields.

Thanks for your suggestions!

---

Updated the implementation and exemption rules in #146440 (comment).

[traviscross]

@bors try

TODO: @craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-146440/retry-regressed-list.txt p=1

[rust-bors]

☀️ Try build successful (CI)
Build commit: 93086cf (93086cfebfccb79083c59d0d3b2052944bba643c, parent: 9e293ae9f8abecb0be5105787d181518c9012a19)

[traviscross]

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-146440/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-146440-1 created and queued.
🤖 Automatically detected try build 93086cf
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-146440-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-146440-1 is completed!
📊 2526 regressed and 0 fixed (7684 total)
📊 1013 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-146440-1/retry-regressed-list.txt ↩

[traviscross]

(Previously it is named unconstructable_pub_struct, I think the new name may be more accurate.)

I'm curious if you could elaborate on this a bit. Sitting with it, unconstructible_pub_structs seems maybe more accurate in one way than unused_unconstructible_pub_structs since we can't actually tell that it's entirely unused due to the transmute case that @scottmcm mentioned, though I still would put this in the unused lint group. By contrast, it seems more OK to call it unconstructible.

Also, s/unconstructible/unconstructable.

I see the appeal in the latter, but constructible is the more canonical form. See the ngrams viewer.

[mu001999]

I'm curious if you could elaborate on this a bit. Sitting with it, unconstructible_pub_structs seems maybe more accurate in one way than unused_unconstructible_pub_structs since we can't actually tell that it's entirely unused due to the transmute case that @scottmcm mentioned

What in my mind was that unconstructible_pub_structs only points to pub structs without reachable constructors. But this lint only checks such structs that are unused in local crate.

But I would agree that another explanation is that this lint strips out the ones used in local crate. Then it could keep being unconstructible_pub_structs.

I see the appeal in the latter, but constructible is the more canonical form. See the ngrams viewer.

Cool! I'll rename it once lint can be merged in and the name is settled.

[traviscross]

Reviewing #146440 (comment), including the subsequent adjustments in #146440 (comment), this looks correct and desirable to me. There's a set of careful mitigations here for the cases we had raised and that were found in the crater run. I don't see anything further to do here.

Proposal: Let's do this, as warn-by-default, with the name unconstructible_pub_structs.

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.