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

Gracefully handle loop labels missing leading ' in different positions #81236

Merged
merged 9 commits into from
Jan 22, 2021
19 changes: 15 additions & 4 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_session::parse::feature_err;
use rustc_span::hygiene::ForLoopLoc;
use rustc_span::source_map::{respan, DesugaringKind, Span, Spanned};
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::{hygiene::ForLoopLoc, DUMMY_SP};
use rustc_target::asm;
use std::collections::hash_map::Entry;
use std::fmt::Write;
Expand Down Expand Up @@ -102,6 +102,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
this.lower_block(body, false),
opt_label,
hir::LoopSource::Loop,
DUMMY_SP,
)
}),
ExprKind::TryBlock(ref body) => self.lower_expr_try_block(body),
Expand Down Expand Up @@ -453,7 +454,12 @@ impl<'hir> LoweringContext<'_, 'hir> {
self.expr_match(span, scrutinee, arena_vec![self; then_arm, else_arm], desugar);

// `[opt_ident]: loop { ... }`
hir::ExprKind::Loop(self.block_expr(self.arena.alloc(match_expr)), opt_label, source)
hir::ExprKind::Loop(
self.block_expr(self.arena.alloc(match_expr)),
opt_label,
source,
span.with_hi(cond.span.hi()),
)
}

/// Desugar `try { <stmts>; <expr> }` into `{ <stmts>; ::std::ops::Try::from_ok(<expr>) }`,
Expand Down Expand Up @@ -748,7 +754,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
// loop { .. }
let loop_expr = self.arena.alloc(hir::Expr {
hir_id: loop_hir_id,
kind: hir::ExprKind::Loop(loop_block, None, hir::LoopSource::Loop),
kind: hir::ExprKind::Loop(loop_block, None, hir::LoopSource::Loop, span),
span,
attrs: ThinVec::new(),
});
Expand Down Expand Up @@ -1709,7 +1715,12 @@ impl<'hir> LoweringContext<'_, 'hir> {
);

// `[opt_ident]: loop { ... }`
let kind = hir::ExprKind::Loop(loop_block, opt_label, hir::LoopSource::ForLoop);
let kind = hir::ExprKind::Loop(
loop_block,
opt_label,
hir::LoopSource::ForLoop,
e.span.with_hi(orig_head_span.hi()),
);
let loop_expr = self.arena.alloc(hir::Expr {
hir_id: self.lower_node_id(e.id),
kind,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1617,7 +1617,9 @@ pub enum ExprKind<'hir> {
/// A conditionless loop (can be exited with `break`, `continue`, or `return`).
///
/// I.e., `'label: loop { <block> }`.
Loop(&'hir Block<'hir>, Option<Label>, LoopSource),
///
/// The `Span` is the loop header (`for x in y`/`while let pat = expr`).
Loop(&'hir Block<'hir>, Option<Label>, LoopSource, Span),
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// A `match` block, with a source that indicates whether or not it is
/// the result of a desugaring, and if so, which kind.
Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/intravisit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr<'v>)
visitor.visit_expr(then);
walk_list!(visitor, visit_expr, else_opt);
}
ExprKind::Loop(ref block, ref opt_label, _) => {
ExprKind::Loop(ref block, ref opt_label, _, _) => {
walk_list!(visitor, visit_label, opt_label);
visitor.visit_block(block);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_pretty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1396,7 +1396,7 @@ impl<'a> State<'a> {
hir::ExprKind::If(ref test, ref blk, ref elseopt) => {
self.print_if(&test, &blk, elseopt.as_ref().map(|e| &**e));
}
hir::ExprKind::Loop(ref blk, opt_label, _) => {
hir::ExprKind::Loop(ref blk, opt_label, _, _) => {
if let Some(label) = opt_label {
self.print_ident(label.ident);
self.word_space(":");
Expand Down
12 changes: 9 additions & 3 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,24 @@ fn pierce_parens(mut expr: &ast::Expr) -> &ast::Expr {

impl EarlyLintPass for WhileTrue {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
if let ast::ExprKind::While(cond, ..) = &e.kind {
if let ast::ExprKind::While(cond, _, label) = &e.kind {
if let ast::ExprKind::Lit(ref lit) = pierce_parens(cond).kind {
if let ast::LitKind::Bool(true) = lit.kind {
if !lit.span.from_expansion() {
let msg = "denote infinite loops with `loop { ... }`";
let condition_span = cx.sess.source_map().guess_head_span(e.span);
let condition_span = e.span.with_hi(cond.span.hi());
cx.struct_span_lint(WHILE_TRUE, condition_span, |lint| {
lint.build(msg)
.span_suggestion_short(
condition_span,
"use `loop`",
"loop".to_owned(),
format!(
"{}loop",
label.map_or_else(String::new, |label| format!(
"{}: ",
label.ident,
))
),
Applicability::MachineApplicable,
)
.emit();
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_mir_build/src/thir/cx/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,9 +546,7 @@ fn make_mirror_unadjusted<'a, 'tcx>(
scrutinee: discr.to_ref(),
arms: arms.iter().map(|a| convert_arm(cx, a)).collect(),
},
hir::ExprKind::Loop(ref body, _, _) => {
ExprKind::Loop { body: block::to_expr_ref(cx, body) }
}
hir::ExprKind::Loop(ref body, ..) => ExprKind::Loop { body: block::to_expr_ref(cx, body) },
hir::ExprKind::Field(ref source, ..) => ExprKind::Field {
lhs: source.to_ref(),
name: Field::new(cx.tcx.field_index(expr.hir_id, cx.typeck_results)),
Expand Down
54 changes: 48 additions & 6 deletions compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ impl<'a> Parser<'a> {
lhs_span: Span,
expr_kind: fn(P<Expr>, P<Ty>) -> ExprKind,
) -> PResult<'a, P<Expr>> {
let mk_expr = |this: &mut Self, rhs: P<Ty>| {
let mk_expr = |this: &mut Self, lhs: P<Expr>, rhs: P<Ty>| {
this.mk_expr(
this.mk_expr_sp(&lhs, lhs_span, rhs.span),
expr_kind(lhs, rhs),
Expand All @@ -597,13 +597,49 @@ impl<'a> Parser<'a> {
// LessThan comparison after this cast.
let parser_snapshot_before_type = self.clone();
let cast_expr = match self.parse_ty_no_plus() {
Ok(rhs) => mk_expr(self, rhs),
Ok(rhs) => mk_expr(self, lhs, rhs),
Err(mut type_err) => {
// Rewind to before attempting to parse the type with generics, to recover
// from situations like `x as usize < y` in which we first tried to parse
// `usize < y` as a type with generic arguments.
let parser_snapshot_after_type = mem::replace(self, parser_snapshot_before_type);

// Check for typo of `'a: loop { break 'a }` with a missing `'`.
match (&lhs.kind, &self.token.kind) {
(
// `foo: `
ExprKind::Path(None, ast::Path { segments, .. }),
TokenKind::Ident(kw::For | kw::Loop | kw::While, false),
) if segments.len() == 1 => {
let snapshot = self.clone();
let label = Label {
ident: Ident::from_str_and_span(
&format!("'{}", segments[0].ident),
segments[0].ident.span,
),
};
match self.parse_labeled_expr(label, AttrVec::new(), false) {
Ok(expr) => {
type_err.cancel();
self.struct_span_err(label.ident.span, "malformed loop label")
.span_suggestion(
label.ident.span,
"use the correct loop label format",
label.ident.to_string(),
Applicability::MachineApplicable,
)
.emit();
return Ok(expr);
}
Err(mut err) => {
err.cancel();
*self = snapshot;
}
}
}
_ => {}
}

match self.parse_path(PathStyle::Expr) {
Ok(path) => {
let (op_noun, op_verb) = match self.token.kind {
Expand All @@ -630,7 +666,8 @@ impl<'a> Parser<'a> {
op_noun,
);
let span_after_type = parser_snapshot_after_type.token.span;
let expr = mk_expr(self, self.mk_ty(path.span, TyKind::Path(None, path)));
let expr =
mk_expr(self, lhs, self.mk_ty(path.span, TyKind::Path(None, path)));

let expr_str = self
.span_to_snippet(expr.span)
Expand Down Expand Up @@ -1067,7 +1104,7 @@ impl<'a> Parser<'a> {
} else if self.eat_keyword(kw::While) {
self.parse_while_expr(None, self.prev_token.span, attrs)
} else if let Some(label) = self.eat_label() {
self.parse_labeled_expr(label, attrs)
self.parse_labeled_expr(label, attrs, true)
} else if self.eat_keyword(kw::Loop) {
self.parse_loop_expr(None, self.prev_token.span, attrs)
} else if self.eat_keyword(kw::Continue) {
Expand Down Expand Up @@ -1228,7 +1265,12 @@ impl<'a> Parser<'a> {
}

/// Parse `'label: $expr`. The label is already parsed.
fn parse_labeled_expr(&mut self, label: Label, attrs: AttrVec) -> PResult<'a, P<Expr>> {
fn parse_labeled_expr(
&mut self,
label: Label,
attrs: AttrVec,
consume_colon: bool,
) -> PResult<'a, P<Expr>> {
let lo = label.ident.span;
let label = Some(label);
let ate_colon = self.eat(&token::Colon);
Expand All @@ -1247,7 +1289,7 @@ impl<'a> Parser<'a> {
self.parse_expr()
}?;

if !ate_colon {
if !ate_colon && consume_colon {
self.error_labeled_expr_must_be_followed_by_colon(lo, expr.span);
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/check_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ impl<'tcx> Visitor<'tcx> for CheckConstVisitor<'tcx> {
// Skip the following checks if we are not currently in a const context.
_ if self.const_kind.is_none() => {}

hir::ExprKind::Loop(_, _, source) => {
hir::ExprKind::Loop(_, _, source, _) => {
self.const_check_violated(NonConstExpr::Loop(*source), e.span);
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {

// Note that labels have been resolved, so we don't need to look
// at the label ident
hir::ExprKind::Loop(ref blk, _, _) => self.propagate_through_loop(expr, &blk, succ),
hir::ExprKind::Loop(ref blk, ..) => self.propagate_through_loop(expr, &blk, succ),

hir::ExprKind::If(ref cond, ref then, ref else_opt) => {
//
Expand Down
94 changes: 67 additions & 27 deletions compiler/rustc_passes/src/loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {

fn visit_expr(&mut self, e: &'hir hir::Expr<'hir>) {
match e.kind {
hir::ExprKind::Loop(ref b, _, source) => {
hir::ExprKind::Loop(ref b, _, source, _) => {
self.with_context(Loop(source), |v| v.visit_block(&b));
}
hir::ExprKind::Closure(_, ref function_decl, b, span, movability) => {
Expand All @@ -68,18 +68,18 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
hir::ExprKind::Block(ref b, Some(_label)) => {
self.with_context(LabeledBlock, |v| v.visit_block(&b));
}
hir::ExprKind::Break(label, ref opt_expr) => {
hir::ExprKind::Break(break_label, ref opt_expr) => {
if let Some(e) = opt_expr {
self.visit_expr(e);
}

if self.require_label_in_labeled_block(e.span, &label, "break") {
if self.require_label_in_labeled_block(e.span, &break_label, "break") {
// If we emitted an error about an unlabeled break in a labeled
// block, we don't need any further checking for this break any more
return;
}

let loop_id = match label.target_id {
let loop_id = match break_label.target_id {
Ok(loop_id) => Some(loop_id),
Err(hir::LoopIdError::OutsideLoopScope) => None,
Err(hir::LoopIdError::UnlabeledCfInWhileCondition) => {
Expand All @@ -89,49 +89,89 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
Err(hir::LoopIdError::UnresolvedLabel) => None,
};

if let Some(loop_id) = loop_id {
if let Node::Block(_) = self.hir_map.find(loop_id).unwrap() {
return;
}
if let Some(Node::Block(_)) = loop_id.and_then(|id| self.hir_map.find(id)) {
return;
}

if opt_expr.is_some() {
let loop_kind = if let Some(loop_id) = loop_id {
Some(match self.hir_map.expect_expr(loop_id).kind {
hir::ExprKind::Loop(_, _, source) => source,
if let Some(break_expr) = opt_expr {
let (head, loop_label, loop_kind) = if let Some(loop_id) = loop_id {
match self.hir_map.expect_expr(loop_id).kind {
hir::ExprKind::Loop(_, label, source, sp) => {
(Some(sp), label, Some(source))
}
ref r => {
span_bug!(e.span, "break label resolved to a non-loop: {:?}", r)
}
})
}
} else {
None
(None, None, None)
};
match loop_kind {
None | Some(hir::LoopSource::Loop) => (),
Some(kind) => {
struct_span_err!(
let mut err = struct_span_err!(
self.sess,
e.span,
E0571,
"`break` with value from a `{}` loop",
kind.name()
)
.span_label(
);
err.span_label(
e.span,
"can only break with a value inside \
`loop` or breakable block",
)
.span_suggestion(
"can only break with a value inside `loop` or breakable block",
);
if let Some(head) = head {
err.span_label(
head,
&format!(
"you can't `break` with a value in a `{}` loop",
kind.name()
),
);
}
err.span_suggestion(
e.span,
&format!(
"instead, use `break` on its own \
without a value inside this `{}` loop",
kind.name()
"use `break` on its own without a value inside this `{}` loop",
kind.name(),
),
format!(
"break{}",
break_label
.label
.map_or_else(String::new, |l| format!(" {}", l.ident))
),
"break".to_string(),
Applicability::MaybeIncorrect,
)
.emit();
);
if let (Some(label), None) = (loop_label, break_label.label) {
match break_expr.kind {
hir::ExprKind::Path(hir::QPath::Resolved(
None,
hir::Path {
segments: [segment],
res: hir::def::Res::Err,
..
},
)) if label.ident.to_string()
== format!("'{}", segment.ident) =>
{
// This error is redundant, we will have already emitted a
// suggestion to use the label when `segment` wasn't found
// (hence the `Res::Err` check).
err.delay_as_bug();
}
_ => {
err.span_suggestion(
break_expr.span,
"alternatively, you might have meant to use the \
available loop label",
label.ident.to_string(),
Applicability::MaybeIncorrect,
);
}
}
}
err.emit();
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h
terminating(then.hir_id.local_id);
}

hir::ExprKind::Loop(ref body, _, _) => {
hir::ExprKind::Loop(ref body, _, _, _) => {
terminating(body.hir_id.local_id);
}

Expand Down
Loading