or-patterns: enable :pat to match top_pat

[mark-i-m]

Allow :pat in macros to match a top-level or-pattern, as discussed in #54883 (comment)

I don't have a good way to run the test locally, so I've included a new UI test to make sure the PR does what I expect... if CI passes, please enqueue a (check-only?) crater run.

r? @petrochenkov

cc #54883

EDIT: notably, this makes :pat match top_pat rather than pat<no_top_alt>, which was an open question in the RFC. This is a breaking change, so we will need to do a crater run and see how to proceed. If the breakage is too much, we may go with a compromise, as outlined in #78935 (comment)

EDIT 2: this patch now implements petrochenkov's proposal in #78935 (comment)

[petrochenkov]

parse_top_pat uses RecoverComma::Yes internally, so it parses ,s as |s.

[petrochenkov]

Personally, I expect a lot of breakage, based on the fact that two tests in our own test suite broke.

We could do a one token lookahead in macro_rules and parse path with or without |s depending on whether the next expected token is | or not.

[mark-i-m]

We could do a one token lookahead in macro_rules and parse path with or without |s depending on whether the next expected token is | or not.

Sorry could you elaborate? Do you mean that if a matcher has :pat followed by |, we would parse without top-level or-patterns? Would we allow top-level leading | in that case? I think it could work, but I'm not sure if there are cases where it would confusing.

[petrochenkov]

Do you mean that if a matcher has :pat followed by |, we would parse without top-level or-patterns?

Yes, if the next token is "simply |" (without considering any repetitions or calculating FOLLOW set), then we enter compatibility mode and parse pat as previously, using parse_pat. (Then crater will also show us what cases of "complex |" exist in practice.)

Would we allow top-level leading | in that case?

No, parse_pat doesn't accept it.

[mark-i-m]

Ok, so I've add | to the first set of :pat in the last commit (which is a breaking change). If we make :pat parse a top_pat, then we will have to make this breakage or else implement the scheme @petrochenkov describes in the previous comment.

Alternately, if we make :pat parse as pat<no_top_alt>, I think we don't need to make any breakage, but it seems like it might also be surprising to people that a top-level or-pattern must be surrounded in ( parens )... On the other hand, this would significantly simplify the implementation.

[mark-i-m]

The current implementation makes :pat equivalent to top_pat without any sort of fallback plan (which is a breaking change), so this will show us the "worst-case breakage"...

@petrochenkov Assuming CI passes this time, could you please enqueue a crater run?

[mark-i-m]

CI is passing :)

[petrochenkov]

@bors try

[bors]

⌛ Trying commit 2dbf53908d42f5182e3c7df27c6a894fa533679c with merge 5da6ef1a86b7737c978f534cbc5032d07b84936e...

[bors]

💔 Test failed - checks-actions

[mark-i-m]

@bors retry

(not sure if I have permissions)

[bors]

@mark-i-m: 🔑 Insufficient privileges: not in try users

[petrochenkov]

@bors try

[bors]

⌛ Trying commit 68098dc with merge 961316100510fe780d266d35bcf68702fbb049e5...

[bors]

☀️ Try build successful - checks-actions
Build commit: 961316100510fe780d266d35bcf68702fbb049e5 (961316100510fe780d266d35bcf68702fbb049e5)

[petrochenkov]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-78935/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-78935-1 created and queued.
🤖 Automatically detected try build 961316100510fe780d266d35bcf68702fbb049e5
⚠️ Try build based on commit 2e57231, but latest commit is 68098dc. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-78935-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-78935-1 is completed!
📊 154 regressed and 0 fixed (10993 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[mark-i-m]

Hmm... so these are the regressions:

1. Attempting to simulate a pattern match in a closure argument e.g. |$($arg:pat),+|
   1. leads to parser error
      • all of the rest -- this is the most common regression
   2. leads to type error
      • frappe -- type annotations needed
      • match_any -- bindings in pattern need to have same type
2. Using | as a repetition separator for macros e.g. $($x:pat)|+... I would have expected this to just work. I think this is probably a bug in my implementation.
   • cssparser
3. Made-up syntax is broken by breaking change.
   • batch_oper -- try to match let a | u8 = 1, b = 2 against let $($p:pat $(| $t:ty)? $(= $e:expr)?),*
4. Ambiguity due to leading vert.
   • lazy_format

---

I think one way to mitigate most of the regressions would be to change the rust parser to have a mode for parsing macros so that if it sees a trailing vert (i.e. <pat> |), it just backs up one and leaves the vert intact, ending the :pat before it. I'm not sure how easy or hard that would be (it doesn't sound that hard). This would (I think) resolve the regressions where someone is trying to simulate closure arguments (aside: maybe we should add a :closure metavariable type?).

The other regressions are hard to resolve, I think.

[petrochenkov]

Using | as a repetition separator for macros e.g. $($x:pat)|+... I would have expected this to just work.

Nah, this needs to be specifically implemented.
This means replacing simple lookahead with a smarter lookahead considering FOLLOW sets, including repetitions etc.

have a mode for parsing macros so that if it sees a trailing vert (i.e. <pat> |), it just backs up one and leaves the vert intact

What do you mean by "trailing vert"?
| followed by something that is not a start of pattern? (That possibly mean EOF.)
In this case it can be done as a part of the simple lookahead, we just look one symbol further.

[petrochenkov]

Anyway, at this point it probably makes sense to avoid smart compatibility conditions and just flip the switch at edition boundary, keeping the old behavior on 2015/2018.

[mark-i-m]

@petrochenkov

What do you mean by "trailing vert"?
| followed by something that is not a start of pattern? (That possibly mean EOF.)
In this case it can be done as a part of the simple lookahead, we just look one symbol further.

Yes, exactly this.

Anyway, at this point it probably makes sense to avoid smart compatibility conditions and just flip the switch at edition boundary, keeping the old behavior on 2015/2018.

I agree. Do you want me to change the PR to implement that (or start a new PR)? Or should we nominate the issue for T-lang and wait for a discussion first?

[petrochenkov]

At your discretion.
(The less involvement from me the better.)

[mark-i-m]

I am going to close this since we have the required data. It sounds like we will be pursuing some sort of edition-based transition for :pat.