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

RFC: ? repetition in macro rules #2298

Merged
merged 6 commits into from
Feb 27, 2018

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented Jan 17, 2018

Rendered

Discussion

EDIT: Implementation in rust-lang/rust#47752

Tracking issue: rust-lang/rust#48075

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jan 17, 2018
@mark-i-m mark-i-m changed the title Add RFC: macro-at-most-once-rep RFC: ? repetition in macro rules Jan 17, 2018
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea in general and I would love to have this in the language. I have some minor stylistic suggestions tho (I hope you don't mind those..) as well as an improvement idea for the Motivation-section.

text/0000-macro-at-most-once-rep.md Outdated Show resolved Hide resolved
text/0000-macro-at-most-once-rep.md Outdated Show resolved Hide resolved
text/0000-macro-at-most-once-rep.md Outdated Show resolved Hide resolved
text/0000-macro-at-most-once-rep.md Outdated Show resolved Hide resolved
text/0000-macro-at-most-once-rep.md Outdated Show resolved Hide resolved

Motivation
----------

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another motivation you could add is an argument from familiarity with regular expressions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mention this in the rationale as a reason why ? was chosen, but since the RFC explicitly chooses to not add {M, N}, I think it would be hard to add familiarity with regexes to the motivation.

@17cupsofcoffee
Copy link

17cupsofcoffee commented Jan 17, 2018

I think this is a great idea - the trailing commas thing in particular has always irritated me, given that rustfmt favours them using them for structs/enums/etc. I do wonder if using ? here has the potential to be a little confusing though, given that it already has a meaning in Rust.

@mark-i-m
Copy link
Member Author

@Centril Thanks for the comments! I tried to address them in the latest commit. Specifically:

  • split motivation section with ## headings and elaborated on my "fury" a bit 😛
  • move most of the content of "reference-level guide" to "rationale" and "drawbacks"

@mark-i-m
Copy link
Member Author

@17cupsofcoffee

I think this is a great idea - the trailing commas thing has always irritated me! I do wonder if using ? here has the potential to be a little confusing though, given that it already has a meaning in Rust.

I thought about this a bit too. However, the use of ? in macros tends to be in different places from the use for early error returning. I personally don't think it will be too much of a problem, especially since writing macros is a bit more of an advanced feature. Still I'm open to ideas for how to further disambiguate :)

@Ixrec
Copy link
Contributor

Ixrec commented Jan 18, 2018

+ and * also have non-macro meanings in Rust, yet Rust macros use their "regex meaning". Even if this double-use is causing problems, I don't think adding ? to the list makes the situation any worse.

@17cupsofcoffee
Copy link

17cupsofcoffee commented Jan 18, 2018

@Ixrec Yeah, it's not a huge issue ("if a ? is after a $(...), it means zero or one repetitions" isn't massively hard to explain) - just wanted to make sure it was taken into consideration 😄 I think consistency with regex is a decent counter-argument.

---------
While there are grammar ambiguities, they can be easily fixed, as noted by @kennytm [here](https://internals.rust-lang.org/t/pre-rfc-at-most-one-repetition-macro-patterns/6557/2?u=mark-i-m):

> There is ambiguity: $($x:ident)?+ today matches a?b?c and not a+. Fortunately this is easy to resolve: you just look one more token ahead and always treat ?* and ?+ to mean separate by the question mark token.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should better be put back to reference-level explanation. You cannot implement ? without clarifying this bit.

@mark-i-m
Copy link
Member Author

@kennytm I moved the note on ambiguity back to the reference section, but left a note in drawbacks.

@Apanatshka
Copy link

I'd just like to mention, since this didn't come up yet, that besides *, + and ?, I've sometimes needed "two-or-more" in grammar specifications of programming languages (think tuples in ML which don't need round brackets around them). I'm not sure if this anecdote is a good reason to add general {m,n} patterns, but I figured it would be nice to have on record.

@mark-i-m
Copy link
Member Author

mark-i-m commented Jan 19, 2018

@Apanatshka You can build "two or more" with pat1 pat2 $(more)*, but I don't think you can build it with {m,n}.

@mark-i-m
Copy link
Member Author

Also, I think ? is sufficient to build "at most N"... You can build "at most N" with N "at most one"s.

@mark-i-m
Copy link
Member Author

Is there any way to request an FCP for this? It's not a very big feature and seems to have wide support. I don't want it to be forgotten...

@gbutler69
Copy link

@mark-i-m '''@Apanatshka You can build "two or more" with pat1 pat2 $(more)*, but I don't think you can build it with {m,n}.'''

Generalized {m,n} would allow:

  • 0 to n : {0,n}
  • m to n: {m,n}
  • m to unlimited: {m,} (e.g. 2 or more {2,}, 3 or more {3,}, 4 or more {4,}, etc)

If you followed the same rules as for Regex.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Is there an ambiguity like $(foo)?* (ie., a ?-separated list...). Playing around on play I got some very strange errors so maybe not? (I had sort of expected that to work?)

That said, I bang my head against this all the time. I think that having + and * work but not ? is definitely more confusing than having it. Therefore, I propose to merge unless somebody has some obvious reason this wouldn't work.

We should address who will implement it (or mentor an implementation of it).

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 25, 2018

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

No concerns currently listed.

Once these reviewers reach consensus, 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!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 25, 2018
@kennytm
Copy link
Member

kennytm commented Jan 25, 2018

@nikomatsakis You typed foo(...) instead of foo!(...) 😂

Yes there is ambiguity, but it is already acknowledged in the RFC with a way to workaround it, so it is not a problem.

@mark-i-m
Copy link
Member Author

@nikomatsakis I have already started implementing: rust-lang/rust@master...mark-i-m:at-most-once-rep

@mark-i-m
Copy link
Member Author

Also, it would be nice (though not necessary) if rust-lang/rust#47732 and rust-lang/rust#47603 were merged before the implementation of this feature (purely for code cleanliness)...

@mark-i-m
Copy link
Member Author

Also, cc @ExpHP, who I believe, went around adding trailing comma support to a bunch of macros in libstd...

@ExpHP
Copy link

ExpHP commented Jan 26, 2018

Ah, I forgot about that! I enumerated all the places they're missing but haven't actually fixed them yet. 😛

I'm glad to see this is already on the track to acceptance. I'd think I'd be hard pressed to find somebody who has written macros and hasn't wanted something like this from day one.

@nrc
Copy link
Member

nrc commented Jan 28, 2018

I'm somewhat reluctant to extend the macro pattern language before we decide if and how the pattern language should change for macros 2.0. But, I guess it is nice to experiment with it since we don't have any firm plans, and I assume this could be left unstable for some time.

@mark-i-m
Copy link
Member Author

@nrc Could you be a bit more specific about your concerns? For example, do you feel that adding this will complicate macros 2.0 somehow?

@nikomatsakis
Copy link
Contributor

Made a tracking issue since we plan to land the implementation before FCP is complete:

rust-lang/rust#48075

@durka
Copy link
Contributor

durka commented Feb 8, 2018 via email

@durka
Copy link
Contributor

durka commented Feb 8, 2018 via email

@mark-i-m
Copy link
Member Author

mark-i-m commented Feb 8, 2018

So the breaking change proposal would also not accept a separator with
$(pat)?, right?

I think that's orthogonal. The breaking change would be to remove ? as a separator. That would have the benefit of ? being exactly the same as + and *, but...

I'm just not sure this rises to the level of breaking changes. It's not a
high-priority bug fix or anything. Yeah it's a feature that should've been
included from the beginning, but we shouldn't set the precedent of doing
breaking changes like that whenever we think they can fly under the radar.

I tend to agree. The current implementation is not a breaking change.


```rust
macro_rules! foo {
($(pat),*,) => { foo!( $(pat),* ) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule should be $(pat),+, so that foo!(,) is not permitted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will fix shortly


```rust
macro_rules! foo {
($(pat),* $(,)?) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like better support for trailing delimiters is a major part of the motivation for this RFC and why people are excited about it. But the old approach shown under "Currently" still seems better than the one here because with ? we cannot disallow an invocation consisting of only a comma, foo!(,). Is the idea that $(,)? would be a quick and easy shorthand but macro authors would continue to use the old approach if they want their macro to be maximally correct? If so, could you brainstorm some ideas for how optional trailing delimiters could be better supported?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way would be a new repetition operator, let's say $(pat),# (# obviously up for 🚲 🏡 ), which accepts a delimited list with optional trailing separator, the same way Rust does in most contexts. I think that could be done orthogonally to this RFC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... that's true. It works well if you have + repetition.

One option is to combine ? with | as described in #2298 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to create another repetition mode which indicates trailing commas: $[pat],*


There are two specific use cases in mind.

## Macro rules with optional parts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point out some macros from crates.io that would benefit from this syntax for optional parts? The example given here strikes me as contrived.

Copy link
Contributor

@durka durka Feb 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have used it in this crate to match the following syntaxes in one rule:

  1. trait Foo { ... }
  2. trait Foo: Bar { ... }
  3. pub trait Foo { ... }
  4. pub trait Foo: Bar { ... }

namely the rule:

($vis:vis trait $traitname:ident $(: $supertrait:ident)? { $($body:tt)* })

(edited to fix local ambiguity)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also strikes me as a candidate for reimplementation: https://docs.rs/clap/2.29.4/clap/macro.clap_app.html

@ExpHP
Copy link

ExpHP commented Feb 10, 2018

One thing I don't see discussed is that this enables some things to be written with a single rule that used to require extra layers of indirection or even incremental TT munchers. For instance, any example where $()? is used inside of $()* or $()+:

macro_rules! my_match {
    (($expr:expr) {
        $( $pat:pat $(if $guard:expr)? => $block:block $(,)? )+
    }) => {
        match $expr { $( $pat $(if $guard)? => $block )+ }
    };
}

fn main() {
    my_match!{ (Some(3)) {
        Some(n) if n % 2 == 0 => { println!("even"); }
        Some(5) => { println!("five"); },
        _ => {}
    }}
}

(Though it still cannot handle all cases unless we had some sort of "alternative operator", like $( pattern1 $| pattern2 $| pattern3 )... (e.g. my_match! requires braces, but an actual match supports unbraced expressions as long as separating commas are present))

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 10, 2018

The final comment period is now complete.

@mark-i-m
Copy link
Member Author

Though it still cannot handle all cases unless we had some sort of "alternative operator", like $( pattern1 $| pattern2 $| pattern3 )

That seems like a much more significant addition, though, and it would require bigger changes to the macro parser.

bors added a commit to rust-lang/rust that referenced this pull request Feb 11, 2018
Implement `?` macro repetition

See rust-lang/rfcs#2298 (with disposition merge)
@mark-i-m
Copy link
Member Author

Ok, I added a bit to the drawbacks section to mention @dtolnay's comments in #2298 (review). For now, I think this is still a reasonable step, and we will get more insight by playing around with the preliminary implementation.

@ExpHP
Copy link

ExpHP commented Feb 11, 2018

That seems like a much more significant addition, though, and it would require bigger changes to the macro parser.

Indeed. I also feel that alternatives carry a cognitive and maintenance burden for macro authors which does not scale well with the size of the problem being solved, and I did not intend to suggest it as a viable alternative. (pun not intended)

(in particular, the fact that a macro must ultimately produce an expr, type, or items means that users would be forced to put many orthogonal and/or nested groups of alternatives into a single pattern)

@ExpHP
Copy link

ExpHP commented Feb 11, 2018

(in particular, the fact that a macro must ultimately produce an expr, type, or items means that users would be forced to put many orthogonal and/or nested groups of alternatives into a single pattern)

Musing a bit further, this reasoning could be applied to many proposals for new macro_rules! features.

It seems that, given the unavoidable limitations of macro_rules!, every possible addition to their grammar carries with it an increase in cognitive burden that is directly connected to the amount of power it provides.

That is to say, I think the ability to parse everything is very distinctly a non-goal.

@mark-i-m
Copy link
Member Author

That is to say, I think the ability to parse everything is very distinctly a non-goal.

I disagree. We should make the macro parser Turing complete.

(😉 )

@durka
Copy link
Contributor

durka commented Feb 11, 2018

Oh, it is. But I philosophically disagree on a different level -- more general features create more power than cognitive burden.

@mark-i-m
Copy link
Member Author

It looks like discussion has died down...

@mark-i-m
Copy link
Member Author

Perhaps this can be merged now?

@Centril Centril merged commit c40b98a into rust-lang:master Feb 27, 2018
@Centril
Copy link
Contributor

Centril commented Feb 27, 2018

Huzzah! The RFC is merged!

Tracking issue: rust-lang/rust#48075

@mark-i-m
Copy link
Member Author

Thanks @Centril 🎉

I think rust-lang/rust#48075 is already a tracking issue.

@mark-i-m
Copy link
Member Author

mark-i-m commented Jun 30, 2018

Request for comments/FCP on newest implementation: rust-lang/rust#51934

@Centril Centril added A-macros Macro related proposals and issues A-syntax Syntax related proposals & ideas labels Nov 23, 2018
@mark-i-m mark-i-m deleted the macro-at-most-once-rep branch March 30, 2020 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Macro related proposals and issues A-syntax Syntax related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet