Add a new lint UNCONSTRUCTABLE_PUB_STRUCT to detect unconstructable public structs

[mu001999]

Add a new lint UNCONSTRUCTABLE_PUB_STRUCT to detect 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, the lint UNCONSTRUCTABLE_PUB_STRUCT won't affect the dead-code lint's result, but allow or deny dead-code lint may affect the result of UNCONSTRUCTABLE_PUB_STRUCT

[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.

[cjgillot]

This tuple needs to become a documented struct.

[mu001999]

@scottmcm

If I have something like pub struct Foo(pub [u8]); it's not "constructible" in a sense, but it might be entirely expected anyway.

Pub struct with only pub fields will be considered as "constructible" implictly. So such types won't be linted.

What if there's something only created via slice_from_raw_parts and pointer casts to get &MyType?

This situation will be linted. This means the provided pub type can only be created by functions from other crates. I think it's a little weird. But I agree it can appear.

So I think this lint should be allowed by default. And deny it sometimes can help finding more dead codes.

[mu001999]

@nikomatsakis

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

The above failed PR check (https://triage.rust-lang.org/gha-logs/rust-lang/rust/55305415884) gives an example in rust-analyzer!

error: pub struct `FixupError` is unconstructable externally and never constructed locally
   --> src/tools/rust-analyzer/crates/hir-ty/src/next_solver/infer/mod.rs:294:1
    |
294 | pub struct FixupError {
    | ^^^^^^^^^^^^^^^^^^^^^
    |
help: this struct may be unused locally and also externally, consider removing it
   --> src/tools/rust-analyzer/crates/hir-ty/src/next_solver/infer/mod.rs:294:1
    |
294 | pub struct FixupError {
    | ^^^
    = note: `#[deny(unconstructable_pub_struct)]` on by default

[mu001999]

@joshtriplett

For instance, something using repr(C) or repr(transparent)

Now will skip types with repr(C) and repr(transparent).

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.

I tried fixing test failures with deny-by-default, but there are too much.

So could we have a crater run without fixing these test failures?

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

[mu001999]

@rustbot label +I-lang-nominated

[joshtriplett]

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

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.

(@mu001999, this also addresses your issue with the testsuite: the testsuite implicitly has allow(dead_code), so it would allow this as well.)

The way we looked at it in the meeting, if these types were pub(crate), they would already be marked as dead. We have an exception for things that are pub, because they're public API and the user could be outside the crate. What this proposal is adding is that that exception for pub shouldn't apply to structs that nobody outside the crate can construct, at which point the same dead-code-checking logic should apply as for pub(crate).

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

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

pub struct Mut(!);
pub struct NonMut(!);

pub trait Mutness {
    type Ref<'a, T> where Self: 'a;
}

impl Mutness for Mut { ... }
impl Mutness for NonMut { ... }

This kind of type might be used in a generic, and never actually used at runtime, only used in the type system as a marker. We're not sure how prevalent this pattern is (and how often the types are unconstructable), but we want to make sure we don't have an excess of false positives caused by marker types that aren't meant to be constructable.

(The exact details of that example aren't the key point; we're mentioning it as a potential source of false positives. We'd like to hear more about the proposed rules before you delve back into further implementation work.)

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).

[bors]

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