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

#[derive(Debug)] fails when a generic parameter shares the name of the generic type #97343

Open
nekodjin opened this issue May 24, 2022 · 16 comments
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@nekodjin
Copy link

nekodjin commented May 24, 2022

I tried this code:

#[derive(Debug)]
pub struct Irrelevant<Irrelevant> {
    irrelevant: Irrelevant,
}

I expected to see this happen:
Code should compile normally as expected, with a correct fmt::Debug implementation. Omitting the derive shows that rustc accepts this as valid and intelligible code. We know that rustc is interpreting the type of irrelevant as the type parameter and not the parent struct, as the latter would produce an error since it would produce a recursive type with infinite size which would cause an error. As rustc defines the rust language, the derive macro should also handle this code correctly.

Instead, this happened:
Compilation errors with the following diagnostic:

error[E0574]: expected struct, variant or union type, found type parameter `Irrelevant`
 --> main.rs:3:10
  |
3 | #[derive(Debug)]
  |          ^^^^^ not a struct, variant or union type
  |
  = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0109]: type arguments are not allowed for this type
 --> main.rs:4:23
  |
3 | #[derive(Debug)]
  |          ----- in this derive macro expansion
4 | pub struct Irrelevant<Irrelevant> {
  |                       ^^^^^^^^^^ type argument not allowed
  |
  = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

Meta

I tested this and got the same result on the following versions:

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: x86_64-unknown-linux-gnu
release: 1.61.0
LLVM version: 14.0.0
rustc 1.63.0-nightly (ee160f2f5 2022-05-23)
binary: rustc
commit-hash: ee160f2f5e73b6f5954bc33f059c316d9e8582c4
commit-date: 2022-05-23
host: x86_64-unknown-linux-gnu
release: 1.63.0-nightly
LLVM version: 14.0.4
@nekodjin nekodjin added the C-bug Category: This is a bug. label May 24, 2022
@nekodjin
Copy link
Author

Running with macro backtrace does not provide any enlightening information as the macro is a compiler builtin.

@Noratrieb
Copy link
Member

Noratrieb commented May 24, 2022

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

pub struct Irrelevant<Irrelevant> {
    irrelevant: Irrelevant,
}

impl<Irrelevant: Debug> Debug for Irrelevant<Irrelevant> {
    fn fmt(&self, f: &mut Formatter) -> ::core::fmt::Result {
        match self {
            Irrelevant { irrelevant } => {
                f.debug_struct("Irrelevant")
                    .field("irrelevant", irrelevant)
                    .finish()
            }
        }
    }
}

Expanding and cleaning up the expansion yields this, which causes the same errors.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1118974f80862e77fe76fa5be67901cf

@Noratrieb
Copy link
Member

So I don't think this is a bug, since the issue is caused by exactly the name resolution specifics that allow the struct in the first place.

So this is a bad diagnostic at worst.

@fmease
Copy link
Member

fmease commented May 24, 2022

From my perspective this is indeed a bug. The built-in macro does not respect hygiene. The output can be fixed by prepending self:: to the implementing type. Playground. This is what the macro should do, too.

@nekodjin
Copy link
Author

I agree. It's hard to see this as anything other than a bug, as I see it.

@Noratrieb
Copy link
Member

Noratrieb commented May 24, 2022

Hm true, they could indeed use self::. Or they could even use the actual Self.

@fmease
Copy link
Member

fmease commented May 24, 2022

@rustbot claim

@fmease
Copy link
Member

fmease commented May 24, 2022

Ah, what a shame, self:: of course doesn't work if the type and the impl are inside of a function 🤦‍♂️ 😞

@fmease
Copy link
Member

fmease commented May 24, 2022

@estebank Do you think this hygiene issue was potentially caused by #90519 or was it likely already present before that?

@fmease fmease removed their assignment May 24, 2022
@inquisitivecrystal inquisitivecrystal added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 25, 2022
@estebank
Copy link
Contributor

@fmease it predates it, it just had worse output:

error[E0574]: expected struct, variant or union type, found type parameter `Irrelevant`
 --> <source>:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^ not a struct, variant or union type
  |
  = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0109]: type arguments are not allowed for this type
 --> <source>:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^ type argument not allowed
  |
  = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0210]: type parameter `Irrelevant` must be used as the type parameter for some local type (e.g., `MyStruct<Irrelevant>`)
 --> <source>:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^ type parameter `Irrelevant` must be used as the type parameter for some local type
  |
  = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
  = note: only traits defined in the current crate can be implemented for a type parameter

error: aborting due to 3 previous errors

The issue with the clashing of the literal struct inside of fmt can be addressed by changing from the name to Self. For the type in the impl block, I'm less certain. As shown above, prepending self:: won't work in any non-mod scope.


As a separate concern, I would like us to improve E0109 to account for this case and recommend you changing the type param Irrelevant so it doesn't clash with the type Irrelevant, regardless of whether it happens in a derive macro or not.

@estebank
Copy link
Contributor

@fmease opened #97458 to only emit a single error. I think E0109 should still be improved to handle this case (it should have its own error code, tbh).

@estebank
Copy link
Contributor

Filed #97459 to improve E0574, while we're at it.

@estebank
Copy link
Contributor

Thinking about it for a second more, the macro could be smart enough to avoid using type param names that shadow the underlying type, but that might be too much magic for marginal improvements, I'll let @rust-lang/libs decide on that. For myself, I think that focusing on the errors themselves would yield enough benefit already. I personally would like to avoid writing code that looks like that, if a macro can be confused so can people.

@estebank estebank added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 27, 2022
@yaahc
Copy link
Member

yaahc commented May 27, 2022

Thinking about it for a second more, the macro could be smart enough to avoid using type param names that shadow the underlying type, but that might be too much magic for marginal improvements, I'll let @rust-lang/libs decide on that. For myself, I think that focusing on the errors themselves would yield enough benefit already.

would that be user visible in any way?

I personally would like to avoid writing code that looks like that, if a macro can be confused so can people.

I definitely agree with this. I think before we go about putting significant amounts of effort into improving this situation we should see some real examples that need the same name on the type and param. The Irrelevant<Irrelevant> example seems contrived.

@estebank
Copy link
Contributor

The Irrelevant<Irrelevant> example seems contrived.

I agree in principle for production code, but I can see how someone can end up with that code during development.

would that be user visible in any way?

This code would start compiling. Any alternative rustc impl would have to learn to also do this disambiguation in their derive(Debug) impl.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 27, 2022
…r=compiler-errors

Modify `derive(Debug)` to use `Self` in struct literal to avoid redundant error

Reduce verbosity in rust-lang#97343.
@estebank
Copy link
Contributor

estebank commented May 28, 2022

#97471 will also add a bit of context, but it isn't great yet:

error[E0109]: type arguments are not allowed for this type
 --> f41.rs:4:19
  |
3 | #[derive(Debug)]
  |          ----- in this derive macro expansion
4 | struct Irrelevant<Irrelevant> {
  |                   ^^^^^^^^^^ type argument not allowed
  |
note: type parameter `Irrelevant` defined here
 --> f41.rs:4:19
  |
4 | struct Irrelevant<Irrelevant> {
  |                   ^^^^^^^^^^
  = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants