-
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
Forbid $$crate
#99193
Forbid $$crate
#99193
Conversation
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
Can we add a test that this is forbidden even through macro indirection? E.g. smth like macro dollar_dollar_crate($d:tt) {
$d $d crate
}
macro use_it() {
use dollar_dollar_crate!($$);
}
use_it!() Edit: also the same, but putting |
9e9b542
to
e2995e6
Compare
macro_rules! dollar_dollar_crate { | ||
($d:tt) => { | ||
const _: usize = $d$d crate::IDX; | ||
//~^ ERROR expected expression, found `$` | ||
}; | ||
} | ||
dollar_dollar_crate!($); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite the test I had in mind. Instead,
macro_rules! dollar_dollar_crate { | |
($d:tt) => { | |
const _: usize = $d$d crate::IDX; | |
//~^ ERROR expected expression, found `$` | |
}; | |
} | |
dollar_dollar_crate!($); | |
macro_rules! dollar_dollar_crate { | |
($d:tt) => { | |
const _: usize = $d$d crate::IDX; | |
//~^ ERROR expected expression, found `$` | |
}; | |
} | |
macro_rules! dollar_dollar_use { | |
($d:tt) => { | |
dollar_dollar_crate!($d); | |
} | |
} | |
dollar_dollar_use!($); |
The fact that the current error message is "expected expression, found $
" and not "$$crate is not allowed" makes me worried that this actually will work i.e. $$crate
is insufficiently gated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as requested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved? There is still no error on the indirect $$crate.
I think this PR is meant to reach stable, right? (i.e. the alternate implementation of @rustbot label relnotes T-lang |
Everything is a best effort to solve a problem. It is not clear if the feature will be removed from 1.63 or if this PR can be back-ported . EDIT: The feature will actually be reverted as described in #99035 (comment) |
e2995e6
to
f4f3913
Compare
Is the lang-team going to merge this PR? Perhaps some other strategy will be used? Perhaps try another compiler reviewer? Its been 6 days and it would be a shame to miss 1.64 for whatever reason. |
beta-nominating -- partially to get eyes on this from T-compiler (who should do the code review), and partially because we do need to beta backport either this PR or a full revert of the original PR. (Either is fine, but if it's the latter someone needs to do the legwork). |
Mechanical revert available at #99435 |
…=Mark-Simulacrum Revert "Stabilize $$ in Rust 1.63.0" This mechanically reverts commit 9edaa76, the one commit from rust-lang#95860. rust-lang#99035; the behavior of `$$crate` is potentially unexpected and not ready to be stabilized. rust-lang#99193 attempts to forbid `$$crate` without also destabilizing `$$` more generally. `@rustbot` modify labels +T-compiler +T-lang +P-medium +beta-nominated +relnotes (applying the labels I think are accurate from the issue and alternative partial revert) cc `@Mark-Simulacrum`
…=Mark-Simulacrum Revert "Stabilize $$ in Rust 1.63.0" This mechanically reverts commit 9edaa76, the one commit from rust-lang#95860. rust-lang#99035; the behavior of `$$crate` is potentially unexpected and not ready to be stabilized. rust-lang#99193 attempts to forbid `$$crate` without also destabilizing `$$` more generally. `@rustbot` modify labels +T-compiler +T-lang +P-medium +beta-nominated +relnotes (applying the labels I think are accurate from the issue and alternative partial revert) cc `@Mark-Simulacrum`
token.span, | ||
&format!("unexpected token: {}", pprust::token_to_string(token)) | ||
); | ||
sess.span_diagnostic.note_without_error("`$$crate` is not allowed in any context"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly span_err on this message? Should we point to the two $ tokens too?
macro_rules! dollar_dollar_crate { | ||
($d:tt) => { | ||
const _: usize = $d$d crate::IDX; | ||
//~^ ERROR expected expression, found `$` | ||
}; | ||
} | ||
dollar_dollar_crate!($); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved? There is still no error on the indirect $$crate.
Sorry, all the obstacles imposed by different actors in different fronts made me lost the motivation/energy to keep coding this PR. Closing. |
Fixes #99035 by simply denying the usage of
$$crate
. Future decisions about how$$crate
should behave (if it should) won't be a breaking change and I am out of a better idea.cc @rust-lang/lang (Responsible team)
cc @petrochenkov (Reviewed most of the implementation)
cc @CAD97 (Created the issue)
cc @Mark-Simulacrum (Nominated the issue)