From 22f794b00fa8d84c9e827f8c8af762ee60074a8a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 23 Jan 2019 02:35:13 +0100 Subject: [PATCH 1/2] Suggest removing leading left angle brackets. This commit adds errors and accompanying suggestions as below: ``` bar::<<<<::Output>(); ^^^ help: remove extra angle brackets ``` --- src/libsyntax/parse/parser.rs | 209 +++++++++++++++++++++++++- src/test/ui/issues/issue-57819.fixed | 47 ++++++ src/test/ui/issues/issue-57819.rs | 47 ++++++ src/test/ui/issues/issue-57819.stderr | 44 ++++++ 4 files changed, 339 insertions(+), 8 deletions(-) create mode 100644 src/test/ui/issues/issue-57819.fixed create mode 100644 src/test/ui/issues/issue-57819.rs create mode 100644 src/test/ui/issues/issue-57819.stderr diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 439eec5b0c48d..6ad07a8e2f1d2 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -242,6 +242,12 @@ pub struct Parser<'a> { desugar_doc_comments: bool, /// Whether we should configure out of line modules as we parse. pub cfg_mods: bool, + /// This field is used to keep track of how many left angle brackets we have seen. This is + /// required in order to detect extra leading left angle brackets (`<` characters) and error + /// appropriately. + /// + /// See the comments in the `parse_path_segment` function for more details. + crate unmatched_angle_bracket_count: u32, } @@ -563,6 +569,7 @@ impl<'a> Parser<'a> { }, desugar_doc_comments, cfg_mods: true, + unmatched_angle_bracket_count: 0, }; let tok = parser.next_tok(); @@ -1027,7 +1034,7 @@ impl<'a> Parser<'a> { /// starting token. fn eat_lt(&mut self) -> bool { self.expected_tokens.push(TokenType::Token(token::Lt)); - match self.token { + let ate = match self.token { token::Lt => { self.bump(); true @@ -1038,7 +1045,15 @@ impl<'a> Parser<'a> { true } _ => false, + }; + + if ate { + // See doc comment for `unmatched_angle_bracket_count`. + self.unmatched_angle_bracket_count += 1; + debug!("eat_lt: (increment) count={:?}", self.unmatched_angle_bracket_count); } + + ate } fn expect_lt(&mut self) -> PResult<'a, ()> { @@ -1054,24 +1069,35 @@ impl<'a> Parser<'a> { /// signal an error. fn expect_gt(&mut self) -> PResult<'a, ()> { self.expected_tokens.push(TokenType::Token(token::Gt)); - match self.token { + let ate = match self.token { token::Gt => { self.bump(); - Ok(()) + Some(()) } token::BinOp(token::Shr) => { let span = self.span.with_lo(self.span.lo() + BytePos(1)); - Ok(self.bump_with(token::Gt, span)) + Some(self.bump_with(token::Gt, span)) } token::BinOpEq(token::Shr) => { let span = self.span.with_lo(self.span.lo() + BytePos(1)); - Ok(self.bump_with(token::Ge, span)) + Some(self.bump_with(token::Ge, span)) } token::Ge => { let span = self.span.with_lo(self.span.lo() + BytePos(1)); - Ok(self.bump_with(token::Eq, span)) + Some(self.bump_with(token::Eq, span)) } - _ => self.unexpected() + _ => None, + }; + + match ate { + Some(x) => { + // See doc comment for `unmatched_angle_bracket_count`. + self.unmatched_angle_bracket_count -= 1; + debug!("expect_gt: (decrement) count={:?}", self.unmatched_angle_bracket_count); + + Ok(x) + }, + None => self.unexpected(), } } @@ -2079,7 +2105,11 @@ impl<'a> Parser<'a> { path_span = self.span.to(self.span); } + // See doc comment for `unmatched_angle_bracket_count`. self.expect(&token::Gt)?; + self.unmatched_angle_bracket_count -= 1; + debug!("parse_qpath: (decrement) count={:?}", self.unmatched_angle_bracket_count); + self.expect(&token::ModSep)?; let qself = QSelf { ty, path_span, position: path.segments.len() }; @@ -2182,9 +2212,15 @@ impl<'a> Parser<'a> { } let lo = self.span; + // We use `style == PathStyle::Expr` to check if this is in a recursion or not. If + // it isn't, then we reset the unmatched angle bracket count as we're about to start + // parsing a new path. + if style == PathStyle::Expr { self.unmatched_angle_bracket_count = 0; } + let args = if self.eat_lt() { // `<'a, T, A = U>` - let (args, bindings) = self.parse_generic_args()?; + let (args, bindings) = + self.parse_generic_args_with_leaning_angle_bracket_recovery(style, lo)?; self.expect_gt()?; let span = lo.to(self.prev_span); AngleBracketedArgs { args, bindings, span }.into() @@ -5319,6 +5355,163 @@ impl<'a> Parser<'a> { } } + /// Parse generic args (within a path segment) with recovery for extra leading angle brackets. + /// For the purposes of understanding the parsing logic of generic arguments, this function + /// can be thought of being the same as just calling `self.parse_generic_args()` if the source + /// had the correct amount of leading angle brackets. + /// + /// ```ignore (diagnostics) + /// bar::<<<::Output>(); + /// ^^ help: remove extra angle brackets + /// ``` + fn parse_generic_args_with_leaning_angle_bracket_recovery( + &mut self, + style: PathStyle, + lo: Span, + ) -> PResult<'a, (Vec, Vec)> { + // We need to detect whether there are extra leading left angle brackets and produce an + // appropriate error and suggestion. This cannot be implemented by looking ahead at + // upcoming tokens for a matching `>` character - if there are unmatched `<` tokens + // then there won't be matching `>` tokens to find. + // + // To explain how this detection works, consider the following example: + // + // ```ignore (diagnostics) + // bar::<<<::Output>(); + // ^^ help: remove extra angle brackets + // ``` + // + // Parsing of the left angle brackets starts in this function. We start by parsing the + // `<` token (incrementing the counter of unmatched angle brackets on `Parser` via + // `eat_lt`): + // + // *Upcoming tokens:* `<<<::Output>;` + // *Unmatched count:* 1 + // *`parse_path_segment` calls deep:* 0 + // + // This has the effect of recursing as this function is called if a `<` character + // is found within the expected generic arguments: + // + // *Upcoming tokens:* `<<::Output>;` + // *Unmatched count:* 2 + // *`parse_path_segment` calls deep:* 1 + // + // Eventually we will have recursed until having consumed all of the `<` tokens and + // this will be reflected in the count: + // + // *Upcoming tokens:* `T as Foo>::Output>;` + // *Unmatched count:* 4 + // `parse_path_segment` calls deep:* 3 + // + // The parser will continue until reaching the first `>` - this will decrement the + // unmatched angle bracket count and return to the parent invocation of this function + // having succeeded in parsing: + // + // *Upcoming tokens:* `::Output>;` + // *Unmatched count:* 3 + // *`parse_path_segment` calls deep:* 2 + // + // This will continue until the next `>` character which will also return successfully + // to the parent invocation of this function and decrement the count: + // + // *Upcoming tokens:* `;` + // *Unmatched count:* 2 + // *`parse_path_segment` calls deep:* 1 + // + // At this point, this function will expect to find another matching `>` character but + // won't be able to and will return an error. This will continue all the way up the + // call stack until the first invocation: + // + // *Upcoming tokens:* `;` + // *Unmatched count:* 2 + // *`parse_path_segment` calls deep:* 0 + // + // In doing this, we have managed to work out how many unmatched leading left angle + // brackets there are, but we cannot recover as the unmatched angle brackets have + // already been consumed. To remedy this, whenever `parse_generic_args` is invoked, we + // make a snapshot of the current parser state and invoke it on that and inspect + // the result: + // + // - If success (ie. when it found a matching `>` character) then the snapshot state + // is kept (this is required to propagate the count upwards). + // + // - If error and in was in a recursive call, then the snapshot state is kept (this is + // required to propagate the count upwards). + // + // - If error and this was the first invocation (before any recursion had taken place) + // then we choose not to keep the snapshot state - that way we haven't actually + // consumed any of the `<` characters, but can still inspect the count from the + // snapshot to know how many `<` characters to remove. Using this information, we can + // emit an error and consume the extra `<` characters before attempting to parse + // the generic arguments again (this time hopefullt successfully as the unmatched `<` + // characters are gone). + // + // In practice, the recursion of this function is indirect and there will be other + // locations that consume some `<` characters - as long as we update the count when + // this happens, it isn't an issue. + let mut snapshot = self.clone(); + debug!("parse_generic_args_with_leading_angle_bracket_recovery: (snapshotting)"); + match snapshot.parse_generic_args() { + Ok(value) => { + debug!( + "parse_generic_args_with_leading_angle_bracket_recovery: (snapshot success) \ + snapshot.count={:?}", + snapshot.unmatched_angle_bracket_count, + ); + mem::replace(self, snapshot); + Ok(value) + }, + Err(mut e) => { + debug!( + "parse_generic_args_with_leading_angle_bracket_recovery: (snapshot failure) \ + snapshot.count={:?}", + snapshot.unmatched_angle_bracket_count, + ); + if style == PathStyle::Expr && snapshot.unmatched_angle_bracket_count > 0 { + // Cancel error from being unable to find `>`. We know the error + // must have been this due to a non-zero unmatched angle bracket + // count. + e.cancel(); + + // Eat the unmatched angle brackets. + for _ in 0..snapshot.unmatched_angle_bracket_count { + self.eat_lt(); + } + + // Make a span over ${unmatched angle bracket count} characters. + let span = lo.with_hi( + lo.lo() + BytePos(snapshot.unmatched_angle_bracket_count) + ); + let plural = snapshot.unmatched_angle_bracket_count > 1; + self.diagnostic() + .struct_span_err( + span, + &format!( + "unmatched angle bracket{}", + if plural { "s" } else { "" } + ), + ) + .span_suggestion_with_applicability( + span, + &format!( + "remove extra angle bracket{}", + if plural { "s" } else { "" } + ), + String::new(), + Applicability::MachineApplicable, + ) + .emit(); + + // Try again without unmatched angle bracket characters. + self.parse_generic_args() + } else { + mem::replace(self, snapshot); + Err(e) + } + }, + } + } + /// Parses (possibly empty) list of lifetime and type arguments and associated type bindings, /// possibly including trailing comma. fn parse_generic_args(&mut self) -> PResult<'a, (Vec, Vec)> { diff --git a/src/test/ui/issues/issue-57819.fixed b/src/test/ui/issues/issue-57819.fixed new file mode 100644 index 0000000000000..3fab21db2d06e --- /dev/null +++ b/src/test/ui/issues/issue-57819.fixed @@ -0,0 +1,47 @@ +// run-rustfix + +#![allow(warnings)] + +// 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 +// ``` + +trait Foo { + type Output; +} + +fn foo() { + // More complex cases with more than one correct leading `<` character: + + bar::<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<::Output>(); +} + +fn bar() {} + +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 + + let _ = vec![1, 2, 3].into_iter().collect::>(); +} diff --git a/src/test/ui/issues/issue-57819.rs b/src/test/ui/issues/issue-57819.rs new file mode 100644 index 0000000000000..5cafbf439be2d --- /dev/null +++ b/src/test/ui/issues/issue-57819.rs @@ -0,0 +1,47 @@ +// run-rustfix + +#![allow(warnings)] + +// 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 +// ``` + +trait Foo { + type Output; +} + +fn foo() { + // More complex cases with more than one correct leading `<` character: + + bar::<<<<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<<<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<<::Output>(); + //~^ ERROR unmatched angle bracket + + bar::<::Output>(); +} + +fn bar() {} + +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 + + let _ = vec![1, 2, 3].into_iter().collect::>(); +} diff --git a/src/test/ui/issues/issue-57819.stderr b/src/test/ui/issues/issue-57819.stderr new file mode 100644 index 0000000000000..493e9835b1ca9 --- /dev/null +++ b/src/test/ui/issues/issue-57819.stderr @@ -0,0 +1,44 @@ +error: unmatched angle brackets + --> $DIR/issue-57819.rs:19:10 + | +LL | bar::<<<<::Output>(); + | ^^^ help: remove extra angle brackets + +error: unmatched angle brackets + --> $DIR/issue-57819.rs:22:10 + | +LL | bar::<<<::Output>(); + | ^^ help: remove extra angle brackets + +error: unmatched angle bracket + --> $DIR/issue-57819.rs:25:10 + | +LL | bar::<<::Output>(); + | ^ help: remove extra angle bracket + +error: unmatched angle brackets + --> $DIR/issue-57819.rs:34:48 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::<<<<>(); + | ^^^^ help: remove extra angle brackets + +error: unmatched angle brackets + --> $DIR/issue-57819.rs:37:48 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::<<<>(); + | ^^^ help: remove extra angle brackets + +error: unmatched angle brackets + --> $DIR/issue-57819.rs:40:48 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::<<>(); + | ^^ help: remove extra angle brackets + +error: unmatched angle bracket + --> $DIR/issue-57819.rs:43:48 + | +LL | let _ = vec![1, 2, 3].into_iter().collect::<>(); + | ^ help: remove extra angle bracket + +error: aborting due to 7 previous errors + From 8ab12f6cc06033f483d085b37b766d681dcc61ca Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 23 Jan 2019 21:39:15 +0100 Subject: [PATCH 2/2] Optimize snapshot usage. This commit implements a suggestion from @estebank that optimizes the use of snapshots. Instead of creating a snapshot for each recursion in `parse_path_segment` and then replacing `self` with them until the first invocation where if leading angle brackets are detected, `self` is not replaced and instead the snapshot is used to inform how parsing should continue. Now, a snapshot is created in the first invocation that acts as a backup of the parser state before any generic arguments are parsed (and therefore, before recursion starts). This backup replaces `self` if after all parsing of generic arguments has concluded we can determine that there are leading angle brackets. Parsing can then proceed from the backup state making use of the now known number of unmatched leading angle brackets to recover. --- src/libsyntax/parse/parser.rs | 125 ++++++++++++++++------------------ 1 file changed, 57 insertions(+), 68 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 6ad07a8e2f1d2..d03a346c3d55b 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -5428,87 +5428,76 @@ impl<'a> Parser<'a> { // // In doing this, we have managed to work out how many unmatched leading left angle // brackets there are, but we cannot recover as the unmatched angle brackets have - // already been consumed. To remedy this, whenever `parse_generic_args` is invoked, we - // make a snapshot of the current parser state and invoke it on that and inspect - // the result: - // - // - If success (ie. when it found a matching `>` character) then the snapshot state - // is kept (this is required to propagate the count upwards). - // - // - If error and in was in a recursive call, then the snapshot state is kept (this is - // required to propagate the count upwards). - // - // - If error and this was the first invocation (before any recursion had taken place) - // then we choose not to keep the snapshot state - that way we haven't actually - // consumed any of the `<` characters, but can still inspect the count from the - // snapshot to know how many `<` characters to remove. Using this information, we can - // emit an error and consume the extra `<` characters before attempting to parse - // the generic arguments again (this time hopefullt successfully as the unmatched `<` - // characters are gone). + // already been consumed. To remedy this, we keep a snapshot of the parser state + // before we do the above. We can then inspect whether we ended up with a parsing error + // and unmatched left angle brackets and if so, restore the parser state before we + // consumed any `<` characters to emit an error and consume the erroneous tokens to + // recover by attempting to parse again. // // In practice, the recursion of this function is indirect and there will be other // locations that consume some `<` characters - as long as we update the count when // this happens, it isn't an issue. - let mut snapshot = self.clone(); + + let is_first_invocation = style == PathStyle::Expr; + // Take a snapshot before attempting to parse - we can restore this later. + let snapshot = if is_first_invocation { + Some(self.clone()) + } else { + None + }; + debug!("parse_generic_args_with_leading_angle_bracket_recovery: (snapshotting)"); - match snapshot.parse_generic_args() { - Ok(value) => { - debug!( - "parse_generic_args_with_leading_angle_bracket_recovery: (snapshot success) \ - snapshot.count={:?}", - snapshot.unmatched_angle_bracket_count, - ); - mem::replace(self, snapshot); - Ok(value) - }, - Err(mut e) => { + match self.parse_generic_args() { + Ok(value) => Ok(value), + Err(ref mut e) if is_first_invocation && self.unmatched_angle_bracket_count > 0 => { + // Cancel error from being unable to find `>`. We know the error + // must have been this due to a non-zero unmatched angle bracket + // count. + e.cancel(); + + // Swap `self` with our backup of the parser state before attempting to parse + // generic arguments. + let snapshot = mem::replace(self, snapshot.unwrap()); + debug!( "parse_generic_args_with_leading_angle_bracket_recovery: (snapshot failure) \ snapshot.count={:?}", snapshot.unmatched_angle_bracket_count, ); - if style == PathStyle::Expr && snapshot.unmatched_angle_bracket_count > 0 { - // Cancel error from being unable to find `>`. We know the error - // must have been this due to a non-zero unmatched angle bracket - // count. - e.cancel(); - - // Eat the unmatched angle brackets. - for _ in 0..snapshot.unmatched_angle_bracket_count { - self.eat_lt(); - } - // Make a span over ${unmatched angle bracket count} characters. - let span = lo.with_hi( - lo.lo() + BytePos(snapshot.unmatched_angle_bracket_count) - ); - let plural = snapshot.unmatched_angle_bracket_count > 1; - self.diagnostic() - .struct_span_err( - span, - &format!( - "unmatched angle bracket{}", - if plural { "s" } else { "" } - ), - ) - .span_suggestion_with_applicability( - span, - &format!( - "remove extra angle bracket{}", - if plural { "s" } else { "" } - ), - String::new(), - Applicability::MachineApplicable, - ) - .emit(); - - // Try again without unmatched angle bracket characters. - self.parse_generic_args() - } else { - mem::replace(self, snapshot); - Err(e) + // Eat the unmatched angle brackets. + for _ in 0..snapshot.unmatched_angle_bracket_count { + self.eat_lt(); } + + // Make a span over ${unmatched angle bracket count} characters. + let span = lo.with_hi( + lo.lo() + BytePos(snapshot.unmatched_angle_bracket_count) + ); + let plural = snapshot.unmatched_angle_bracket_count > 1; + self.diagnostic() + .struct_span_err( + span, + &format!( + "unmatched angle bracket{}", + if plural { "s" } else { "" } + ), + ) + .span_suggestion_with_applicability( + span, + &format!( + "remove extra angle bracket{}", + if plural { "s" } else { "" } + ), + String::new(), + Applicability::MachineApplicable, + ) + .emit(); + + // Try again without unmatched angle bracket characters. + self.parse_generic_args() }, + Err(e) => Err(e), } }