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

[tmandry]

We discussed this in the lang meeting today. Those present were happy to approve this, and also to go straight to a hard error, crater permitting.

@rfcbot fcp merge lang

[rust-rfcbot]

@tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Thanks @mejrs, @JonathanBrouwer.

@rfcbot reviewed

[mejrs]

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

[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.

[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