Github Stabilize `:pat_param` and remove `:pat2021` by mark-i-m · Pull Request #...

nikomatsakis commented 29 days ago

The main reason I have not r+'d this is that there was some active debate about the best name. There are two proposals on the table:

• introduce pat2015 and pat2021 and pat changes from pat2015 to pat2021 in the Rust 2021 edition
  • pro: one can reference either meaning easily in any edition
  • con: the edition-based names are not very meaningful, and there are still valid reasons beyond back-compat to use pat2015 (if you want a single pattern and not a list of them)
• introduce pat_atom for the current pat, and change pat to |-separated list in 2021 Edition
  • pro: 'pat atom' is a reasonably meaningful name, though kind of parser jargon
  • con: no way to reference |-pattern in older editions
• introduce pats and leave pat unchanged
  • pro: backwards compatible
  • con: most uses of pat could actually accept | and now they don't work

As far as I know, the final option was rejected and nobody is asking for it, but I included it out of completeness. Some other possible names for pat-atom:

• pat_single
• pat_no_or
• pat1

joshtriplett commented 29 days ago

I think there's value in going ahead with the name pat2015 regardless. If it turns out that a significant number of people want to use pat2015 in later editions for reasons other than edition migration, we can always introduce an alias like pat_no_or later, while still keeping pat2015. But I think the name pat2015 is usefully evocative for the purposes of edition migration.

m-ou-se commented 28 days ago

pat2015 seems like the least useful name of all possibilities. "whatever pat meant in 2015 if you can still remember" isn't a very meaningful name, especially since the difference is very simple (not allowing |) which we can also express directly in the name. Considering we already know about valid use cases where you want the non-or variant (e.g. |$arg:pat_atom| $e:expr), I don't think we should make this mistake on purpose and only fix it when it turns out to affect more people that we like, since we can easily just fix it now.

joshtriplett commented 28 days ago

I'm not arguing against having another name for it, and if we believe people are likely to end up needing it, I don't object to giving it another name; I'm just trying to make it not a blocker for the edition, by raising the possibility of introducing an alias later. If we can agree on a name quickly, that works.

I think pat_atom is not a good name; it feels like it's seen through a compiler view, and I don't think we use "atom" anywhere else. If we're going to give it a describe name, I think pat_no_or seems reasonable.

I mildly like the idea of keeping pat2015 and making any other name an alias, for simplicity's sake when doing an edition migration. But I won't push for that idea, and I won't object to just calling it pat_no_or or similar.

nikomatsakis commented 28 days ago

I think pat_atom is not a good name; it feels like it's seen through a compiler view, and I don't think we use "atom" anywhere else. If we're going to give it a describe name, I think pat_no_or seems reasonable.

I think pat_no_or was @m-ou-se's first suggestion. I counterproposed pat_atom because I would prefer a name that describes what it is, not what it isn't. That said, I agree that "atom" isn't the best -- it makes sense to me, because it's the "atom" from which a full pattern pat is created, but it's not ideal.

I appreciate your point Josh about unblocking the edition. On the other hand, I don't think I want a ton of aliases for the same thing and we're not like "up against it" when it comes to edition timing. I don't want a long bikeshed, but it seems like we can indulge a short one.

I think I'd prefer to zoom out a bit and think about general Edition policy. The question here really seems to be: do we always introduce a 2015 vs 2021 name, or do we only do that when the 2015 item is effectively deprecated and we don't expect anyone to use it. @m-ou-se is arguing for the former, and I think @joshtriplett you are (perhaps implicitly) arguing for the latter. I'm not 100% sure what I believe, but I would prefer we pick the choice we think the end user would want and not let timing dictate it unless we have to.

nikomatsakis commented 28 days ago

When it comes to the name itself, can we spend a bit of time brainstorming options?

I will say that pat_no_or seems clear, though I wonder if it would seem as clear if you weren't coming into the middle of all this context.

Also, I could imagine this pattern arising in other places too with other separators and I'd prefer if we had a name that could be re-used -- although that may be a fool's errand. I guess a pattern (e.g., expr_no_comma or something) that can be reused is also decent. As I wrote, pat_atom was meant to convey the "atom from which a full pattern is composed", but it's not ideal for the reasons given.

single_pat is the next thing I thought of, but I think it should have the form pat_XX. It's too bad English puts adjectives first, it doesn't play well with sorting alphabetically. =) pat_single or pat_one doesn't feel ... as obvious.

pat_element?

digama0 commented 28 days ago

I will put in a vote for pat1, it's short and indicates that it is related to pat.

nikomatsakis commented 28 days ago •

I think that instead of just tossing out names, we should submit entries based on what the rust reference might say. Here is the relevant part of the rust reference. Currently it says pat: a Pattern. I guess it would be something like

• pat -- a Pattern like Some(_); patterns can include |, so this also matches Some(_) | None
• pat1 -- a single Pattern, excluding | at the top level

It's actually kind of hard to write! I feel like I want another noun.

m-ou-se commented 28 days ago

The definition at https://doc.rust-lang.org/reference/patterns.html right now doesn't allow for Some(_) | None, right? I don't see anything there that would cover |. I suppose the reference will also need a way to refer to 'pattern with |' and 'pattern without |'.

digama0 commented 28 days ago

I think that instead of just tossing out names, we should submit entries based on what the rust reference might say. Here is the relevant part of the rust reference. Currently it says pat: a Pattern. I guess it would be something like

* `pat` -- a [Pattern](https://doc.rust-lang.org/reference/patterns.html) like `Some(_)`; patterns can include `|`, so this also matches `Some(_) | None`

* `pat1` -- a single [Pattern](https://doc.rust-lang.org/reference/patterns.html), excluding `|` at the top level

It's actually kind of hard to write! I feel like I want another noun.

Actually, the way it is stated in the reference, it isn't difficult at all. Each matcher just points to a nonterminal in the BNF, so it would just be "a [Pattern]" and "a [PatternWithoutOr]" with links to the respective BNF productions (note that Pattern and PatternWithoutOr are mutually recursive, so they will both exist in the BNF anyway). I just noticed that PatternWithoutRange already exists, so there is a precedent for this.

nikomatsakis commented 28 days ago

@m-ou-se yes, the reference is typically not updated until the stabilization is complete, though there ought to be a pending PR-- ah, there it is, @mark-i-m opened rust-lang/reference#957.

nikomatsakis commented 28 days ago •

@digama0 right, but I would like people to not have to read the BNF -- I also think the name of the production should (ideally) line up with the fragment specifier name. So that suggests naming things pat_no_or and PatNoOr (and PatNoRange) if we want to follow that precedent. I guess my resistance is weakening. =) I don't think this is that important.

nikomatsakis commented 28 days ago •

I'd still like to see the docs written. Maybe something like this?

• pat -- matches a Pattern like Some(_) or Some(_) | None
• pat_no_or -- matches PatternNoOr, which is a pattern that excludes | (e.g., Some(_) or None but not Some(_) | None)

digama0 commented 28 days ago

For completeness, @mark-i-m 's PR proposes PatternNoTopAlt for the name in the BNF instead of PatternWithoutOr.

m-ou-se commented 28 days ago

@m-ou-se yes, the reference is typically not updated until the stabilization is complete, though there ought to be a pending PR-- ah, there it is, @mark-i-m opened rust-lang/reference#957.

What I meant is that the reference will also need a word to refer to these 'not-or patterns', and it'd be good if the :pat2015 name is the same or similar as what the reference uses/will use.

That PR proposes PatternNoTopAlt in the reference, which would match something like :pat_no_top_alt. I'd prefer an alternative, but it'd be good to consider the name in the reference and the name for :pat2015 together.

nikomatsakis commented 28 days ago

I definitely think "no top alt" is not a very obvious name :)

nikomatsakis commented 27 days ago

Ah, actually, what about pat_alternative as a name instead of pat_atom? Then the write-up is something like:

• pat -- a [Pattern] like Some(_); patterns can include a number of alternatives separated by |, so this also matches Some(_) | None
• pat_alternative -- a single [PatternAlternative], like Some(_) or None (but not Some(_) | None, although | can be embedded via parentheses)

mark-i-m commented 24 days ago

FWIW, "no top alt" is the name from the RFC. I agree it's not a great name.

Also, I would like to point out that pat_no_or sounds like or-patterns are entirely disabled, whereas we only disallow them at the top level.

How about subpat? i.e. we would have pat which allows top-level or, and subpat which doesn't.

nikomatsakis commented 24 days ago

@mark-i-m can you phrase that suggestion as "text in the reference"? I'd like to see how it looks.

mark-i-m commented 23 days ago

@nikomatsakis perhaps something like:

• pat -- any [Pattern], e.g. Some(foo) or Enum::A | Enum::B. This includes all types of patterns, including any number of subpatterns or'ed together (with |), as long as the types and bindings agree.
• subpat -- any [PatternNoTopAlt] (aside: perhaps we change the name of the production to SubPattern?), e.g. Some(foo) or Enum::A or Enum::B. This includes any individual subpattern that can appear in a pat, but it does not allow top-level or-patterns, like pat does.

nikomatsakis commented 22 days ago

@mark-i-m thanks. I guess I think "subpattern" is viable. I think I vaguely prefer pat_alternative, because it is more specific and because it also starts with pat, so it'll appear in the right place when lists are alphabetically ordered. But it's a weak preference.

digama0 commented 21 days ago

The issue I have with pat_alternative with pat is that I read it the other way around: if those were the names, then I would expect pat to be just a single pattern and pat_alternative contains alternatives of patterns, i.e. pat | pat.

nikomatsakis commented 21 days ago

@digama0 ah, that's useful feedback. I can see that.

nikomatsakis commented 16 days ago •

OK, it seems like we've collected a number of options and we can now kind of pull them into a write-up for comparison and just pick one. Folks who care can vote using emoji =) cc @rust-lang/lang

(As always, votes are informative, not binding)

Some notes:

• Atom strikes some as jargon
• Alternative was understood by some as meaning "contains multiple alternatives", not "a single alternative"
• The "without |" can always use parentheses to include multiple patterns

m-ou-se commented 16 days ago

So we wanted to change the meaning of :pat to include or-patterns, but it turned out we actually want to keep the old meaning of :pat around too (and not just for migration). So now the plan is to still change the meaning of :pat and add a new word to take on the old meaning of :pat. Does that still make sense with this new insight?

Why not just keep :pat for non-or-patterns? Then the 'one or more patterns separated by |' can just be a new word (e.g. the plural version :pats or something). Then it's no longer an edition change, and doesn't require a migration. It'd also be easier to document.

nikomatsakis commented 16 days ago

Notes from lang team planning meeting:

• subpat assumes no further "subsets" of patterns will be chosen
• we do want it to alphabetize when you type it
• 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
• meeting recommendation: pat_param!

rfcbot commented 6 days ago

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.

The RFC will be merged soon.

mark-i-m commented 5 days ago

This PR should be ready to merge.

compiler/rustc_ast/src/token.rs

Outdated

Show resolved

nikomatsakis commented 4 days ago

So, @mark-i-m, reviewing the PR, it doesn't quite match my expectation. I did not expect to have pat2021, just the new version of pat. The idea then would be that we will migrate things to use pat_param where needed, so that they are compatible when they move to Rust 2021.

mark-i-m commented 3 days ago

@nikomatsakis The behavior currently implemented is:

• all editions:
  • pat_param does not match or-patterns
  • pat2021 does match or-patterns
  • pat2021 is unstable behind gate edition_macro_pats
• edition 2015/18: pat == pat_param
• edition 2021: pat == pat2021

In the implementation, pat desugars to either pat_param or pat2021 depending on the edition of the code defining the macro.

Does that match your expectation? Or am I misunderstanding something?

nikomatsakis commented 3 days ago

@mark-i-m the main surprise is that I don't expect to have a pat2021 nonterminal available to end-users at all.

That said, I don't actually have a problem with it; thinking more about it, I might even be in favor, as it fits our general approach of exposing functionality to older editions (e.g., via k#foo keywords) when we can easily do so.

I'm not 100% sure what the FCP decision was, but I'd be curious to hear what the rest of @rust-lang/lang thinks about this.

I think I am now in favor of landing "as is".

nikomatsakis commented 3 days ago

Leaving this nominated for the @rust-lang/lang triage meeting today.

ehuss commented 3 days ago

Just checking if this is likely to merge in 1.53 (before Friday, or a beta backport later). I'd like to know if it should be included in rust-lang/reference#957. I don't think we have ever done documentation backports, though I guess that is possible, I would prefer to avoid it if we know how this PR will shake out.

nikomatsakis commented 3 days ago •

We discussed this in today's @rust-lang/lang meeting and the meeting consensus, I believe, was that we should expose the following specifiers:

and have no pat2021 fragment specifier externally.

Some notes about our reasoning:

• Question:
  • do we have pat2021 that is 'pat with |' in all editions
• Reason to do so:
  • It is nice if older editions can access newer semantics, albeit with a more confusing way
• Reasons not to do so:
  • (a) they can acccess the newer semantics by doing $($p:pat)|+ (or by upgrading to the newer edition)
    • But: this is significant more complex and they may well make a subtle mistake
      • But: macros do this today
    • But: why not adopt newer edition? probably because want support for old compiler
  • (b) newer editions have no use for this because it's just the same as pat
  • (c) if we add a new variant of pat, we can give it a meaningful name like pat_param at that time
  • (d) cruft, YAGNI
    • It would be a shame if we ended up with a big list of pat2021, pat2027, ...
• Precedent:
  • we tend to give years for 'old names that are deprecated but retained for backwards compat'

mark-i-m commented 2 days ago

@nikomatsakis Should be ready. Let me know what you think. Thanks!

nikomatsakis commented 2 days ago

@bors r+

bors commented 2 days ago

Commit 2a9db91 has been approved by nikomatsakis

bors commented 2 days ago

Testing commit 2a9db91 with merge ca075d2...

bors commented yesterday

Test successful - checks-actions
Approved by: nikomatsakis
Pushing ca075d2 to master...