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.

Sign up for GitHub

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

Jump to bottom

[WIP] [let_chains, 4/N] Introduce hir::ExprKind::Let #68577

Closed
wants to merge 11 commits into from
Closed

[WIP] [let_chains, 4/N] Introduce hir::ExprKind::Let #68577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9a0391f
let_chains: introduce `hir::ExprKind::Let(..)`
Centril Jan 20, 2020
4810cf6
let_chains: adjust lowering
Centril Jan 22, 2020
b20fcd3
let_chains: adjust ast_validation
Centril Jan 25, 2020
d741d8b
let_chains: adjust HAIR lowering with hack
Centril Jan 22, 2020
17a5a05
let_chains: fix region scopes with hack
Centril Jan 22, 2020
6f02799
check_match: extract common logic
Centril Jan 23, 2020
5cf33a6
let_chains: match check 'let' exprs
Centril Jan 24, 2020
7b69082
let_chains: fix ICE in generator_interior
Centril Jan 25, 2020
deed92a
let_chains: adjust for 'while let'
Centril Jan 26, 2020
4a7b537
let_chains: --bless some tests
Centril Jan 26, 2020
6ed41a8
let_chains: address FIXME in check_match
Centril Jan 27, 2020
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
16 changes: 3 additions & 13 deletions src/librustc/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ pub(super) fn note_and_explain_region(
Some(Node::Expr(expr)) => match expr.kind {
hir::ExprKind::Call(..) => "call",
hir::ExprKind::MethodCall(..) => "method call",
hir::ExprKind::Match(.., hir::MatchSource::IfLetDesugar { .. }) => "if let",
hir::ExprKind::Match(.., hir::MatchSource::WhileLetDesugar) => "while let",
hir::ExprKind::Match(.., hir::MatchSource::IfDesugar { .. }) => "if",
hir::ExprKind::Match(.., hir::MatchSource::WhileDesugar) => "while",
hir::ExprKind::Match(.., hir::MatchSource::ForLoopDesugar) => "for",
hir::ExprKind::Match(..) => "match",
_ => "expression",
Expand Down Expand Up @@ -610,10 +610,6 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
scrut_hir_id,
..
}) => match source {
hir::MatchSource::IfLetDesugar { .. } => {
let msg = "`if let` arms have incompatible types";
err.span_label(cause.span, msg);
}
hir::MatchSource::TryDesugar => {
if let Some(ty::error::ExpectedFound { expected, .. }) = exp_found {
let scrut_expr = self.tcx.hir().expect_expr(scrut_hir_id);
Expand Down Expand Up @@ -1985,9 +1981,6 @@ impl<'tcx> ObligationCause<'tcx> {
CompareImplTypeObligation { .. } => Error0308("type not compatible with trait"),
MatchExpressionArm(box MatchExpressionArmCause { source, .. }) => {
Error0308(match source {
hir::MatchSource::IfLetDesugar { .. } => {
"`if let` arms have incompatible types"
}
hir::MatchSource::TryDesugar => {
"try expression alternatives have incompatible types"
}
Expand Down Expand Up @@ -2023,10 +2016,7 @@ impl<'tcx> ObligationCause<'tcx> {
CompareImplMethodObligation { .. } => "method type is compatible with trait",
CompareImplTypeObligation { .. } => "associated type is compatible with trait",
ExprAssignable => "expression is assignable",
MatchExpressionArm(box MatchExpressionArmCause { source, .. }) => match source {
hir::MatchSource::IfLetDesugar { .. } => "`if let` arms have compatible types",
_ => "`match` arms have compatible types",
},
MatchExpressionArm(_) => "`match` arms have compatible types",
IfExpression { .. } => "`if` and `else` have incompatible types",
IfExpressionWithNoElse => "`if` missing an `else` returns `()`",
MainFunctionType => "`main` function has the correct type",
Expand Down
198 changes: 61 additions & 137 deletions src/librustc_ast_lowering/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
let ohs = self.lower_expr(ohs);
hir::ExprKind::AddrOf(k, m, ohs)
}
ExprKind::Let(ref pat, ref scrutinee) => self.lower_expr_let(e.span, pat, scrutinee),
ExprKind::Let(ref pat, ref scrutinee) => {
hir::ExprKind::Let(self.lower_pat(pat), self.lower_expr(scrutinee))
}
ExprKind::If(ref cond, ref then, ref else_opt) => {
self.lower_expr_if(e.span, cond, then, else_opt.as_deref())
}
Expand Down Expand Up @@ -243,51 +245,36 @@ impl<'hir> LoweringContext<'_, 'hir> {
}
}

/// Emit an error and lower `ast::ExprKind::Let(pat, scrutinee)` into:
/// ```rust
/// match scrutinee { pats => true, _ => false }
/// ```
fn lower_expr_let(&mut self, span: Span, pat: &Pat, scrutinee: &Expr) -> hir::ExprKind<'hir> {
// If we got here, the `let` expression is not allowed.

if self.sess.opts.unstable_features.is_nightly_build() {
self.sess
.struct_span_err(span, "`let` expressions are not supported here")
.note("only supported directly in conditions of `if`- and `while`-expressions")
.note("as well as when nested within `&&` and parenthesis in those conditions")
.emit();
} else {
self.sess
.struct_span_err(span, "expected expression, found statement (`let`)")
.note("variable declaration using `let` is a statement")
.emit();
/// Lower the condition of an `if` or `while` expression.
/// This entails some special handling of immediate `let` expressions as conditions.
/// Namely, if the given `cond` is not a `let` expression then it is wrapped in `drop-temps`.
fn lower_expr_cond(&mut self, cond: &Expr) -> &'hir hir::Expr<'hir> {
// Lower the `cond` expression.
let cond = self.lower_expr(cond);
// Normally, the `cond` of `if cond` will drop temporaries before evaluating the blocks.
// This is achieved by using `drop-temps { cond }`, equivalent to `{ let _t = $cond; _t }`.
// However, for backwards compatibility reasons, `if let pat = scrutinee`, like `match`
// does not drop the temporaries of `scrutinee` before evaluating the blocks.
match cond.kind {
hir::ExprKind::Let(..) => cond,
_ => {
let reason = DesugaringKind::CondTemporary;
let span = self.mark_span_with_reason(reason, cond.span, None);
self.expr_drop_temps(span, cond, ThinVec::new())
}
}
}

// For better recovery, we emit:
// ```
// match scrutinee { pat => true, _ => false }
// ```
// While this doesn't fully match the user's intent, it has key advantages:
// 1. We can avoid using `abort_if_errors`.
// 2. We can typeck both `pat` and `scrutinee`.
// 3. `pat` is allowed to be refutable.
// 4. The return type of the block is `bool` which seems like what the user wanted.
let scrutinee = self.lower_expr(scrutinee);
let then_arm = {
let pat = self.lower_pat(pat);
let expr = self.expr_bool(span, true);
self.arm(pat, expr)
};
let else_arm = {
let pat = self.pat_wild(span);
let expr = self.expr_bool(span, false);
self.arm(pat, expr)
};
hir::ExprKind::Match(
scrutinee,
arena_vec![self; then_arm, else_arm],
hir::MatchSource::Normal,
)
/// Lower `then` into `true => then`.
fn lower_then_arm(&mut self, span: Span, then: &Block) -> hir::Arm<'hir> {
let then_expr = self.lower_block_expr(then);
let then_pat = self.pat_bool(span, true);
self.arm(then_pat, self.arena.alloc(then_expr))
}

fn lower_else_arm(&mut self, span: Span, else_expr: &'hir hir::Expr<'hir>) -> hir::Arm<'hir> {
let else_pat = self.pat_wild(span);
self.arm(else_pat, else_expr)
}

fn lower_expr_if(
Expand All @@ -297,116 +284,53 @@ impl<'hir> LoweringContext<'_, 'hir> {
then: &Block,
else_opt: Option<&Expr>,
) -> hir::ExprKind<'hir> {
// FIXME(#53667): handle lowering of && and parens.

// `_ => else_block` where `else_block` is `{}` if there's `None`:
let else_pat = self.pat_wild(span);
let (else_expr, contains_else_clause) = match else_opt {
None => (self.expr_block_empty(span), false),
Some(els) => (self.lower_expr(els), true),
let scrutinee = self.lower_expr_cond(cond);
let then_arm = self.lower_then_arm(span, then);
let else_expr = match else_opt {
None => self.expr_block_empty(span), // Use `{}` if there's no `else` block.
Some(els) => self.lower_expr(els),
};
let else_arm = self.arm(else_pat, else_expr);

// Handle then + scrutinee:
let then_expr = self.lower_block_expr(then);
let (then_pat, scrutinee, desugar) = match cond.kind {
// `<pat> => <then>`:
ExprKind::Let(ref pat, ref scrutinee) => {
let scrutinee = self.lower_expr(scrutinee);
let pat = self.lower_pat(pat);
(pat, scrutinee, hir::MatchSource::IfLetDesugar { contains_else_clause })
}
// `true => <then>`:
_ => {
// Lower condition:
let cond = self.lower_expr(cond);
let span_block =
self.mark_span_with_reason(DesugaringKind::CondTemporary, cond.span, None);
// Wrap in a construct equivalent to `{ let _t = $cond; _t }`
// to preserve drop semantics since `if cond { ... }` does not
// let temporaries live outside of `cond`.
let cond = self.expr_drop_temps(span_block, cond, ThinVec::new());
let pat = self.pat_bool(span, true);
(pat, cond, hir::MatchSource::IfDesugar { contains_else_clause })
}
};
let then_arm = self.arm(then_pat, self.arena.alloc(then_expr));

let else_arm = self.lower_else_arm(span, else_expr);
let desugar = hir::MatchSource::IfDesugar { contains_else_clause: else_opt.is_some() };
hir::ExprKind::Match(scrutinee, arena_vec![self; then_arm, else_arm], desugar)
}

/// We desugar: `'label: while $cond $body` into:
///
/// ```
/// 'label: loop {
/// match $cond {
/// true => $body,
/// _ => break,
/// }
/// }
/// ```
///
/// where `$cond` is wrapped in `drop-temps { $cond }` if it isn't a `Let` expression.
fn lower_expr_while_in_loop_scope(
&mut self,
span: Span,
cond: &Expr,
body: &Block,
opt_label: Option<Label>,
) -> hir::ExprKind<'hir> {
// FIXME(#53667): handle lowering of && and parens.

// Note that the block AND the condition are evaluated in the loop scope.
// This is done to allow `break` from inside the condition of the loop.

// Lower the condition:
let scrutinee = self.with_loop_condition_scope(|t| t.lower_expr_cond(cond));
// `true => body`:
let then_arm = self.lower_then_arm(span, body);
// `_ => break`:
let else_arm = {
let else_pat = self.pat_wild(span);
let else_expr = self.expr_break(span, ThinVec::new());
self.arm(else_pat, else_expr)
};

// Handle then + scrutinee:
let then_expr = self.lower_block_expr(body);
let (then_pat, scrutinee, desugar, source) = match cond.kind {
ExprKind::Let(ref pat, ref scrutinee) => {
// to:
//
// [opt_ident]: loop {
// match <sub_expr> {
// <pat> => <body>,
// _ => break
// }
// }
let scrutinee = self.with_loop_condition_scope(|t| t.lower_expr(scrutinee));
let pat = self.lower_pat(pat);
(pat, scrutinee, hir::MatchSource::WhileLetDesugar, hir::LoopSource::WhileLet)
}
_ => {
// We desugar: `'label: while $cond $body` into:
//
// ```
// 'label: loop {
// match drop-temps { $cond } {
// true => $body,
// _ => break,
// }
// }
// ```

// Lower condition:
let cond = self.with_loop_condition_scope(|this| this.lower_expr(cond));
let span_block =
self.mark_span_with_reason(DesugaringKind::CondTemporary, cond.span, None);
// Wrap in a construct equivalent to `{ let _t = $cond; _t }`
// to preserve drop semantics since `while cond { ... }` does not
// let temporaries live outside of `cond`.
let cond = self.expr_drop_temps(span_block, cond, ThinVec::new());
// `true => <then>`:
let pat = self.pat_bool(span, true);
(pat, cond, hir::MatchSource::WhileDesugar, hir::LoopSource::While)
}
};
let then_arm = self.arm(then_pat, self.arena.alloc(then_expr));

let else_expr = self.expr_break(span, ThinVec::new());
let else_arm = self.lower_else_arm(span, else_expr);
// `match <scrutinee> { ... }`
let match_expr = self.expr_match(
scrutinee.span,
scrutinee,
arena_vec![self; then_arm, else_arm],
desugar,
);

let match_arms = arena_vec![self; then_arm, else_arm];
let match_desugar = hir::MatchSource::WhileDesugar;
let match_expr = self.expr_match(scrutinee.span, scrutinee, match_arms, match_desugar);
// `[opt_ident]: loop { ... }`
hir::ExprKind::Loop(self.block_expr(self.arena.alloc(match_expr)), opt_label, source)
let loop_block = self.block_expr(self.arena.alloc(match_expr));
hir::ExprKind::Loop(loop_block, opt_label, hir::LoopSource::While)
}

/// Desugar `try { <stmts>; <expr> }` into `{ <stmts>; ::std::ops::Try::from_ok(<expr>) }`,
Expand Down
78 changes: 63 additions & 15 deletions src/librustc_ast_passes/ast_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ struct AstValidator<'a> {
/// certain positions.
is_assoc_ty_bound_banned: bool,

/// Used to allow `let` expressions in certain syntactic locations.
is_let_allowed: bool,

lint_buffer: &'a mut LintBuffer,
}

Expand Down Expand Up @@ -96,6 +99,27 @@ impl<'a> AstValidator<'a> {
self.bound_context = old;
}

fn with_let_allowed(&mut self, allowed: bool, f: impl FnOnce(&mut Self, bool)) {
let old = mem::replace(&mut self.is_let_allowed, allowed);
f(self, old);
self.is_let_allowed = old;
}

/// Emits an error banning the `let` expression provided in the given location.
fn ban_let_expr(&self, expr: &'a Expr) {
Copy link
Contributor Author

@Centril Centril Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably a better idea to move this to feature gating under e.g. #![feature(let_expr)] so that we can test ::Let more than what let_chains alone would allow. We could also move that, or the ast_validation logic, out of this PR and land it separately.

let sess = &self.session;
if sess.opts.unstable_features.is_nightly_build() {
sess.struct_span_err(expr.span, "`let` expressions are not supported here")
.note("only supported directly in conditions of `if`- and `while`-expressions")
.note("as well as when nested within `&&` and parenthesis in those conditions")
.emit();
} else {
sess.struct_span_err(expr.span, "expected expression, found statement (`let`)")
.note("variable declaration using `let` is a statement")
.emit();
}
}

fn visit_assoc_ty_constraint_from_generic_args(&mut self, constraint: &'a AssocTyConstraint) {
match constraint.kind {
AssocTyConstraintKind::Equality { .. } => {}
Expand Down Expand Up @@ -502,23 +526,46 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}

fn visit_expr(&mut self, expr: &'a Expr) {
match &expr.kind {
ExprKind::Closure(_, _, _, fn_decl, _, _) => {
self.check_fn_decl(fn_decl);
}
ExprKind::InlineAsm(..) if !self.session.target.target.options.allow_asm => {
struct_span_err!(
self.session,
expr.span,
E0472,
"asm! is unsupported on this target"
)
.emit();
self.with_let_allowed(false, |this, let_allowed| {
match &expr.kind {
ExprKind::Closure(_, _, _, fn_decl, _, _) => {
this.check_fn_decl(fn_decl);
}
ExprKind::InlineAsm(..) if !this.session.target.target.options.allow_asm => {
struct_span_err!(
this.session,
expr.span,
E0472,
"asm! is unsupported on this target"
)
.emit();
}
ExprKind::Let(..) if !let_allowed => this.ban_let_expr(expr),
// `(e)` or `e && e` does not impose additional constraints on `e`.
ExprKind::Paren(_) | ExprKind::Binary(Spanned { node: BinOpKind::And, .. }, ..) => {
this.with_let_allowed(let_allowed, |this, _| visit::walk_expr(this, expr));
return; // We've already walked into `expr`.
}
// However, we do allow it in the condition of the `if` expression.
// We do not allow `let` in `then` and `opt_else` directly.
ExprKind::If(cond, then, opt_else) => {
this.visit_block(then);
walk_list!(this, visit_expr, opt_else);
this.with_let_allowed(true, |this, _| this.visit_expr(cond));
return; // We've already walked into `expr`.
}
// The same logic applies to `While`.
ExprKind::While(cond, then, opt_label) => {
walk_list!(this, visit_label, opt_label);
this.visit_block(then);
this.with_let_allowed(true, |this, _| this.visit_expr(cond));
return; // We've already walked into `expr`.
}
_ => {}
}
_ => {}
}

visit::walk_expr(self, expr);
visit::walk_expr(this, expr);
});
}

fn visit_ty(&mut self, ty: &'a Ty) {
Expand Down Expand Up @@ -1049,6 +1096,7 @@ pub fn check_crate(session: &Session, krate: &Crate, lints: &mut LintBuffer) ->
bound_context: None,
is_impl_trait_banned: false,
is_assoc_ty_bound_banned: false,
is_let_allowed: false,
lint_buffer: lints,
};
visit::walk_crate(&mut validator, krate);
Expand Down
Loading