prevent checking redundant_field_names lint on Derived code

[azdle]

This check was removed in #17294 and replaced with a later per-field check, however the field's spans can (maybe always do?) point back to the fields in the original struct declaration for struct expressions in derived code. This was causing this lint to warn on derive-generated code.

I wrote a more complete explanation here: #17525 (comment)

changelog: [redundant_field_names]: don't check derived expressions

Fixes: #17525

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[azdle]

Also, I should say, I didn't add a test for this because I would need a proc macro crate to generate code that would always, itself, fail the lint. As far as I can tell there isn't one for that yet and adding a dedicated crate for that to the test suite would dwarf this change and I wasn't sure if I should or not. I'm happy to add that if it's wanted.

[Jarcho]

This doesn't fix all the ways proc-macros can mess up when generating code. In thiserror it's generating $field: $field as though it were in the callers context even though the colon should have the macro's context. It happens to generate the expression as a whole in the correct context, but nothing stops macros from messing that up as well.

After the current in_external_macro check you can add:

field.span.check_text(cx, |src| {
  let field_name = field.ident.as_str();
  src.strip_prefix(name)
    .and_then(|src| src.strip_suffix(name))
    .is_some_and(|src| src.trim() == ":")
})

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Jarcho]

To add a test you can use

use proc_macros::with_span;
with_span! {
  field_name
  Struct { field_name: field_name }
}

The macro skips the first token and outputs the rest with the span of each set to the first token. This emulates the problematic code gen of other proc-macros.

[azdle]

To add a test you can use

use proc_macros::with_span;
with_span! {
  field_name
  Struct { field_name: field_name }
}

The macro skips the first token and outputs the rest with the span of each set to the first token. This emulates the problematic code gen of other proc-macros.

I'm not quite sure I understand this suggestion. In playing with it in a test, the lint produces warnings for code on the second line with or without the fix I'm suggesting. The expression as a whole still has a span that is in_external_macro -> false. Which feels correct to me since the 'macro code' is still written locally in the test. So, I don't think that's quite the same case. But, maybe I misunderstood your intention and/or what with_span! does?

Though, I did also discover tests/ui/auxiliary/proc_macro_derive.rs, which I assume would be built as 'external macros' for the tests (that's the only way proc macros work, right?), so using a macro from there would end up with an expression that is in_external_macro -> true. I would just need to add a macro in there that uses the input TokenStream to get the field names (who's spans are in_external_macro -> false) and use those write some redundant field names to test the exact case I was originally trying to fix. Does that sound like a reasonable way to test this?

I haven't had the time to fully understand your first comment yet, so apologies if I'm completely speaking past any context from that.

[Jarcho]

with_span exists to emulates the bad token output of other proc-macros so we don't have to write an endless number of them. Usually the tokens are output with something like quote_spanned!(name.span=> Struct { $name: $name ) which would make every token share the same span, none of which are marked as being from a macro.

[azdle]

which would make every token share the same span, none of which are marked as being from a macro.

Assuming this is also true about the with_span, that wouldn't be able to trigger the bug then. There isn't a problem when all of the spans marked as being from a macro or when all the spans are not marked as being from a macro.

The bug is specifically when only the fields' idents are not from a macro, but the outer construction/format of the struct expression is. Which I think is how these macros are generally intended to be built, so I don't think this is a case of 'all the ways proc-macros can mess up when generating code'.

I threw a separate commit in with a test on a super simple derive proc macro (with no span overrides at all) that fails before the existing fix commit that I hope shows what I mean.

@rustbot ready

[Jarcho]

The bug is entirely that the field span is being marked incorrectly because all three tokens are given the same span. When the macro outputs foo: foo with all tokens having the same span the expression field's span then gets exactly the same span. Anything else the macro outputs is irrelevant to this since the span of the generation of the expression field is the broken part. We catch this by checking the source text of the field expression to see if it matches what we expect it to. This is the most reliable thing we can use.

[azdle]

The bug is entirely that the field span is being marked incorrectly because all three tokens are given the same span. When the macro outputs foo: foo with all tokens having the same span [...]

They don't all have the same span.

In the returned TokenStream in my test the two v are in span 0, but the colon between them is in span 9.

    Group {
        delimiter: Brace,
        stream: TokenStream [
            Ident {
                ident: "DerivedS",
                span: #0 bytes(2011..2019),
            },
            Group {
                delimiter: Brace,
                stream: TokenStream [
                    Ident {
                        ident: "v",
                        span: #0 bytes(2030..2031),
                    },
                    Punct {
                        ch: ':',
                        spacing: Alone,
                        span: #9 bytes(1944..1997),
                    },
                    Ident {
                        ident: "v",
                        span: #0 bytes(2030..2031),
                    },
                ],
                span: #9 bytes(1944..1997),
            },
        ],
        span: #9 bytes(1944..1997),
    },

[Jarcho]

Looking further it looks like rustc actually ignores the colon token, which is just wrong since it loses the context the expression field was written in.

[CommanderStorm]

community review:
not sure if this should have is_from_proc_macro too. What do you think?

View changes since this review

[CommanderStorm]

usually, there is an is_from_proc_macro check right next to this I think.
Is it purposely ommitted?