[coolreader18]

View all comments

Reference PR:

• Allow : after pat_param reference#2370

As on the tin. Perhaps this requires a more formal decision process, but in my view this is something of a bug. Originally, pat_param was just the bikeshodden name of pat2015, c.f. #83386 (comment):

• pat_no_or would be used for closure arguments; can we have something that talks about that (e.g., pat_arg, pat_param -- pattern for a closure argument)
• pat_param is preferred because it is a parameter, not the argument passed to a call

But if it truly is a matcher for "a pattern you can use for a parameter", there shouldn't be a problem allowing it to be followed by a colon, given that that's kind of the definitional constraint for that grammar. And if, in the distant future, the language decides to separate the name and type of a parameter with something other than a colon, well, that'll need an edition change anyway.

In my opinion, this would resolve #87724: I'd imagine that most-if-not-all use cases for a pat followed by a colon are for matching function signatures or let statements, which don't let you use | patterns anyway. But, we get to have our cake and eat it too by still reserving : for future extensions to match expression (pat) patterns. If it ever is desired to make pat match the whole $pat : $ty syntax, as mentioned in #87724 (comment), that's something that can be done over an edition boundary and/or just for pat matchers.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 20 candidates

[nnethercote]

This definitely would need approval from the lang team.

cc @rust-lang/lang @rust-lang/lang-advisors
cc @petrochenkov

[traviscross]

Makes sense to me. Thanks @coolreader18, @nnethercote.

@rfcbot fcp merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• extend-migration-lint resolved by Allow : after pat_param #162264 (comment)

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.

[tmandry]

It's surprising we don't already allow this. Thanks for the PR.

@rfcbot reviewed

[scottmcm]

This absolutely makes sense to me. And if for some reason there's an edge case, we have the option later to introduce a new matcher that doesn't allow this.

@rfcbot reviewed

---

There's potentially some surprises that this could cause if people are writing new macros in e2015 (where param means pat_param), but surprises for people doing that doesn't really bother me.

If future if we do want colons for things like Some(x: u32) that wouldn't be impacted by this either because that inner pattern is a full pat, not a pat param.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[traviscross]

@rfcbot concern extend-migration-lint

Do we need to extend rust_2021_incompatible_or_patterns to handle pat followed by :? Let's please add a test for the migration here.

[traviscross]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[coolreader18]

There's potentially some surprises that this could cause if people are writing new macros in e2015 (where param means pat_param), but surprises for people doing that doesn't really bother me.

Yeah I considered making it such that only explicit pat_param allows this, but figured it didn't matter very much.

[coolreader18]

Actually maybe I will make it such that only explicit pat_param allows this? That way we don't need to edit a years-old migration lint to be now about "old versions of or-patterns OR param-followed-by-colon"

[steffahn]

Technically FOLLOW(pat_param) = {FOLLOW(pat), Colon} is not accurate for all editions. I'd say it's more like this:

Suggested change

	// FOLLOW(pat_param) = {FOLLOW(pat), Colon}
	// FOLLOW(pat_param) = {FOLLOW(pat), Colon[, Or]}
	// optional inclusion of `Or` depends on the edition; this is tested separately,
	// in `macro-pat-pattern-followed-by-or.rs` / `macro-pat-pattern-followed-by-or-in-2021.rs`

View changes since the review

[steffahn]

@coolreader18 wait... I got that suggestion completely wrong. The optional inclusions need to be on // FOLLOW(pat) not // FOLLOW(pat_param) 😅

[steffahn]

So hmm... that means this ↓ might be what's actually correct?

// FOLLOW(pat) = {FatArrow, Comma, Eq,[ Or,] Ident(if), MetaVarDecl(Guard), Ident(in)}
// the added inclusion of `Or` is only on editions 2021 or later; this is tested separately,
// in `macro-pat-pattern-followed-by-or.rs` / `macro-pat-pattern-followed-by-or-in-2021.rs`
macro_rules! follow_pat {
    …
}

// FOLLOW(pat_param) = FOLLOW(pat) ∪ {Colon, Or}
macro_rules! follow_pat {
    …
}

[steffahn]

@coolreader18 oh.. just noticing also you used follow_pat twice, probably a new name like follow_pat_param is better ^^

[traviscross]

To summarize, this PR proposes that pat_param should live up to its name by being useful for matching parameters. To do that, we need to allow : after the fragment in a matcher.

But then what should we do about pat prior to Rust 2021? In those editions, pat is an alias for pat_param. The edition migrations don't handle : following pat and so wouldn't migrate $p:pat : to $p:pat_param : (resulting in a broken migration). Should we fix the migrations? The author proposes that we not do that. Instead, the proposal is to allow : only after pat_param and not after pat, even in the editions where pat is otherwise an alias for pat_param.

That makes logical sense given our motivations: the name pat doesn't imply usefulness for matching parameters, so there's no reason to retroactively support : after pat in the earlier editions. It creates some tension, since the behavior of pat_param and that of older-edition pat now differ. But this is less bad than it sounds, as they both continue to match the same grammar — only what's allowed to follow the fragment changes.

We talked about this in the lang meeting today. The solution makes sense to me. Thanks to @coolreader18 for this work.

@rfcbot resolve extend-migration-lint

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rustbot]

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

rustfmt is developed in its own repository. If possible, consider making this change to rust-lang/rustfmt instead.

cc @rust-lang/rustfmt

[coolreader18]

I've added a commit that splits NtPatKind::PatParam { inferred: bool } into two variants, NtPatKind::PatParam and NtPatKind::Pat2018, since they're now more distinct. If it'd be better to do that in a separate PR or to not do it at all, let me know.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[nnethercote]

There is a test tests/ui/macros/macro-follow-rpass.rs that is a partner of macro-follow.rs. Should it be updated?

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.