Allow | after pat in macro_rules!

[SimonSapin]

Per #20563 (comment)

This enables writing macros that accept $($pattern:pat)|+ and expand to the same thing in a match statement. See for example the matches! macro.

r? @alexcrichton

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[alexcrichton]

Could you add a test for this as well?

I'd like a second opinion on this, but I believe this to be valid as I don't believe it's possible for us to ever expand the pattern grammar to include a trailing | due to that being how we separate patterns today and would cause an ambiguity.

[sfackler]

I want to ensure that we'll be able to expand the pattern syntax to allow stuff like Some(1 | 2), but I think that'll be fine here. foo | bar will just end up being parsed as a single pat by a $($p:pat)|* matcher instead of two if we end up making that grammar change, right?

[SimonSapin]

@sfackler $($p:pat)|* specifically would be fine (I think) if foo | bar becomes a single pat, but something else like $p:pat | $e: expr might not.

[sfackler]

Ah right. Maybe we want to make the check a bit smarter and add separate FOLLOW and BETWEEN sets?

[SimonSapin]

To clarify: my motivation is to write a macro that accepts whatever comes before => in a match expression, and reproduces it as-is in a match expression in the expansion. (Namely, matches!).

I don’t care how this is achieved or if I have to change the implementation (e.g. if a single $p:pat includes |) as long as I don’t have to change the usage syntax or make it a compiler plugin.

[SimonSapin]

I’m not convinced this is the right fix, but I’m counting on Cunningham’s Law.

[SimonSapin]

Added a test, but make check fails with the output below. I’m not sure what it means.

failures:

---- [pretty] run-pass/matches-macro.rs stdout ----

error: pretty-printed source (expanded) does not typecheck
status: exit code: 101
command: x86_64-unknown-linux-gnu/stage2/bin/rustc - --no-trans --crate-type=lib --target=x86_64-unknown-linux-gnu -L x86_64-unknown-linux-gnu/test/run-pass -L x86_64-unknown-linux-gnu/test/run-pass/matches-macro.stage2-x86_64-unknown-linux-gnulibaux --cfg rtopt --cfg debug -O -L x86_64-unknown-linux-gnu/rt
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
warning: --no-trans is deprecated in favor of -Z no-trans
<anon>:24:69: 24:71 error: expected one of `,`, `.`, or `}`, found `&&`
<anon>:24             match bar.char_at(0) { '+' | '-' => true, _ => false, } &&
                                                                              ^~

------------------------------------------

thread '[pretty] run-pass/matches-macro.rs' panicked at 'explicit panic', /home/simon/projects/rust/src/compiletest/runtest.rs:1453



failures:
    [pretty] run-pass/matches-macro.rs

test result: FAILED. 1779 passed; 1 failed; 75 ignored; 0 measured

thread '<main>' panicked at 'Some tests failed', /home/simon/projects/rust/src/compiletest/compiletest.rs:269
/home/simon/projects/rust/mk/tests.mk:778: recipe for target 'tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-pretty-rpass.ok' failed
make: *** [tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-pretty-rpass.ok] Error 101

[alexcrichton]

Our pretty printer isn't necessarily always the best (especially with macros), so feel free to add // ignore-pretty

[ftxqxd]

How exactly is | in patterns ambiguous? rust-lang/rfcs#99 proposed adding alternation to patterns properly, and removing | in match arms, using the | patterns instead. That should be unambiguous because | would be removed in match because it would be unnecessary. That was postponed because it was a backwards-compatible change, but if we go through with this, it will no longer be possible.

Also, if this PR went through and foo | bar later became parsed as a single pattern, code like this would break:

macro_rules! foo {
    ($($p: pat)|+) => {{
        $(let $p = 1i32;)+
    }}
}

foo!(x | y);

I disagree that this change should go through primarily because it's (in my opinion) the wrong solution to a relatively minor aesthetic problem. The right solution (to me) is to extend | into patterns, and then macros like is_match! can just take a single $p: pat. For now, until we get | in patterns, macros can just use a different token like , or || instead.

[SimonSapin]

The right solution (to me) is to extend | into patterns, and then macros like is_match! can just take a single $p: pat.

That sound great! Can I haz it?

For now, until we get | in patterns, macros can just use a different token like , or || instead.

Or, not.

[SimonSapin]

Closing since this is probably not the right fix, and there is a workaround. I opened #20843 for the original issue.