Warnings in code generated by proc_macro_derive should be on derive() attr

[SimonSapin]

… not on the type definition.

This warning started appearing in Servo recently:

warning: unreachable expression, #[warn(unreachable_code)] on by default
   --> /home/simon/projects/servo1/components/script_traits/lib.rs:403:1
    |
403 | pub enum Milliseconds {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^

warning: unreachable expression, #[warn(unreachable_code)] on by default
   --> /home/simon/projects/servo1/components/script_traits/lib.rs:406:1
    |
406 | pub enum Nanoseconds {}
    | ^^^^^^^^^^^^^^^^^^^^^^^

It turns out that these types have #[derive(Clone, Copy, HeapSizeOf)], with a dependency defining #[proc_macro_derive(HeapSizeOf)]. The warning is in code generated by this macro.

But at first I couldn’t figure it out. I had to ask on IRC and sfackler suggested it could be generated code.

It would have helped if the span of the error was not on the type definition, but on the attribute that caused code to be generated in the first place. Something like this:

405 | #[derive(Clone, Copy, HeapSizeOf)]
    |                       ^^^^^^^^^^

The error message could also mention macros or generated code, like "unreachable expression in expansion of derive_HeapSizeOf".

[jseyfried]

This was caused by #36308, which intentionally changed custom derive error spans from the custom derive name's span to the item's span.

For the record, I think the custom derive name's span is a better choice, but neither is optimal. Hopefully we'll change custom derives so that they cannot modify the underlying item (cc #35900) so that we can get the best of both worlds.

[dtolnay]

@SimonSapin the reasoning behind #36308 is that custom derives are used many more times than they are implemented.

Sure, someone may implement a custom derive and their implementation contains 10 bugs. The first time each bug shows up, some early adopter will get a weird message, somehow track it down, file a ticket, and the bug is fixed. Hopefully another 10,000 people will use the custom derive without hitting bugs.

The assumption is that a custom derive given a valid struct should not be generating invalid code. This assumption was violated in your case but I don't think it is the right thing to optimize for.

If a custom derive generates invalid code, chances are it is because one of the 10,000 late adopters screwed something up in their struct, not one of the 10 early adopters hitting a bug in the implementation. There are so many things to screw up in a struct (using a type that is not imported, lifetimes snafu, wrong generic types, ...) and so many more late adopters that your situation feels like a rounding error. The error messages should reflect this balance.

[SimonSapin]

Ok, so maybe the span should not be reverted.

Still, I think the current error message can be improved. At least it should mention that the warning is in code generated by a custom derive (and which one). Even if the root cause is in the type definition, that would help explain why the error message is talking about an (unreachable) expression when there is no expression at the designated span.

[keeperofdakeys]

Now that proc_macro_derive functions no longer return the item, the span information of the derived items is no longer changed to that of the original item.