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

rustc complains about unused code if item is used in #[automatically_derived] context #126031

Open
aumetra opened this issue Jun 5, 2024 · 8 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@aumetra
Copy link

aumetra commented Jun 5, 2024

Code

I tried this code:

use std::fmt::{Debug, Display};

struct Identity<T>(T);

impl<T> Display for Identity<T>
where
    T: Display,
{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

struct Thing {}

#[automatically_derived]
impl Debug for Thing {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", Identity("Thing"))
    }
}

fn main() {
    let hello = Thing {};
    println!("{hello:?}");
}

I expected to see this happen: The code compiles without any warnings since, the Identity struct is clearly used.

Instead, this happened: The compiler emits a dead_code lint, telling me that the struct is never constructed

Example where this can occur: when using the derive_more crate to derive a specialized Debug implementation, and the usage of derive_more::Debug utilizes a formatter struct to avoid allocating into an intermediate heap allocated buffer

Real-world example

Version it worked on

It most recently worked on: Rust 1.77.2

Version with regression

rustc --version --verbose:

rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: x86_64-unknown-linux-gnu
release: 1.78.0
LLVM version: 18.1.2

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

@aumetra aumetra added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jun 5, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Jun 5, 2024
@aumetra
Copy link
Author

aumetra commented Jun 5, 2024

Okay, I submitted it under the wrong category. Apologies for that. Clearly a diagnostic issue.

@aumetra
Copy link
Author

aumetra commented Jun 5, 2024

@rustbot modify labels: -regression-from-stable-to-stable +A-diagnostics

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints and removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Jun 5, 2024
@aumetra
Copy link
Author

aumetra commented Jun 5, 2024

@rustbot modify labels: -I-prioritize +T-compiler

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 5, 2024
@apiraino
Copy link
Contributor

apiraino commented Jun 5, 2024

Actually I think you were right, seems to be a diagnostic regression introduced in c69fda7 (so my bisection reports):

cc @mu001999 since author of #121752 and @pnkfelix (reviewer of said patch)

@rustbot label +regression-from-stable-to-stable +P-medium -needs-triage

@rustbot rustbot added P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 5, 2024
@apiraino apiraino added P-low Low priority and removed P-medium Medium priority labels Jun 5, 2024
@mu001999
Copy link
Contributor

mu001999 commented Jun 5, 2024

@rustbot claim

@mu001999
Copy link
Contributor

mu001999 commented Jun 6, 2024

@apiraino After some investigation I don't think this is a regression. We can construct the following, which will also trigger dead_code lint in previous versions:

use std::fmt::Debug;

struct T;

struct Thing {}

#[automatically_derived]
impl Debug for Thing {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let _x = T;
        write!(f, "1")
    }
}

fn main() {
    let hello = Thing {};
    println!("{hello:?}");
}

Before 1.78, dead code analysis will check all impls regardless of whether the struct is used or not, so that we don't see such warnings cause it has an impl block. In fact, the reason is that the impl Debug is not automatically derived, and we can't construct such cases with only using #[derive(Debug)].

@aumetra in the Real-world example, it's because #[debug("{}", BinaryToStringEncoder(msg))] introduces the user-defined approach to impl Debug.


I don't know which macro the #[debug("{}", BinaryToStringEncoder(msg))] belongs to, but I think one way to solve this is to not generate #[automatically_derived] if users add such #[debug(...)] on fields.

@aumetra
Copy link
Author

aumetra commented Jun 6, 2024

I don't know which macro the #[debug("{}", BinaryToStringEncoder(msg))] belongs to, but I think one way to solve this is to not generate #[automatically_derived] if users add such #[debug(...)] on fields.

It's actually part of derive_more, so a pretty abundant crate in the ecosystem. But I guess not a lot of people are hitting this issue because not a lot of people do something like this.

@mu001999
Copy link
Contributor

mu001999 commented Jun 7, 2024

@aumetra furthermore, automatically_derived is helpful to lint never read fields by treating reading in the Debug impl as trivial reading:

#[derive(Debug)]
struct T(i32);

fn main() {
    let x = T(1);
    println!("{x:?}")
}

so that we can still get field 0 is never read, cause it's only used in the derived impl Debug.

As a comparison, Identity in #126031 (comment) is also only constructed in the Debug impl, that's why we lint it like those fields only used for debugging.

I guess another way to solve this in the real-world example is to print differently when meeting different fmt::Formatter's config in impl Debug for Binary, and then use the specific fmt_str in the #[debug(...)] on fields to determine the behavior.

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 C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants