Skip to content

Suggest a path separator if a stray colon is found in a match arm #87101

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

Merged
merged 1 commit into from
Jul 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions compiler/rustc_expand/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ use rustc_data_structures::stack::ensure_sufficient_stack;
use rustc_data_structures::sync::Lrc;
use rustc_errors::{Applicability, FatalError, PResult};
use rustc_feature::Features;
use rustc_parse::parser::{AttemptLocalParseRecovery, ForceCollect, Parser, RecoverComma};
use rustc_parse::parser::{
AttemptLocalParseRecovery, ForceCollect, Parser, RecoverColon, RecoverComma,
};
use rustc_parse::validate_attr;
use rustc_session::lint::builtin::UNUSED_DOC_COMMENTS;
use rustc_session::lint::BuiltinLintDiagnostics;
Expand Down Expand Up @@ -930,9 +932,11 @@ pub fn parse_ast_fragment<'a>(
}
}
AstFragmentKind::Ty => AstFragment::Ty(this.parse_ty()?),
AstFragmentKind::Pat => {
AstFragment::Pat(this.parse_pat_allow_top_alt(None, RecoverComma::No)?)
}
AstFragmentKind::Pat => AstFragment::Pat(this.parse_pat_allow_top_alt(
None,
RecoverComma::No,
RecoverColon::Yes,
)?),
AstFragmentKind::Arms
| AstFragmentKind::Fields
| AstFragmentKind::FieldPats
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::pat::{RecoverComma, PARAM_EXPECTED};
use super::pat::{RecoverColon, RecoverComma, PARAM_EXPECTED};
use super::ty::{AllowPlus, RecoverQPath, RecoverReturnSign};
use super::{AttrWrapper, BlockMode, ForceCollect, Parser, PathStyle, Restrictions, TokenType};
use super::{SemiColonMode, SeqSep, TokenExpectType, TrailingToken};
Expand Down Expand Up @@ -1813,7 +1813,7 @@ impl<'a> Parser<'a> {
/// The `let` token has already been eaten.
fn parse_let_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
let lo = self.prev_token.span;
let pat = self.parse_pat_allow_top_alt(None, RecoverComma::Yes)?;
let pat = self.parse_pat_allow_top_alt(None, RecoverComma::Yes, RecoverColon::Yes)?;
self.expect(&token::Eq)?;
let expr = self.with_res(self.restrictions | Restrictions::NO_STRUCT_LITERAL, |this| {
this.parse_assoc_expr_with(1 + prec_let_scrutinee_needs_par(), None.into())
Expand Down Expand Up @@ -1876,7 +1876,7 @@ impl<'a> Parser<'a> {
_ => None,
};

let pat = self.parse_pat_allow_top_alt(None, RecoverComma::Yes)?;
let pat = self.parse_pat_allow_top_alt(None, RecoverComma::Yes, RecoverColon::Yes)?;
if !self.eat_keyword(kw::In) {
self.error_missing_in_for_loop();
}
Expand Down Expand Up @@ -2083,7 +2083,7 @@ impl<'a> Parser<'a> {
let attrs = self.parse_outer_attributes()?;
self.collect_tokens_trailing_token(attrs, ForceCollect::No, |this, attrs| {
let lo = this.token.span;
let pat = this.parse_pat_allow_top_alt(None, RecoverComma::Yes)?;
let pat = this.parse_pat_allow_top_alt(None, RecoverComma::Yes, RecoverColon::Yes)?;
let guard = if this.eat_keyword(kw::If) {
let if_span = this.prev_token.span;
let cond = this.parse_expr()?;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::lexer::UnmatchedBrace;
pub use attr_wrapper::AttrWrapper;
pub use diagnostics::AttemptLocalParseRecovery;
use diagnostics::Error;
pub use pat::RecoverComma;
pub use pat::{RecoverColon, RecoverComma};
pub use path::PathStyle;

use rustc_ast::ptr::P;
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_parse/src/parser/nonterminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_ast_pretty::pprust;
use rustc_errors::PResult;
use rustc_span::symbol::{kw, Ident};

use crate::parser::pat::RecoverComma;
use crate::parser::pat::{RecoverColon, RecoverComma};
use crate::parser::{FollowedByType, ForceCollect, Parser, PathStyle};

impl<'a> Parser<'a> {
Expand Down Expand Up @@ -125,7 +125,7 @@ impl<'a> Parser<'a> {
token::NtPat(self.collect_tokens_no_attrs(|this| match kind {
NonterminalKind::PatParam { .. } => this.parse_pat_no_top_alt(None),
NonterminalKind::PatWithOr { .. } => {
this.parse_pat_allow_top_alt(None, RecoverComma::No)
this.parse_pat_allow_top_alt(None, RecoverComma::No, RecoverColon::No)
}
_ => unreachable!(),
})?)
Expand Down
78 changes: 70 additions & 8 deletions compiler/rustc_parse/src/parser/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ pub enum RecoverComma {
No,
}

/// Whether or not to recover a `:` when parsing patterns that were meant to be paths.
#[derive(PartialEq, Copy, Clone)]
pub enum RecoverColon {
Yes,
No,
}

/// The result of `eat_or_separator`. We want to distinguish which case we are in to avoid
/// emitting duplicate diagnostics.
#[derive(Debug, Clone, Copy)]
Expand Down Expand Up @@ -58,8 +65,9 @@ impl<'a> Parser<'a> {
&mut self,
expected: Expected,
rc: RecoverComma,
ra: RecoverColon,
) -> PResult<'a, P<Pat>> {
self.parse_pat_allow_top_alt_inner(expected, rc).map(|(pat, _)| pat)
self.parse_pat_allow_top_alt_inner(expected, rc, ra).map(|(pat, _)| pat)
}

/// Returns the pattern and a bool indicating whether we recovered from a trailing vert (true =
Expand All @@ -68,6 +76,7 @@ impl<'a> Parser<'a> {
&mut self,
expected: Expected,
rc: RecoverComma,
ra: RecoverColon,
) -> PResult<'a, (P<Pat>, bool)> {
// Keep track of whether we recovered from a trailing vert so that we can avoid duplicated
// suggestions (which bothers rustfix).
Expand All @@ -89,6 +98,56 @@ impl<'a> Parser<'a> {
// If we parsed a leading `|` which should be gated,
// then we should really gate the leading `|`.
// This complicated procedure is done purely for diagnostics UX.
let mut first_pat = first_pat;

if let (RecoverColon::Yes, token::Colon) = (ra, &self.token.kind) {
if matches!(
first_pat.kind,
PatKind::Ident(BindingMode::ByValue(Mutability::Not), _, None)
| PatKind::Path(..)
) && self.look_ahead(1, |token| token.is_ident() && !token.is_reserved_ident())
{
// The pattern looks like it might be a path with a `::` -> `:` typo:
// `match foo { bar:baz => {} }`
let span = self.token.span;
// We only emit "unexpected `:`" error here if we can successfully parse the
// whole pattern correctly in that case.
let snapshot = self.clone();

// Create error for "unexpected `:`".
match self.expected_one_of_not_found(&[], &[]) {
Err(mut err) => {
self.bump(); // Skip the `:`.
match self.parse_pat_no_top_alt(expected) {
Err(mut inner_err) => {
// Carry on as if we had not done anything, callers will emit a
// reasonable error.
inner_err.cancel();
err.cancel();
*self = snapshot;
}
Ok(pat) => {
// We've parsed the rest of the pattern.
err.span_suggestion(
span,
"maybe write a path separator here",
"::".to_string(),
Applicability::MachineApplicable,
);
err.emit();
first_pat =
self.mk_pat(first_pat.span.to(pat.span), PatKind::Wild);
}
}
}
_ => {
// Carry on as if we had not done anything. This should be unreachable.
*self = snapshot;
}
};
}
}

if let Some(leading_vert_span) = leading_vert_span {
// If there was a leading vert, treat this as an or-pattern. This improves
// diagnostics.
Expand Down Expand Up @@ -140,7 +199,8 @@ impl<'a> Parser<'a> {
// We use `parse_pat_allow_top_alt` regardless of whether we actually want top-level
// or-patterns so that we can detect when a user tries to use it. This allows us to print a
// better error message.
let (pat, trailing_vert) = self.parse_pat_allow_top_alt_inner(expected, rc)?;
let (pat, trailing_vert) =
self.parse_pat_allow_top_alt_inner(expected, rc, RecoverColon::No)?;
let colon = self.eat(&token::Colon);

if let PatKind::Or(pats) = &pat.kind {
Expand Down Expand Up @@ -350,7 +410,7 @@ impl<'a> Parser<'a> {
} else if self.check(&token::OpenDelim(token::Bracket)) {
// Parse `[pat, pat,...]` as a slice pattern.
let (pats, _) = self.parse_delim_comma_seq(token::Bracket, |p| {
p.parse_pat_allow_top_alt(None, RecoverComma::No)
p.parse_pat_allow_top_alt(None, RecoverComma::No, RecoverColon::No)
})?;
PatKind::Slice(pats)
} else if self.check(&token::DotDot) && !self.is_pat_range_end_start(1) {
Expand Down Expand Up @@ -563,8 +623,9 @@ impl<'a> Parser<'a> {

/// Parse a tuple or parenthesis pattern.
fn parse_pat_tuple_or_parens(&mut self) -> PResult<'a, PatKind> {
let (fields, trailing_comma) =
self.parse_paren_comma_seq(|p| p.parse_pat_allow_top_alt(None, RecoverComma::No))?;
let (fields, trailing_comma) = self.parse_paren_comma_seq(|p| {
p.parse_pat_allow_top_alt(None, RecoverComma::No, RecoverColon::No)
})?;

// Here, `(pat,)` is a tuple pattern.
// For backward compatibility, `(..)` is a tuple pattern as well.
Expand Down Expand Up @@ -873,8 +934,9 @@ impl<'a> Parser<'a> {

/// Parse tuple struct or tuple variant pattern (e.g. `Foo(...)` or `Foo::Bar(...)`).
fn parse_pat_tuple_struct(&mut self, qself: Option<QSelf>, path: Path) -> PResult<'a, PatKind> {
let (fields, _) =
self.parse_paren_comma_seq(|p| p.parse_pat_allow_top_alt(None, RecoverComma::No))?;
let (fields, _) = self.parse_paren_comma_seq(|p| {
p.parse_pat_allow_top_alt(None, RecoverComma::No, RecoverColon::No)
})?;
if qself.is_some() {
self.sess.gated_spans.gate(sym::more_qualified_paths, path.span);
}
Expand Down Expand Up @@ -1033,7 +1095,7 @@ impl<'a> Parser<'a> {
// Parsing a pattern of the form `fieldname: pat`.
let fieldname = self.parse_field_name()?;
self.bump();
let pat = self.parse_pat_allow_top_alt(None, RecoverComma::No)?;
let pat = self.parse_pat_allow_top_alt(None, RecoverComma::No, RecoverColon::No)?;
hi = pat.span;
(pat, fieldname, false)
} else {
Expand Down
66 changes: 66 additions & 0 deletions src/test/ui/parser/issue-87086-colon-path-sep.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Tests that a suggestion is issued if the user wrote a colon instead of
// a path separator in a match arm.

enum Foo {
Bar,
Baz,
}

fn f() -> Foo { Foo::Bar }

fn g1() {
match f() {
Foo:Bar => {}
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
_ => {}
}
match f() {
Foo::Bar:Baz => {}
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
_ => {}
}
match f() {
Foo:Bar::Baz => {}
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
_ => {}
}
match f() {
Foo: Bar::Baz if true => {}
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
_ => {}
}
if let Bar:Baz = f() {
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
}
}

fn g1_neg() {
match f() {
ref Foo: Bar::Baz => {}
//~^ ERROR: expected one of
_ => {}
}
}

fn g2_neg() {
match f() {
mut Foo: Bar::Baz => {}
//~^ ERROR: expected one of
_ => {}
}
}

fn main() {
let myfoo = Foo::Bar;
match myfoo {
Foo::Bar => {}
Foo:Bar::Baz => {}
//~^ ERROR: expected one of
//~| HELP: maybe write a path separator here
}
}
68 changes: 68 additions & 0 deletions src/test/ui/parser/issue-87086-colon-path-sep.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
error: expected one of `@` or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:13:12
|
LL | Foo:Bar => {}
| ^
| |
| expected one of `@` or `|`
| help: maybe write a path separator here: `::`

error: expected one of `!`, `(`, `...`, `..=`, `..`, `::`, `{`, or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:19:17
|
LL | Foo::Bar:Baz => {}
| ^
| |
| expected one of 8 possible tokens
| help: maybe write a path separator here: `::`

error: expected one of `@` or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:25:12
|
LL | Foo:Bar::Baz => {}
| ^
| |
| expected one of `@` or `|`
| help: maybe write a path separator here: `::`

error: expected one of `@` or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:31:12
|
LL | Foo: Bar::Baz if true => {}
| ^
| |
| expected one of `@` or `|`
| help: maybe write a path separator here: `::`

error: expected one of `@` or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:36:15
|
LL | if let Bar:Baz = f() {
| ^
| |
| expected one of `@` or `|`
| help: maybe write a path separator here: `::`

error: expected one of `=>`, `@`, `if`, or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:44:16
|
LL | ref Foo: Bar::Baz => {}
| ^ expected one of `=>`, `@`, `if`, or `|`

error: expected one of `=>`, `@`, `if`, or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:52:16
|
LL | mut Foo: Bar::Baz => {}
| ^ expected one of `=>`, `@`, `if`, or `|`

error: expected one of `@` or `|`, found `:`
--> $DIR/issue-87086-colon-path-sep.rs:62:12
|
LL | Foo:Bar::Baz => {}
| ^
| |
| expected one of `@` or `|`
| help: maybe write a path separator here: `::`

error: aborting due to 8 previous errors