Migration for or patterns

[nikomatsakis]

As part of the | patterns work (tracking issue), we plan to change the meaning of the $x:pat matcher in macros within the 2021 Edition. The current plan of record is:

• The pat2015 matcher will match legacy patterns, which exclude the | character.
• The pat2021 matcher will match new patterns, which include the | character.
• The pat matcher will be equivalent to pat2021 from Rust 2021 forward, and equivalent to pat2015 on older editions, since past crater runs (e.g., 1 and 2) found significant and complex breakage from just changing pat to pat2021 uniformly.

The goal of this issue is to issue a useful lint that suggests that converting $x:pat to $x:pat2015 if converting to Rust 2021 could cause breakage. We don't want to do this for all $x:pat usages, however, because the vast majority would benefit from being $x:pat2021.

Examples where lint should trigger

Here is an example of a macro that we DO want to convert (source):

#[macro_export]
macro_rules! match_any {
    ( $expr:expr , $( $( $pat:pat )|+ => $expr_arm:expr ),+ ) => {
        match $expr {
            $(
                $( $pat => $expr_arm, )+
            )+
        }
    };
}

Here is another example (source):

/// Helper macro for declaring byte-handler functions with correlating constants.
/// This becomes handy due to a lookup table present below.
macro_rules! define_handlers {
    { $(const $static_name:ident: $name:ident |$tok:pat, $byte:pat| $code:block)* } => {
        $(
            fn $name($tok: &mut Tokenizer, $byte: u8) -> Result<Token> $code

            const $static_name: fn(&mut Tokenizer, u8) -> Result<Token> = $name;
        )*
    }
}

[nikomatsakis]

Mentoring instructions

There is some existing logic in macros to try and detect cases where a matcher is followed by an illegal character. I think we can build on this logic easily enough. Here is an example function I found when ripgreping around:

rust/compiler/rustc_expand/src/mbe/macro_rules.rs

Lines 853 to 871 in 3ad1e4d

	// Checks that `matcher` is internally consistent and that it
	// can legally be followed by a token `N`, for all `N` in `follow`.
	// (If `follow` is empty, then it imposes no constraint on
	// the `matcher`.)
	//
	// Returns the set of NT tokens that could possibly come last in
	// `matcher`. (If `matcher` matches the empty sequence, then
	// `maybe_empty` will be set to true.)
	//
	// Requires that `first_sets` is pre-computed for `matcher`;
	// see `FirstSets::new`.
	fn check_matcher_core(
	sess: &ParseSess,
	features: &Features,
	attrs: &[ast::Attribute],
	first_sets: &FirstSets,
	matcher: &[mbe::TokenTree],
	follow: &TokenSet,
	) -> TokenSet {

I think these three lines are roughly where we want to make changes:

rust/compiler/rustc_expand/src/mbe/macro_rules.rs

Lines 964 to 967 in 3ad1e4d

	if let TokenTree::MetaVarDecl(_, name, kind) = *token {
	for next_token in &suffix_first.tokens {
	match is_in_follow(next_token, kind) {
	IsInFollow::Yes => {}

What this code is currently doing is checking if you have something like $x:expr Y that Y is a character allowed to follow expr. The idea was to future-proof against extensions to what expr could match by accepting only Y characters that already separate expressions, like , or ;. Unfortunately, for $x:pat Y we accepted a Y or |, which is the whole problem here. So what we want to do is to add an extra case here that says "if this is a pat meta variable, and the next_token is `|, then issue a lint warning."

[nikomatsakis]

There is a Zulip thread on or patterns here:

https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/or.20patterns

[Rustin170506]

@rustbot claim

[Rustin170506]

I would like to try this.

[nikomatsakis]

@hi-rustin great!

[m-ou-se]

I changed the difficulty label from easy to medium, since it's not easy to generate a lint at that place in the rustc code.

[m-ou-se]

Producing a lint there with sess.buffer_lint() 'works' for warn-by-default lints. But when making it an allow-by-default lint (as this should be), it triggers an internal compiler error.

error: internal compiler error: failed to process buffered lint here

[nikomatsakis]

@m-ou-se good catch, I forgot about that. I suspect this ICE is... simply not necessary. @hi-rustin have you had a chance to do any exploration here?

The other thing is that @m-ou-se pointed out that we probably want to modify the rules for pat in 2021 at least, so that we give a hard error if | is in the follow-set. We can likely do that as part of this PR, but I will wait until we have the PR in hand to worry about it.

[Rustin170506]

@hi-rustin have you had a chance to do any exploration here?

I'll give it a try, but at the moment I'm not very familiar with the compiler code and it might be difficult.

[nikomatsakis]

@hi-rustin if it would be helpful, we could schedule some time to chat on Zulip synchronously, presuming our time zones can be made to overlap =) If that would be useful to you, let me know and I'll have contact you via email to schedule a time.

[Rustin170506]

@hi-rustin if it would be helpful, we could schedule some time to chat on Zulip synchronously, presuming our time zones can be made to overlap =) If that would be useful to you, let me know and I'll have contact you via email to schedule a time.

Thank you for your support! I've written some code that I'll submit later.

[Rustin170506]

@hi-rustin if it would be helpful, we could schedule some time to chat on Zulip synchronously, presuming our time zones can be made to overlap =) If that would be useful to you, let me know and I'll have contact you via email to schedule a time.

I've created a draft PR, could you please take a look?

cc: #83468