From 6c399d155c6307563a2022fe98bc2e596af1cfc4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 21 Jan 2019 19:42:06 +0100 Subject: [PATCH 1/3] Add error for trailing angle brackets. This commit adds a error (and accompanying machine applicable suggestion) for trailing angle brackets on function calls with a turbofish. --- src/libsyntax/ast.rs | 14 ++++ src/libsyntax/parse/parser.rs | 97 +++++++++++++++++++++++++++ src/test/ui/issues/issue-54521-1.rs | 16 +++++ src/test/ui/issues/issue-54521.fixed | 22 ++++++ src/test/ui/issues/issue-54521.rs | 22 ++++++ src/test/ui/issues/issue-54521.stderr | 26 +++++++ 6 files changed, 197 insertions(+) create mode 100644 src/test/ui/issues/issue-54521-1.rs create mode 100644 src/test/ui/issues/issue-54521.fixed create mode 100644 src/test/ui/issues/issue-54521.rs create mode 100644 src/test/ui/issues/issue-54521.stderr diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index 405cf612543fb..e520ac3bdd499 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -140,6 +140,20 @@ pub enum GenericArgs { } impl GenericArgs { + pub fn is_parenthesized(&self) -> bool { + match *self { + Parenthesized(..) => true, + _ => false, + } + } + + pub fn is_angle_bracketed(&self) -> bool { + match *self { + AngleBracketed(..) => true, + _ => false, + } + } + pub fn span(&self) -> Span { match *self { AngleBracketed(ref data) => data.span, diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 439eec5b0c48d..d7c209d12a8fc 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2757,6 +2757,8 @@ impl<'a> Parser<'a> { // Assuming we have just parsed `.`, continue parsing into an expression. fn parse_dot_suffix(&mut self, self_arg: P, lo: Span) -> PResult<'a, P> { let segment = self.parse_path_segment(PathStyle::Expr, true)?; + self.check_trailing_angle_brackets(&segment); + Ok(match self.token { token::OpenDelim(token::Paren) => { // Method call `expr.f()` @@ -2784,6 +2786,101 @@ impl<'a> Parser<'a> { }) } + /// This function checks if there are trailing angle brackets and produces + /// a diagnostic to suggest removing them. + /// + /// ```ignore (diagnostic) + /// let _ = vec![1, 2, 3].into_iter().collect::>>>(); + /// ^^ help: remove extra angle brackets + /// ``` + fn check_trailing_angle_brackets(&mut self, segment: &PathSegment) { + // This function is intended to be invoked from `parse_dot_suffix` where there are two + // cases: + // + // - A field access (eg. `x.foo`) + // - A method call (eg. `x.foo()`) + // + // This function is called after parsing `.foo` and before parsing any parenthesis (if + // present). This includes any angle bracket arguments, such as `.foo::`. + + // We only care about trailing angle brackets if we previously parsed angle bracket + // arguments. This helps stop us incorrectly suggesting that extra angle brackets be + // removed in this case: + // + // `x.foo >> (3)` (where `x.foo` is a `u32` for example) + // + // This case is particularly tricky as we won't notice it just looking at the tokens - + // it will appear the same (in terms of upcoming tokens) as below (since the `::` will + // have already been parsed): + // + // `x.foo::>>(3)` + let parsed_angle_bracket_args = segment.args + .as_ref() + .map(|args| args.is_angle_bracketed()) + .unwrap_or(false); + + debug!( + "check_trailing_angle_brackets: parsed_angle_bracket_args={:?}", + parsed_angle_bracket_args, + ); + if !parsed_angle_bracket_args { + return; + } + + // Keep the span at the start so we can highlight the sequence of `>` characters to be + // removed. + let lo = self.span; + + // We need to look-ahead to see if we have `>` characters without moving the cursor forward + // (since we might have the field access case and the characters we're eating are + // actual operators and not trailing characters - ie `x.foo >> 3`). + let mut position = 0; + + // The first tokens we will encounter are shift right tokens (`>>`) since pairs of `>` + // characters will have been grouped together by the tokenizer. + let mut number_of_shr = 0; + while self.look_ahead(position, |t| *t == token::BinOp(token::BinOpToken::Shr)) { + number_of_shr += 1; + position += 1; + } + + // Afterwards, there will be at most one `>` character remaining (more than one and it'd + // have shown up as a `>>`). + let encountered_gt = self.look_ahead(position, |t| *t == token::Gt); + if encountered_gt { + position += 1; + } + + // If we didn't find any trailing `>>` characters or a trailing `>`, then we have + // nothing to error about. + debug!( + "check_trailing_angle_brackets: encountered_gt={:?} number_of_shr={:?}", + encountered_gt, number_of_shr, + ); + if !encountered_gt && number_of_shr < 1 { + return; + } + + // Finally, double check that we have a left parenthesis next as otherwise this is the + // field case. + if self.look_ahead(position, |t| *t == token::OpenDelim(token::Paren)) { + // Eat from where we started until the left parenthesis so that parsing can continue + // as if we didn't have those extra angle brackets. + self.eat_to_tokens(&[&token::OpenDelim(token::Paren)]); + let span = lo.until(self.span); + + self.diagnostic() + .struct_span_err(span, "unmatched angle bracket") + .span_suggestion_with_applicability( + span, + "remove extra angle bracket", + String::new(), + Applicability::MachineApplicable, + ) + .emit(); + } + } + fn parse_dot_or_call_expr_with_(&mut self, e0: P, lo: Span) -> PResult<'a, P> { let mut e = e0; let mut hi; diff --git a/src/test/ui/issues/issue-54521-1.rs b/src/test/ui/issues/issue-54521-1.rs new file mode 100644 index 0000000000000..d6a14a6e11f67 --- /dev/null +++ b/src/test/ui/issues/issue-54521-1.rs @@ -0,0 +1,16 @@ +// compile-pass + +// This test checks that the `remove extra angle brackets` error doesn't happen for some +// potential edge-cases.. + +struct X { + len: u32, +} + +fn main() { + let x = X { len: 3 }; + + let _ = x.len > (3); + + let _ = x.len >> (3); +} diff --git a/src/test/ui/issues/issue-54521.fixed b/src/test/ui/issues/issue-54521.fixed new file mode 100644 index 0000000000000..84ab6866cf133 --- /dev/null +++ b/src/test/ui/issues/issue-54521.fixed @@ -0,0 +1,22 @@ +// run-rustfix + +// This test checks that the following error is emitted and the suggestion works: +// +// ``` +// let _ = vec![1, 2, 3].into_iter().collect::>>>(); +// ^^ help: remove extra angle brackets +// ``` + +fn main() { + let _ = vec![1, 2, 3].into_iter().collect::>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>(); + //~^ ERROR unmatched angle bracket +} diff --git a/src/test/ui/issues/issue-54521.rs b/src/test/ui/issues/issue-54521.rs new file mode 100644 index 0000000000000..f1d6850417880 --- /dev/null +++ b/src/test/ui/issues/issue-54521.rs @@ -0,0 +1,22 @@ +// run-rustfix + +// This test checks that the following error is emitted and the suggestion works: +// +// ``` +// let _ = vec![1, 2, 3].into_iter().collect::>>>(); +// ^^ help: remove extra angle brackets +// ``` + +fn main() { + let _ = vec![1, 2, 3].into_iter().collect::>>>>>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>>>>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>>>(); + //~^ ERROR unmatched angle bracket + + let _ = vec![1, 2, 3].into_iter().collect::>>(); + //~^ ERROR unmatched angle bracket +} diff --git a/src/test/ui/issues/issue-54521.stderr b/src/test/ui/issues/issue-54521.stderr new file mode 100644 index 0000000000000..a67e9ca8daf40 --- /dev/null +++ b/src/test/ui/issues/issue-54521.stderr @@ -0,0 +1,26 @@ +error: unmatched angle bracket + --> $DIR/issue-54521.rs:11:60 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::>>>>>(); + | ^^^^ help: remove extra angle bracket + +error: unmatched angle bracket + --> $DIR/issue-54521.rs:14:60 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::>>>>(); + | ^^^ help: remove extra angle bracket + +error: unmatched angle bracket + --> $DIR/issue-54521.rs:17:60 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::>>>(); + | ^^ help: remove extra angle bracket + +error: unmatched angle bracket + --> $DIR/issue-54521.rs:20:60 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::>>(); + | ^ help: remove extra angle bracket + +error: aborting due to 4 previous errors + From 3f0fc9b03569e03dbdf5fdc3a67f246aad3b40b8 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 21 Jan 2019 21:16:46 +0100 Subject: [PATCH 2/3] Pluralize error messages. This commit pluralizes error messages when more than a single trailing `>` character is present. --- src/libsyntax/parse/parser.rs | 11 +++++++++-- src/test/ui/issues/issue-54521.stderr | 12 ++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index d7c209d12a8fc..6a881eb624196 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2869,11 +2869,18 @@ impl<'a> Parser<'a> { self.eat_to_tokens(&[&token::OpenDelim(token::Paren)]); let span = lo.until(self.span); + // We needn't check `encountered_gt` to determine if we should pluralize "bracket". + // `encountered_gt` can only represent a single `>` character, if `number_of_shr >= 1` + // then there is either `>>` or `>>>` - in either case a plural is warranted. + let plural = number_of_shr >= 1; self.diagnostic() - .struct_span_err(span, "unmatched angle bracket") + .struct_span_err( + span, + &format!("unmatched angle bracket{}", if plural { "s" } else { "" }), + ) .span_suggestion_with_applicability( span, - "remove extra angle bracket", + &format!("remove extra angle bracket{}", if plural { "s" } else { "" }), String::new(), Applicability::MachineApplicable, ) diff --git a/src/test/ui/issues/issue-54521.stderr b/src/test/ui/issues/issue-54521.stderr index a67e9ca8daf40..ffefbfd0348a8 100644 --- a/src/test/ui/issues/issue-54521.stderr +++ b/src/test/ui/issues/issue-54521.stderr @@ -1,20 +1,20 @@ -error: unmatched angle bracket +error: unmatched angle brackets --> $DIR/issue-54521.rs:11:60 | LL | let _ = vec![1, 2, 3].into_iter().collect::>>>>>(); - | ^^^^ help: remove extra angle bracket + | ^^^^ help: remove extra angle brackets -error: unmatched angle bracket +error: unmatched angle brackets --> $DIR/issue-54521.rs:14:60 | LL | let _ = vec![1, 2, 3].into_iter().collect::>>>>(); - | ^^^ help: remove extra angle bracket + | ^^^ help: remove extra angle brackets -error: unmatched angle bracket +error: unmatched angle brackets --> $DIR/issue-54521.rs:17:60 | LL | let _ = vec![1, 2, 3].into_iter().collect::>>>(); - | ^^ help: remove extra angle bracket + | ^^ help: remove extra angle brackets error: unmatched angle bracket --> $DIR/issue-54521.rs:20:60 From 914d142c02b558a597055c66a0e7e09115416211 Mon Sep 17 00:00:00 2001 From: David Wood Date: Tue, 22 Jan 2019 00:35:31 +0100 Subject: [PATCH 3/3] Extend trailing `>` detection for paths. This commit extends the trailing `>` detection to also work for paths such as `Foo::>:Baz`. This involves making the existing check take the token that is expected to follow the path being checked as a parameter. Care is taken to ensure that this only happens on the construction of a whole path segment and not a partial path segment (during recursion). Through this enhancement, it was also observed that the ordering of right shift token and greater than tokens was overfitted to the examples being tested. In practice, given a sequence of `>` characters: `>>>>>>>>>` ..then they will be split into `>>` eagerly: `>> >> >> >> >`. ..but when a `<` is prepended, then the first `>>` is split: ` > >> >> >> >` ..and then when another `<` is prepended, a right shift is first again: `Vec<> >> >> >> >` In the previous commits, a example that had two `<<` characters was always used and therefore it was incorrectly assumed that `>>` would always be first - but when there is a single `<`, this is not the case. --- src/libsyntax/parse/parser.rs | 94 ++++++++++++++++--------- src/test/ui/issues/issue-54521-2.fixed | 22 ++++++ src/test/ui/issues/issue-54521-2.rs | 22 ++++++ src/test/ui/issues/issue-54521-2.stderr | 26 +++++++ 4 files changed, 131 insertions(+), 33 deletions(-) create mode 100644 src/test/ui/issues/issue-54521-2.fixed create mode 100644 src/test/ui/issues/issue-54521-2.rs create mode 100644 src/test/ui/issues/issue-54521-2.stderr diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 6a881eb624196..d380948b8913d 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2149,7 +2149,27 @@ impl<'a> Parser<'a> { enable_warning: bool) -> PResult<'a, ()> { loop { - segments.push(self.parse_path_segment(style, enable_warning)?); + let segment = self.parse_path_segment(style, enable_warning)?; + if style == PathStyle::Expr { + // In order to check for trailing angle brackets, we must have finished + // recursing (`parse_path_segment` can indirectly call this function), + // that is, the next token must be the highlighted part of the below example: + // + // `Foo::>::Qux` + // ^ here + // + // As opposed to the below highlight (if we had only finished the first + // recursion): + // + // `Foo::>::Qux` + // ^ here + // + // `PathStyle::Expr` is only provided at the root invocation and never in + // `parse_path_segment` to recurse and therefore can be checked to maintain + // this invariant. + self.check_trailing_angle_brackets(&segment, token::ModSep); + } + segments.push(segment); if self.is_import_coupler() || !self.eat(&token::ModSep) { return Ok(()); @@ -2757,7 +2777,7 @@ impl<'a> Parser<'a> { // Assuming we have just parsed `.`, continue parsing into an expression. fn parse_dot_suffix(&mut self, self_arg: P, lo: Span) -> PResult<'a, P> { let segment = self.parse_path_segment(PathStyle::Expr, true)?; - self.check_trailing_angle_brackets(&segment); + self.check_trailing_angle_brackets(&segment, token::OpenDelim(token::Paren)); Ok(match self.token { token::OpenDelim(token::Paren) => { @@ -2793,15 +2813,19 @@ impl<'a> Parser<'a> { /// let _ = vec![1, 2, 3].into_iter().collect::>>>(); /// ^^ help: remove extra angle brackets /// ``` - fn check_trailing_angle_brackets(&mut self, segment: &PathSegment) { - // This function is intended to be invoked from `parse_dot_suffix` where there are two + fn check_trailing_angle_brackets(&mut self, segment: &PathSegment, end: token::Token) { + // This function is intended to be invoked after parsing a path segment where there are two // cases: // - // - A field access (eg. `x.foo`) - // - A method call (eg. `x.foo()`) + // 1. A specific token is expected after the path segment. + // eg. `x.foo(`, `x.foo::(` (parenthesis - method call), + // `Foo::`, or `Foo::::` (mod sep - continued path). + // 2. No specific token is expected after the path segment. + // eg. `x.foo` (field access) // - // This function is called after parsing `.foo` and before parsing any parenthesis (if - // present). This includes any angle bracket arguments, such as `.foo::`. + // This function is called after parsing `.foo` and before parsing the token `end` (if + // present). This includes any angle bracket arguments, such as `.foo::` or + // `Foo::`. // We only care about trailing angle brackets if we previously parsed angle bracket // arguments. This helps stop us incorrectly suggesting that extra angle brackets be @@ -2836,43 +2860,47 @@ impl<'a> Parser<'a> { // actual operators and not trailing characters - ie `x.foo >> 3`). let mut position = 0; - // The first tokens we will encounter are shift right tokens (`>>`) since pairs of `>` - // characters will have been grouped together by the tokenizer. + // We can encounter `>` or `>>` tokens in any order, so we need to keep track of how + // many of each (so we can correctly pluralize our error messages) and continue to + // advance. let mut number_of_shr = 0; - while self.look_ahead(position, |t| *t == token::BinOp(token::BinOpToken::Shr)) { - number_of_shr += 1; - position += 1; - } - - // Afterwards, there will be at most one `>` character remaining (more than one and it'd - // have shown up as a `>>`). - let encountered_gt = self.look_ahead(position, |t| *t == token::Gt); - if encountered_gt { + let mut number_of_gt = 0; + while self.look_ahead(position, |t| { + trace!("check_trailing_angle_brackets: t={:?}", t); + if *t == token::BinOp(token::BinOpToken::Shr) { + number_of_shr += 1; + true + } else if *t == token::Gt { + number_of_gt += 1; + true + } else { + false + } + }) { position += 1; } - // If we didn't find any trailing `>>` characters or a trailing `>`, then we have - // nothing to error about. + // If we didn't find any trailing `>` characters, then we have nothing to error about. debug!( - "check_trailing_angle_brackets: encountered_gt={:?} number_of_shr={:?}", - encountered_gt, number_of_shr, + "check_trailing_angle_brackets: number_of_gt={:?} number_of_shr={:?}", + number_of_gt, number_of_shr, ); - if !encountered_gt && number_of_shr < 1 { + if number_of_gt < 1 && number_of_shr < 1 { return; } - // Finally, double check that we have a left parenthesis next as otherwise this is the - // field case. - if self.look_ahead(position, |t| *t == token::OpenDelim(token::Paren)) { - // Eat from where we started until the left parenthesis so that parsing can continue + // Finally, double check that we have our end token as otherwise this is the + // second case. + if self.look_ahead(position, |t| { + trace!("check_trailing_angle_brackets: t={:?}", t); + *t == end + }) { + // Eat from where we started until the end token so that parsing can continue // as if we didn't have those extra angle brackets. - self.eat_to_tokens(&[&token::OpenDelim(token::Paren)]); + self.eat_to_tokens(&[&end]); let span = lo.until(self.span); - // We needn't check `encountered_gt` to determine if we should pluralize "bracket". - // `encountered_gt` can only represent a single `>` character, if `number_of_shr >= 1` - // then there is either `>>` or `>>>` - in either case a plural is warranted. - let plural = number_of_shr >= 1; + let plural = number_of_gt > 1 || number_of_shr >= 1; self.diagnostic() .struct_span_err( span, diff --git a/src/test/ui/issues/issue-54521-2.fixed b/src/test/ui/issues/issue-54521-2.fixed new file mode 100644 index 0000000000000..a91c4fe43ea46 --- /dev/null +++ b/src/test/ui/issues/issue-54521-2.fixed @@ -0,0 +1,22 @@ +// run-rustfix + +// This test checks that the following error is emitted and the suggestion works: +// +// ``` +// let _ = Vec::>>::new(); +// ^^ help: remove extra angle brackets +// ``` + +fn main() { + let _ = Vec::::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::::new(); + //~^ ERROR unmatched angle bracket +} diff --git a/src/test/ui/issues/issue-54521-2.rs b/src/test/ui/issues/issue-54521-2.rs new file mode 100644 index 0000000000000..3639aac87ee7f --- /dev/null +++ b/src/test/ui/issues/issue-54521-2.rs @@ -0,0 +1,22 @@ +// run-rustfix + +// This test checks that the following error is emitted and the suggestion works: +// +// ``` +// let _ = Vec::>>::new(); +// ^^ help: remove extra angle brackets +// ``` + +fn main() { + let _ = Vec::>>>>::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::>>>::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::>>::new(); + //~^ ERROR unmatched angle bracket + + let _ = Vec::>::new(); + //~^ ERROR unmatched angle bracket +} diff --git a/src/test/ui/issues/issue-54521-2.stderr b/src/test/ui/issues/issue-54521-2.stderr new file mode 100644 index 0000000000000..9556b83b730a4 --- /dev/null +++ b/src/test/ui/issues/issue-54521-2.stderr @@ -0,0 +1,26 @@ +error: unmatched angle brackets + --> $DIR/issue-54521-2.rs:11:25 + | +LL | let _ = Vec::>>>>::new(); + | ^^^^ help: remove extra angle brackets + +error: unmatched angle brackets + --> $DIR/issue-54521-2.rs:14:25 + | +LL | let _ = Vec::>>>::new(); + | ^^^ help: remove extra angle brackets + +error: unmatched angle brackets + --> $DIR/issue-54521-2.rs:17:25 + | +LL | let _ = Vec::>>::new(); + | ^^ help: remove extra angle brackets + +error: unmatched angle bracket + --> $DIR/issue-54521-2.rs:20:25 + | +LL | let _ = Vec::>::new(); + | ^ help: remove extra angle bracket + +error: aborting due to 4 previous errors +