Skip to content
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

$$crate unexpectedly fails to defer which crate $crate refers to #99035

Open
CAD97 opened this issue Jul 8, 2022 · 14 comments
Open

$$crate unexpectedly fails to defer which crate $crate refers to #99035

CAD97 opened this issue Jul 8, 2022 · 14 comments
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@CAD97
Copy link
Contributor

CAD97 commented Jul 8, 2022

$$ was stabilized in #95860 for 1.63. (reverted in #99435; now requires feature gate again.)

I tried this code:

// lib.rs
pub const WHICH: &str = "lib.rs";

#[macro_export]
macro_rules! define_which_macro {
    () => {
        macro_rules! which {
            () => {
                $$crate::WHICH
            };
        }
    };
}

// main.rs
use lib::define_which_macro;

pub const WHICH: &str = "main.rs";

define_which_macro!();

fn main() {
    dbg!(which!());
}

I expected to see this happen:

My intuitive expectation comes from expanding the macros as following:

define_which_macro!();
fn main() {
    dbg!(which!());
}
// =====>
macro_rules! which {
    () => {
        $crate::WHICH
    }
}
fn main() {
    dbg!(which!());
}
// =====>
fn main() {
    dbg!(crate::WHICH);
}

lib provides a macro whose expansion is specified to contain $$ crate; this seemingly should behave as writing $crate in the crate where the macro is expanded; i.e., main.rs should be printed.

Instead, this happened:

lib.rs is printed: the $crate in the expansion of which!() refers to lib.

This likely happens because the $crate compound identifier token refers to the crate indicated by the crate token's span, as indicated by the following example printing main.rs:

// lib.rs
#[macro_export]
macro_rules! define_which_macro {
    ($krate:tt) => {
        macro_rules! which {
            () => {
                $$$krate::WHICH
            };
        }
    };
}

// main.rs
define_which_macro!(crate);
fn main() {
    dbg!(which!());
}

and the following printing lib.rs:

// lib.rs
#[macro_export]
macro_rules! define_which_macro {
    ($d:tt) => {
        macro_rules! which {
            () => {
                $d crate::WHICH
            };
        }
    };
}

// main.rs
define_which_macro!($);
fn main() {
    dbg!(which!());
}

Desired behavior

This seems difficult to resolve. The stated purpose of $$ is to make writing macro-expanded-macros significantly more tractable than the previous [$dollar:tt] pattern defining a manual binder expanding to $. As such, it's fairly clearly the intent that writing $$crate should allow you to expand to a macro using $crate to refer to its defining crate, not the root macro's crate. (If you want the root macro's crate, you can already use just $crate just fine.)

Even though $$ is unstable, though, refining this behavior might be edge-case breaking, as you could already observe the current behavior of $crate's crate choice by using a manual $dollar:tt binder to get a $ into the expanded output.

I have three concepts for how to make $$crate work as is likely intended:

$crate uses the $ token's crate

... and $$'s expanded $'s crate gets set to the crate which expanded it, not the crate of either input $.1

This changes the behavior of $dollar $krate to refer to $dollar's crate rather than $krate's crate.

$crate uses the "gluing" crate

When the $crate token is glued into a single ident token, it gets its crate set to the crate def which did the gluing.

This changes the behavior of the $dollar $krate to refer to the gluing crate rather than $krate's crate.

Wait, which is the "gluing" crate then?

There's two options here:

  • The crate for the macro_rules! name's token. This means that using a known name like in the original example would make $$crate behave the same as $crate, but would make most macro-defining-macros which take the name of the macro to define (or glue based on an input token using paste!, which respects the span of the first used binder) work as expected and produce a $crate referring to the crate which provided the name of the newly-defined macro. This also gives an easily predictable answer to what crate is referred to when multiple levels of macro indirection are involved.
  • The crate the macro name is registered into the namespace of. It is the author's opinion that this is the most useful option, as any other macro involved can insert a normal $crate token to refer to their namespace, and cannot refer to the crate any defined items end up in unless using a properly escaped $$crate refers to that crate.

Really kludge it just for $$

$$ expands into a token that behaves like $ except for the fact that $crate behaves like one of the previous two solutions when using a $$-created $.

This avoids changing the behavior of $dollar $krate, but makes $dollar $krate behave differently from $$ $krate.

Meta

rustc --version --verbose:

rustc 1.64.0-nightly (27eb6d701 2022-07-04)
binary: rustc
commit-hash: 27eb6d7018e397cf98d51c205e3576951d766323
commit-date: 2022-07-04
host: x86_64-pc-windows-msvc
release: 1.64.0-nightly
LLVM version: 14.0.6

Footnotes

  1. I have no idea what span the expanded $ gets currently. The "correct" choice is probably to take the span of the second input $ as-is.

@CAD97 CAD97 added the C-bug Category: This is a bug. label Jul 8, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Jul 8, 2022

$crate is taught as

Within a macro imported from a crate named foo, the special macro variable $crate will expand to ::foo. [old 1.5 edition of The Book]

Hygiene is also the reason that we need the $crate metavariable when our macro needs access to other items in the defining crate. What this special metavariable does is that it expands to an absolute path to the defining crate. [The Little Book of Rust Macros]

Both of these are subtly wrong: $crate "expands" (really: is glued into) a single identifier, not a (partial) path, and this is observable by calling macros in a macro expansion. However, they agree that $crate is semantically a "reserved binder" which expands to the crate that the containing macro_rules! is defined in.

What crate the macro is defined in is somewhat ambiguous for a macro expanded macro, but treating $crate more like a reserved binder and less like a written token (resolution option: $crate uses the "gluing" crate) would be more consistent with how $crate is conceptualized and taught today.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 8, 2022

Related: #99037 is another issue resulting from $crate's treatment as a glued token rather than a reserved metevariable/binder.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.63.0 milestone Jul 8, 2022
@Mark-Simulacrum Mark-Simulacrum added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 8, 2022
@Mark-Simulacrum
Copy link
Member

Nominating for T-lang to make a call on whether we should do anything here before 1.63 releases, since there's a relatively short time window there.

@joshtriplett
Copy link
Member

@CAD97 We discussed this in today's @rust-lang/lang meeting. We do agree that the behavior is surprising, and it'd be worth reverting so we have time to determine the right behavior.

@Mark-Simulacrum Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 12, 2022
@joshtriplett
Copy link
Member

Speaking for myself only: I think for the macros-writing-macros case, it makes more sense to me that $$crate should turn into $crate at the site of expansion, so whatever macro invokes that macro should be the one whose $crate is used.

@c410-f3r
Copy link
Contributor

@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jul 12, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 12, 2022
@Mark-Simulacrum
Copy link
Member

Going to tag as a regression so it gets tracked appropriately as something that needs work before the next release.

@steffahn
Copy link
Member

steffahn commented Jul 12, 2022

@CAD97 In the OP

and the following printing main.rs:

Is this supposed to say “and the following printing libs.rs”? (I haven’t tested the behavior, but you appear to be trying to describe two code examples with different behavior.)

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 12, 2022

@steffahn oops, yes, you're correct. (I'll triple check when I get a chance but yes, the current behavior is that the $crate glued ident uses the span of the crate token.) (EDIT: got a chance: confirmed that the OP is now correct in saying the second example prints lib.rs.)

@apiraino
Copy link
Contributor

apiraino commented Jul 13, 2022

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium T-compiler

@rustbot rustbot added P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 13, 2022
@steffahn
Copy link
Member

steffahn commented Jul 13, 2022

Regarding the idea of fixing this with something like #99193...

This is not actually a regression, right? It's undesired behavior in a new feature.

I don't fully understand how this is sufficient reason for a beta backport of - essentially - changes to the feature; IMO the proper approach would be to revert stabilization of $$ entirely and apply the desired changes on nightly for release in no-earlier-than 1.64 (even if those changes are only to restrict $$'s interaction with $crate in some way). Doing design work (even if it's the design of new restrictions on a to-be-stabilized feature) always has the downside that there's less time left for testing the new behavior.

I don't know if there's an official source of what is or isn't “allowed” in a beta backport, so perhaps my opinion here is irrelevant, and this approach is actually appropriate.

@Mark-Simulacrum
Copy link
Member

FWIW, I tend to agree that the preferred approach would be to back out the stabilization on beta; we can separately land #99193 for future versions (1.64+).

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 18, 2022

Relevant:

let mark = if ident.name == kw::DollarCrate {
// When resolving `$crate` from a `macro_rules!` invoked in a `macro`,
// we don't want to pretend that the `macro_rules!` definition is in the `macro`
// 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.
ctxt = ctxt.normalize_to_macro_rules();

This suggests that the current crate-assignment behavior of $crate is known not to work correctly for $$crate. Unfortunately, the comment is slightly incorrect in saying that such configurations are not stably accessible, as $d crate can hit the case that it doesn't handle correctly (and that is hit more easily with $$crate).

I think fixing this ($crate refers to the local crate of the expansion which glued it) may be as simple as the following diff I just wrote:

diff --git a/compiler/rustc_expand/src/mbe/quoted.rs b/compiler/rustc_expand/src/mbe/quoted.rs
index d4b8563a036..d7049eb28a5 100644
--- a/compiler/rustc_expand/src/mbe/quoted.rs
+++ b/compiler/rustc_expand/src/mbe/quoted.rs
@@ -9,7 +9,7 @@
 use rustc_span::symbol::{kw, sym, Ident};
 
 use rustc_span::edition::Edition;
-use rustc_span::{Span, SyntaxContext};
+use rustc_span::{ExpnId, Span, SyntaxContext};
 
 const VALID_FRAGMENT_NAMES_MSG: &str = "valid fragment specifiers are \
                                         `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `lifetime`, \
@@ -221,6 +221,12 @@ fn parse_tree(
                     let (ident, is_raw) = token.ident().unwrap();
                     let span = ident.span.with_lo(span.lo());
                     if ident.name == kw::Crate && !is_raw {
+                        // If we use `ident.span` here, $crate will refer to the crate where
+                        // the crate token appears; we want it to refer to the crate which
+                        // is actually defining this macro (the current local crate). This is
+                        // most easily accomplished with a macro expanded macro_rules! which
+                        // uses `$$ crate` to get a `$crate` in the expanded macro definition.
+                        let span = span.with_call_site_ctxt(ExpnId::root());
                         TokenTree::token(token::Ident(kw::DollarCrate, is_raw), span)
                     } else {
                         TokenTree::MetaVar(span, ident)

but of course this doesn't work as-is (this apply_mark is directly forbidden), I'll need to dig into this further.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 19, 2022
…=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`
ehuss pushed a commit to ehuss/rust that referenced this issue Jul 22, 2022
…=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
Copy link
Member

Untagging as regression from 1.63, as we reverted in #99435 (and that PR was backported).

@Mark-Simulacrum Mark-Simulacrum removed this from the 1.63.0 milestone Aug 7, 2022
@Mark-Simulacrum Mark-Simulacrum removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Aug 7, 2022
cedricschwyter added a commit to KyrillGobber/huehuehue that referenced this issue Jul 1, 2023
refactor:higher-order http method macro

we want to switch to rust nightly to be able to make use of rust
metavariable expansions as defined by RFC rust-lang/rfcs#3086 and
as tracked by rust-lang/rust#83527. other references include
rust-lang/rust#99035.

this feature was stabilized in 1.63, then unstabilized again in
rust-lang/rust#99435 and is now only available in rust nightly, awaiting
restabilization. however, the feature is stable enough for our use case,
which is why i'm going ahead and enabling it.
cedricschwyter added a commit to KyrillGobber/huehuehue that referenced this issue Jul 1, 2023
refactor:higher-order http method macro

we want to switch to rust nightly to be able to make use of rust
metavariable expansions as defined by RFC rust-lang/rfcs#3086 and
as tracked by rust-lang/rust#83527. other references include
rust-lang/rust#99035.

this feature was stabilized in 1.63, then unstabilized again in
rust-lang/rust#99435 and is now only available in rust nightly, awaiting
restabilization. however, the feature is stable enough for our use case,
which is why i'm going ahead and enabling it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants