Error on non-literal expressions in doc attributes on macro calls

[mejrs]

View all comments

Recently I discovered that, since Rust 1.94, doc attributes on macro invocations can have arbitrary expressions in them:

// accidentally stabilized in 1.94
#[doc = concat!("", "")]
#[doc = {
    let a = 1;
    let b = 1;
    let sum = a + b;
    assert_eq!(sum, 2);
}]
println!();

As part of the attribute parsing rework this was accidentally allowed. Note that doc attributes (or any doc comment) on macro invocations do nothing, because documentation for macro invocations is not rendered - this emits a lint saying macros must produce doc comments as part of their expansion.

With this PR, it now emits a FCW, like #57571. As this is so niche it's probable this could go straight to an error but there's quite a crater queue so I'd rather do this now and try turning it into an error later.

error: invalid expression in `doc` attribute on macro invocation
  --> $DIR/attr-on-mac-call.rs:115:13
   |
LL |     #[doc = concat!("", "")]
   |     --------^^^^^^^^^^^^^^^^^^^^^^^- help: remove the attribute
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #57571 <https://github.com/rust-lang/rust/issues/57571>
   = note: `#[deny(ill_formed_attribute_input)]` (part of `#[deny(future_incompatible)]`) on by default

error: invalid expression in `doc` attribute on macro invocation
  --> $DIR/attr-on-mac-call.rs:118:13
   |
LL |        #[doc = {
   |  ______-       ^
   | | _____________|
LL | ||         let a = 1;
LL | ||         let b = 1;
LL | ||         let sum = a + b;
LL | ||         assert_eq!(sum, 2);
LL | ||     }]
   | ||_____^- help: remove the attribute
   |  |_____|
   |
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #57571 <https://github.com/rust-lang/rust/issues/57571>

error: aborting due to 7 previous errors; 30 warnings emitted

Note that #[doc = mac!()] is included in this. While this is allowed everywhere else as normally attribute parsing only sees it after its expansion, it is not expanded here, but we do need to check attributes here since we can't check them later as they're lost by then.

#[doc = mac!()] // this would be expanded second, but any remaining attributes
                // on macro invocations are dropped before they are expanded.
println!(); // this is expanded first

I don't think it is worth trying to make particular case work - this would involve checking that the expression would expand to a string literal:

• These expansions have a defined order (see also @petrochenkov's comment at #t-compiler > attribute parsing rework @ 💬)
• so it would be quite a hack to check whether the expression would expand to a string literal
• changing macro expansion order might be possible but is a big can of worms and undesirable.
• it would serve no use case, as the attribute is dropped regardless

This change would also make it consistent with all other key-value attributes. For an example, the following are allowed

#[deprecated = "foo"] // just `unused_attributes` warning
println!();
    
#[deprecated = concat!()]
struct Foo;

but this is not:

#[deprecated = concat!()]
println!();

error: attribute value must be a literal
 --> src/main.rs:2:20
  |
2 | #[deprecated = concat!()]

r? @JonathanBrouwer

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[mejrs]

@JonathanBrouwer this is ready for review now

[mejrs]

I'll go ahead and crater this asap, I'd rather not have the complexity of emitting the lint anyway

[mejrs]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: f4dd856 (f4dd856795debe636f2ba39a900f162f5abeb7e8)
Base parent: c999cef (c999cef531ea9059e189e82fe0e82c5daf249bc9)

[mejrs]

@craterbot check

[craterbot]

👌 Experiment pr-160904 created and queued.
🤖 Automatically detected try build f4dd856
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-160904 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-160904 is completed!
📊 1 regressed and 2 fixed (1124431 total)
📊 6127 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-160904/retry-regressed-list.txt ↩

[mejrs]

Just one! I win at crater golf!

[JonathanBrouwer]

r=me after lang FCP
Crater results look reasonable to me, only 1 crate broken, which is already fixed by @mejrs (thanks!) in Martan03/termint#13

View changes since this review

[scottmcm]

Always happy to go straight to error for things that are clearly bugs that weren't supposed to work, so glad crater said we could :)

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.