-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix the features
macro.
#1680
Merged
Merged
Fix the features
macro.
#1680
+1
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks. |
You can see the problem in action on the rustc CI at rust-lang/rust#124141 (comment) |
Urgau
approved these changes
Nov 25, 2024
CI should be fixed now, can you rebase? |
The first rule of the `features` macro looks like this: ``` macro_rules! features { ( @target: $target:ident; @cfg: $cfg:meta; @MACRO_NAME: $macro_name:ident; @MACRO_ATTRS: $(#[$macro_attrs:meta])* $(@BIND_FEATURE_NAME: $bind_feature:tt; $feature_impl:tt; $(#[$deprecate_attr:meta];)?)* $(@NO_RUNTIME_DETECTION: $nort_feature:tt; )* $(@feature: #[$stability_attr:meta] $feature:ident: $feature_lit:tt; $(without cfg check: $feature_cfg_check:literal;)? $(implied by target_features: [$($target_feature_lit:tt),*];)? $(#[$feature_comment:meta])*)* ) => { ``` Notice all the `tt` specifiers. They are used because they are forwarded to another macro. Only `ident`, `lifetime`, and `tt` specifiers can be forwarded this way. But there is an exception: `$feature_lit:tt`, which was added recently. In theory it should cause an error like this: ``` error: no rules expected `literal` metavariable --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91 | 51 | / macro_rules! $macro_name { 52 | | $( 53 | | ($feature_lit) => { 54 | | $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? ... | | ^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call ... | 88 | | }; 89 | | } | |_________- in this expansion of `is_x86_feature_detected!` | ::: std/tests/run-time-detect.rs:145:27 | 145 | println!("tsc: {:?}", is_x86_feature_detected!("tsc")); | ------------------------------- in this macro invocation | note: while trying to match keyword `true` --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:12:55 | 12 | ($feature:tt, $feature_lit:tt, without cfg check: true) => { | ^^^^ = note: captured metavariables except for `:tt`, `:ident` and `:lifetime` cannot be compared to other tokens = note: see <https://doc.rust-lang.org/nightly/reference/macros-by-example.html#forwarding-a-matched-fragment> for more information ``` (The URL at the end of the error has more details about this forwarding limitation.) In practice it doesn't cause this error. I'm not sure why, but the existing macro implementation in rustc is far from perfect, so it's believable that it does the wrong thing here. Why does this matter? Because rust-lang/rust#124141 is modifying the macro implementation, and when that PR is applied the error *does* occur. (It's one of several cases I have found where the existing compiler accepts code it shouldn't, but #124141 causes that code to be rejected.) Fortunately the fix is simple: replace the `literal` specifier with `tt`.
nnethercote
force-pushed
the
fix-features-macro
branch
from
November 30, 2024 20:50
a1a9b95
to
5d5026b
Compare
I rebased. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first rule of the
features
macro looks like this:Notice all the
tt
specifiers. They are used because they are forwarded to another macro. Onlyident
,lifetime
, andtt
specifiers can be forwarded this way.But there is an exception:
$feature_lit:tt
, which was added recently. In theory it should cause an error like this:(The URL at the end of the error has more details about this forwarding limitation.)
In practice it doesn't cause this error. I'm not sure why, but the existing macro implementation in rustc is far from perfect, so it's believable that it does the wrong thing here.
Why does this matter? Because rust-lang/rust#124141 is modifying the macro implementation, and when that PR is applied the error does occur. (It's one of several cases I have found where the existing compiler accepts code it shouldn't, but #124141 causes that code to be rejected.)
Fortunately the fix is simple: replace the
literal
specifier withtt
.