Add a new lint UNCONSTRUCTIBLE_PUB_STRUCTS

[mu001999]

View all comments

Add a new lint UNCONSTRUCTIBLE_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.

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

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

[nikomatsakis]

@rfcbot reviewed

[Mark-Simulacrum]

https://docs.rs/crate/rocket/latest/source/tests/responder_lifetime-issue-345.rs seems to be triggering this, but the code there looks reasonable?

[INFO] [stdout] error: struct `CustomResponder` is never constructed
[INFO] [stdout]   --> tests/responder_lifetime-issue-345.rs:10:12
[INFO] [stdout]    |
[INFO] [stdout] 10 | pub struct CustomResponder<'r, R> {
[INFO] [stdout]    |            ^^^^^^^^^^^^^^^
[INFO] [stdout]    |
[INFO] [stdout]    = note: this `pub` struct has private fields, no public constructor, and is not otherwise reachable through the external API, so consider providing a public constructor or removing it
[INFO] [stdout]    = note: `#[deny(unused_unconstructable_pub_structs)]` (part of `#[deny(unused)]`) on by default
[INFO] [stdout] 

#![allow(dead_code)] // This test is only here so that we can ensure it compiles.

#[macro_use] extern crate rocket;

use rocket::{Request, State};
use rocket::response::{Responder, Result};

struct SomeState;

pub struct CustomResponder<'r, R> {
    responder: R,
    state: &'r SomeState,
}

impl<'r, 'o: 'r, R: Responder<'r, 'o>> Responder<'r, 'o> for CustomResponder<'r, R> {
    fn respond_to(self, req: &'r Request<'_>) -> Result<'o> {
        self.responder.respond_to(req)
    }
}

#[get("/unit_state")]
fn unit_state(state: &State<SomeState>) -> CustomResponder<()> {
    CustomResponder { responder: (), state: &*state }
}

#[get("/string_state")]
fn string_state(state: &State<SomeState>) -> CustomResponder<String> {
    CustomResponder { responder: "".to_string(), state: &*state }
}

Should this be under dead_code instead of unused?

With a T-compiler hat on, if it's not added to dead_code, we should add it to the lints we allow at a CLI level in compiletest (

rust/src/tools/compiletest/src/runtest.rs

Line 1872 in 042c759

// Add `-A unused` before `config` flags and in-test (`props`) flags, so that they can

) rather than editing hundreds of tests. I'm not sure why that's not already there given -Aunused... I think I'm maybe a bit confused over whether -Aunused contains -Adead_code or what the exact story there is.

[joshtriplett]

👍 for stabilizing this, but under its current name, "unconstructable", with the 'a'.

Note on spelling: https://www.merriam-webster.com/dictionary/constructable

constructability noun
or less commonly constructibility

This was already changed in response to #146440 (comment) . Let's not churn again.

Our existing usage in the repository also trends towards the 'a':

~/src/rust$ git grep -i constructable
src/doc/rustc-dev-guide/src/diagnostics/error-guaranteed.md:`ErrorGuaranteed` is a zero-sized type that is unconstructable outside of the
src/doc/rustc-dev-guide/src/ty.md:`TyKind::Error` that prevents it from being constructable elsewhere.
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Callable<'_>` would have to be implemented for the type `ConstructableImpl<'0>`, for any lifetime `'0`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Callable<'1>` is actually implemented for the type `ConstructableImpl<'1>`, for some specific lifetime `'1`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Getter<'1>` would have to be implemented for the type `GetterImpl<'0, ConstructableImpl<'_>>`, for any two lifetimes `'0` and `'1`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Getter<'2>` is actually implemented for the type `GetterImpl<'2, ConstructableImpl<'_>>`, for some specific lifetime `'2`
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: `Callable<'_>` would have to be implemented for the type `ConstructableImpl<'0>`, for any lifetime `'0`...
tests/ui/async-await/higher-ranked-auto-trait-13.no_assumptions.stderr:   = note: ...but `Callable<'1>` is actually implemented for the type `ConstructableImpl<'1>`, for some specific lifetime `'1`
tests/ui/async-await/higher-ranked-auto-trait-13.rs:struct ConstructableImpl<'a> {
tests/ui/async-await/higher-ranked-auto-trait-13.rs:impl<'a> Callable<'a> for ConstructableImpl<'a> {
tests/ui/async-await/higher-ranked-auto-trait-13.rs:        List::<'_, GetterImpl<ConstructableImpl<'_>>> { data, item_size: (), phantom: PhantomData };
tests/ui/consts/const_forget.rs:// const constructable
tests/ui/nll/issue-45696-no-variant-box-recur.rs:// the above, in that they are similarly non-constructable data types
tests/ui/structs/default-field-values/empty-struct.rs:// sentinel field. As of now, that field can't be made private so it is only constructable with this
~/src/rust$ git grep -i constructible
tests/ui/async-await/issues/issue-59972.rs:// types as entirely uninhabited, when they were in fact constructible. This

(I expect the compiler's spelling-correction logic will always tell the user how to spell it. But perhaps we should just pick a name that doesn't have this problem.)

[tmandry]

I think the constructable/constructible spelling thing is going to be annoying to deal with, so I'm +1 on finding a name that doesn't use the word. I haven't thought of any alternatives I like, though.

[traviscross]

(I expect the compiler's spelling-correction logic will always tell the user how to spell it. But perhaps we should just pick a name that doesn't have this problem.)

I'm happy to pick a name that avoids this, though I don't have any immediate ideas other than, perhaps, pub_structs_without_constructors. The property we're interested in here is whether it's constructible.

Regarding the spelling of constructible:

1. Mathematics uses constructible:
   • The constructible universe.
   • The axiom of constructibility.
   • Constructible numbers.
2. Computer science uses constructible:
   • A constructible function, as in a time-constructible function or a space-constructible function.
3. The ngrams data is clear; the Webster's claim is about constructibility vs constructability, the noun form, where there is more tension due to use of the latter in the construction industry as a term of art. For the adjective form, Collins gets this right, putting constructible in the main entry and labeling the other as a variant spelling.
4. Construct is a Latinate word that's been lexicalized — i.e., it's a word derived from Latin that's become part of English rather than being a borrowed word — and many of these words take -ible, as in reproducible, extensible, accessible, responsible, convertible, reversible, reducible, and — most notably for our purposes here — destructible (the -able form of this is a misspelling, not an alternate). Constructible is one of these words.

If we wouldn't misspell destructible, then it would seem odd to me for us to go out of our way to pick the -able variant of constructible when the -ible form is supported by precedent relevant to our use, usage data, and etymology. That would seem unnecessarily inconsistent. The grep of our source code mostly comes back with one test that uses ConstructableImpl in a type name, with that repeated across the .stderr revision files. This would be our first use of the term within the surface area of the language, so this is our opportunity to avoid this inconsistency with destructible.

[mu001999]

@Mark-Simulacrum

Should this be under dead_code instead of unused?

Currently, dead_code is under the unused group, and we do not yet support nested lint groups. If we did support them, I would agree with putting this under a dead_code group.

With a T-compiler hat on, if it's not added to dead_code, we should add it to the lints we allow at a CLI level in compiletest (
) rather than editing hundreds of tests. I'm not sure why that's not already there given -Aunused... I think I'm maybe a bit confused over whether -Aunused contains -Adead_code or what the exact story there is.

We already pass -A unused, and as noted above, dead_code is part of the unused group. However, not all compiletests allow it: at least, //@ run-pass tests, aux crates, and rustfix won't pass it.

[steffahn]

• The constructible universe.

Ah, that’s why I’ve felt like I had seen that word somewhere a bunch of times before!

---

Looking at the updated note+help after bab791c, I don’t think it’s quite right yet…

consider adding a field of type ! or () if it is intended to be used as an unconstructable type

since adding () won’t do much, will it? Also, avoiding the word “unconstructable”¹ in this place is possible (maybe even desirable?) since it’s rather about making it an uninhabited type, right?

The way to make it a ZST would not be to “add a field of type ()”, but to wrap (all) existing fields in PhantomData, I guess 🤔… right?

Footnotes

1. in either spelling… 🙈 — is this also something where having an “A”-“I” policy will help‽ ↩

[mu001999]

@steffahn

since adding () won’t do much, will it?

() is special here because we have

Uninhabited, containing only ZST fields, or containing unit fields. Since they're usually intentional type-level markers

---

The way to make it a ZST would not be to “add a field of type ()”, but to wrap (all) existing fields in PhantomData, I guess 🤔… right?

To me, PhantomData is less like ()/! indicating that the type is intentionally unconstructable, and more like a rule inside the implementation of this lint.

In other words, I think ()/! are more like markers and easy to add, while PhantomData is more like #[repr(C)] and is an implementation detail 🤔.

---

Also, avoiding the word “unconstructable”1 in this place is possible (maybe even desirable?)

Updated the wording

[mu001999]

How about pub_structs_cannot_be_constructed? It might be a bit long, though.

[traviscross]

How about pub_structs_cannot_be_constructed? It might be a bit long, though.

It's a bit long and doesn't really match the grammatical structure of a lint name.

[mu001999]

Personally, I lean toward unconstructible_pub_structs, followed by pub_structs_without_constructors.

Can we reach consensus on this, or should we brainstorm on Zulip to see if there’s a better name?

[traviscross]

It remains nominated. We'll (likely) consider it in the call next week, unless it's gone into FCP.

[Mark-Simulacrum]

We already pass -A unused, and as noted above, dead_code is part of the unused group. However, not all compiletests allow it: at least, //@ run-pass tests, aux crates, and rustfix won't pass it.

I see. I think it would make sense to me to add a -Aunconstructable_pub_structs unconditionally to compiletest, at least as part of landing this PR: it significantly reduces the diff, and I'm not convinced the warnings are useful in the tests I see modified here. We can consider whether to enable it in a separate PR.

View changes since the review

[mu001999]

Agreed, in fact I also plan to merge this with allow-by-default first, then make it deny-by-default in a seperate PR.

These tests were modified to check if there is false-positives in existing tests and pass CI and then run the crater.

[Mark-Simulacrum]

I would think the "reachable path from external APIs" is what is ruling out this lint applying to code like this:

pub struct Bar(u32); // no lint
pub fn foo(_: Bar) {}

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:

#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Bar(u32); // yes lint

impl Copy for Bar {}
impl Clone for Bar {
        fn clone(&self) -> Self { todo!() }
}

and yet if one of those is an inherent impl, then the lint stops applying:

pub struct Bar(u32); // no lint
impl Bar { pub fn foo() {} }

or takes Bar as an argument:

pub struct Bar(u32); // no lint
impl From<Bar> for u32 {
        fn from(_: Bar) -> u32 { todo!() }
}

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

[mu001999]

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 like impl Bar { pub fn foo() {} }, since the signature does not depend on Bar, 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. The analysis here is wrong, see below.

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.

[Mark-Simulacrum]

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.

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:

pub struct Bar(u32); // no lint

pub fn foo() {
        Bar;
}

[mu001999]

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::foo is public, then it will propagate to Bar, because Bar::foo depends on Bar (at type-level at least), regardless of whether its body actually uses Bar.

[Mark-Simulacrum]

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() {} } and pub 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:

pub struct Bar(u32);

pub trait Exposed: Sized {
    fn output() -> Self { todo!() }
}

impl Exposed for Bar { }

and this lints, which is fully incorrect:

pub struct Bar(u32);

pub trait Exposed: Sized {
    fn output() -> Self { todo!() }
}

trait Foo {
    fn output() -> Self;
}
impl Foo for Bar {
    fn output() -> Self { Bar(0) }
}

impl<T: Foo> Exposed for T {
        fn output() -> Self { T::output() }
}

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.

[mu001999]

I don't see much difference between impl Bar { pub fn foo() {} } and pub fn foo(_: Option) {}, personally. Both are callable without constructing Bar, but only one of them triggers the lint.

They both won't trigger the lint because they both will mark Bar as live (for now). But seems what you said is mainly for the future.

[mu001999]

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.

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.

[Mark-Simulacrum]

Do we exclude types with repr(C)/repr(transparent) on them as well?

View changes since the review

[mu001999]

Yes, I will add the rule into the description

[Mark-Simulacrum]

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 _Foo seems like the wrong recommendation: either allowing the lint, adding a ! or () field, making it an empty enum (enum Foo {}) all seem like better options.

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?

View changes since the review

[Mark-Simulacrum]

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 unused_private_fields or so?

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 help text / messaging on the field-level lints to say that the struct is also not constructable, but omit the lint on the struct itself?

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?

View changes since the review

[mu001999]

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 help text / messaging on the field-level lints to say that the struct is also not constructable, but omit the lint on the struct itself?

Currently "field is never read" is a part of dead_code. This means we need to extend the scope of lint dead_code to include such structs, otherwise "field is never read" couldn't cover such structs. And I'm afraid this couldn't be done partly like only for field-level lints, which means we cannot achieve the goal, i.e., "extend the help text / messaging on the field-level lints".

I still prefer providing a separate lint, and keeping the scope of dead_code limited to non-exported items. Then users can allow or deny this lint separately.

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?

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!

[mu001999]

According to #146440 (comment), I plan to close this PR. If one day I find a solution, I'll reopen it (or as a new PR). But now, I believe the underlying assumptions of this PR are unsound.

Thanks to everyone who participated!

[traviscross]

Thanks @mu001999 for your work on this, and thanks @Mark-Simulacrum for the fantastic edge-case analysis.