rust icon indicating copy to clipboard operation
rust copied to clipboard

Resolve $crate at the expansion-local crate

Open CAD97 opened this issue 3 years ago • 16 comments

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...

CAD97 avatar Jul 19 '22 03:07 CAD97

r? @estebank

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

rust-highfive avatar Jul 19 '22 03:07 rust-highfive

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 avatar Jul 19 '22 03:07 CAD97

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 avatar Jul 19 '22 03:07 CAD97

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` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
   |
1  | use unic_langid::LanguageIdentifier;
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` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
   |
   |
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` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this struct
   |
1  | use unic_langid::subtags::Region;
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)

rust-log-analyzer avatar Jul 19 '22 04:07 rust-log-analyzer

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

CAD97 avatar Jul 19 '22 05:07 CAD97

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,
            )
        }
    };
}

CAD97 avatar Jul 19 '22 06:07 CAD97

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)

rust-log-analyzer avatar Jul 19 '22 20:07 rust-log-analyzer

r? @petrochenkov

estebank avatar Jul 20 '22 19:07 estebank

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.

petrochenkov avatar Jul 24 '22 21:07 petrochenkov

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.

CAD97 avatar Jul 25 '22 15:07 CAD97

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

pnkfelix avatar Sep 08 '22 14:09 pnkfelix

I need to refresh hygiene details in my memory before reviewing this PR and https://github.com/rust-lang/rust/pull/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.

petrochenkov avatar Sep 08 '22 18:09 petrochenkov

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

bors avatar Oct 04 '22 09:10 bors

FWIW, I really dislike what this PR does. https://github.com/rust-lang/rust/issues/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 https://github.com/rust-lang/rfcs/pull/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 avatar Oct 08 '22 12:10 petrochenkov

(I'm interested in someone pursuing https://github.com/rust-lang/rust/pull/99447#issuecomment-1204120473 tho.)

petrochenkov avatar Oct 08 '22 12:10 petrochenkov

https://github.com/rust-lang/rust/issues/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)
        }
    }
}

CAD97 avatar Oct 17 '22 16:10 CAD97

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

JohnCSimon avatar Jun 17 '23 14:06 JohnCSimon

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

CAD97 avatar Sep 03 '23 20:09 CAD97

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

@rustbot label +t-lang

apiraino avatar Sep 14 '23 12:09 apiraino

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.

petrochenkov avatar Jan 24 '24 12:01 petrochenkov

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).

CAD97 avatar Feb 15 '24 01:02 CAD97