Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dead-code false positive on named structs with a never field #128053

Closed
ia0 opened this issue Jul 22, 2024 · 11 comments · Fixed by #128104
Closed

dead-code false positive on named structs with a never field #128053

ia0 opened this issue Jul 22, 2024 · 11 comments · Fixed by #128104
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. F-never_type `#![feature(never_type)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ia0
Copy link
Contributor

ia0 commented Jul 22, 2024

Code

#![feature(never_type)]

pub struct JustEmpty(!);

pub struct FancyEmpty {
    _empty: !,
}

Current output

warning: struct `FancyEmpty` is never constructed
 --> src/lib.rs:5:12
  |
5 | pub struct FancyEmpty {
  |            ^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `playground` (lib) generated 1 warning

Desired output

Compiles without warnings.

Rationale and extra context

The struct is meant to not be constructible. Also the behavior is incoherent between tuple structs (correct behavior) and named structs (incorrect behavior).

Also, this is a regression, this didn't use to be the case.

Other cases

No response

Rust Version

rustc 1.82.0-nightly (92c6c0380 2024-07-21)
binary: rustc
commit-hash: 92c6c03805408a1a261b98013304e9bbf59ee428
commit-date: 2024-07-21
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 18.1.7

Anything else?

No response

@ia0 ia0 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 22, 2024
@veera-sivarajan
Copy link
Contributor

@rustbot label +F-never_Type +D-inconsistent

@rustbot rustbot added D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. F-never_type `#![feature(never_type)]` labels Jul 22, 2024
@mu001999
Copy link
Contributor

mu001999 commented Jul 23, 2024

We will also get such warnings for private structs with fields of never type (play):

#![feature(never_type)]

struct JustEmpty(!);

struct FancyEmpty {
    _empty: !,
}

The struct is never constructed although it is not constructible. This behavior is expected and is not a regression. Did you meet any cases using this way in real world?


For the incoherent behavior, we also have:

pub struct T1(()); // no warning

pub struct T2 { // warning never constructed
    _x: (),
}

This because we only skip positional ZST (PhantomData and generics), this policy is also applied for never read fields:

struct T1(()); // no warning

struct T2 {
    x: (), // warning never read field
}

fn main() {
    let _t1 = T1(());
    let _t2 = T2 {
        x: (),
    };
}

@ia0
Copy link
Contributor Author

ia0 commented Jul 23, 2024

Did you meet any cases using this way in real world?

https://github.com/google/wasefire/blob/4c042d8e8ae68a9fef2a600aca524c4b628c39d9/crates/board/src/usb/serial.rs#L76-L81

This behavior is expected

Can you elaborate in which sense it is expected? I can understand that it is expected given the current implementation of Rust. It seems between stable and nightly a new dead-code lint was implemented. This lint is expected for inhabited types. However it has false positives on inhabited types, i.e. types that are only meant for type-level usage (for example to implement a trait).

I agree we should probably categorize this issue as a false positive of a new lint rather than a regression (technically).

For the incoherent behavior

Thanks, I can see the idea behind it. I'm not convinced about it though, but it doesn't bother me either. So let's just keep this issue focused on the false positive rather than the inconsistent behavior.

@mu001999
Copy link
Contributor

@ia0 How do you think about private structs with fields of never type?

@ia0
Copy link
Contributor Author

ia0 commented Jul 23, 2024

@ia0 How do you think about private structs with fields of never type?

I would consider it the same as public structs, because they can still be "constructed" at the type-level. So they are dynamic dead--code but not dead-code since they are used statically. It would be dead-code though if the type is never used (both in dynamic code and static code, i.e. types).

@ia0
Copy link
Contributor Author

ia0 commented Jul 23, 2024

Actually, thinking about this issue, I don't think this is a false positive only for the never type. I think the whole lint is wrong. The fact that the type is empty is just a proof that constructing it at term-level is not necessary for the type to be "alive" (aka usable). This could also be the case with non-empty types. A lot of people just use unit types for type-level usage only (instead of empty types). So my conclusion would be that linting a type to be dead-code because it is never constructed is brittle as it can't be sure that the type is not meant to be constructed at term-level (and thus only used at type-level). Adding #[allow(dead_code)] is not a solution because if the type is actually not used at all (including in types) then we actually want the warning. Here is an example:

// This type is not dead code. It is simply meant to be used at type-level.
pub struct Foo<T> {
    _x: T,
}

pub trait Bar {
    fn bar();
}

impl<T> Bar for Foo<T> {
    fn bar() {}
}

#[test]
fn test() {
    Foo::<i32>::bar();
}

The same thing with a private struct:

// This type is not dead code. It is simply used at type-level (to select an trait implementation).
struct Foo<T> {
    _x: T,
}

pub trait Bar {
    fn bar();
}

impl<T> Bar for Foo<T> {
    fn bar() {}
}

pub fn test() {
    Foo::<i32>::bar();
}

And now actual dead code:

// This type is dead code. It is private and not used (both at term- and type-level).
struct Foo<T> {
    _x: T,
}

pub trait Bar {
    fn bar();
}

// This doesn't count as usage but as definition.
impl<T> Bar for Foo<T> {
    fn bar() {}
}

@mu001999
Copy link
Contributor

mu001999 commented Jul 24, 2024

Actually, thinking about this issue, I don't think this is a false positive only for the never type. I think the whole lint is wrong. The fact that the type is empty is just a proof that constructing it at term-level is not necessary for the type to be "alive" (aka usable). This could also be the case with non-empty types. A lot of people just use unit types for type-level usage only (instead of empty types). So my conclusion would be that linting a type to be dead-code because it is never constructed is brittle as it can't be sure that the type is not meant to be constructed at term-level (and thus only used at type-level). Adding #[allow(dead_code)] is not a solution because if the type is actually not used at all (including in types) then we actually want the warning. Here is an example:

// This type is not dead code. It is simply meant to be used at type-level.
pub struct Foo<T> {
    _x: T,
}

pub trait Bar {
    fn bar();
}

impl<T> Bar for Foo<T> {
    fn bar() {}
}

#[test]
fn test() {
    Foo::<i32>::bar();
}

@ia0 I think a better way is:

// This type is not dead code. It is simply meant to be used at type-level.
pub struct Foo<T> {
    x: std::marker::PhantomData<T>,
}

pub trait Bar {
    fn bar();
}

impl<T> Bar for Foo<T> {
    fn bar() {}
}

#[test]
fn test() {
    Foo::<i32>::bar();
}

PhantomData is usually used to do such things like just a proof. Maybe we can add a help for this case.

@mu001999
Copy link
Contributor

mu001999 commented Jul 24, 2024

But I agree that we shouldn't emit warnings for pub structs with any fields of never type (and also unit type), this is an intentional behavior.

@ia0
Copy link
Contributor Author

ia0 commented Jul 24, 2024

I think a better way is

Oh yes good point. So all good to proceed with adding never type to the list of special types.

@bors bors closed this as completed in 91b18a0 Jul 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 30, 2024
Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov

Not lint pub structs without pub constructors intentionally

Fixes rust-lang#128053
@ia0
Copy link
Contributor Author

ia0 commented Jul 31, 2024

@mu001999 Thanks a lot for the quick fix!

Once it hits nightly, I'll give it a try. I'm a bit afraid though that this won't fix the underlying problem of understanding which types are not meant to exist at runtime. For example, sometimes instead of using the never type, I also use empty enums such that I can give a name and separate identity to that type. Let's see if it's an issue. If yes, I guess ultimately there would be 2 options:

  • Just use allow(dead_code) in those cases.
  • Introduce a OnlyType trait (or better name) for types that are not meant to exist at runtime like unit, never, and phantom types, but could be extended with user types like empty enums.

I'll ping this thread again if I actually hit this issue.

@ia0
Copy link
Contributor Author

ia0 commented Aug 2, 2024

I could test it (using nightly-2024-08-01) and it fixed my problems. I just had to change a core::convert::Infallible to ! in a crate where I didn't have #![feature(never_type)] already. Ultimately, Infallible is going to be a type alias for ! so maybe it's not worth supporting it.

And thinking about empty enums, this should never be an issue for my particular usage, which is to build empty types. Because either the type takes no parameters in which case I use an empty enum directly. Or it takes parameters and I have to use a struct with PhantomData fields, and to make the struct empty I also have to add a never field. So my concern in the previous message is not an issue for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. F-never_type `#![feature(never_type)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants