Error reporting from attribute macros regressed in 1.46.0

[ahl]

We take particular care to make sure that compilation errors associated with the items to which our attribute macros are applied are comprehensible. To that end we have tests to check the output of a variety of expected illegal programs to make sure developers could reasonably be expected to understand and correct those errors. In 1.46.0 we noticed a change in that behavior. While there were some beneficial changes to the way errors qualify structs, traits, etc. there seems to be a regression where 1.46.0 shows less information than in 1.45.0

I've put together a small repo that demonstrates the problem: https://github.com/ahl/span_regression

The first example is this macro:

#[proc_macro_attribute]
pub fn identity(
    _attr: proc_macro::TokenStream,
    item: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    item
}

Now consider this test program:

struct Thing {
    x: i32,
    y: i32,
}

#[identity]
fn doit() -> Thing {
    Thing {
        "howdy".to_string(),
    }
}

Under 1.45.0 one of the errors produced looks like this:

error[E0063]: missing fields `x`, `y` in initializer of `Thing`
  --> examples/test.rs:10:5
   |
10 |     Thing {
   |     ^^^^^ missing `x`, `y`

Under 1.46.0 that has changed to this:

error[E0063]: missing fields `x`, `y` in initializer of `Thing`

Note that under 1.46.0 no code is underlined.

Now consider a slightly more complicated macro:

#[proc_macro_attribute]
pub fn nested(
    _attr: proc_macro::TokenStream,
    item: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    let mut ret = proc_macro::TokenStream::new();
    let pre = vec![
        proc_macro::TokenTree::Ident(proc_macro::Ident::new("fn", proc_macro::Span::call_site())),
        proc_macro::TokenTree::Ident(proc_macro::Ident::new("foo", proc_macro::Span::call_site())),
        proc_macro::TokenTree::Group(proc_macro::Group::new(
            proc_macro::Delimiter::Parenthesis,
            proc_macro::TokenStream::new(),
        )),
        proc_macro::TokenTree::Group(proc_macro::Group::new(proc_macro::Delimiter::Brace, item)),
    ];
    ret.extend(pre);
    ret
}

(Note I had been using quote! but wanted to make sure that wasn't causing the problem)

With a similar example as above on 1.45.0:

error: expected identifier, found `"yo"`
  --> examples/test.rs:20:9
   |
19 |     Thing {
   |     ----- while parsing this struct
20 |         "yo".to_string(),
   |         ^^^^ expected identifier

With 1.46.0:

error[E0308]: mismatched types
  --> examples/test.rs:17:1
   |
17 | #[nested]
   | ^^^^^^^^^
   | |
   | expected struct `Thing`, found `()`
   | expected `Thing` because of return type

Rather than pointing to the offending code, the rustc error now points (unhelpfully) to the macro itself.

The general improvements made to error reporting in 1.46.0 (simpler naming, reduced duplicate errors) is greatly appreciated. This was one small regression I saw amongst the otherwise monotonic improvements.

[LeSeulArtichaut]

If I remember correctly, we should still be able to use cargo-bisect-rustc to bisect a regression in 1.46.0.
@rustbot ping cleanup

[rustbot]

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @camelid @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

[LeSeulArtichaut]

Assigning P-high as discussed as part of the Prioritization WG procedure.

[estebank]

Cc @petrochenkov

[Aaron1011]

This is caused by #73345 - since we no longer pass a Nonterminal to an attribute macro, the pretty-print/reparse check get used (and fails), which causes us to lose spans.

This is a very interesting corner case - the span_regression error is testing the behavior of a proc-macro called on a syntactically invalid Item. It looks like this relies on the parser managing to recover an Item, despite the fact that the body is syntactically invalid.

I think there are two possible ways to fix this:

1. Disable the pretty-print/reparse check when we recovered from parse errors during the attribute target - this would restore the 1.45.0 behavior of the span_regression repository. However, this would effectively mean that the parser recovery behavior is now part of the stable surface of the compiler, instead of just a trick for producing more meaningful error messages. Any future changes to parser recovery could cause us to start invoking an attribute macro, if we were able to parse enough of the target to produce an AST struct.
2. Skip invoking attribute macros if there were any errors during the parsing of the attribute target, regardless of recovery behavior. Since attributes produce an arbitrary TokenStream as output, this would require us to remove the target item entirely (since we can't know what the attribute macro would have produced). Effectively, custom attribute macros would disable parser recovery.

I'm leaning towards option 2 (assuming that it doesn't cause significant regressions in practice). Bang-proc-macros are the way to invoke a proc-macro on an arbitrary TokenStream - however, attributes macros are (syntactically) just another attribute, so it doesn't seem to make sense to invoke them on something that's not a syntactically valid attribute target. While the span_regression example is 'good enough' for recovery to be able to kick in, I think going with approach 1 would cause us to end up with an unprincipled list of a special cases where we invoke an attribute macro on a syntactically invalid target.

On the other hand, it's possible that option 2 could lead to a large number of spurious errors in practice - if the user introduces a syntax error when modifying a commonly-used struct (which has an attribute macro applied), removing this struct entirely could cause many spurious resolution errors. Invoking the attribute macro might lead to better (but still invalid) tokens being produced, leading to cleaner error messages.

[Aaron1011]

@ahl Thanks for providing such a nicer reproducer!