in which parentheses are suggested for should-have-been-tuple-patterns

zackmdavis · zackmdavis · commit ed6679cd6fed · 2018-02-27T21:08:57.000-08:00
Programmers used to working in some other languages (such as Python or Go) might expect to be able to destructure values with comma-separated identifiers but no parentheses on the left side of an assignment. Previously, the first name in such code would get parsed as a single-indentifier pattern—recognizing, for example, the `let a` in `let a, b = (1, 2);`—whereupon we would have a fatal syntax error on seeing an unexpected comma rather than the expected semicolon (all the way nearer to the end of `parse_full_stmt`). Instead, let's look for that comma when parsing the pattern, and if we see it, make-believe that we're parsing the remaining elements in a tuple pattern, so that we can suggest wrapping it all in parentheses. We need to do this in a separate wrapper method called on the top-level pattern (or `|`-patterns) in a `let` statement, `for` loop, `if`- or `while let` expression, or match arm rather than within `parse_pat` itself, because `parse_pat` gets called recursively to parse the sub-patterns within a tuple pattern. Resolves #48492.
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
@@ -3294,7 +3294,7 @@ impl<'a> Parser<'a> {
                           mut attrs: ThinVec<Attribute>) -> PResult<'a, P<Expr>> {
         // Parse: `for <src_pat> in <src_expr> <src_loop_block>`
 
-        let pat = self.parse_pat()?;
+        let pat = self.parse_top_level_pat()?;
         if !self.eat_keyword(keywords::In) {
             let in_span = self.prev_span.between(self.span);
             let mut err = self.sess.span_diagnostic
@@ -3466,7 +3466,7 @@ impl<'a> Parser<'a> {
     fn parse_pats(&mut self) -> PResult<'a, Vec<P<Pat>>> {
         let mut pats = Vec::new();
         loop {
-            pats.push(self.parse_pat()?);
+            pats.push(self.parse_top_level_pat()?);
 
             if self.token == token::OrOr {
                 let mut err = self.struct_span_err(self.span,
@@ -3690,6 +3690,37 @@ impl<'a> Parser<'a> {
         }))
     }
 
+    /// A wrapper around `parse_pat` with some special error handling for the
+    /// "top-level" patterns in a match arm, `for` loop, `let`, &c. (in contast
+    /// to subpatterns within such).
+    pub fn parse_top_level_pat(&mut self) -> PResult<'a, P<Pat>> {
+        let pat = self.parse_pat()?;
+        if self.token == token::Comma {
+            // An unexpected comma after a top-level pattern is a clue that the
+            // user (perhaps more accustomed to some other language) forgot the
+            // parentheses in what should have been a tuple pattern; emit a
+            // suggestion-enhanced error here rather than choking on the comma
+            // later.
+            let comma_span = self.span;
+            self.bump();
+            if let Err(mut err) = self.parse_pat_tuple_elements(false) {
+                // We didn't expect this to work anyway; we just wanted
+                // to advance to the end of the comma-sequence so we know
+                // the span to suggest parenthesizing
+                err.cancel();
+            }
+            let seq_span = pat.span.to(self.prev_span);
+            let mut err = self.struct_span_err(comma_span,
+                                               "unexpected `,` in pattern");
+            if let Ok(seq_snippet) = self.sess.codemap().span_to_snippet(seq_span) {
+                err.span_suggestion(seq_span, "try adding parentheses",
+                                    format!("({})", seq_snippet));
+            }
+            err.emit();
+        }
+        Ok(pat)
+    }
+
     /// Parse a pattern.
     pub fn parse_pat(&mut self) -> PResult<'a, P<Pat>> {
         maybe_whole!(self, NtPat, |x| x);
@@ -3881,7 +3912,7 @@ impl<'a> Parser<'a> {
     /// Parse a local variable declaration
     fn parse_local(&mut self, attrs: ThinVec<Attribute>) -> PResult<'a, P<Local>> {
         let lo = self.prev_span;
-        let pat = self.parse_pat()?;
+        let pat = self.parse_top_level_pat()?;
 
         let (err, ty) = if self.eat(&token::Colon) {
             // Save the state of the parser before parsing type normally, in case there is a `:`
diff --git a/src/test/ui/did_you_mean/issue-48492-tuple-destructure-missing-parens.rs b/src/test/ui/did_you_mean/issue-48492-tuple-destructure-missing-parens.rs
@@ -0,0 +1,89 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <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.
+
+#![allow(unused)]
+
+#[derive(Copy, Clone)]
+enum Nucleotide {
+    Adenine,
+    Thymine,
+    Cytosine,
+    Guanine
+}
+
+#[derive(Clone)]
+struct Autosome;
+
+#[derive(Clone)]
+enum Allosome {
+    X(Vec<Nucleotide>),
+    Y(Vec<Nucleotide>)
+}
+
+impl Allosome {
+    fn is_x(&self) -> bool {
+        match *self {
+            Allosome::X(_) => true,
+            Allosome::Y(_) => false,
+        }
+    }
+}
+
+#[derive(Clone)]
+struct Genome {
+    autosomes: [Autosome; 22],
+    allosomes: (Allosome, Allosome)
+}
+
+fn find_start_codon(strand: &[Nucleotide]) -> Option<usize> {
+    let mut reading_frame = strand.windows(3);
+    while let b1, b2, b3 = reading_frame.next().expect("there should be a start codon") {
+        //~^ ERROR unexpected `,` in pattern
+        // ...
+    }
+    None
+}
+
+fn find_thr(strand: &[Nucleotide]) -> Option<usize> {
+    let mut reading_frame = strand.windows(3);
+    let mut i = 0;
+    if let b1, b2, b3 = reading_frame.next().unwrap() {
+        //~^ ERROR unexpected `,` in pattern
+        match (b1, b2, b3) {
+            //~^ ERROR cannot find value `b2` in this scope
+            //~| ERROR cannot find value `b3` in this scope
+            Nucleotide::Adenine, Nucleotide::Cytosine, _ => {
+                //~^ ERROR unexpected `,` in pattern
+                return Some(i);
+            },
+            _ => {}
+        }
+        i += 1;
+    }
+    None
+}
+
+fn analyze_sex_chromosomes(women: &[Genome], men: &[Genome]) {
+    for x, _barr_body in women.iter().map(|woman| woman.allosomes.clone()) {
+        //~^ ERROR unexpected `,` in pattern
+        // ...
+    }
+    for x, y @ Allosome::Y(_) in men.iter().map(|man| man.allosomes.clone()) {
+        //~^ ERROR unexpected `,` in pattern
+        // ...
+    }
+}
+
+fn main() {
+    let genomes = Vec::new();
+    let women, men: (Vec<Genome>, Vec<Genome>) = genomes.iter().cloned()
+    //~^ ERROR unexpected `,` in pattern
+        .partition(|g: &Genome| g.allosomes.0.is_x() && g.allosomes.1.is_x());
+}
diff --git a/src/test/ui/did_you_mean/issue-48492-tuple-destructure-missing-parens.stderr b/src/test/ui/did_you_mean/issue-48492-tuple-destructure-missing-parens.stderr
@@ -0,0 +1,50 @@
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:47:17
+   |
+47 |     while let b1, b2, b3 = reading_frame.next().expect("there should be a start codon") {
+   |               --^------- help: try adding parentheses: `(b1, b2, b3)`
+
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:57:14
+   |
+57 |     if let b1, b2, b3 = reading_frame.next().unwrap() {
+   |            --^------- help: try adding parentheses: `(b1, b2, b3)`
+
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:62:32
+   |
+62 |             Nucleotide::Adenine, Nucleotide::Cytosine, _ => {
+   |             -------------------^------------------------ help: try adding parentheses: `(Nucleotide::Adenine, Nucleotide::Cytosine, _)`
+
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:74:10
+   |
+74 |     for x, _barr_body in women.iter().map(|woman| woman.allosomes.clone()) {
+   |         -^----------- help: try adding parentheses: `(x, _barr_body)`
+
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:78:10
+   |
+78 |     for x, y @ Allosome::Y(_) in men.iter().map(|man| man.allosomes.clone()) {
+   |         -^------------------- help: try adding parentheses: `(x, y @ Allosome::Y(_))`
+
+error: unexpected `,` in pattern
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:86:14
+   |
+86 |     let women, men: (Vec<Genome>, Vec<Genome>) = genomes.iter().cloned()
+   |         -----^---- help: try adding parentheses: `(women, men)`
+
+error[E0425]: cannot find value `b2` in this scope
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:59:20
+   |
+59 |         match (b1, b2, b3) {
+   |                    ^^ did you mean `b1`?
+
+error[E0425]: cannot find value `b3` in this scope
+  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:59:24
+   |
+59 |         match (b1, b2, b3) {
+   |                        ^^ did you mean `b1`?
+
+error: aborting due to 8 previous errors
+
