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

Stabilize count, ignore, index, and len (macro_metavar_expr) #122808

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Mar 21, 2024

Stabilization proposal

This PR proposes the stabilization of a subset of #![feature(macro_metavar_expr)] or more specifically, the stabilization of count, ignore, index and len.

Tracking issue: #83527
Version: 1.80 (June 13 2024 => beta, July 25 2024 => stable).

What is stabilized

Count

The number of times a meta variable repeats in total.

The output of count depends on where it is placed as well the provided index. If no index is provided, then it will always start at the innermost level.

macro_rules! count_idents {
    ( $( $i:ident ),* ) => {
        ${count($i)}
    };
}

fn main() {
    assert_eq!(count_idents!(a, b, c), 3);
}

Ignore

Binds a meta variable for repetition, but expands to nothing.

macro_rules! count {
    ( $( $i:stmt ),* ) => {{
        0 $( + 1 ${ignore($i)} )*
    }};
}

fn main() {
    assert_eq!(count!(if true {} else {}, let _: () = (), || false), 3);
}

Index

The current index of the inner-most repetition.

index is a counter that starts from 0 until the end of the repetition.

trait Foo {
    fn bar(&self) -> usize;
}

macro_rules! impl_tuple {
    ( $( $name:ident ),* ) => {
        impl<$( $name, )*> Foo for ($( $name, )*)
        where
            $( $name: AsRef<[u8]>, )*
        {
            fn bar(&self) -> usize {
                let mut sum: usize = 0;
                $({
                    const $name: () = ();
                    sum = sum.wrapping_add(self.${index()}.as_ref().len());
                })*
                sum
            }
        }
    };
}

impl_tuple!(A, B, C, D);

fn main() {
}

len

The current index starting from the inner-most repetition.

len indicates the sum of meta variable elements, aka length, of the current repetition.

macro_rules! array_3d {
    ( $( $( $number:literal ),* );* ) => {
        [
            $(
                [
                    $( $number + ${len()}, )*
                ],
            )*
        ]
    };
}

fn main() {
    assert_eq!(array_3d!(0, 1; 2, 3; 4, 5), [[2, 3], [4, 5], [6, 7]]);
}

Motivation

Meta variable expressions not only facilitate the use of macros but also allow things that can't be done today like in the $index example.

An initial effort to stabilize this feature was made in #111908 but ultimately reverted because of possible obstacles related to syntax and expansion.

Nevertheless, #83527 (comment) tried to address some questions and fortunately the lang team accept #117050 the unblocking suggestions.

Here we are today after ~4 months so everything should be mature enough for wider use.

After RFC changes

In order to unblock progress, some changes were applied in #118958. These changes did not trigger any opposition debates and were quickly integrated.

  1. The depth direction of count changed from outermost-to-innermost to innermost-to-outermost.
  2. Metavariables were originally referenced by their names like in count(some_metavariable) but now they must be preceded by $. For example, count($some_metavariable).

What isn't stabilized

$$ is not being stabilized due to unresolved concerns.

History

Tests

This list is a subset of https://github.com/rust-lang/rust/tree/master/tests/ui/macros/rfc-3086-metavar-expr.

Possible future work

With enough consensus, other nightly expressions can be added for experimentation and possibly stabilized in the future. For example, #118958.

Thanks @markbt for creating the RFC and thanks to @petrochenkov and @mark-i-m for reviewing the implementations.

@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 21, 2024
@rust-log-analyzer

This comment has been minimized.

@c410-f3r c410-f3r changed the title Stabilize count, index, ignore and length in Rust 1.80 Stabilize count, ignore, index, and length in Rust 1.80 Mar 21, 2024
@fmease
Copy link
Member

fmease commented Mar 21, 2024

@c410-f3r Has this been documented in the Reference already? The box is not ticked over at the tracking issue. If not, stabilization is blocked on adding sufficient documentation.

@fmease fmease added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-lang-nominated Nominated for discussion during a lang team meeting. labels Mar 21, 2024
@joshtriplett

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 21, 2024
@joshtriplett joshtriplett added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 21, 2024
@joshtriplett

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 21, 2024
@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot concern document-in-reference

@rfcbot
Copy link

rfcbot commented Mar 21, 2024

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 21, 2024
correabuscar added a commit to correabuscar/tlborm that referenced this pull request May 27, 2024
this is also seen in the recent stabilization PR attempt: rust-lang/rust#122808 and rust playground confirms it.
correabuscar added a commit to correabuscar/tlborm that referenced this pull request May 27, 2024
this is also seen in the recent stabilization PR attempt: rust-lang/rust#122808 and rust playground confirms it.
Veykril pushed a commit to Veykril/tlborm that referenced this pull request May 28, 2024
this is also seen in the recent stabilization PR attempt: rust-lang/rust#122808 and rust playground confirms it.
@RalfJung
Copy link
Member

RalfJung commented May 29, 2024

I tend to agree with @tgross35 -- having to refer to capture groups by index is a really awkward and fragile API, I don't think we should stabilize that if better alternatives exist. After all we generally let the user name things they need to refer to (variables, functions, types, even loops to break out of) instead of using opaque numbers and counting.

@workingjubilee
Copy link
Member

Is there consensus for ${ignore($var)} and ${count($var)} at least? Those would make certain patterns much easier to handle.

@RalfJung
Copy link
Member

RalfJung commented May 30, 2024

ignore seems like a no-brainer to me. (Not sure this is the best possible name, but I can't come up with a better one either.)

count does seem to have very surprising interaction with repetitions. Judging from the reference PR:

macro_rules! count_value_nested {
    ( $( $name:ident: $( $value:literal ),* );+ ) => {
        [ $(
            // using `count` within a repetition group will return the number
            // of times `$value` is matched _within that group_.
            ${count($value)},
        )+ ]
    };
}

So, this is a repetition group that only contains $value. And somehow that becomes a once-per-$name repetition? That's not how repetition behaves anywhere else, is it? I would have expected that I must use $name somewhere in this repetition group to make this a once-per-$name group, and then I can count the $value in it -- the argument of $count would not be part of the automatic detection of the repetition group in my mental model, that has to already be clear without.

Which repetition group does this count?

macro_rules! count_test {
    ( $( $supername:ident { $( $name:ident: $( $value:ident )* ),+ } )* ) => {
        [ $(
            ${count($value)},
        )+ ]
    };
}

This could reasonably be for i in supername { i.total_numer_of_value() } but also for i in supername { for j in i.name { j.total_numer_of_value() } }.

@RalfJung
Copy link
Member

RalfJung commented May 30, 2024

Trying this out, it seems like like it is the former, counting the total number of value for each supername.

Usually a metavar "must appear in exactly the same number, kind, and nesting order of repetitions in the transcriber as it did in the matcher". With count that seems to be different. Is it something like "must appear in strictly fewer repetitions than it did in the matcher, and then it counts the number of inner repetitions in the current instance of the outer repetitions it occurs in"?

@workingjubilee
Copy link
Member

looks kinda like group.iter().flat_map(|expansion| expansion.get(metavar)).count()

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 9, 2024

As previously commented, the suggestion around named placeholders is orthogonal to the current behavior approved in the RFC and both can co-exist in the future.

Fate imposed yet another syntax-related blocking concern in a stabilization PR, as such, I ask the responsible party to decide the final outcome.

@rustbot labels +I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 9, 2024
@jhpratt jhpratt changed the title Stabilize count, ignore, index, and length in Rust 1.80 Stabilize count, ignore, index, and length (macro_metavar_expr) Jun 18, 2024
@tmandry
Copy link
Member

tmandry commented Jun 19, 2024

@rfcbot concern depth levels are confusing

Before reading this thread I had a strong reaction to the RFC exactly in line with @RalfJung's, and I had thoughts along the lines of what @tgross35 is proposing. So I think there's something worth paying attention to here. I see it as a major downside, quite possibly a failure, of the lang team process that we got this far without such syntax concerns being surfaced. With that said, even though the concern is a late breaking one, it feels significant enough to me that I don't think it should be minimized.

Rust's declarative macro syntax can already be quite intimidating to the uninitiated, and we should be careful in how we spend our "readability budget" here. This emphasis on readability is the reason I'm not that swayed by the argument of future compatibility: Even if we add named expansions later, the syntax for unnamed depth levels will always exist if we stabilize it now, and people who read Rust macro code will have to know about it.

To make my concern more concrete: I think adding recursion depths is a significant step up in both capability and complexity, but I would want that to be equally matched with stronger usability as well as motivation for the feature. The usability piece is quite lacking, in my opinion, in that the recursion depths are unlabeled integers without an obvious meaning. I certainly think they are non-obvious to the reader, and in some cases even to the writer who already knows about this feature (I speculate that many users might forget how count works with nested metavars and depths, even after using it a few times, because it doesn't seem to work like anything else in the language). I also don't see a motivation for these specific capabilities laid out in the RFC or the stabilization report.


In the interest of making forward progress, I'll put forward a proposal that I frame as a question. The implied question in each of these is how much the proposal limits the use cases enabled by these features. If the answer is "not that much" I think we should go forward with stabilizing this subset for now; otherwise, I hope it's a useful starting point for getting things moving.

  • Can the stabilization proposal be subsetted not to support depth levels on any of these operators?
  • Can count specifically be limited to supporting only metavars with a single recursion level?
    • If that's too limiting: What is the motivation for counting all nested expansions instead of only counting the outermost level (the one that would be expanded in the next use of $()*)? If hypothetically we switched it to instead only count the outermost level (without supporting explicit depths but holding open the possibility to support them in the future), would that enable significantly more use cases? edit: Nevermind, see Stabilize count, ignore, index, and len (macro_metavar_expr) #122808 (comment)

@RalfJung
Copy link
Member

RalfJung commented Jun 20, 2024

If that's too limiting: What is the motivation for counting all nested expansions instead of only counting the outermost level (the one that would be expanded in the next use of $()*)?

I don't exactly know what you mean here, could you give an example?

FWIW, I experimented with count a bit and here are the results. Without named repetitions, I don't think there is another interpretation that makes sense. Possibly there is a subset that makes sense, like only supporting count($x) when x is exactly one level inside the current nesting -- but it's not clear that that actually makes it more understandable.

The only point that gives me pause around count is -- if/when we have named repetition groups, how will they work with count, and is that compatible with today's behavior?

@tmandry
Copy link
Member

tmandry commented Jun 20, 2024

I don't exactly know what you mean here, could you give an example?

What I meant is, why not make count($x) equivalent to count($x, 1) instead of count($x, ∞)? We would then have to invent syntax for the latter if we wanted to preserve expressiveness, but this default aligns more with my expectations.

@RalfJung
Copy link
Member

RalfJung commented Jun 20, 2024 via email

@tmandry
Copy link
Member

tmandry commented Jun 20, 2024

I'm not sure what count(x, 0) would mean, based on this from the RFC:

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.

Reading this over again though, I no longer think it makes sense to limit to 1 by default. ${count(x)} should of course count the number of expansions for $x, not the number of expansions for the next-expandable metavariable that $x happens to be nested in. We could be conservative in only allowing it in the same contexts where $x can already be expanded, but I can see it being useful outside of those contexts.

I still think specifying a max depth level is non-obvious and of dubious value, since you could always rewrite the excerpt above as something like the following; are there cases where you can't do this?

 ${count(a)} ${count(b)} ${count(x)} $( a $( b $( $x )* )* )*

@RalfJung
Copy link
Member

RalfJung commented Jun 21, 2024

I'm not sure what count(x, 0) would mean, based on this from the RFC:

I have linked this already above, see here. That's what I reverse engineered based on my experiments, anyway. The default is definitely 0, according to that PR.

Maybe the semantics changed since the RFC was accepted?

Would be good to get an up=to-date description of the proposed semantics confirmed by the people involved in the implementation -- Cc @c410-f3r , not sure who else?

I still think specifying a max depth level is non-obvious and of dubious value, since you could always rewrite the excerpt above as something like the following; are there cases where you can't do this?

I think the issue is around matchers like $( [[ $( {{ $( $x, )* }} )* ]] )* where there is no variable in the outer repetition groups that one can refer to.

@c410-f3r
Copy link
Contributor Author

I will try to address all questions this weekend in my free time.

cc rust-lang/reference#1485 (comment)

@c410-f3r
Copy link
Contributor Author

I see it as a major downside, quite possibly a failure, of the lang team process that we got this far without such syntax concerns being surfaced.

Thank you. macro_metavar_expr has been around for +3 years and its implementation has been available for +2 years. Although part of the game, it is surprising and demotivating to see syntax concerns in stabilizing PRs (#95860).

The only point that gives me pause around count is -- if/when we have named repetition groups, how will they work with count, and is that compatible with today's behavior?

It is more complex but not impossible IMO. One way is to attach a corresponding matcher depth to each named repetition and then evaluate which ones can be associated based on the current transcriber position.

Syntax-wise, the distinction between numbers and strings can create appropriated implementation branches.

What I meant is, why not make count($x) equivalent to count($x, 1) instead of count($x, ∞)? We would then have to invent syntax for the latter if we wanted to preserve expressiveness, but this default aligns more with my expectations.

AFAIK it is currently equivalent to count($x, 0),

Before #117050 count($x) was equal to count($x, N) with N being the number of nested repetitions minus one. Now count($x) is equal to count($x, 0) which can also be seen as the innermost repetition.

rust-lang/reference#1485 (comment) will probably provide a better overview.

I still think specifying a max depth level is non-obvious and of dubious value, since you could always rewrite the excerpt above as something like the following; are there cases where you can't do this?

${count(a)} ${count(b)} ${count(x)} $( a $( b $( $x )* )* )*

Unfortunately it is not possible to count without a metavariable. AFAICT, a and b are just references used in the RFC to illustrative how the algorithm works.

But yeah, the specification of a depth is non-obvious as it depends on the number of nested matcher repetitions as well as the positioning.

the people involved in the implementation

@markbt, @petrochenkov and @mark-i-m

@tmandry
Copy link
Member

tmandry commented Jun 28, 2024

I see, I was confused by reading the RFC for more detail, but that is now outdated. @c410-f3r Can you please include a list of differences from the RFC in the stabilization report (with rationale; feel free to summarize and link to more context)?

@c410-f3r
Copy link
Contributor Author

I see, I was confused by reading the RFC for more detail, but that is now outdated. @c410-f3r Can you please include a list of differences from the RFC in the stabilization report (with rationale; feel free to summarize and link to more context)?

Sure. The report has been updated.

@alex-semenyuk
Copy link
Member

@c410-f3r
From triage team. Does it ready for review? if so could you please fix merge conflicts before

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 16, 2024

Well, I can rebase but I don't think it will make much of a difference. The actual issue lies within the decision around the use of indices in count, index and len.

This PR has been waiting for more than 2 months for a response from the responsible team. AFAICT, all questions posted here as well as in rust-lang/reference#1485 have already been answered to the best of my ability.

@alex-semenyuk alex-semenyuk removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 16, 2024
@joshtriplett joshtriplett changed the title Stabilize count, ignore, index, and length (macro_metavar_expr) Stabilize count, ignore, index, and len (macro_metavar_expr) Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-nominated Nominated for discussion during a lang team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.