-
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
Stabilize 'attr_literals' feature. #53044
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Whoops! Forgot to update the UI output. One of the tests is failing for seemingly unrelated reasons:
This is the path that leads to this error: rust/src/librustc/traits/on_unimplemented.rs Lines 158 to 177 in c11f2d2
For some reason, the Taking a closer look now. Edit: Figured it out! Looks like the feature gate
Turns out nothing else in the compiler checks that attributes look like meta-items, so I've put this back in with an updated comment. |
e2fac40
to
cf009e5
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
cf009e5
to
f32504d
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
f32504d
to
949b29d
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Rather interestingly, this PR has uncovered several issues and inconsistencies in the codebase when it comes to dealing with attributes and determining whether arbitrary tokens are allowed in them. In particular, in #40346, attribute syntax was expanded to allow for arbitrary tokens as input. This change was not part of any RFC and was made without either a new RFC or a new feature gate. In #40346 (comment), @jseyfried notes that the change should only take affect when The recently stabilization of As such, I've decided to add a new feature, |
This is outdated information, #50120 restricted attribute grammar for proc macro (or rather modern proc macro) attributes. The "literals in attributes" RFC affects only non-macro attributes that are parsed as so called "meta-items" and have a separate very restricted grammar (ident + literal + list of idents/literals/lists). As I previously mentioned on the tracking issue, the whole situation is mess, but at least stabilization of arbitrary literals in addition to currently stable string literals will not make it worse, but simultaneous support for arbitrary tokens in meta-items certainly will. |
@petrochenkov I'm not sure what you're proposing, if anything. The situation is that there are tests that assume that arbitrary tokens are allowed and there are tests that check that arbitrary tokens are not allowed. The only way I can see to make both situations work is to have a feature flag that determines when attributes are checked to be meta-items. Are you proposing that there's another way? Edit: perhaps you're saying that I should change the check to be: if attr.from_proc_macro() {
assert_is_meta_item(&attr);
} That is, allow arbitrary tokens as inputs to non-proc-macro attributes but disallow them as inputs to proc-macro attributes. Is that right? |
r? @petrochenkov (I think you've got a better handle on this) |
@SergioBenitez
Ok, now I'm proposing to keep the patch as is, except for restoring the if self.context.features.use_extern_macros() && attr::is_known(attr) {
return
} clause. (I'm working on blockers for stabilization of |
949b29d
to
dbd157b
Compare
Alright, looks like the CI is happy! |
r=me after FCP in #34981 is complete. |
@SergioBenitez Now that FCP has completed, we can merge this once you have a chance to rebase! 😄 |
dbd157b
to
104bfd6
Compare
This comment has been minimized.
This comment has been minimized.
104bfd6
to
705ce69
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
705ce69
to
ed0bd38
Compare
@cramertj @petrochenkov All's ready! |
@bors r+ |
📌 Commit ed0bd38 has been approved by |
…nkov Stabilize 'attr_literals' feature. RFC Issue: rust-lang/rfcs#1559 Tracking Issue: #34981 Reference PR: rust-lang/reference#388.
☀️ Test successful - status-appveyor, status-travis |
RFC Issue: rust-lang/rfcs#1559
Tracking Issue: #34981
Reference PR: rust-lang/reference#388.