-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Fix proc_macro::Ident
's handling of $crate
#141996
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
Conversation
It should be ok to have a method that creates an Technically, So when originally implementing validation for Perhaps it's ok to just admit now that |
Also, in the past |
This needs a library API team approval. |
cc @rust-lang/lang @rust-lang/wg-macros |
Cc @programmerjake who I know has some deep thoughts about |
Here is what I understand of this PR: This changes The identifier produced by macro_rules! receive_dollar_crate {
($dollar_crate:ident) => {
$dollar_crate::main
};
}
macro_rules! produce_dollar_crate {
() => {
receive_dollar_crate!($crate)
};
}
fn main() {
let _ = produce_dollar_crate!();
} When observed within the same crate (i.e. analogous in the macro_rules case to When observed within a different crate (i.e. This API exposes 3 different usage patterns which were not previously expressible. Please consider adding tests.
|
I don't see any sense in this PR. What is really needed is ability to refer to other crates. And this is exactly what dtolnay proposed in his linked comment: #54363 (comment) |
Minor nit, I will certainly add some tests to try to cover the new usage patterns, I just want to be clear that all of those usages can already be done today with // crate A
pub struct TA;
B::example_b!();
// crate B
pub struct TB;
#[macro_export]
macro_rules! example_b {
() => {
C::example_c!($crate)
};
}
// crate C
fn with_span(i: &Ident, span: Span) -> Ident {
let mut i = i.clone();
i.set_span(span);
i
}
#[proc_macro]
pub fn example_c(input: TokenStream) -> TokenStream {
let Some(TokenTree:Ident(krate_b)) = input.into_iter().next() else { panic!() };
let krate_c = with_span(&krate_b, Span::mixed_site());
let krate_a = with_span(&krate_b, Span::call_site());
quote! {
use #krate_a::TA as _;
use #krate_b::TB as _;
use #krate_c::example_c as _; // can't export a TC from a proc-macro
}
} This PR is mostly to fix the round-trip |
@rust-lang/libs-api: Stabilization notes: #141996 (comment), with corrections in #141996 (comment). |
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members: 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! See this document for info about what commands tagged team members can give me. |
3276565
to
0cc4e79
Compare
This comment has been minimized.
This comment has been minimized.
0cc4e79
to
6d1e93d
Compare
Expanded the tests to cover all combinations of passing |
@rustbot labels +I-lang-nominated +P-lang-drag-1 This seems reasonable to me, and thanks in particular to @dtolnay for the careful analysis. Since it is adding a new capability to the language, and it's rather tied in with the various details of the expansion behavior and hygiene, despite the delegation we've made here, whose exact bounds I forget, I'd just like to be sure people on lang see this, so let's nominate. cc @ehuss |
@dtolnay I don't object to this feature, it seems to be doing the logical thing, but I suspect it will be less useful for proc-macros than it is in But with proc-macros, at least the way I do it (maybe there's a better way), I usually have the proc macros in a crate of their own and then have them re-exported (e.g., |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@rustbot label -S-waiting-on-fcp |
Is anything else required in order to merge? |
It needs a reviewer OK. @rustbot ready |
@bors r+ |
…=petrochenkov Fix `proc_macro::Ident`'s handling of `$crate` This PR is addresses a few minor bugs, all relating to `proc_macro::Ident`'s support for `$crate`. `Ident` currently supports `$crate` (as can be seen in the `mixed-site-span` test), but: * `proc_macro::Symbol::can_be_raw` is out of sync with `rustc_span::Symbol::can_be_raw` * former doesn't cover `$crate` while the latter does cover `kw::DollarCrate` * `Ident::new` rejects `$crate` * This conflicts with the [reference definition](https://doc.rust-lang.org/nightly/reference/macros-by-example.html#r-macro.decl.meta.specifier) of `ident` which includes `$crate`. * This also conflicts with the documentation on [`Display for Ident`](https://doc.rust-lang.org/proc_macro/struct.Ident.html#impl-Display-for-Ident) which says the output "should be losslessly convertible back into the same identifier". This PR fixes the above issues and extends the `mixed-site-span` test to exercise these fixed code paths, as well as validating the different possible spans resolve `$crate` as expected (for both the new and old `$crate` construction code paths).
Rollup of 9 pull requests Successful merges: - #141996 (Fix `proc_macro::Ident`'s handling of `$crate`) - #142911 (Remove support for dynamic allocas) - #142950 (mbe: Rework diagnostics for metavariable expressions) - #143270 (tests/codegen/enum/enum-match.rs: accept negative range attribute) - #143298 (`tests/ui`: A New Order [23/N]) - #143398 (tidy: add support for `--extra-checks=auto:` feature) - #143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations) - #143644 (Add triagebot stdarch mention ping) - #143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 9 pull requests Successful merges: - #141996 (Fix `proc_macro::Ident`'s handling of `$crate`) - #142950 (mbe: Rework diagnostics for metavariable expressions) - #143011 (Make lint `ambiguous_glob_imports` deny-by-default and report-in-deps) - #143265 (Mention as_chunks in the docs for chunks) - #143270 (tests/codegen/enum/enum-match.rs: accept negative range attribute) - #143298 (`tests/ui`: A New Order [23/N]) - #143396 (Move NaN tests to floats/mod.rs) - #143398 (tidy: add support for `--extra-checks=auto:` feature) - #143644 (Add triagebot stdarch mention ping) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #141996 - Daniel-Aaron-Bloom:dollar_crate, r=petrochenkov Fix `proc_macro::Ident`'s handling of `$crate` This PR is addresses a few minor bugs, all relating to `proc_macro::Ident`'s support for `$crate`. `Ident` currently supports `$crate` (as can be seen in the `mixed-site-span` test), but: * `proc_macro::Symbol::can_be_raw` is out of sync with `rustc_span::Symbol::can_be_raw` * former doesn't cover `$crate` while the latter does cover `kw::DollarCrate` * `Ident::new` rejects `$crate` * This conflicts with the [reference definition](https://doc.rust-lang.org/nightly/reference/macros-by-example.html#r-macro.decl.meta.specifier) of `ident` which includes `$crate`. * This also conflicts with the documentation on [`Display for Ident`](https://doc.rust-lang.org/proc_macro/struct.Ident.html#impl-Display-for-Ident) which says the output "should be losslessly convertible back into the same identifier". This PR fixes the above issues and extends the `mixed-site-span` test to exercise these fixed code paths, as well as validating the different possible spans resolve `$crate` as expected (for both the new and old `$crate` construction code paths).
…=petrochenkov Fix `proc_macro::Ident`'s handling of `$crate` This PR is addresses a few minor bugs, all relating to `proc_macro::Ident`'s support for `$crate`. `Ident` currently supports `$crate` (as can be seen in the `mixed-site-span` test), but: * `proc_macro::Symbol::can_be_raw` is out of sync with `rustc_span::Symbol::can_be_raw` * former doesn't cover `$crate` while the latter does cover `kw::DollarCrate` * `Ident::new` rejects `$crate` * This conflicts with the [reference definition](https://doc.rust-lang.org/nightly/reference/macros-by-example.html#r-macro.decl.meta.specifier) of `ident` which includes `$crate`. * This also conflicts with the documentation on [`Display for Ident`](https://doc.rust-lang.org/proc_macro/struct.Ident.html#impl-Display-for-Ident) which says the output "should be losslessly convertible back into the same identifier". This PR fixes the above issues and extends the `mixed-site-span` test to exercise these fixed code paths, as well as validating the different possible spans resolve `$crate` as expected (for both the new and old `$crate` construction code paths).
Rollup of 9 pull requests Successful merges: - rust-lang#141996 (Fix `proc_macro::Ident`'s handling of `$crate`) - rust-lang#142950 (mbe: Rework diagnostics for metavariable expressions) - rust-lang#143011 (Make lint `ambiguous_glob_imports` deny-by-default and report-in-deps) - rust-lang#143265 (Mention as_chunks in the docs for chunks) - rust-lang#143270 (tests/codegen/enum/enum-match.rs: accept negative range attribute) - rust-lang#143298 (`tests/ui`: A New Order [23/N]) - rust-lang#143396 (Move NaN tests to floats/mod.rs) - rust-lang#143398 (tidy: add support for `--extra-checks=auto:` feature) - rust-lang#143644 (Add triagebot stdarch mention ping) r? `@ghost` `@rustbot` modify labels: rollup
This PR is addresses a few minor bugs, all relating to
proc_macro::Ident
's support for$crate
.Ident
currently supports$crate
(as can be seen in themixed-site-span
test), but:proc_macro::Symbol::can_be_raw
is out of sync withrustc_span::Symbol::can_be_raw
$crate
while the latter does coverkw::DollarCrate
Ident::new
rejects$crate
ident
which includes$crate
.Display for Ident
which says the output "should be losslessly convertible back into the same identifier".This PR fixes the above issues and extends the
mixed-site-span
test to exercise these fixed code paths, as well as validating the different possible spans resolve$crate
as expected (for both the new and old$crate
construction code paths).