From 4e68ddca90a8f3ee580445b1fc83a6a6d0c7f94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 23 May 2019 13:10:24 -0700 Subject: [PATCH] review comments: move back some methods and clean up wording --- src/libsyntax/parse/diagnostics.rs | 185 ++++++++++++++---- src/libsyntax/parse/parser.rs | 163 +++------------ .../ui/invalid-self-argument/bare-fn-start.rs | 6 +- .../bare-fn-start.stderr | 6 +- src/test/ui/invalid-self-argument/bare-fn.rs | 6 +- .../ui/invalid-self-argument/bare-fn.stderr | 6 +- src/test/ui/invalid-self-argument/trait-fn.rs | 4 +- .../ui/invalid-self-argument/trait-fn.stderr | 4 +- src/test/ui/parser/self-in-function-arg.rs | 2 +- .../ui/parser/self-in-function-arg.stderr | 6 +- 10 files changed, 196 insertions(+), 192 deletions(-) diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 7df9b80c3c5..9431b559da5 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -5,18 +5,15 @@ use crate::ast::{ }; use crate::parse::{SeqSep, token, PResult, Parser}; use crate::parse::parser::{BlockMode, PathStyle, SemiColonMode, TokenType, TokenExpectType}; -use crate::parse::token; use crate::print::pprust; use crate::ptr::P; use crate::source_map::Spanned; use crate::symbol::kw; use crate::ThinVec; -use crate::tokenstream::TokenTree; use crate::util::parser::AssocOp; -use errors::{Applicability, DiagnosticBuilder, DiagnosticId, FatalError}; +use errors::{Applicability, DiagnosticBuilder, DiagnosticId}; use syntax_pos::{Span, DUMMY_SP, MultiSpan}; use log::{debug, trace}; -use std::slice; pub enum Error { FileNotFoundForModule { @@ -148,36 +145,8 @@ impl RecoverQPath for Expr { } impl<'a> Parser<'a> { - pub fn look_ahead(&self, dist: usize, f: F) -> R where - F: FnOnce(&token::Token) -> R, - { - if dist == 0 { - return f(&self.token) - } - - f(&match self.token_cursor.frame.tree_cursor.look_ahead(dist - 1) { - Some(tree) => match tree { - TokenTree::Token(_, tok) => tok, - TokenTree::Delimited(_, delim, _) => token::OpenDelim(delim), - }, - None => token::CloseDelim(self.token_cursor.frame.delim), - }) - } - - crate fn look_ahead_span(&self, dist: usize) -> Span { - if dist == 0 { - return self.span - } - - match self.token_cursor.frame.tree_cursor.look_ahead(dist - 1) { - Some(TokenTree::Token(span, _)) => span, - Some(TokenTree::Delimited(span, ..)) => span.entire(), - None => self.look_ahead_span(dist - 1), - } - } - pub fn fatal(&self, m: &str) -> DiagnosticBuilder<'a> { - self.sess.span_diagnostic.struct_span_fatal(self.span, m) + self.span_fatal(self.span, m) } pub fn span_fatal>(&self, sp: S, m: &str) -> DiagnosticBuilder<'a> { @@ -243,6 +212,145 @@ impl<'a> Parser<'a> { err } + pub fn expected_one_of_not_found( + &mut self, + edible: &[token::Token], + inedible: &[token::Token], + ) -> PResult<'a, bool /* recovered */> { + fn tokens_to_string(tokens: &[TokenType]) -> String { + let mut i = tokens.iter(); + // This might be a sign we need a connect method on Iterator. + let b = i.next() + .map_or(String::new(), |t| t.to_string()); + i.enumerate().fold(b, |mut b, (i, a)| { + if tokens.len() > 2 && i == tokens.len() - 2 { + b.push_str(", or "); + } else if tokens.len() == 2 && i == tokens.len() - 2 { + b.push_str(" or "); + } else { + b.push_str(", "); + } + b.push_str(&a.to_string()); + b + }) + } + + let mut expected = edible.iter() + .map(|x| TokenType::Token(x.clone())) + .chain(inedible.iter().map(|x| TokenType::Token(x.clone()))) + .chain(self.expected_tokens.iter().cloned()) + .collect::>(); + expected.sort_by_cached_key(|x| x.to_string()); + expected.dedup(); + let expect = tokens_to_string(&expected[..]); + let actual = self.this_token_to_string(); + let (msg_exp, (label_sp, label_exp)) = if expected.len() > 1 { + let short_expect = if expected.len() > 6 { + format!("{} possible tokens", expected.len()) + } else { + expect.clone() + }; + (format!("expected one of {}, found `{}`", expect, actual), + (self.sess.source_map().next_point(self.prev_span), + format!("expected one of {} here", short_expect))) + } else if expected.is_empty() { + (format!("unexpected token: `{}`", actual), + (self.prev_span, "unexpected token after this".to_string())) + } else { + (format!("expected {}, found `{}`", expect, actual), + (self.sess.source_map().next_point(self.prev_span), + format!("expected {} here", expect))) + }; + self.last_unexpected_token_span = Some(self.span); + let mut err = self.fatal(&msg_exp); + if self.token.is_ident_named("and") { + err.span_suggestion_short( + self.span, + "use `&&` instead of `and` for the boolean operator", + "&&".to_string(), + Applicability::MaybeIncorrect, + ); + } + if self.token.is_ident_named("or") { + err.span_suggestion_short( + self.span, + "use `||` instead of `or` for the boolean operator", + "||".to_string(), + Applicability::MaybeIncorrect, + ); + } + let sp = if self.token == token::Token::Eof { + // This is EOF, don't want to point at the following char, but rather the last token + self.prev_span + } else { + label_sp + }; + match self.recover_closing_delimiter(&expected.iter().filter_map(|tt| match tt { + TokenType::Token(t) => Some(t.clone()), + _ => None, + }).collect::>(), err) { + Err(e) => err = e, + Ok(recovered) => { + return Ok(recovered); + } + } + + let is_semi_suggestable = expected.iter().any(|t| match t { + TokenType::Token(token::Semi) => true, // we expect a `;` here + _ => false, + }) && ( // a `;` would be expected before the current keyword + self.token.is_keyword(kw::Break) || + self.token.is_keyword(kw::Continue) || + self.token.is_keyword(kw::For) || + self.token.is_keyword(kw::If) || + self.token.is_keyword(kw::Let) || + self.token.is_keyword(kw::Loop) || + self.token.is_keyword(kw::Match) || + self.token.is_keyword(kw::Return) || + self.token.is_keyword(kw::While) + ); + let cm = self.sess.source_map(); + match (cm.lookup_line(self.span.lo()), cm.lookup_line(sp.lo())) { + (Ok(ref a), Ok(ref b)) if a.line != b.line && is_semi_suggestable => { + // The spans are in different lines, expected `;` and found `let` or `return`. + // High likelihood that it is only a missing `;`. + err.span_suggestion_short( + label_sp, + "a semicolon may be missing here", + ";".to_string(), + Applicability::MaybeIncorrect, + ); + err.emit(); + return Ok(true); + } + (Ok(ref a), Ok(ref b)) if a.line == b.line => { + // When the spans are in the same line, it means that the only content between + // them is whitespace, point at the found token in that case: + // + // X | () => { syntax error }; + // | ^^^^^ expected one of 8 possible tokens here + // + // instead of having: + // + // X | () => { syntax error }; + // | -^^^^^ unexpected token + // | | + // | expected one of 8 possible tokens here + err.span_label(self.span, label_exp); + } + _ if self.prev_span == syntax_pos::DUMMY_SP => { + // Account for macro context where the previous span might not be + // available to avoid incorrect output (#54841). + err.span_label(self.span, "unexpected token"); + } + _ => { + err.span_label(sp, label_exp); + err.span_label(self.span, "unexpected token"); + } + } + Err(err) + } + /// Eats and discards tokens until one of `kets` is encountered. Respects token trees, /// passes through any errors encountered. Used for error recovery. crate fn eat_to_tokens(&mut self, kets: &[&token::Token]) { @@ -939,9 +1047,6 @@ impl<'a> Parser<'a> { String::new(), Applicability::MachineApplicable, ); - err.note("if you meant to use emplacement syntax, it is obsolete (for now, anyway)"); - err.note("for more information on the status of emplacement syntax, see <\ - https://github.com/rust-lang/rust/issues/27779#issuecomment-378416911>"); err.emit(); } } @@ -1050,12 +1155,12 @@ impl<'a> Parser<'a> { ) -> PResult<'a, ast::Arg> { let sp = arg.pat.span; arg.ty.node = TyKind::Err; - let mut err = self.struct_span_err(sp, "unexpected `self` argument in function"); + let mut err = self.struct_span_err(sp, "unexpected `self` parameter in function"); if is_trait_item { - err.span_label(sp, "must be the first associated function argument"); + err.span_label(sp, "must be the first associated function parameter"); } else { - err.span_label(sp, "not valid as function argument"); - err.note("`self` is only valid as the first argument of an associated function"); + err.span_label(sp, "not valid as function parameter"); + err.note("`self` is only valid as the first parameter of an associated function"); } err.emit(); Ok(arg) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 6d866d7f3d4..6c29437362c 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -49,7 +49,7 @@ use crate::tokenstream::{self, DelimSpan, TokenTree, TokenStream, TreeAndJoint}; use crate::symbol::{kw, sym, Symbol}; use crate::parse::diagnostics::Error; -use errors::{Applicability, DiagnosticBuilder, DiagnosticId}; +use errors::{Applicability, DiagnosticBuilder, DiagnosticId, FatalError}; use rustc_target::spec::abi::{self, Abi}; use syntax_pos::{Span, BytePos, DUMMY_SP, FileName, hygiene::CompilerDesugaringKind}; use log::debug; @@ -58,6 +58,7 @@ use std::borrow::Cow; use std::cmp; use std::mem; use std::path::{self, Path, PathBuf}; +use std::slice; #[derive(Debug)] /// Whether the type alias or associated type is a concrete type or an existential type @@ -595,23 +596,6 @@ impl<'a> Parser<'a> { edible: &[token::Token], inedible: &[token::Token], ) -> PResult<'a, bool /* recovered */> { - fn tokens_to_string(tokens: &[TokenType]) -> String { - let mut i = tokens.iter(); - // This might be a sign we need a connect method on Iterator. - let b = i.next() - .map_or(String::new(), |t| t.to_string()); - i.enumerate().fold(b, |mut b, (i, a)| { - if tokens.len() > 2 && i == tokens.len() - 2 { - b.push_str(", or "); - } else if tokens.len() == 2 && i == tokens.len() - 2 { - b.push_str(" or "); - } else { - b.push_str(", "); - } - b.push_str(&a.to_string()); - b - }) - } if edible.contains(&self.token) { self.bump(); Ok(false) @@ -621,120 +605,7 @@ impl<'a> Parser<'a> { } else if self.last_unexpected_token_span == Some(self.span) { FatalError.raise(); } else { - let mut expected = edible.iter() - .map(|x| TokenType::Token(x.clone())) - .chain(inedible.iter().map(|x| TokenType::Token(x.clone()))) - .chain(self.expected_tokens.iter().cloned()) - .collect::>(); - expected.sort_by_cached_key(|x| x.to_string()); - expected.dedup(); - let expect = tokens_to_string(&expected[..]); - let actual = self.this_token_to_string(); - let (msg_exp, (label_sp, label_exp)) = if expected.len() > 1 { - let short_expect = if expected.len() > 6 { - format!("{} possible tokens", expected.len()) - } else { - expect.clone() - }; - (format!("expected one of {}, found `{}`", expect, actual), - (self.sess.source_map().next_point(self.prev_span), - format!("expected one of {} here", short_expect))) - } else if expected.is_empty() { - (format!("unexpected token: `{}`", actual), - (self.prev_span, "unexpected token after this".to_string())) - } else { - (format!("expected {}, found `{}`", expect, actual), - (self.sess.source_map().next_point(self.prev_span), - format!("expected {} here", expect))) - }; - self.last_unexpected_token_span = Some(self.span); - let mut err = self.fatal(&msg_exp); - if self.token.is_ident_named("and") { - err.span_suggestion_short( - self.span, - "use `&&` instead of `and` for the boolean operator", - "&&".to_string(), - Applicability::MaybeIncorrect, - ); - } - if self.token.is_ident_named("or") { - err.span_suggestion_short( - self.span, - "use `||` instead of `or` for the boolean operator", - "||".to_string(), - Applicability::MaybeIncorrect, - ); - } - let sp = if self.token == token::Token::Eof { - // This is EOF, don't want to point at the following char, but rather the last token - self.prev_span - } else { - label_sp - }; - match self.recover_closing_delimiter(&expected.iter().filter_map(|tt| match tt { - TokenType::Token(t) => Some(t.clone()), - _ => None, - }).collect::>(), err) { - Err(e) => err = e, - Ok(recovered) => { - return Ok(recovered); - } - } - - let is_semi_suggestable = expected.iter().any(|t| match t { - TokenType::Token(token::Semi) => true, // we expect a `;` here - _ => false, - }) && ( // a `;` would be expected before the current keyword - self.token.is_keyword(kw::Break) || - self.token.is_keyword(kw::Continue) || - self.token.is_keyword(kw::For) || - self.token.is_keyword(kw::If) || - self.token.is_keyword(kw::Let) || - self.token.is_keyword(kw::Loop) || - self.token.is_keyword(kw::Match) || - self.token.is_keyword(kw::Return) || - self.token.is_keyword(kw::While) - ); - let cm = self.sess.source_map(); - match (cm.lookup_line(self.span.lo()), cm.lookup_line(sp.lo())) { - (Ok(ref a), Ok(ref b)) if a.line != b.line && is_semi_suggestable => { - // The spans are in different lines, expected `;` and found `let` or `return`. - // High likelihood that it is only a missing `;`. - err.span_suggestion_short( - label_sp, - "a semicolon may be missing here", - ";".to_string(), - Applicability::MaybeIncorrect, - ); - err.emit(); - return Ok(true); - } - (Ok(ref a), Ok(ref b)) if a.line == b.line => { - // When the spans are in the same line, it means that the only content between - // them is whitespace, point at the found token in that case: - // - // X | () => { syntax error }; - // | ^^^^^ expected one of 8 possible tokens here - // - // instead of having: - // - // X | () => { syntax error }; - // | -^^^^^ unexpected token - // | | - // | expected one of 8 possible tokens here - err.span_label(self.span, label_exp); - } - _ if self.prev_span == DUMMY_SP => { - // Account for macro context where the previous span might not be - // available to avoid incorrect output (#54841). - err.span_label(self.span, "unexpected token"); - } - _ => { - err.span_label(sp, label_exp); - err.span_label(self.span, "unexpected token"); - } - } - Err(err) + self.expected_one_of_not_found(edible, inedible) } } @@ -1188,6 +1059,34 @@ impl<'a> Parser<'a> { self.expected_tokens.clear(); } + pub fn look_ahead(&self, dist: usize, f: F) -> R where + F: FnOnce(&token::Token) -> R, + { + if dist == 0 { + return f(&self.token) + } + + f(&match self.token_cursor.frame.tree_cursor.look_ahead(dist - 1) { + Some(tree) => match tree { + TokenTree::Token(_, tok) => tok, + TokenTree::Delimited(_, delim, _) => token::OpenDelim(delim), + }, + None => token::CloseDelim(self.token_cursor.frame.delim), + }) + } + + crate fn look_ahead_span(&self, dist: usize) -> Span { + if dist == 0 { + return self.span + } + + match self.token_cursor.frame.tree_cursor.look_ahead(dist - 1) { + Some(TokenTree::Token(span, _)) => span, + Some(TokenTree::Delimited(span, ..)) => span.entire(), + None => self.look_ahead_span(dist - 1), + } + } + /// Is the current token one of the keywords that signals a bare function type? fn token_is_bare_fn_keyword(&mut self) -> bool { self.check_keyword(kw::Fn) || diff --git a/src/test/ui/invalid-self-argument/bare-fn-start.rs b/src/test/ui/invalid-self-argument/bare-fn-start.rs index 95a2b69c05c..a003a01941b 100644 --- a/src/test/ui/invalid-self-argument/bare-fn-start.rs +++ b/src/test/ui/invalid-self-argument/bare-fn-start.rs @@ -1,6 +1,6 @@ fn a(&self) { } -//~^ ERROR unexpected `self` argument in function -//~| NOTE not valid as function argument -//~| NOTE `self` is only valid as the first argument of an associated function +//~^ ERROR unexpected `self` parameter in function +//~| NOTE not valid as function parameter +//~| NOTE `self` is only valid as the first parameter of an associated function fn main() { } diff --git a/src/test/ui/invalid-self-argument/bare-fn-start.stderr b/src/test/ui/invalid-self-argument/bare-fn-start.stderr index ba1092ca377..23de6502094 100644 --- a/src/test/ui/invalid-self-argument/bare-fn-start.stderr +++ b/src/test/ui/invalid-self-argument/bare-fn-start.stderr @@ -1,10 +1,10 @@ -error: unexpected `self` argument in function +error: unexpected `self` parameter in function --> $DIR/bare-fn-start.rs:1:6 | LL | fn a(&self) { } - | ^^^^^ not valid as function argument + | ^^^^^ not valid as function parameter | - = note: `self` is only valid as the first argument of an associated function + = note: `self` is only valid as the first parameter of an associated function error: aborting due to previous error diff --git a/src/test/ui/invalid-self-argument/bare-fn.rs b/src/test/ui/invalid-self-argument/bare-fn.rs index 43c87fb79d8..73d68e8b7a5 100644 --- a/src/test/ui/invalid-self-argument/bare-fn.rs +++ b/src/test/ui/invalid-self-argument/bare-fn.rs @@ -1,6 +1,6 @@ fn b(foo: u32, &mut self) { } -//~^ ERROR unexpected `self` argument in function -//~| NOTE not valid as function argument -//~| NOTE `self` is only valid as the first argument of an associated function +//~^ ERROR unexpected `self` parameter in function +//~| NOTE not valid as function parameter +//~| NOTE `self` is only valid as the first parameter of an associated function fn main() { } diff --git a/src/test/ui/invalid-self-argument/bare-fn.stderr b/src/test/ui/invalid-self-argument/bare-fn.stderr index 16a2d4b4b37..601a51bb4a9 100644 --- a/src/test/ui/invalid-self-argument/bare-fn.stderr +++ b/src/test/ui/invalid-self-argument/bare-fn.stderr @@ -1,10 +1,10 @@ -error: unexpected `self` argument in function +error: unexpected `self` parameter in function --> $DIR/bare-fn.rs:1:16 | LL | fn b(foo: u32, &mut self) { } - | ^^^^^^^^^ not valid as function argument + | ^^^^^^^^^ not valid as function parameter | - = note: `self` is only valid as the first argument of an associated function + = note: `self` is only valid as the first parameter of an associated function error: aborting due to previous error diff --git a/src/test/ui/invalid-self-argument/trait-fn.rs b/src/test/ui/invalid-self-argument/trait-fn.rs index 620a06db557..1e8220d7b4a 100644 --- a/src/test/ui/invalid-self-argument/trait-fn.rs +++ b/src/test/ui/invalid-self-argument/trait-fn.rs @@ -2,8 +2,8 @@ struct Foo {} impl Foo { fn c(foo: u32, self) {} - //~^ ERROR unexpected `self` argument in function - //~| NOTE must be the first associated function argument + //~^ ERROR unexpected `self` parameter in function + //~| NOTE must be the first associated function parameter fn good(&mut self, foo: u32) {} } diff --git a/src/test/ui/invalid-self-argument/trait-fn.stderr b/src/test/ui/invalid-self-argument/trait-fn.stderr index 00fedea3fea..96a2251c036 100644 --- a/src/test/ui/invalid-self-argument/trait-fn.stderr +++ b/src/test/ui/invalid-self-argument/trait-fn.stderr @@ -1,8 +1,8 @@ -error: unexpected `self` argument in function +error: unexpected `self` parameter in function --> $DIR/trait-fn.rs:4:20 | LL | fn c(foo: u32, self) {} - | ^^^^ must be the first associated function argument + | ^^^^ must be the first associated function parameter error: aborting due to previous error diff --git a/src/test/ui/parser/self-in-function-arg.rs b/src/test/ui/parser/self-in-function-arg.rs index 502c2c0b74a..6172ffe1b03 100644 --- a/src/test/ui/parser/self-in-function-arg.rs +++ b/src/test/ui/parser/self-in-function-arg.rs @@ -1,3 +1,3 @@ -fn foo(x:i32, self: i32) -> i32 { self } //~ ERROR unexpected `self` argument in function +fn foo(x:i32, self: i32) -> i32 { self } //~ ERROR unexpected `self` parameter in function fn main() {} diff --git a/src/test/ui/parser/self-in-function-arg.stderr b/src/test/ui/parser/self-in-function-arg.stderr index e1fc10306cc..f58df9b9e79 100644 --- a/src/test/ui/parser/self-in-function-arg.stderr +++ b/src/test/ui/parser/self-in-function-arg.stderr @@ -1,10 +1,10 @@ -error: unexpected `self` argument in function +error: unexpected `self` parameter in function --> $DIR/self-in-function-arg.rs:1:15 | LL | fn foo(x:i32, self: i32) -> i32 { self } - | ^^^^ not valid as function argument + | ^^^^ not valid as function parameter | - = note: `self` is only valid as the first argument of an associated function + = note: `self` is only valid as the first parameter of an associated function error: aborting due to previous error