Add a new concat metavar expr #118958

c410-f3r · 2023-12-15T00:18:44Z

Revival of #111930

Giving it another try now that #117050 was merged.

With the new rules, meta-variable expressions must be referenced with a dollar sign ($) and this can cause misunderstands with $concat.

macro_rules! foo {
    ( $bar:ident ) => {
        const ${concat(VAR, bar)}: i32 = 1;
    };
}

// Will produce `VARbar` instead of `VAR_123`
foo!(_123);

In other words, forgetting the dollar symbol can produce undesired outputs.

cc #29599

rustbot · 2023-12-15T00:18:53Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

compiler/rustc_expand/src/mbe/metavar_expr.rs

compiler/rustc_expand/src/mbe/transcribe.rs

tests/ui/macros/rfc-3086-metavar-expr/syntax-errors.rs

tgross35 · 2023-12-22T18:13:55Z

@rustbot label +A-macros

tgross35

Looks pretty good to me. I started some discussion in wg-macros https://rust-lang.zulipchat.com/#narrow/stream/404510-wg-macros/topic/concat_idents.20as.20metavar.20.24.7B.2E.2E.2E.7D.

I don't think you need to mention the RFC in the title/commit message since this specific function wasn't called out in the RFC.

compiler/rustc_expand/src/mbe/metavar_expr.rs

tgross35 · 2023-12-22T18:26:02Z

compiler/rustc_expand/src/mbe/metavar_expr.rs

+            "concat" => {
+                let lhs_is_var = try_eat_dollar(&mut iter);
+                let lhs_ident = parse_ident(&mut iter, sess, ident.span)?;
+                if !try_eat_comma(&mut iter) {


Do we even need commas? Referencing paste again, the syntax is

[<Q R S T>] // turns into QRST

Which is somewhat easier to read without the commas. (Similar to C Q ## R ## S ## T, though that definitely isn't easier to read)

The comma was added to follow the syntax of the other meta-variable expressions. For example: count($foo, 0).

In my personal opinion, the matter about case conversion and omission of commas should be discussed at a later opportunity.

WaffleLapkin · 2023-12-27T17:13:53Z

(please use @rustbot review once my review is needed, this is currently S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. )

tgross35 · 2024-01-05T06:30:06Z

@c410-f3r is this ready for review? I didn't intend for my comments to be blocking

c410-f3r · 2024-01-05T17:24:16Z

Yeap, it is ready for review

@rustbot labels -S-waiting-on-author +S-waiting-on-review

WaffleLapkin · 2024-01-12T11:36:22Z

tests/ui/macros/rfc-3086-metavar-expr/syntax-errors.stderr

+error: expected identifier, found `$`
+  --> $DIR/syntax-errors.rs:178:15
+   |
+LL |         const ${concat($sign name, _123)}: () = ();
+   |               ^ expected identifier
+...
+LL |     tt_that_is_dollar_sign_with_concat!($, FOO);
+   |     ------------------------------------------- in this macro invocation
+   |
+   = note: this error originates in the macro `tt_that_is_dollar_sign_with_concat` (in Nightly builds, run with -Z macro-backtrace for more info)


Is it possible to point to a better span? this looks like the problem is that const ${concat(...)} is not supported at all, but the actual problem seems to be $sign?..

This diagnostic isn't originated from parse_ident. Mabe something after expansion?

If a better span is desirable, then a separated investigation should be created.

parse_ident shouldn't take a span from the outside in general.
It should use span of the unexpected token in its errors.

IFAICT, this specific diagnostic wasn't emitted by parse_ident.

But regardless parse_ident was modified to use as much span from tokens as possible. The outside span is still being used in case the RefTokenTreeCursor iterator doesn't yield.

compiler/rustc_expand/src/mbe/metavar_expr.rs

compiler/rustc_expand/src/mbe/transcribe.rs

WaffleLapkin · 2024-01-12T12:11:55Z

compiler/rustc_expand/src/mbe/transcribe.rs

+            result.push(TokenTree::Token(
+                Token::from_ast_ident(Ident::new(Symbol::intern(&concatenated), visited_span())),
+                Spacing::Alone,
+            ));


Looking at the tests I still don't quite get how hygiene works...tests/ui/macros/rfc-3086-metavar-expr/concat-hygiene.rs implies that xy and ${concat($x, $y)} are different, but then tests/ui/macros/rfc-3086-metavar-expr/concat.rs uses things defined inside the macro outside of it...

Marker is the responsible entity that process hygiene for tokens emitted by meta-variable expressions.

Its internal aspects are not entirely clear to me but if you want to know how everything works I guess @petrochenkov can give you a better answer.

The current implementation marks the result as coming from the macro (using span of the metavar expression itself), regardless of contexts of the concatenated identifiers.
concat_idents!() macro works in a similar way, ignoring input contexts, but the output context is assigned in a different way.

What the desired behavior should be is a design question.

Are we supposed to use local variables produced by concat inside macro_rules (macros 1.0) outside of the macro? Not sure.

Are we supposed to use any names produced by concat inside macro (macros 2.0) outside of the macro?
Likely yes, so some kind of hygiene opt out, or support for inheriting context from some token, should be added, but that applies to many things produced by macro, not just concat.

tgross35 · 2024-03-21T00:25:05Z

It might not be a bad idea to put this under a separate feature gate from the rest of macro_metavar_expr since it is probably pretty unlikely to stabilize together. Not much of a difference at this point, just slightly less future work.

c410-f3r · 2024-04-17T17:03:21Z

r? compiler

compiler/rustc_expand/src/mbe/metavar_expr.rs

compiler/rustc_expand/src/mbe/transcribe.rs

Nadrieril · 2024-04-18T18:46:50Z

r? @tgross35 may I let you review this?

tgross35 · 2024-04-18T18:56:13Z

@Nadrieril I don't know enough about this area to make a call (also don't have r+), but this looks fine to me with a feature gate added. If this isn't your area either then I guess we can see who gets the hot potato next.

r? compiler

tgross35 · 2024-04-18T18:56:43Z

Reroll since Wesley passed off the review initially

r? compiler

nnethercote · 2024-04-19T03:04:06Z

I suspect r? @petrochenkov might be more suitable?

tests/ui/macros/rfc-3086-metavar-expr/concat-hygiene.rs

petrochenkov · 2024-04-19T13:44:22Z

tests/ui/macros/rfc-3086-metavar-expr/syntax-errors.rs

+
+macro_rules! tt_that_is_dollar_sign_with_concat {
+    ($sign:tt, $name:ident) => {
+        const ${concat($sign name, _123)}: () = ();


This is not specific to concat, right?
Macros don't generally treat $dollar name as $name in any contexts.

Looks like this test is therefore unnecessary. Removed.

compiler/rustc_expand/src/mbe/metavar_expr.rs

petrochenkov · 2024-04-19T14:08:39Z

tests/ui/macros/rfc-3086-metavar-expr/syntax-errors.stderr

+error: expected identifier, found `$`
+  --> $DIR/syntax-errors.rs:178:15
+   |
+LL |         const ${concat($sign name, _123)}: () = ();
+   |               ^ expected identifier
+...
+LL |     tt_that_is_dollar_sign_with_concat!($, FOO);
+   |     ------------------------------------------- in this macro invocation
+   |
+   = note: this error originates in the macro `tt_that_is_dollar_sign_with_concat` (in Nightly builds, run with -Z macro-backtrace for more info)


parse_ident shouldn't take a span from the outside in general.
It should use span of the unexpected token in its errors.

petrochenkov · 2024-04-19T14:10:51Z

compiler/rustc_expand/src/mbe/metavar_expr.rs

+                let mut result = Vec::new();
+                result.push(element(&mut iter, sess, ident.span)?);
+                if !try_eat_comma(&mut iter) {
+                    return Err(sess.dcx.struct_span_err(ident.span, "expected comma"));


Again, this should use span of the unexpected token.

compiler/rustc_expand/src/mbe/metavar_expr.rs

compiler/rustc_expand/src/mbe/transcribe.rs

petrochenkov · 2024-04-19T14:53:04Z

compiler/rustc_expand/src/mbe/transcribe.rs

+            result.push(TokenTree::Token(
+                Token::from_ast_ident(Ident::new(Symbol::intern(&concatenated), visited_span())),
+                Spacing::Alone,
+            ));


The current implementation marks the result as coming from the macro (using span of the metavar expression itself), regardless of contexts of the concatenated identifiers.
concat_idents!() macro works in a similar way, ignoring input contexts, but the output context is assigned in a different way.

What the desired behavior should be is a design question.

Are we supposed to use local variables produced by concat inside macro_rules (macros 1.0) outside of the macro? Not sure.

Are we supposed to use any names produced by concat inside macro (macros 2.0) outside of the macro?
Likely yes, so some kind of hygiene opt out, or support for inheriting context from some token, should be added, but that applies to many things produced by macro, not just concat.

c410-f3r · 2024-04-21T17:33:34Z

Thanks for the review. Let me know if anything is missing.

Questions around potential design concerns were placed in the requested tracking issue.

rustbot assigned WaffleLapkin Dec 15, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2023

WaffleLapkin reviewed Dec 17, 2023

View reviewed changes

WaffleLapkin 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 Dec 17, 2023

c410-f3r force-pushed the concat-again branch from 5e8fac5 to b08d90b Compare December 18, 2023 21:55

This comment has been minimized.

Sign in to view

c410-f3r force-pushed the concat-again branch 2 times, most recently from 97b22d8 to c04e3a6 Compare December 18, 2023 22:18

rustbot added the A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) label Dec 22, 2023

tgross35 reviewed Dec 22, 2023

View reviewed changes

c410-f3r force-pushed the concat-again branch from c04e3a6 to 7695bdf Compare December 26, 2023 22:36

This comment has been minimized.

Sign in to view

c410-f3r force-pushed the concat-again branch from aed2b0e to 5242d59 Compare December 26, 2023 22:51

c410-f3r changed the title ~~[RFC-3086] Add a new concat metavar expr~~ Add a new concat metavar expr Dec 26, 2023

rustbot 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 Jan 5, 2024

This comment was marked as resolved.

Sign in to view

WaffleLapkin requested changes Jan 12, 2024

View reviewed changes

rustbot 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 Jan 12, 2024

c410-f3r force-pushed the concat-again branch from 5242d59 to ecbb5d6 Compare February 4, 2024 00:12

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Feb 4, 2024

c410-f3r mentioned this pull request Mar 21, 2024

Stabilize count, ignore, index, and length in Rust 1.80 #122808

Open

rustbot assigned Nadrieril and unassigned wesleywiser Apr 17, 2024

tgross35 reviewed Apr 17, 2024

View reviewed changes

compiler/rustc_expand/src/mbe/metavar_expr.rs Show resolved Hide resolved

tgross35 reviewed Apr 17, 2024

View reviewed changes

compiler/rustc_expand/src/mbe/transcribe.rs Outdated Show resolved Hide resolved

rustbot assigned tgross35 and unassigned Nadrieril Apr 18, 2024

rustbot assigned wesleywiser and unassigned tgross35 Apr 18, 2024

rustbot assigned nnethercote and unassigned wesleywiser Apr 18, 2024

rustbot assigned petrochenkov and unassigned nnethercote Apr 19, 2024

petrochenkov reviewed Apr 19, 2024

View reviewed changes

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 Apr 19, 2024

c410-f3r mentioned this pull request Apr 21, 2024

Tracking Issue for macro_metavar_expr_concat #124225

Open

8 tasks

c410-f3r force-pushed the concat-again branch 3 times, most recently from e810b88 to 2ce0a7b Compare April 21, 2024 14:48

This comment has been minimized.

Sign in to view

Add a new concat metavar expr

2176528

c410-f3r force-pushed the concat-again branch from 2ce0a7b to 2176528 Compare April 21, 2024 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new concat metavar expr #118958

Add a new concat metavar expr #118958

c410-f3r commented Dec 15, 2023

rustbot commented Dec 15, 2023

This comment has been minimized.

tgross35 commented Dec 22, 2023

tgross35 left a comment •

edited

tgross35 Dec 22, 2023

c410-f3r Dec 26, 2023

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented Dec 27, 2023

tgross35 commented Jan 5, 2024

c410-f3r commented Jan 5, 2024

This comment was marked as resolved.

WaffleLapkin Jan 12, 2024

c410-f3r Mar 3, 2024 •

edited

petrochenkov Apr 19, 2024

c410-f3r Apr 21, 2024

WaffleLapkin Jan 12, 2024

c410-f3r Mar 3, 2024

petrochenkov Apr 19, 2024

tgross35 commented Mar 21, 2024

c410-f3r commented Apr 17, 2024

Nadrieril commented Apr 18, 2024

tgross35 commented Apr 18, 2024 •

edited

tgross35 commented Apr 18, 2024

nnethercote commented Apr 19, 2024

petrochenkov Apr 19, 2024

c410-f3r Apr 21, 2024

petrochenkov Apr 19, 2024

petrochenkov Apr 19, 2024

petrochenkov Apr 19, 2024

This comment has been minimized.

c410-f3r commented Apr 21, 2024

Add a new concat metavar expr #118958

Are you sure you want to change the base?

Add a new concat metavar expr #118958

Conversation

c410-f3r commented Dec 15, 2023

rustbot commented Dec 15, 2023

This comment has been minimized.

tgross35 commented Dec 22, 2023

tgross35 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented Dec 27, 2023

tgross35 commented Jan 5, 2024

c410-f3r commented Jan 5, 2024

This comment was marked as resolved.

Choose a reason for hiding this comment

c410-f3r Mar 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 commented Mar 21, 2024

c410-f3r commented Apr 17, 2024

Nadrieril commented Apr 18, 2024

tgross35 commented Apr 18, 2024 • edited

tgross35 commented Apr 18, 2024

nnethercote commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

c410-f3r commented Apr 21, 2024

tgross35 left a comment •

edited

c410-f3r Mar 3, 2024 •

edited

tgross35 commented Apr 18, 2024 •

edited