New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for RFC 554: `pattern_parentheses` feature #51087

Closed
kennytm opened this Issue May 26, 2018 · 24 comments

Comments

Projects
None yet
@kennytm
Member

kennytm commented May 26, 2018

This tracks the pattern_parentheses feature which allows using parenthesis to group patterns.
(relevant RFC issue: rust-lang/rfcs#554).

match x {
    (y) => {}
}

bors added a commit that referenced this issue May 26, 2018

Auto merge of #51090 - kennytm:tidy-check-missing-tracking-issue, r=a…
…lexcrichton

Ensure every unstable language feature has a tracking issue.

Filled in the missing numbers:

* `abi_ptx` → #38788
* `generators` → #43122
* `global_allocator` → #27389

Reused existing tracking issues because they were decomposed from a larger feature

* `*_target_feature` → #44839 (reusing the old `target_feature` number)
* `proc_macros_*` → #38356 (reusing the to-be-stabilized `proc_macros` number)

Filed new issues

* `exhaustive_patterns` → #51085
* `pattern_parentheses` → #51087
* `wasm_custom_section` and `wasm_import_module` → #51088

bors added a commit that referenced this issue May 27, 2018

Auto merge of #51090 - kennytm:tidy-check-missing-tracking-issue, r=a…
…lexcrichton

Ensure every unstable language feature has a tracking issue.

Filled in the missing numbers:

* `abi_ptx` → #38788
* `generators` → #43122
* `global_allocator` → #27389

Reused existing tracking issues because they were decomposed from a larger feature

* `*_target_feature` → #44839 (reusing the old `target_feature` number)
* `proc_macros_*` → #38356 (reusing the to-be-stabilized `proc_macros` number)

Filed new issues

* `exhaustive_patterns` → #51085
* `pattern_parentheses` → #51087
* `wasm_custom_section` and `wasm_import_module` → #51088
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 29, 2018

Nominating for lang-team discussion. I think that we ought to stabilize this feature. It extends the pattern grammar with parentheses — really not much to say about it. That said, looking about the repo i didn't find many tests. One of my motivations is to address the "precedence fix" of ..= patterns described here -- in particular, I'd like you to be able to write &(3 ..= 5). (But I don't know that we have a test for this, will have to check.)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 29, 2018

(Also I think we should make sure to extend the Rust Reference before stabilizing.)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 31, 2018

@rfcbot fcp merge

I would like to move to stabilize this feature. I am going to break my own rules by not providing a stability report that links to tests, but as part of the stabilization PR (or earlier...) we can add some of the tests that I think are missing (or maybe they are there and I did not see them).

What is being stabilized

Introducing the ability to have parenthesized patterns (P). This can be used to enable precedence between patterns, and is specifically needed to have a pattern like &(3 ..= 5) or &(3 ... 5) (note that, for reasons of backwards compatibility, &3 ... 5 is accepted but discouraged).

@rfcbot

This comment has been minimized.

rfcbot commented May 31, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

rfcbot commented Jun 14, 2018

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

@scottmcm

This comment has been minimized.

Member

scottmcm commented Jun 14, 2018

Definitely feels like this needs careful tests, especially about weird macro stuff I don't understand, but if people are fine with doing that as part of stabilization then I'm fine with that too.

@rfcbot reviewed

@cramertj

This comment has been minimized.

Member

cramertj commented Jun 14, 2018

macro_rules! foo {
    ($x:pat) => { "pattern" };
    ($x:expr) => { "expr" };
}

fn main() {
    println!("{}", foo!((4)));
}

This currently compiles on stable in 1.26 and prints "pattern", so we've already stabilized that this syntax parses as a pattern. I think that for changes like this we should start tracking branches hit during parsing in order to feature gate, rather than traversing the resulting AST, which fails to catch cases like this where (4) isn't used as a pattern but parses successfully as a pattern.

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 14, 2018

@cramertj We can probably solve that by moving to a sort of "persistent forest" for caching parsed AST fragments, for both macro invocations, but also the $x:foo fragments.
There are other benefits to an approach like that, but this is probably not the place to list them.

(Note that we can't know the feature gate set before parsing and macro-expanding the crate)

@cramertj

This comment has been minimized.

Member

cramertj commented Jun 14, 2018

@eddyb

Note that we can't know the feature gate set before parsing and macro-expanding the crate

Right-- I was proposing that we track the set of features used during parsing and macro expansion and only report errors once we had finished and seen that the list of features used wasn't a subset of the features enabled.

@scottmcm

This comment has been minimized.

Member

scottmcm commented Jun 18, 2018

This probably doesn't rise to a full concern, but I think the unused_parens lints must be extended to also catch unused parentheses in patterns before this gets stabilized.

    match (1) { // Warns, as expected
        (_) => {} // Doesn't warn, but should for consistency
    }

https://play.rust-lang.org/?gist=9f08276033eab518cd8055ff148c266a&version=nightly

@rfcbot

This comment has been minimized.

rfcbot commented Jun 24, 2018

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

@nrc

This comment has been minimized.

Member

nrc commented Jul 3, 2018

@rust-lang/compiler @petrochenkov Is anyone keen to implement the stability PR for this (or create an issue with mentoring instructions)? It is a factor in one of the edition lints (#51043 (comment)) so is somewhat urgent.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 15, 2018

Triage: no movement since 2018-07-04; cc @rust-lang/compiler

@Centril Centril added the T-compiler label Sep 20, 2018

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 20, 2018

Mentoring instructions

This is a straightforward stabilization PR, so you can follow the instructions found in the forge page "So you want to stabilize a feature?".

In this case, the feature gate is: pattern_parentheses

@scottmcm

This comment has been minimized.

Member

scottmcm commented Sep 20, 2018

Also, unused_patterns lint is still not updated; #51087 (comment)

@pnkfelix pnkfelix assigned pnkfelix and unassigned pnkfelix Sep 20, 2018

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 20, 2018

@scottmcm ah, good catch, although I think it's not a blocker, as we can extend lints after the fact. Regardless, here are some tips for how to update that lint. The lint itself is declared here:

declare_lint! {
pub(super) UNUSED_PARENS,
Warn,
"`if`, `match`, `while` and `return` do not need parentheses"
}

The meat of it is found in the check_expr callback here, which is invoked for every expression:

fn check_expr(&mut self, cx: &EarlyContext, e: &ast::Expr) {

As you can see, it checks for various places where a lint would not be needed, such as in the body if an If, and then invokes this routine check_unused_patterns_core to issue a warning.

For patterns, I imagine we'll want to do something similar, but adding a check_pat method (this is the definition from the trait).

That said, I think this is quite a bit harder than the stabilization itself. If the person who handles this issue wants to do it, seems great, but if they are not familiar with the compiler, I think we could leave it out from the first PR, and then move this to a distinct issue to be added at some later time.

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented Sep 20, 2018

i've been looking for a good way to contribute and this seems like something i could take a look at this weekend but if someone else wants to tackle this ahead of time let me know! -- i will probably skip the lint bit

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented Sep 23, 2018

@nikomatsakis i followed the forge guide for PR #54497.

I've built the compiler locally and run on a test file incorporating the above sample: https://play.rust-lang.org/?gist=c8579523967ed072ea07cac191ee52cd&version=nightly&mode=debug&edition=2015

I ran all of the tests (./x.py test) and don't think I introduced any new problems.

Two questions:

  1. Should we add more thorough tests that explicitly test for this pattern? There are tests which implicitly check some modes of behavior (e.g. /src/test/ui/run-pass/binding/pat-tuple-7.rs).

  2. Any pointers on documentation? Seems like this should be updated: https://doc.rust-lang.org/reference/expressions/match-expr.html, but not sure what else makes sense right now.

This PR doesn't include the lint issue discussed above.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 24, 2018

Filed #54538 for the unused-patterns lint.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 24, 2018

@ralexstokes

Should we add more thorough tests that explicitly test for this pattern?

Well, more tests are always good. I see that I did not an absence of tests here. I'm not sure there are any particularly tricky scenarios here, but if you wanted to try to add parenthesis into various existing pattern tests, it seems like it would be good.

Any pointers on documentation? Seems like this should be updated: https://doc.rust-lang.org/reference/expressions/match-expr.html, but not sure what else makes sense right now.

Good question. I would think we would want to update the grammar on patterns, but I didn't see that one was defined in there. (cc @alercah @Havvy)

@Havvy

This comment has been minimized.

Contributor

Havvy commented Sep 25, 2018

Cc @ehuss

bors added a commit that referenced this issue Sep 26, 2018

Auto merge of #54497 - ralexstokes:stabilize_pattern_parentheses, r=n…
…ikomatsakis

Stabilize pattern_parentheses feature

Addresses #51087 .

Stabilizes the previously unstable feature `pattern_parentheses` which enables the use of `()` in match patterns.
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 27, 2018

Removing P-high marker— though I would definitely appreciate some feedback from the @rust-lang/docs folk on what doc if any they feel is needed for this. Seems pretty minor though.

@Havvy

This comment has been minimized.

Contributor

Havvy commented Sep 27, 2018

The reference is updated. We'll also want to update TRPL though, I think.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 27, 2018

Filed in the TRPL repo; closing therefore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment