Skip to content
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

Parse alternative incorrect uses of await and recover #60873

Merged
merged 10 commits into from
May 17, 2019
17 changes: 15 additions & 2 deletions src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ pub struct LoweringContext<'a> {
is_generator: bool,
is_async_body: bool,

/// Used to get the current `fn`'s def span to point to when using `await`
/// outside of an `async fn`.
current_item: Option<Span>,

catch_scopes: Vec<NodeId>,
loop_scopes: Vec<NodeId>,
is_in_loop_condition: bool,
Expand Down Expand Up @@ -250,6 +254,7 @@ pub fn lower_crate(
node_id_to_hir_id: IndexVec::new(),
is_generator: false,
is_async_body: false,
current_item: None,
is_in_trait_impl: false,
lifetimes_to_define: Vec::new(),
is_collecting_in_band_lifetimes: false,
Expand Down Expand Up @@ -3116,6 +3121,7 @@ impl<'a> LoweringContext<'a> {
ItemKind::Fn(ref decl, ref header, ref generics, ref body) => {
let fn_def_id = self.resolver.definitions().local_def_id(id);
self.with_new_scopes(|this| {
this.current_item = Some(ident.span);
let mut lower_fn = |decl: &FnDecl| {
// Note: we don't need to change the return type from `T` to
// `impl Future<Output = T>` here because lower_body
Expand Down Expand Up @@ -3654,6 +3660,7 @@ impl<'a> LoweringContext<'a> {
} else {
lower_method(sig)
};
self.current_item = Some(i.span);

(generics, hir::ImplItemKind::Method(sig, body_id))
}
Expand Down Expand Up @@ -4270,6 +4277,7 @@ impl<'a> LoweringContext<'a> {
let fn_decl = self.lower_fn_decl(decl, None, false, None);

self.with_new_scopes(|this| {
this.current_item = Some(fn_decl_span);
let mut is_generator = false;
let body_id = this.lower_body(Some(decl), |this| {
let e = this.lower_expr(body);
Expand Down Expand Up @@ -5551,13 +5559,18 @@ impl<'a> LoweringContext<'a> {
// }
// }
if !self.is_async_body {
span_err!(
let mut err = struct_span_err!(
self.sess,
await_span,
E0728,
"`await` is only allowed inside `async` functions and blocks"
);
self.sess.abort_if_errors();
err.span_label(await_span, "only allowed inside `async` functions and blocks");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems redundant with the message above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to remove all primary spans without labels from the cli output. The main msg should be generic and the label text more targeted and suitable to show inline in (for example) VSCode.

Would you be ok if the main message was "await in a function or block not marked with async" or similar?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have two messages conveying the same information-- the wording isn't the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some people ignore the error message entirely and focus exclusively on the spans. I agree that unnecessary text is suboptimal, but I do not

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> $DIR/issue-51751.rs:11:20
   |
LL |     let finished = result.await;
   |                    ^^^^^^^^^^^^

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> $DIR/issue-51751.rs:11:20
   |
LL |     let finished = result.await;
   |                    ^^^^^^^^^^^^ only allowed inside `async` functions and blocks

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> $DIR/issue-51751.rs:11:20
   |
LL | fn main() {
   |    ---- this is not `async`
LL |     let result = inc(10000);
LL |     let finished = result.await;
   |                    ^^^^^^^^^^^^ only allowed inside `async` functions and blocks

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks
  --> $DIR/issue-51751.rs:11:20
   |
LL | fn main() {
   |    ---- this is not `async`
LL |     let result = inc(10000);
LL |     let finished = result.await;
   |                    ^^^^^^^^^^^^

The last case is what this would be like if we remove the label, which has some text in the secondary label, but no text on the primary label, which I feel is suboptimal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank Hmm, ok -- leave it as is then; in context I think your solution is the best UX.

if let Some(item_sp) = self.current_item {
err.span_label(item_sp, "this is not `async`");
}
err.emit();
return hir::ExprKind::Err;
}
let span = self.sess.source_map().mark_span_with_reason(
CompilerDesugaringKind::Await,
Expand Down
308 changes: 305 additions & 3 deletions src/libsyntax/parse/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
use crate::ast;
use crate::ast::{Expr, ExprKind, Item, ItemKind, Pat, PatKind, QSelf, Ty, TyKind};
use crate::parse::parser::PathStyle;
use crate::ast::{BlockCheckMode, Expr, ExprKind, Item, ItemKind, Pat, PatKind, QSelf, Ty, TyKind};
use crate::parse::parser::{BlockMode, PathStyle, TokenType, SemiColonMode};
use crate::parse::token;
use crate::parse::PResult;
use crate::parse::Parser;
use crate::print::pprust;
use crate::ptr::P;
use crate::symbol::keywords;
use crate::ThinVec;
use errors::Applicability;
use errors::{Applicability, DiagnosticBuilder};
use syntax_pos::Span;
use log::debug;

pub trait RecoverQPath: Sized + 'static {
const PATH_STYLE: PathStyle = PathStyle::Expr;
Expand Down Expand Up @@ -223,4 +225,304 @@ impl<'a> Parser<'a> {
false
}
}

/// Consume alternative await syntaxes like `await <expr>`, `await? <expr>`, `await(<expr>)`
/// and `await { <expr> }`.
crate fn parse_incorrect_await_syntax(
&mut self,
lo: Span,
await_sp: Span,
) -> PResult<'a, (Span, ExprKind)> {
let is_question = self.eat(&token::Question); // Handle `await? <expr>`.
let expr = if self.token == token::OpenDelim(token::Brace) {
// Handle `await { <expr> }`.
// This needs to be handled separatedly from the next arm to avoid
// interpreting `await { <expr> }?` as `<expr>?.await`.
self.parse_block_expr(
None,
self.span,
BlockCheckMode::Default,
ThinVec::new(),
)
} else {
self.parse_expr()
}.map_err(|mut err| {
err.span_label(await_sp, "while parsing this incorrect await expression");
err
})?;
let expr_str = self.sess.source_map().span_to_snippet(expr.span)
.unwrap_or_else(|_| pprust::expr_to_string(&expr));
let suggestion = format!("{}.await{}", expr_str, if is_question { "?" } else { "" });
let sp = lo.to(expr.span);
let app = match expr.node {
ExprKind::Try(_) => Applicability::MaybeIncorrect, // `await <expr>?`
_ => Applicability::MachineApplicable,
};
self.struct_span_err(sp, "incorrect use of `await`")
.span_suggestion(sp, "`await` is a postfix operation", suggestion, app)
.emit();
Ok((sp, ExprKind::Await(ast::AwaitOrigin::FieldLike, expr)))
}

/// If encountering `future.await()`, consume and emit error.
crate fn recover_from_await_method_call(&mut self) {
if self.token == token::OpenDelim(token::Paren) &&
self.look_ahead(1, |t| t == &token::CloseDelim(token::Paren))
{
// future.await()
let lo = self.span;
self.bump(); // (
let sp = lo.to(self.span);
self.bump(); // )
let mut err = self.struct_span_err(sp, "incorrect use of `await`");
estebank marked this conversation as resolved.
Show resolved Hide resolved
err.span_suggestion(
sp,
"`await` is not a method call, remove the parentheses",
String::new(),
Applicability::MachineApplicable,
);
err.emit()
}
}

crate fn could_ascription_be_path(&self, node: &ast::ExprKind) -> bool {
self.token.is_ident() &&
if let ast::ExprKind::Path(..) = node { true } else { false } &&
!self.token.is_reserved_ident() && // v `foo:bar(baz)`
self.look_ahead(1, |t| t == &token::OpenDelim(token::Paren)) ||
self.look_ahead(1, |t| t == &token::Lt) && // `foo:bar<baz`
self.look_ahead(2, |t| t.is_ident()) ||
self.look_ahead(1, |t| t == &token::Colon) && // `foo:bar:baz`
self.look_ahead(2, |t| t.is_ident()) ||
self.look_ahead(1, |t| t == &token::ModSep) && // `foo:bar::baz`
self.look_ahead(2, |t| t.is_ident())
}

crate fn bad_type_ascription(
&self,
err: &mut DiagnosticBuilder<'a>,
lhs_span: Span,
cur_op_span: Span,
next_sp: Span,
maybe_path: bool,
) {
err.span_label(self.span, "expecting a type here because of type ascription");
let cm = self.sess.source_map();
let next_pos = cm.lookup_char_pos(next_sp.lo());
let op_pos = cm.lookup_char_pos(cur_op_span.hi());
if op_pos.line != next_pos.line {
err.span_suggestion(
cur_op_span,
"try using a semicolon",
";".to_string(),
Applicability::MaybeIncorrect,
);
} else {
if maybe_path {
err.span_suggestion(
cur_op_span,
"maybe you meant to write a path separator here",
"::".to_string(),
Applicability::MaybeIncorrect,
);
} else {
err.note("type ascription is a nightly-only feature that lets \
estebank marked this conversation as resolved.
Show resolved Hide resolved
you annotate an expression with a type: `<expr>: <type>`");
err.span_note(
lhs_span,
"this expression expects an ascribed type after the colon",
);
err.help("this might be indicative of a syntax error elsewhere");
}
}
}

crate fn recover_seq_parse_error(
&mut self,
delim: token::DelimToken,
lo: Span,
result: PResult<'a, P<Expr>>,
) -> P<Expr> {
match result {
Ok(x) => x,
Err(mut err) => {
err.emit();
// recover from parse error
self.consume_block(delim);
self.mk_expr(lo.to(self.prev_span), ExprKind::Err, ThinVec::new())
}
}
}

crate fn recover_closing_delimiter(
&mut self,
tokens: &[token::Token],
mut err: DiagnosticBuilder<'a>,
) -> PResult<'a, bool> {
let mut pos = None;
// we want to use the last closing delim that would apply
for (i, unmatched) in self.unclosed_delims.iter().enumerate().rev() {
if tokens.contains(&token::CloseDelim(unmatched.expected_delim))
&& Some(self.span) > unmatched.unclosed_span
{
pos = Some(i);
}
}
match pos {
Some(pos) => {
// Recover and assume that the detected unclosed delimiter was meant for
// this location. Emit the diagnostic and act as if the delimiter was
// present for the parser's sake.

// Don't attempt to recover from this unclosed delimiter more than once.
let unmatched = self.unclosed_delims.remove(pos);
let delim = TokenType::Token(token::CloseDelim(unmatched.expected_delim));

// We want to suggest the inclusion of the closing delimiter where it makes
// the most sense, which is immediately after the last token:
//
// {foo(bar {}}
// - ^
// | |
// | help: `)` may belong here (FIXME: #58270)
// |
// unclosed delimiter
if let Some(sp) = unmatched.unclosed_span {
err.span_label(sp, "unclosed delimiter");
}
err.span_suggestion_short(
self.sess.source_map().next_point(self.prev_span),
&format!("{} may belong here", delim.to_string()),
delim.to_string(),
Applicability::MaybeIncorrect,
);
err.emit();
self.expected_tokens.clear(); // reduce errors
Ok(true)
}
_ => Err(err),
}
}

/// Recover from `pub` keyword in places where it seems _reasonable_ but isn't valid.
crate fn eat_bad_pub(&mut self) {
if self.token.is_keyword(keywords::Pub) {
match self.parse_visibility(false) {
Ok(vis) => {
let mut err = self.diagnostic()
estebank marked this conversation as resolved.
Show resolved Hide resolved
.struct_span_err(vis.span, "unnecessary visibility qualifier");
err.span_label(vis.span, "`pub` not permitted here");
err.emit();
}
Err(mut err) => err.emit(),
}
}
}

// Eat tokens until we can be relatively sure we reached the end of the
// statement. This is something of a best-effort heuristic.
//
// We terminate when we find an unmatched `}` (without consuming it).
crate fn recover_stmt(&mut self) {
self.recover_stmt_(SemiColonMode::Ignore, BlockMode::Ignore)
}

// If `break_on_semi` is `Break`, then we will stop consuming tokens after
// finding (and consuming) a `;` outside of `{}` or `[]` (note that this is
// approximate - it can mean we break too early due to macros, but that
// should only lead to sub-optimal recovery, not inaccurate parsing).
//
// If `break_on_block` is `Break`, then we will stop consuming tokens
// after finding (and consuming) a brace-delimited block.
crate fn recover_stmt_(&mut self, break_on_semi: SemiColonMode, break_on_block: BlockMode) {
let mut brace_depth = 0;
let mut bracket_depth = 0;
let mut in_block = false;
debug!("recover_stmt_ enter loop (semi={:?}, block={:?})",
break_on_semi, break_on_block);
loop {
debug!("recover_stmt_ loop {:?}", self.token);
match self.token {
token::OpenDelim(token::DelimToken::Brace) => {
brace_depth += 1;
self.bump();
if break_on_block == BlockMode::Break &&
brace_depth == 1 &&
bracket_depth == 0 {
in_block = true;
}
}
token::OpenDelim(token::DelimToken::Bracket) => {
bracket_depth += 1;
self.bump();
}
token::CloseDelim(token::DelimToken::Brace) => {
if brace_depth == 0 {
debug!("recover_stmt_ return - close delim {:?}", self.token);
break;
}
brace_depth -= 1;
self.bump();
if in_block && bracket_depth == 0 && brace_depth == 0 {
debug!("recover_stmt_ return - block end {:?}", self.token);
break;
}
}
token::CloseDelim(token::DelimToken::Bracket) => {
bracket_depth -= 1;
if bracket_depth < 0 {
bracket_depth = 0;
}
self.bump();
}
token::Eof => {
debug!("recover_stmt_ return - Eof");
break;
}
token::Semi => {
self.bump();
if break_on_semi == SemiColonMode::Break &&
brace_depth == 0 &&
bracket_depth == 0 {
debug!("recover_stmt_ return - Semi");
break;
}
}
token::Comma => {
if break_on_semi == SemiColonMode::Comma &&
estebank marked this conversation as resolved.
Show resolved Hide resolved
brace_depth == 0 &&
bracket_depth == 0 {
debug!("recover_stmt_ return - Semi");
break;
} else {
self.bump();
}
}
_ => {
self.bump()
}
}
}
}

crate fn consume_block(&mut self, delim: token::DelimToken) {
let mut brace_depth = 0;
loop {
if self.eat(&token::OpenDelim(delim)) {
brace_depth += 1;
} else if self.eat(&token::CloseDelim(delim)) {
if brace_depth == 0 {
return;
} else {
brace_depth -= 1;
continue;
}
} else if self.token == token::Eof || self.eat(&token::CloseDelim(token::NoDelim)) {
return;
} else {
self.bump();
}
}
}

}
Loading