Resolve $crate at the expansion-local crate #99445

CAD97 · 2022-07-19T03:36:50Z

Fixes #99035. The glued $crate token now gets the resolution context of the local crate when gluing $crate into a single token. This is almost certainly what is intended by a macro author.

This does change the behavior of stable code. Specifically, previously $crate would resolve to the crate of the crate token, and if a macro uses a tt binder that expands to $, it is possible for it to replicate the behavior made much more accessible by $$crate.

However, this behavior really isn't all that desirable. If a crate token is passed in, it already resolves to its local crate, so turning it into a $crate token is a resolution no-op. If a macro_rules! definition wants a def-site crate, it can just write $crate. The current behavior of $crate becoming a def-site version of the crate token is not useful; $crate resolving to the gluing crate is.

Except... it looks like this is allowing proc-macro-hacked proc-macros to use $crate to refer to the declaration crate...

rust-highfive · 2022-07-19T03:36:53Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

CAD97 · 2022-07-19T03:38:35Z

Note: #99035 is marked as a regression because $$crate "regressed" from not working to problematic semantics. The revert is the proper fix to the "regression;" this fixes the underlying issue in a way that should allow the feature to re-stabilize.

CAD97 · 2022-07-19T03:56:33Z

That's odd that I didn't run into this error locally. I'm worried that this means something is relying on $crate resolving to crate's def-site rather than the macro def-site...

CAD97 · 2022-07-19T05:04:37Z

Okay, I was able to reproduce locally, so I'll at least be able to take a look into this.

CAD97 · 2022-07-19T06:22:15Z

I was able to confirm that something interesting is going on here: #[proc_macro_hack] allows the proc macro's expansion to use $crate, and unic-langid's langid! is using that.

I'm not exactly sure how this ever worked in the first place tbqh. Some part of the proc-macro-hack is giving the emitted tokens a span in the declaration crate. With this change, as the proc_macro_call!() is generated in the caller's crate, the $crate is glued there and refers to the caller's crate, not the declaration crate.

macro tracing

#![feature(trace_macros)]

trace_macros!(true);

fn main() {
    let _ = unic_langid::langid!("en-us");
}

With current nightly:

❯ cargo +nightly expand
    Checking playground v0.0.0 (D:\git\cad97\playground)
warning: some trace filter directives would enable traces that are disabled statically
 | `rustc_expand::mbe::quoted=debug` would enable the DEBUG level for the `rustc_expand::mbe::quoted` target
 = note: the static max level is `info`
 = help: to enable DEBUG logging, remove the `max_level_info` feature
note: trace_macro
 --> src\main.rs:6:13
  |
6 |     let _ = unic_langid::langid!("en-us");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: expanding `langid! { "en-us" }`
  = note: to `{
              #[derive($crate :: _proc_macro_hack_langid)] #[allow(dead_code)] enum
              ProcMacroHack { Value = (stringify! { "en-us" }, 0).1, } proc_macro_call!
              ()
          }`
  = note: expanding `proc_macro_call! {  }`
  = note: to `unsafe
          {
              $crate :: LanguageIdentifier ::
              from_raw_parts_unchecked(unsafe
              { $crate :: subtags :: Language :: from_raw_unchecked(28261u64) }, None,
              Some(unsafe
              { $crate :: subtags :: Region :: from_raw_unchecked(21333u32) }), None)
          }`
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s

#![feature(prelude_import)]
#![feature(trace_macros)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
fn main() {
    let _ = {
        #[allow(dead_code)]
        enum ProcMacroHack {
            Value = ("\"en-us\"", 0).1,
        }
        unsafe {
            ::unic_langid_macros::LanguageIdentifier::from_raw_parts_unchecked(
                unsafe { ::unic_langid_macros::subtags::Language::from_raw_unchecked(28261u64) },
                None,
                Some(unsafe {
                    ::unic_langid_macros::subtags::Region::from_raw_unchecked(21333u32)
                }),
                None,
            )
        }
    };
}

With this patch:

❯ cargo +stage1 expand
    Checking playground v0.0.0 (D:\git\cad97\playground)
DEBUG rustc_expand::mbe::quoted $crate changed its target crate from DefId(19:0) to DefId(0:0)
note: trace_macro
 --> src\main.rs:6:13
  |
6 |     let _ = unic_langid::langid!("en-us");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: expanding `langid! { "en-us" }`
  = note: to `{
              #[derive($crate :: _proc_macro_hack_langid)] #[allow(dead_code)] enum
              ProcMacroHack { Value = (stringify! { "en-us" }, 0).1, } proc_macro_call!
              ()
          }`
  = note: expanding `proc_macro_call! {  }`
  = note: to `unsafe
          {
              $crate :: LanguageIdentifier ::
              from_raw_parts_unchecked(unsafe
              { $crate :: subtags :: Language :: from_raw_unchecked(28261u64) }, None,
              Some(unsafe
              { $crate :: subtags :: Region :: from_raw_unchecked(21333u32) }), None)
          }`
error[E0433]: failed to resolve: could not find `LanguageIdentifier` in the crate root
 --> src\main.rs:6:13
  |
6 |     let _ = unic_langid::langid!("en-us");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `$crate`
  |
  = note: this error originates in the macro `proc_macro_call` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
  |
3 | use unic_langid::LanguageIdentifier;
  |
help: if you import `LanguageIdentifier`, refer to it directly
 --> D:\.rust\cargo\registry\src\github.com-1ecc6299db9ec823\unic-langid-macros-0.9.0\src\lib.rs:4:1
4 - #[proc_macro_hack]
4 + #[proc_macro_hack]
  |
error[E0433]: failed to resolve: unresolved import
 --> src\main.rs:6:13
  |
6 |     let _ = unic_langid::langid!("en-us");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `$crate::subtags`
  |
  = note: this error originates in the macro `proc_macro_call` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
  |
3 | use unic_langid::subtags::Language;
  |
help: if you import `Language`, refer to it directly
 --> D:\.rust\cargo\registry\src\github.com-1ecc6299db9ec823\unic-langid-macros-0.9.0\src\lib.rs:4:1
4 - #[proc_macro_hack]
4 + #[proc_macro_hack]
  |
error[E0433]: failed to resolve: unresolved import
 --> src\main.rs:6:13
  |
6 |     let _ = unic_langid::langid!("en-us");
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `$crate::subtags`
  |
  = note: this error originates in the macro `proc_macro_call` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
  |
3 | use unic_langid::subtags::Region;
  |
help: if you import `Region`, refer to it directly
 --> D:\.rust\cargo\registry\src\github.com-1ecc6299db9ec823\unic-langid-macros-0.9.0\src\lib.rs:4:1
4 - #[proc_macro_hack]
4 + #[proc_macro_hack]
  |
For more information about this error, try `rustc --explain E0433`.

#![feature(prelude_import)]
#![feature(trace_macros)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
fn main() {
    let _ = {
        #[allow(dead_code)]
        enum ProcMacroHack {
            Value = ("\"en-us\"", 0).1,
        }
        unsafe {
            crate::LanguageIdentifier::from_raw_parts_unchecked(
                unsafe { crate::subtags::Language::from_raw_unchecked(28261u64) },
                None,
                Some(unsafe { crate::subtags::Region::from_raw_unchecked(21333u32) }),
                None,
            )
        }
    };
}

estebank

This looks good to me, but I'd feel more comfortable if @petrochenkov took a look at it too.

CAD97 · 2022-07-19T20:01:09Z

compiler/rustc_resolve/src/lib.rs

-            // FIXME: This is only a guess and it doesn't work correctly for `macro_rules!`
-            // definitions actually produced by `macro` and `macro` definitions produced by
-            // `macro_rules!`, but at least such configurations are not stable yet.
-            ctxt = ctxt.normalize_to_macro_rules();


Note: I removed this because it didn't change the behavior of any test (in the default compiler mode x.py test), not because removing it was necessary.

In general, I'm not certain how much of this arm is still necessary now that we're directly giving $crate a new mark when gluing it but at the very least removing the branch entirely didn't seem to work.

CAD97 · 2022-07-19T20:02:56Z

compiler/rustc_expand/src/mbe/quoted.rs

+                        let mut expn = span.ctxt().outer_expn_data();
+                        expn.parent = ExpnId::root();
+                        let span =
+                            span.with_call_site_ctxt(LocalExpnId::fresh(expn, ctx).to_expn_id());


Note to self: push the local change you made while testing to use Span::fresh_expansion rather than with_call_site_ctxt

rust-log-analyzer · 2022-07-19T20:29:58Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling rustc_macros v0.1.0 (/checkout/compiler/rustc_macros)
error[E0433]: failed to resolve: could not find `LanguageIdentifier` in the crate root
  --> compiler/rustc_macros/src/diagnostics/fluent.rs:91:45
   |
91 |     let mut bundle = FluentBundle::new(vec![langid!("en-US")]);
   |                                             ^^^^^^^^^^^^^^^^ not found in `$crate`
   |
   = note: this error originates in the macro `proc_macro_call` which comes from the expansion of the macro `langid` (in Nightly builds, run with -Z macro-backtrace for more info)
   |
1  | use unic_langid::LanguageIdentifier;
   |
   |
help: if you import `LanguageIdentifier`, refer to it directly


4  - #[proc_macro_hack]
4  + #[proc_macro_hack]

error[E0433]: failed to resolve: unresolved import
  --> compiler/rustc_macros/src/diagnostics/fluent.rs:91:45
   |
   |
91 |     let mut bundle = FluentBundle::new(vec![langid!("en-US")]);
   |                                             ^^^^^^^^^^^^^^^^ not found in `$crate::subtags`
   |
   = note: this error originates in the macro `proc_macro_call` which comes from the expansion of the macro `langid` (in Nightly builds, run with -Z macro-backtrace for more info)
   |
   |
1  | use unic_langid::subtags::Language;
   |
help: if you import `Language`, refer to it directly


4  - #[proc_macro_hack]
4  + #[proc_macro_hack]

error[E0433]: failed to resolve: unresolved import
  --> compiler/rustc_macros/src/diagnostics/fluent.rs:91:45
   |
   |
91 |     let mut bundle = FluentBundle::new(vec![langid!("en-US")]);
   |                                             ^^^^^^^^^^^^^^^^ not found in `$crate::subtags`
   |
   = note: this error originates in the macro `proc_macro_call` which comes from the expansion of the macro `langid` (in Nightly builds, run with -Z macro-backtrace for more info)
   |
1  | use unic_langid::subtags::Region;
   |
   |
help: if you import `Region`, refer to it directly


4  - #[proc_macro_hack]
4  + #[proc_macro_hack]

   Compiling tracing-tree v0.2.0
   Compiling chalk-solve v0.80.0
   Compiling rustc_log v0.0.0 (/checkout/compiler/rustc_log)

cjgillot · 2022-07-20T19:43:15Z

compiler/rustc_resolve/src/lib.rs

-            // as described in `SyntaxContext::apply_mark`, so we ignore prepended opaque marks.
-            // FIXME: This is only a guess and it doesn't work correctly for `macro_rules!`
-            // definitions actually produced by `macro` and `macro` definitions produced by
-            // `macro_rules!`, but at least such configurations are not stable yet.


We may need to test the 3 combinations: macro_rules in macro_rules (existing), macro_rules inside macro, macro inside macro_rules.

estebank · 2022-07-20T19:55:34Z

r? @petrochenkov

petrochenkov · 2022-07-24T21:26:57Z

This is a tricky area and I need to look at this, but I won't be able to do it very soon, maybe in a week or two.

CAD97 · 2022-07-25T15:11:54Z

I just want to say that while (I think) this approach works as I desired, this isn't the "ideal" way to handle $crate. Namely, because since $crate is glued eagerly, it's turned into the $crate token in macro patterns as well.

The "perfect world semantics" of $crate would be as a reserved binder name. The token in the binder would just be a def-site crate.

pnkfelix · 2022-09-08T14:34:10Z

@CAD97 do you think we should hold off on looking at this until after the work on #99447 is finished?

petrochenkov · 2022-09-08T18:45:34Z

I need to refresh hygiene details in my memory before reviewing this PR and #100387, it will require at least one full working day.
I've been very busy the last couple of months so now I'm very tired and on vacation, I'll find the day some time after the return.

bors · 2022-10-04T09:52:48Z

☔ The latest upstream changes (presumably #102644) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2022-10-08T12:00:43Z

FWIW, I really dislike what this PR does.
#99035 implies that $$ has anything to do with changing hygiene, but it does not!

Imagine that you have the same situation with macros 2.0 and proper def-site crate:

macro outer {
    macro inner {
        crate::Struct // outer_crate::Struct
    }
}

What would you do to switch crate from outer's crate root to inner's crate root? There aren't any $s here.
The answer is you'd use a hygiene opt-out like rust-lang/rfcs#2498 or similar:

macro outer {
    macro inner {
        #crate::Struct //  inner_crate::Struct
    }
}

, and same with $crate

macro outer {
    macro inner {
        $#crate::Struct //  inner_crate::Struct
    }
}

Switching to call-site hygiene is a separate (not yet implemented) feature, it doesn't seem right to me to bolt it on $$.

petrochenkov · 2022-10-08T12:05:36Z

(I'm interested in someone pursuing #99447 (comment) tho.)

CAD97 · 2022-10-17T16:57:53Z

#99035 implies that $$ has anything to do with changing hygiene, but it does not!

So, I both agree and disagree with this interpretation. I agree that this PR's implementation is suboptimal, but I disagree that $crate should use the def site of the crate token.

My model is that $crate is an expansion metavariable the same as any other $binder. The difference is that $crate is a predefined binder, and the expansion of that binder is a def-site crate token for the macro which contains the $crate.

There is no "$$ impacts hygiene" involved here. The hygiene result is from "expanding" $crate. The difference is in pseudocode

impl MacroExpansionContext {
    fn expand(binder: Ident) -> TokenStream {
        if (binder == kw::crate) {
            #[cfg(current-behavior)] {
                Ident::new(kw::crate, Span::def_site_at(binder))
            }
            #[cfg(expander-model)] {
                Ident::new(kw::crate, Span::def_site_at(self))
            }
        } else {
            self.resolve_binding(binder)
        }
    }
}

JohnCSimon · 2023-06-17T14:13:41Z

@CAD97
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

CAD97 · 2023-09-03T20:25:38Z

This is pending deciding what the correct behavior is, as far as I'm aware. I opened a Zulip thread to discuss.

apiraino · 2023-09-14T12:56:29Z

Zulip thread opened for T-lang (I'll add the relevant label to remind me of that)

@rustbot label +t-lang

petrochenkov · 2024-01-24T12:16:19Z

Copypasting the status from Zulip thread:

I want to look at that PR again (and marked it with waiting on review), but it is time consuming and low priority, so no specific timeline.
I think my general stance is still to not touch it, and keep it as is, and instead pursue the hygienic crate, which is not a weird accidental amalgamation of two tokens.

CAD97 · 2024-02-15T01:29:01Z

I was writing a (probably overly long) post about my opinion/status here when I reached a conclusion I hadn't before: macro_rules! metavars do hygienic (def_site) name lookup. I guess I just never properly tested it, and [$]crate feels more like an item/path (which aren't hygienic) than a variable name lookup.

Thus, $crate (in expansion position) can be logically explained as a def_site lookup of a pre-defined, ambiently-available metavar. (The gluing in pattern position still makes no sense.) This makes me no longer actively dislike the current $crate semantics. In fact, despite still holding that it's not particularly intuitive nor useful for $$crate and $crate to double-expand identically, I actually agree that it's the correct behavior given that keywords aren't reserved names for macro binders.

As such, I'm going to close this PR since I now agree that this isn't the correct approach, at least not on its own. The ability to name the crate of a macro expanded macro definition is still desirable, but that can be filed into the same expressive gap as naming the defining module.

If we can at least make attempts to use $$crate warn, I'll be happy to remove my concern about and retry stabilization of $$. I'd prefer to make all $keyword metavars into span independent metavar information based on the containing macro definition site, but getting $$ is better than nothing (so long as current-semantics $$crate results in a warning).

Resolve $crate at the expansion-local crate

5998dfa

rust-highfive assigned estebank Jul 19, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Jul 19, 2022

estebank reviewed Jul 19, 2022

View reviewed changes

CAD97 commented Jul 19, 2022

View reviewed changes

tweak $crate glue slightly

1485d06

cjgillot reviewed Jul 20, 2022

View reviewed changes

rust-highfive unassigned estebank Jul 20, 2022

CAD97 mentioned this pull request Aug 3, 2022

Forbid $crate in macro patterns #99447

Closed

petrochenkov mentioned this pull request Sep 8, 2022

Check uniqueness of impl items by trait item when applicable. #100387

Merged

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2022

CAD97 mentioned this pull request Oct 17, 2022

Tracking Issue for RFC 3086: macro metavariable expressions #83527

Open

4 tasks

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2023

rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 14, 2023

CAD97 closed this Feb 15, 2024

apiraino removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve $crate at the expansion-local crate #99445

Resolve $crate at the expansion-local crate #99445

CAD97 commented Jul 19, 2022 •

edited

Loading

rust-highfive commented Jul 19, 2022

CAD97 commented Jul 19, 2022 •

edited

Loading

CAD97 commented Jul 19, 2022

This comment has been minimized.

CAD97 commented Jul 19, 2022

CAD97 commented Jul 19, 2022

estebank left a comment

CAD97 Jul 19, 2022

CAD97 Jul 19, 2022

rust-log-analyzer commented Jul 19, 2022

cjgillot Jul 20, 2022

estebank commented Jul 20, 2022

petrochenkov commented Jul 24, 2022

CAD97 commented Jul 25, 2022

pnkfelix commented Sep 8, 2022

petrochenkov commented Sep 8, 2022

bors commented Oct 4, 2022

petrochenkov commented Oct 8, 2022

petrochenkov commented Oct 8, 2022

CAD97 commented Oct 17, 2022

JohnCSimon commented Jun 17, 2023

CAD97 commented Sep 3, 2023

apiraino commented Sep 14, 2023

petrochenkov commented Jan 24, 2024

CAD97 commented Feb 15, 2024

Resolve $crate at the expansion-local crate #99445

Resolve $crate at the expansion-local crate #99445

Conversation

CAD97 commented Jul 19, 2022 • edited Loading

rust-highfive commented Jul 19, 2022

CAD97 commented Jul 19, 2022 • edited Loading

CAD97 commented Jul 19, 2022

This comment has been minimized.

CAD97 commented Jul 19, 2022

CAD97 commented Jul 19, 2022

estebank left a comment

Choose a reason for hiding this comment

CAD97 Jul 19, 2022

Choose a reason for hiding this comment

CAD97 Jul 19, 2022

Choose a reason for hiding this comment

rust-log-analyzer commented Jul 19, 2022

cjgillot Jul 20, 2022

Choose a reason for hiding this comment

estebank commented Jul 20, 2022

petrochenkov commented Jul 24, 2022

CAD97 commented Jul 25, 2022

pnkfelix commented Sep 8, 2022

petrochenkov commented Sep 8, 2022

bors commented Oct 4, 2022

petrochenkov commented Oct 8, 2022

petrochenkov commented Oct 8, 2022

CAD97 commented Oct 17, 2022

JohnCSimon commented Jun 17, 2023

CAD97 commented Sep 3, 2023

apiraino commented Sep 14, 2023

petrochenkov commented Jan 24, 2024

CAD97 commented Feb 15, 2024

CAD97 commented Jul 19, 2022 •

edited

Loading

CAD97 commented Jul 19, 2022 •

edited

Loading