-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate builtin attributes for macro args. #88680
Conversation
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
I'm not too happy with this change, as it seems to add some specific validation that would probably better be part of something more general. However, attribute validation happens long after expansion, and I'm not aware of any pre-expansion validation where this could live. |
We actually validate some built-in attributes during expansion, those that are applied to macro calls and disappear during expansion. macro_rules! m { () => () }
#[path] // error: malformed `path` attribute input
m!();
fn main() {} This happens in You can also reuse |
This seems fine. |
r? @petrochenkov is much better suited to review this. But the approach and idea seems reasonable enough to me as well. |
cadfab3
to
5f8c009
Compare
I'm not sure if you meant that it should check in I also fixed a minor issue with the suggested fix not checking if it was an inner attribute. |
Nah, I only used I liked the initial version of the PR better, TBH. |
The inner attribute fix and moving the |
5f8c009
to
3fffc1a
Compare
Sure, no problem. Unfortunately, that means adding a dependency from rustc_middle to rustc_parse, which does not seem good to me? |
Can |
Or |
…uggestion. Moving to a dedicated function in preparation for other validation. The suggestion given didn't consider if it was an inner attribute.
3fffc1a
to
75f058d
Compare
Sure, I updated it to do the check in |
@bors r+ |
📌 Commit 75f058d has been approved by |
⌛ Testing commit 75f058d with merge 065a2df26aa956cf9e6548f5f53b838794de7611... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors retry Error in |
☀️ Test successful - checks-actions |
Finished benchmarking commit (f6e6ddc): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
The following code used to compile up-to-and-including #[path = concat!(env!("OUT_DIR"), "/v1_22/mod.rs")]
mod v1_22; And starting with
Is this intended? What was the effect of the above path attribute before |
The effect before was that the Hm, so my expectation was that nobody would be using that, since the attribute is ignored and you would need a We could maybe downgrade it to a future-incompatibility warning if people feel this was too aggressive. |
@ehuss indeed, there is a Personally, I am fine with this change, just wanted to understand why it used to work. |
This adds some validation for
path
,crate_type
, andrecursion_limit
attributes so that they will now return an error if you attempt to pass a macro into them (such as#[path = foo!()]
). Previously, the attribute would be completely ignored. These attributes are special because their values need to be known before/during expansion.cc #87681