Implement RFC 1925 #44108

mattico · 2017-08-26T22:14:37Z

rust-highfive · 2017-08-26T22:14:40Z

(rust_highfive has picked a reviewer for you, use r? to override)

mattico · 2017-08-26T22:15:37Z

src/libsyntax/feature_gate.rs

+    fn visit_arm(&mut self, arm: &'a ast::Arm) {
+        if arm.used_beginning_vert {
+            gate_feature_post!(&self, match_beginning_vert,
+                               arm.pats[0].span,


Is there a better span to use here?

I suppose used_beginning_vert could become Option<Span>...

It should be possible to call emit_feature_err immediately from parse_arm, the precise span is available there.

In addition, changes to AST (used_beginning_vert: bool) will likely break rustfmt and you'll have to send PR to rustfmt repo as well, do submodule updates, etc, so I recommend against it.

Oh, I didn't know that features were already available during parsing!

I don't know if they are available as well, but it's worth trying, because the alternative is not especially pleasant.

petrochenkov · 2017-08-26T22:25:56Z

src/test/compile-fail/feature-gate-match_beginning_vert.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


This needs a // gate-test-match_beginning_vert annotation, so the test is counted as a feature gate test.

That shouldn't be necessary as long as the test is named correctly, no? That's what tidy said, and most of the other tests (ex) don't have them.

Okay, if this passes testing, then it's good.

arqunis · 2017-08-26T22:50:44Z

src/libsyntax/parse/parser.rs

@@ -3149,6 +3149,13 @@ impl<'a> Parser<'a> {
        maybe_whole!(self, NtArm, |x| x);

        let attrs = self.parse_outer_attributes()?;
+        // Allow a '|' before the pats (RFC 1925)
+        let beginning_vert;


This should be just

let beginning_vert = if self.eat(&token::BinOp(token::Or)) { Some(self.prev_span) } else { None };

edit: Nevermind, that can work as well.

mattico · 2017-08-27T16:57:34Z

I can't find any way to query features in the parser, and I'm pretty sure it doesn't exist. Nothing else in the parser (outside of libsyntax_ext) uses features, and the features need to be parsed! I see a few paths forward:

Enable this unconditionally. It's not too dangerous of a change, IMO.
Enable this but only on nightly. This way we'd have a chance to catch any strange edge cases before it goes to stable, but it's not gated in the traditional sense. (currently implemented in this PR)
Make the changes to have a real feature gate. Altering the AST to store this information is the path of least resistance, but it has some issues as pointed out by Implement RFC 1925 #44108 (comment)

petrochenkov · 2017-08-27T17:19:42Z

I can't find any way to query features in the parser, and I'm pretty sure it doesn't exist

Ok, sorry for pointing the wrong way.

Altering the AST to store this information is the path of least resistance, but it has some issues

Not issues, just possible inconveniences with updating rustfmt and rls.

carols10cents · 2017-08-28T12:16:06Z

r? @arielb1

arielb1 · 2017-08-29T18:56:39Z

r? @petrochenkov - he's our parser guy

arielb1 · 2017-08-29T18:57:56Z

But currently, I think a feature gate is wide-open

[00:49:52] ---- [compile-fail] compile-fail/feature-gate-match_beginning_vert.rs stdout ----
[00:49:52] 	
[00:49:52] error: compile-fail test compiled successfully!
[00:49:52] status: exit code: 0

[00:49:52] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/feature-gate-match_beginning_vert.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/feature-gate-match_beginning_vert.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/feature-gate-match_beginning_vert.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux" "-A" "unused"
[00:49:52] stdout:
[00:49:52] ------------------------------------------
[00:49:52] 
[00:49:52] ------------------------------------------
[00:49:52] stderr:
[00:49:52] ------------------------------------------
[00:49:52] 
[00:49:52] ------------------------------------------
[00:49:52] 
[00:49:52] thread '[compile-fail] compile-fail/feature-gate-match_beginning_vert.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2435:8
[00:49:52] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:49:52] 
[00:49:52] 
[00:49:52] failures:
[00:49:52]     [compile-fail] compile-fail/feature-gate-match_beginning_vert.rs
[00:49:52] 
[00:49:52] test result: FAILED. 2724 passed; 1 failed; 13 ignored; 0 measured; 0 filtered out
[00:49:52]

arielb1 · 2017-08-29T18:59:19Z

src/libsyntax/parse/parser.rs

@@ -3149,6 +3150,14 @@ impl<'a> Parser<'a> {
        maybe_whole!(self, NtArm, |x| x);

        let attrs = self.parse_outer_attributes()?;
+        if self.eat(&token::BinOp(token::Or))
+            && !self.sess.unstable_features.is_nightly_build() {


You should use sess.features.borrow().match_beginning_vert here, otherwise the feature gate won't work

sess is a ParseSess which does not have a features field.

mattico · 2017-08-29T19:11:02Z

@arielb1 yep! waiting for confirmation that a feature gate is necessary/desired before I reimplement it by mucking with the AST.

petrochenkov · 2017-09-01T16:03:31Z

@mattico
Citing @nikomatsakis:

Basically every time I've skipped a feature gate I've regretted it.

Let's add a feature gate.

mattico · 2017-09-01T16:46:23Z

@petrochenkov sounds good, will do!

mattico · 2017-09-01T19:28:25Z

Interesting failures. Taking a look.

mattico · 2017-09-01T22:00:02Z

I ran a build of rustfmt, it works fine with this change. They only access single fields of Arm, so it doesn't break anything. RLS also works. Clippy was already broken before these changes.

petrochenkov · 2017-09-02T13:34:23Z

@bors r+

bors · 2017-09-02T13:34:24Z

📌 Commit 22ca03b has been approved by petrochenkov

bors · 2017-09-02T22:22:59Z

⌛ Testing commit 22ca03b with merge 6f66730...

Implement RFC 1925 cc #44101

bors · 2017-09-03T00:51:43Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 6f66730 to master...

rust-highfive assigned steveklabnik Aug 26, 2017

mattico commented Aug 26, 2017

View reviewed changes

petrochenkov reviewed Aug 26, 2017

View reviewed changes

arqunis suggested changes Aug 26, 2017

View reviewed changes

mattico force-pushed the match-pipe branch 2 times, most recently from c9bb16e to f4dc91a Compare August 27, 2017 03:50

rust-highfive assigned arielb1 and unassigned steveklabnik Aug 28, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2017

rust-highfive assigned petrochenkov and unassigned arielb1 Aug 29, 2017

arielb1 reviewed Aug 29, 2017

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2017

mattico force-pushed the match-pipe branch 2 times, most recently from f531b24 to e631b8d Compare September 1, 2017 17:27

Implement RFC 1925

f866152

mattico force-pushed the match-pipe branch from e631b8d to f866152 Compare September 1, 2017 17:46

mattico added 2 commits September 1, 2017 14:39

Fix arm visitor

eb143c3

Fix unstable book example

22ca03b

bors added a commit that referenced this pull request Sep 2, 2017

Auto merge of #44108 - mattico:match-pipe, r=petrochenkov

6f66730

Implement RFC 1925 cc #44101

bors merged commit 22ca03b into rust-lang:master Sep 3, 2017

bors mentioned this pull request Sep 3, 2017

hide internal types/traits from std docs via new #[doc(masked)] attribute #44026

Merged

mattico deleted the match-pipe branch September 3, 2017 02:52

aturon mentioned this pull request Sep 15, 2017

Tracking issue for RFC 1925: Allow an optional vert at the beginning of a match branch #44101

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 1925 #44108

Implement RFC 1925 #44108

mattico commented Aug 26, 2017

rust-highfive commented Aug 26, 2017

mattico Aug 26, 2017 •

edited

Loading

petrochenkov Aug 26, 2017

mattico Aug 26, 2017

petrochenkov Aug 26, 2017

petrochenkov Aug 26, 2017

mattico Aug 26, 2017

petrochenkov Aug 26, 2017

arqunis Aug 26, 2017 •

edited

Loading

mattico commented Aug 27, 2017

petrochenkov commented Aug 27, 2017

carols10cents commented Aug 28, 2017

arielb1 commented Aug 29, 2017

arielb1 commented Aug 29, 2017

arielb1 Aug 29, 2017

mattico Aug 29, 2017

mattico commented Aug 29, 2017

petrochenkov commented Sep 1, 2017

mattico commented Sep 1, 2017

mattico commented Sep 1, 2017

mattico commented Sep 1, 2017

petrochenkov commented Sep 2, 2017

bors commented Sep 2, 2017

bors commented Sep 2, 2017

bors commented Sep 3, 2017

Implement RFC 1925 #44108

Implement RFC 1925 #44108

Conversation

mattico commented Aug 26, 2017

rust-highfive commented Aug 26, 2017

mattico Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arqunis Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

mattico commented Aug 27, 2017

petrochenkov commented Aug 27, 2017

carols10cents commented Aug 28, 2017

arielb1 commented Aug 29, 2017

arielb1 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattico commented Aug 29, 2017

petrochenkov commented Sep 1, 2017

mattico commented Sep 1, 2017

mattico commented Sep 1, 2017

mattico commented Sep 1, 2017

petrochenkov commented Sep 2, 2017

bors commented Sep 2, 2017

bors commented Sep 2, 2017

bors commented Sep 3, 2017

mattico Aug 26, 2017 •

edited

Loading

arqunis Aug 26, 2017 •

edited

Loading