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

Tracking Issue for RFC 3086: macro metavariable expressions #83527

Open
1 of 4 tasks
nikomatsakis opened this issue Mar 26, 2021 · 47 comments
Open
1 of 4 tasks

Tracking Issue for RFC 3086: macro metavariable expressions #83527

nikomatsakis opened this issue Mar 26, 2021 · 47 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-macro-metavar-expr feature(macro_metavar_expr) S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 26, 2021

This is a tracking issue for the RFC "3086" (rust-lang/rfcs#3086).
The feature gate for the issue is #![feature(macro_metavar_expr)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved questions and bugs

Implementation history

@nikomatsakis nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Mar 26, 2021
@nikomatsakis nikomatsakis added the F-macro-metavar-expr feature(macro_metavar_expr) label Mar 26, 2021
@markbt
Copy link

markbt commented Mar 27, 2021

There is a working prototype on the markbt/rust/metavariable_expressions branch. This needs feature gating, and there are a couple of TODOs to resolve, but it's otherwise in reasonable shape. I'm planning to work on it over the coming weeks.

@markbt
Copy link

markbt commented Apr 7, 2021

Update (2021-04-07)

I've not yet started work on productionizing the prototype on the markbt/rust/metavariable_expressions branch. I plan to start later this month, free time permitting.

dalcde added a commit to SpectralSequences/sseq that referenced this issue Apr 12, 2021
enum_dispatch is somewhat broken, and we need to write manual dispatch
code for pyo3 anyway (and have written similar dispatch code for many
other things).

rust-lang/rust#83527 would make it more
convenient to make our manual dispatch code generic.
@markbt
Copy link

markbt commented May 9, 2021

Update (2021-05-09)

I still haven't started on this yet as some stuff came up last month that prevented from having the time to work on it. It's still in my plan to work on it, and hopefully I'll have some time soon.

@markbt
Copy link

markbt commented Jun 29, 2021

Update (2021-06-29)

Still no progress, as I haven't had any spare time to work on this project. I'm still planning to work on it, and hopefully will get some time soon.

@joshtriplett
Copy link
Member

@markbt If you don't expect to find the bandwidth in the near future, would you potentially be interested in seeking help in the form of another owner for this initiative? If you're still interested in driving this, that's fine, but if you'd like us to switch gears from pinging you to broadcasting that the project could use help, we'd be happy to do that.

@markbt
Copy link

markbt commented Jul 9, 2021

I'd be happy with any help if there's someone available. I still plan to work on it, but personal stuff is getting in the way at the moment. Sorry about that.

To recap: I have a working prototype on my branch at https://github.com/markbt/rust/tree/metavariable_expressions . The next steps are to rebase that onto the latest master, and then polish it up so that it's ready for inclusion. Then there's also the doc work to make sure the new feature is documented well. Help with any of this would be appreciated.

@c410-f3r
Copy link
Contributor

@markbt Are you still working on this feature? If not, then I can pick up from where you stopped

@markbt
Copy link

markbt commented Jan 29, 2022 via email

@c410-f3r
Copy link
Contributor

Thank you @markbt

I will take a look at the branch and then get in touch if any questions arise

@c410-f3r
Copy link
Contributor

The implementation has been merged so please try testing the feature as much as possible to find any potential bugs

@mark-i-m
Copy link
Member

Thanks @c410-f3r for all your hard work!

From my limited experience with this feature, I have some concerns about the design:

  • The depth arguments are confusing because some of the functions count up from the most nested depth and some count down from the outermost level.
  • Also, indexing does not universally start at 0, and an index of 1 means different things for different meta-variable expressions.
  • I find the semantics of count confusing and hard to keep track of. Which level is it counting? Does depth 1 mean count everything or count the 2nd nested loop? Does depth 1 mean count everything or count only the outermost loop? Maybe a name like total_count would be clearer? Maybe the depth option should not be optional? Maybe count should be removed, and people can just sum the output of length?

@c410-f3r
Copy link
Contributor

c410-f3r commented Mar 15, 2022

Thank you @mark-i-m for reviewing #93545 and for all your contributions to the compiler.

Hehehehe... You, me and @petrochenkov had trouble trying to understand this feature

  • The depth arguments are confusing because some of the functions count up from the most nested depth and some count down from the outermost level.

Indeed! It is much easier cognitively to keep everything with the same direction instead of having count going from outer-to-inner and index/length from inner-to-outer. Although I am not sure if semantic would be impacted.

  • Also, indexing does not universally start at 0, and an index of 1 means different things for different meta-variable expressions.
  • I find the semantics of count confusing and hard to keep track of. Which level is it counting? Does depth 1 mean count everything or count the 2nd nested loop? Does depth 1 mean count everything or count only the outermost loop? Maybe a name like total_count would be clearer? Maybe the depth option should not be optional? Maybe count should be removed, and people can just sum the output of length?

Yeap, someone that wrote ${count(foo)} will probably expect that ${count(foo, 1)} will return some value related to the amount of foos instead of the actual number of outer repetitions.

If repetitions are nested, then an optional depth parameter can be used to limit the number of nested repetitions that are counted. For example, a macro expansion like:

${count(x, 1)} ${count(x, 2)} ${count(x, 3)} $( a $( b $( $x )* )* )*

The three values this expands to are the number of outer-most repetitions (the number of times a would be generated), the sum of the number of middle repetitions (the number of times b would be generated), and the total number of repetitions of $x.

https://github.com/markbt/rfcs/blob/macro_metavar_expr/text/0000-macro-metavar-expr.md#count

And as you said, throwing nested loops into the equation will alter indexing making understanding even harder. Not to mention mixing other meta-variable expressions like length 🙁.

#![feature(macro_metavar_expr)]

fn main() {
    macro_rules! mac {
        ( $( [ $( $i:ident )* ] )* ) => {{
            // ***** No loop *****
            
            println!("{}", ${count(i)}); // 5
            println!("{}", ${count(i, 0)}); // 2
            
            // Same as ${count(i)}
            //println!("{}", ${count(i, 1)});
            
            // Fobirdden. Index out of bounds
            //println!("{}", ${count(i, 2)});
            
            // ***** Outer-most loop *****
            
            $(
                println!("{}", ${count(i)}); // 3 and 2
                
                // Same as ${count(i)}
                //println!("{}", ${count(i, 0)});
                
                // Fobirdden. Index out of bounds
                //println!("{}", ${count(i, 1)});
            )*

            // ***** Outer-most and inner-most loops *****
            
            $(
                $(
                    ${ignore(i)}

                    // Forbidden. Can't be placed inside the inner-most repetition
                    //println!("{}", ${count(i)});
                )*
            )*
        }};
    }
    
    mac!([a b c] [d e]);
}

Maybe total_count and a mandatory depth can be nice modifications but I am also not sure about the removal of count (Useful for nested stuff). Overall, I think that users will have a hard time even with a good set of documentation.

As for myself, I learned to accept the things as they are currently defined in RFC, hehehehe 😁

@c410-f3r
Copy link
Contributor

Any thoughts @markbt?

@camsteffen
Copy link
Contributor

camsteffen commented Mar 16, 2022

  • The depth arguments are confusing because some of the functions count up from the most nested depth and some count down from the outermost level.

+1. Being inconsistent here seems out of the question. I would vote for "distance from innermost", agreeing with the rationale in the RFC:

The meaning of the depth parameter in index and count originally counted inwards from the outer-most nesting. This was changed to count outwards from the inner-most nesting so that expressions can be copied to a different nesting depth without needing to change them.

Adding to this, having the depth parameters represent "distance from outermost" will lead to the following (unfortunate) user story:

I have a macro build_foo!. I want to refactor it to build_many_foos!, so I wrap the macro definition with $(...),*. Now all of the depth parameters are off by one so I increment them.

The only bad thing is that the English meaning of "depth" lends to "distance from outermost". But this is less important IMO.

We could support both by having negative numbers (e.g. ${index(-1)}) work as "distance from innermost", but that is probably not necessary. You'd also have to decide what to do with 0.


Maybe count should be removed, and people can just sum the output of length?

Yes this might be a "less is more" scenario. You could potentially get what you need using expressions of index and length, like ${length(1)+index(2)} or ${length(1)*length(2)}, but we'd have to support math within ${..}. Edit: this doesn't work

Position Total
Current repetition index length
All repetitions count

Has it been considered to make theses values bind-able within the macro pattern? This would take depth out of the picture completely since the variables are syntactically attached to a repetition. Inspired by ngFor in Angular.

macro_rules! mac {
    ($[i=index, l=length]($word:ident),*) => {
        $(println!("{}/{}: {}", $i, $l, stringify!($word));)*
    };
}

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.
@cybersoulK
Copy link

@CAD97 for real? $$ and $ignore were supposed to be stabilized for over a year now.
have you actually had a need for nested macros? i do, and this is a must requirement, why block stabilization because of $$crate, i never used it, and i can't se myself using $crate at all

@cybersoulK
Copy link

cybersoulK commented Aug 8, 2023

i am guessing the solution would be to merge $$ and $ignore ASAP,
and make usage of "$crate" behind an unstable feature until you decide what do to with $$crate

@CAD97
Copy link
Contributor

CAD97 commented Aug 8, 2023

have you actually had a need for nested macros?

Yes, and I discovered the interesting behavior of $crate when writing a macro-expanded macro-expanded macro definition making use of $$ (at that time; it now uses stable-compatible techniques). I would absolutely and immediately benefit from the stabilization of both $$ and ${ignore} (or even better for my use cases, some kind of $:empty matcher).

i can't se myself using $crate at all [...] make usage of "$crate" behind an unstable feature

$crate is stable and very needed when defining resilient exported macros to be used by downstream crates. If your macro is #[macro_export]ed, you should be using $crate to refer to any item defined in your crate or upstream to you, such that downstream usage can't shadow your expected names and cause issues.

This becomes extremely necessary when using unsafe in the macro expansion, such that you can be absolutely sure that you completely control what code gets trusted and aren't exporting a macro making a soundness assumption that the user won't shadow any names that it uses.

until you decide what do to with $$crate

I originally claimed it wouldn't be possible to stabilize $$crate without

$$ and $ignore were supposed to be stabilized for over a year now.

$$ was in fact temporarily accepted for stabilization before it was backed out, but ${ignore} was not. ${ignore} was originally part of the proposed stabilization PR, but it was dropped before it was accepted due to design concerns.

why block stabilization

I don't actually have any power to directly block stabilization, let alone to revert an accepted stabilization on beta; if T-lang1 didn't agree with my concern about $$ they could just ignore me and stabilize it anyway. I was the one to post the revert PR, but I did so on request of T-lang.

Though tbf, I'm not blameless; I've proposed various resolutions and haven't followed up with an implementation. (This reminds me, I should ideally try my hand at properly implementing my desired fix for $crate semantics relatively soon, so it has a chance of landing for edition2024.) Time and motivation and hard to find concurrently, and no unpaid volunteer owes the project anything.

Footnotes

  1. Technically, I am now a part of T-opsem (thus the [Member] tag), which is a subteam of T-lang. However, 1) I was not at the time of the revert (T-opsem didn't even exist), and 2) that membership does not confer T-lang membership or powers. Even if it did for some reason, I'd defer to the rest of T-lang on this.

@cybersoulK
Copy link

cybersoulK commented Aug 9, 2023

@CAD97

$crate feels hacky to me. With macros 1.0, i force the user to import the entire scope of the macro, if it requires it.

I admit i am not an expert, but it might be worth to read my ideas for macro 2.0:

#39412 (comment)

@c410-f3r
Copy link
Contributor

c410-f3r commented Oct 3, 2023

Proposal

In hopes of making some progress, two approaches based on the feedback provided so far will be proposed here. Feel free to discuss alternatives that will lead to consensus if any of the following suggestions are not desired.

1. Innermost vs Outermost indexes

count uses outermost indices while length uses innermost indices and this inconsistency creates unnecessary confusion.

meta

To improve the situation, the order of all elements should start from the innermost index to the outermost index.

Mentions

2. $ prefix

Taking count as an example, should the syntax be count(some_metavariable) or count($some_metavariable)? The original RFC specified that metavariable expressions should refer metavariables without $ prefixes but there were some arguments in favour of $.

For unblocking purposes, the requirement of $ is being suggested. Such enforcement doesn't appear to incur a significant overhead besides the additional typing and interactions with $$ or multiple $$s shouldn't be a problem as long as the final expanded $ refers a metavariable.

Mentions

@markbt
Copy link

markbt commented Oct 30, 2023

It's nice to see this progressing. We recently found some more places where this would be useful, e.g. in constructing a tuple from a method that takes indexes using an expression like ( $( ${ignore($t)} row.get(${index()}), )* ).

1. Indexes

For ${index()} and ${length()} I think the inner-to-outer ordering is important, as it means you can move these expressions without having to re-index them.

For ${count(...)} I think either order is fine. I chose outer-to-inner as to my mind the count proceeds from outside to inside, but actually it doesn't matter. For reference I use this diagram to think about it (based on the definitions in the previous comment):

    .-- current
    |   .-- proposed
    |   |
N = 2   0  ----------.                     (default)
N = 1   1  -------.  |
N = 0   2  ----.  |  |
               v  v  v
$count($x, N)  [  (  $x $length(L)  )  ]
L = 0                <----------->         (default)
L = 1             <----------------->
L = 2           <--------------------->

This proposal switches the order of N, but it's fine for it to mean either way.

2. $ prefix

Again, I think this is fine. In fact, for $ignore it makes more sense, as we could later generalize it to more complex expressions with side-effects.

3. The $$crate problem.

Has anything been decided about this? A possible alternative would be to define ${crate()} which expands to the same thing as $crate but correctly in the case of recursive macros. It could be a warning to specify $$crate, and users should use $${crate()} instead, with ordinary $crate essentially becoming shorthand for ${crate()}.

@nikomatsakis
Copy link
Contributor Author

Note from lang team: we discussed @c410-f3r's proposed changes in our meeting today and agree we should move forward, but please update the comment at top of the issue. More here.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 9, 2023
…enkov

[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 12, 2023
…chenkov

[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 13, 2023
…enkov

[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 14, 2023
[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang/rust#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
@rslife
Copy link

rslife commented Dec 17, 2023

Nesting ignores issue:

#![feature(macro_metavar_expr)]

struct Foo(i32);
struct Bar(i32);
struct Baz(i32);

macro_rules! mk_p {
    ($p_ty:ty $(, foo=$foo:ident)? $(, bar=$bar:ident)?) => {
        $(${ignore($foo)} mk_p!($p_ty, foo=$foo, foo_or_bar=foo_or_bar); )?
        $(${ignore($bar)} mk_p!($p_ty, bar=$bar, foo_or_bar=foo_or_bar); )?
    };
    ($p_ty:ty $(, foo=$foo:ident)? $(, bar=$bar:ident)? $(, baz=$baz:ident)? $(, foo_or_bar=$foo_or_bar:ident)?) => {
        impl $p_ty {
            fn p(&self) {
                $(
                    ${ignore($baz)}
                    eprintln!("baz={}", self.0);
                )?

                $(
                    ${ignore($foo_or_bar)}
                    let i = self.0 + 5;
                    $(
                        ${ignore($foo)}
                        eprintln!("foo={i}");
                    )?
                    $(
                        ${ignore($bar)}
                        eprintln!("bar={i}");
                    )?
                )?
            }
        }
    };
}

mk_p!(Foo, foo=foo);
mk_p!(Bar, bar=bar);
mk_p!(Baz, baz=baz);


fn main() {}

Error:

error: meta-variable `foo_or_bar` repeats 1 time, but `bar` repeats 0 times
  --> src/main.rs:20:18
   |
20 |                   $(
   |  __________________^
21 | |                     ${ignore($foo_or_bar)}
22 | |                     let i = self.0 + 5;
23 | |                     $(
...  |
30 | |                     )?
31 | |                 )?
   | |_________________^

error: meta-variable `foo_or_bar` repeats 1 time, but `foo` repeats 0 times
  --> src/main.rs:20:18
   |
20 |                   $(
   |  __________________^
21 | |                     ${ignore($foo_or_bar)}
22 | |                     let i = self.0 + 5;
23 | |                     $(
...  |
30 | |                     )?
31 | |                 )?
   | |_________________^


Is this a current limitation or intended behavior?

@i18nsite
Copy link

i18nsite commented Jan 4, 2024

the resolved

use ignore!

old

can $index support $index for xxx ? for example ${task.index}

image

@c410-f3r
Copy link
Contributor

c410-f3r commented Feb 9, 2024

#117050 was merged ~2 months ago and no related issues have been created since then.

Can we finally proceed with the stabilization of everything but $$? Does anyone still have any kind of blocking concern?

@c410-f3r
Copy link
Contributor

A stabilization attempt is available at #122808

lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang/rust#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
[`RFC 3086`] Attempt to try to resolve blocking concerns

Implements what is described at rust-lang/rust#83527 (comment) to hopefully make some progress.

It is unknown if such approach is or isn't desired due to the lack of further feedback, as such, it is probably best to nominate this PR to the official entities.

`@rustbot` labels +I-compiler-nominated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-macro-metavar-expr feature(macro_metavar_expr) S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests