Skip to content

Commit

Permalink
Rollup merge of rust-lang#64111 - Centril:ast-only-patkind-or, r=petr…
Browse files Browse the repository at this point in the history
…ochenkov

or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve

Following up on work in rust-lang#63693 and rust-lang#61708, in this PR we:

- Uniformly use `PatKind::Or(...)` in AST:

   - Change `ast::Arm.pats: Vec<P<Pat>>` => `ast::Arm.pat: P<Pat>`

   - Change `ast::ExprKind::Let.0: Vec<P<Pat>>` => `ast::ExprKind::Let.0: P<Pat>`

- Adjust `librustc_resolve/late.rs` to correctly handle or-patterns at any level of nesting as a result.

  In particular, the already-bound check which rejects e.g. `let (a, a);` now accounts for or-patterns. The consistency checking (ensures no missing bindings and binding mode consistency) also now accounts for or-patterns. In the process, a bug was found in the current compiler which allowed:

   ```rust
   enum E<T> { A(T, T), B(T) }
   use E::*;
   fn foo() {
       match A(0, 1) {
           B(mut a) | A(mut a, mut a) => {}
       }
   }
   ```

   The new algorithms took a few iterations to get right. I tried several clever schemes but ultimately a version based on a stack of hashsets and recording product/sum contexts was chosen since it is more clearly correct.

- Clean up `librustc_resolve/late.rs` by, among other things, using a new `with_rib` function to better ensure stack dicipline.

- Do not push the change in AST to HIR for now to avoid doing too much in this PR. To cope with  this, we introduce a temporary hack in `rustc::hir::lowering` (clearly marked in the diff).

cc rust-lang#54883
cc @dlrobertson @matthewjasper
r? @petrochenkov
  • Loading branch information
Mark-Simulacrum authored Sep 6, 2019
2 parents 5f1acb7 + 16ba502 commit a24934b
Show file tree
Hide file tree
Showing 32 changed files with 1,273 additions and 623 deletions.
41 changes: 33 additions & 8 deletions src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,19 +425,44 @@ impl<'a> LoweringContext<'a> {

impl<'tcx, 'interner> Visitor<'tcx> for MiscCollector<'tcx, 'interner> {
fn visit_pat(&mut self, p: &'tcx Pat) {
match p.node {
if let PatKind::Paren(..) | PatKind::Rest = p.node {
// Doesn't generate a HIR node
PatKind::Paren(..) | PatKind::Rest => {},
_ => {
if let Some(owner) = self.hir_id_owner {
self.lctx.lower_node_id_with_owner(p.id, owner);
}
}
};
} else if let Some(owner) = self.hir_id_owner {
self.lctx.lower_node_id_with_owner(p.id, owner);
}

visit::walk_pat(self, p)
}

// HACK(or_patterns; Centril | dlrobertson): Avoid creating
// HIR nodes for `PatKind::Or` for the top level of a `ast::Arm`.
// This is a temporary hack that should go away once we push down
// `arm.pats: HirVec<P<Pat>>` -> `arm.pat: P<Pat>` to HIR. // Centril
fn visit_arm(&mut self, arm: &'tcx Arm) {
match &arm.pat.node {
PatKind::Or(pats) => pats.iter().for_each(|p| self.visit_pat(p)),
_ => self.visit_pat(&arm.pat),
}
walk_list!(self, visit_expr, &arm.guard);
self.visit_expr(&arm.body);
walk_list!(self, visit_attribute, &arm.attrs);
}

// HACK(or_patterns; Centril | dlrobertson): Same as above. // Centril
fn visit_expr(&mut self, e: &'tcx Expr) {
if let ExprKind::Let(pat, scrutinee) = &e.node {
walk_list!(self, visit_attribute, e.attrs.iter());
match &pat.node {
PatKind::Or(pats) => pats.iter().for_each(|p| self.visit_pat(p)),
_ => self.visit_pat(&pat),
}
self.visit_expr(scrutinee);
self.visit_expr_post(e);
return;
}
visit::walk_expr(self, e)
}

fn visit_item(&mut self, item: &'tcx Item) {
let hir_id = self.lctx.allocate_hir_id_counter(item.id);

Expand Down
77 changes: 39 additions & 38 deletions src/librustc/hir/lowering/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl LoweringContext<'_> {
let ohs = P(self.lower_expr(ohs));
hir::ExprKind::AddrOf(m, ohs)
}
ExprKind::Let(ref pats, ref scrutinee) => self.lower_expr_let(e.span, pats, scrutinee),
ExprKind::Let(ref pat, ref scrutinee) => self.lower_expr_let(e.span, pat, 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 @@ -227,16 +227,11 @@ impl LoweringContext<'_> {
}
}

/// Emit an error and lower `ast::ExprKind::Let(pats, scrutinee)` into:
/// 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,
pats: &[AstP<Pat>],
scrutinee: &Expr
) -> hir::ExprKind {
fn lower_expr_let(&mut self, span: Span, pat: &Pat, scrutinee: &Expr) -> hir::ExprKind {
// If we got here, the `let` expression is not allowed.
self.sess
.struct_span_err(span, "`let` expressions are not supported here")
Expand All @@ -246,23 +241,23 @@ impl LoweringContext<'_> {

// For better recovery, we emit:
// ```
// match scrutinee { pats => true, _ => false }
// 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 `pats` and `scrutinee`.
// 3. `pats` is allowed to be refutable.
// 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 pats = pats.iter().map(|pat| self.lower_pat(pat)).collect();
let pat = self.lower_pat_top_hack(pat);
let expr = self.expr_bool(span, true);
self.arm(pats, P(expr))
self.arm(pat, P(expr))
};
let else_arm = {
let pats = hir_vec![self.pat_wild(span)];
let pat = self.pat_wild(span);
let expr = self.expr_bool(span, false);
self.arm(pats, P(expr))
self.arm(hir_vec![pat], P(expr))
};
hir::ExprKind::Match(
P(scrutinee),
Expand Down Expand Up @@ -291,13 +286,12 @@ impl LoweringContext<'_> {
// Handle then + scrutinee:
let then_blk = self.lower_block(then, false);
let then_expr = self.expr_block(then_blk, ThinVec::new());
let (then_pats, scrutinee, desugar) = match cond.node {
let (then_pat, scrutinee, desugar) = match cond.node {
// `<pat> => <then>`:
ExprKind::Let(ref pats, ref scrutinee) => {
ExprKind::Let(ref pat, ref scrutinee) => {
let scrutinee = self.lower_expr(scrutinee);
let pats = pats.iter().map(|pat| self.lower_pat(pat)).collect();
let desugar = hir::MatchSource::IfLetDesugar { contains_else_clause };
(pats, scrutinee, desugar)
let pat = self.lower_pat_top_hack(pat);
(pat, scrutinee, hir::MatchSource::IfLetDesugar { contains_else_clause })
}
// `true => <then>`:
_ => {
Expand All @@ -312,13 +306,11 @@ impl LoweringContext<'_> {
// to preserve drop semantics since `if cond { ... }` does not
// let temporaries live outside of `cond`.
let cond = self.expr_drop_temps(span_block, P(cond), ThinVec::new());

let desugar = hir::MatchSource::IfDesugar { contains_else_clause };
let pats = hir_vec![self.pat_bool(span, true)];
(pats, cond, desugar)
let pat = self.pat_bool(span, true);
(hir_vec![pat], cond, hir::MatchSource::IfDesugar { contains_else_clause })
}
};
let then_arm = self.arm(then_pats, P(then_expr));
let then_arm = self.arm(then_pat, P(then_expr));

hir::ExprKind::Match(P(scrutinee), vec![then_arm, else_arm].into(), desugar)
}
Expand All @@ -345,8 +337,8 @@ impl LoweringContext<'_> {
// Handle then + scrutinee:
let then_blk = self.lower_block(body, false);
let then_expr = self.expr_block(then_blk, ThinVec::new());
let (then_pats, scrutinee, desugar, source) = match cond.node {
ExprKind::Let(ref pats, ref scrutinee) => {
let (then_pat, scrutinee, desugar, source) = match cond.node {
ExprKind::Let(ref pat, ref scrutinee) => {
// to:
//
// [opt_ident]: loop {
Expand All @@ -356,9 +348,8 @@ impl LoweringContext<'_> {
// }
// }
let scrutinee = self.with_loop_condition_scope(|t| t.lower_expr(scrutinee));
let pats = pats.iter().map(|pat| self.lower_pat(pat)).collect();
let desugar = hir::MatchSource::WhileLetDesugar;
(pats, scrutinee, desugar, hir::LoopSource::WhileLet)
let pat = self.lower_pat_top_hack(pat);
(pat, scrutinee, hir::MatchSource::WhileLetDesugar, hir::LoopSource::WhileLet)
}
_ => {
// We desugar: `'label: while $cond $body` into:
Expand All @@ -383,14 +374,12 @@ impl LoweringContext<'_> {
// to preserve drop semantics since `while cond { ... }` does not
// let temporaries live outside of `cond`.
let cond = self.expr_drop_temps(span_block, P(cond), ThinVec::new());

let desugar = hir::MatchSource::WhileDesugar;
// `true => <then>`:
let pats = hir_vec![self.pat_bool(span, true)];
(pats, cond, desugar, hir::LoopSource::While)
let pat = self.pat_bool(span, true);
(hir_vec![pat], cond, hir::MatchSource::WhileDesugar, hir::LoopSource::While)
}
};
let then_arm = self.arm(then_pats, P(then_expr));
let then_arm = self.arm(then_pat, P(then_expr));

// `match <scrutinee> { ... }`
let match_expr = self.expr_match(
Expand Down Expand Up @@ -440,7 +429,7 @@ impl LoweringContext<'_> {
hir::Arm {
hir_id: self.next_id(),
attrs: self.lower_attrs(&arm.attrs),
pats: arm.pats.iter().map(|x| self.lower_pat(x)).collect(),
pats: self.lower_pat_top_hack(&arm.pat),
guard: match arm.guard {
Some(ref x) => Some(hir::Guard::If(P(self.lower_expr(x)))),
_ => None,
Expand All @@ -450,6 +439,16 @@ impl LoweringContext<'_> {
}
}

/// HACK(or_patterns; Centril | dlrobertson): For now we don't push down top level or-patterns
/// `p | q` into `hir::PatKind::Or(...)` as post-lowering bits of the compiler are not ready
/// to deal with it. This should by fixed by pushing it down to HIR and then HAIR.
fn lower_pat_top_hack(&mut self, pat: &Pat) -> HirVec<P<hir::Pat>> {
match pat.node {
PatKind::Or(ref ps) => ps.iter().map(|x| self.lower_pat(x)).collect(),
_ => hir_vec![self.lower_pat(pat)],
}
}

pub(super) fn make_async_expr(
&mut self,
capture_clause: CaptureBy,
Expand Down Expand Up @@ -1255,7 +1254,6 @@ impl LoweringContext<'_> {
ThinVec::from(attrs.clone()),
));
let ok_pat = self.pat_ok(span, val_pat);

self.arm(hir_vec![ok_pat], val_expr)
};

Expand Down Expand Up @@ -1486,7 +1484,10 @@ impl LoweringContext<'_> {
}
}

fn arm(&mut self, pats: hir::HirVec<P<hir::Pat>>, expr: P<hir::Expr>) -> hir::Arm {
/// HACK(or_patterns; Centril | dlrobertson): For now we don't push down top level or-patterns
/// `p | q` into `hir::PatKind::Or(...)` as post-lowering bits of the compiler are not ready
/// to deal with it. This should by fixed by pushing it down to HIR and then HAIR.
fn arm(&mut self, pats: HirVec<P<hir::Pat>>, expr: P<hir::Expr>) -> hir::Arm {
hir::Arm {
hir_id: self.next_id(),
attrs: hir_vec![],
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ impl EarlyLintPass for UnusedDocComment {
}

fn check_arm(&mut self, cx: &EarlyContext<'_>, arm: &ast::Arm) {
let arm_span = arm.pats[0].span.with_hi(arm.body.span.hi());
let arm_span = arm.pat.span.with_hi(arm.body.span.hi());
self.warn_if_doc(cx, arm_span, "match arms", false, &arm.attrs);
}

Expand Down
10 changes: 3 additions & 7 deletions src/librustc_lint/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -493,10 +493,8 @@ impl EarlyLintPass for UnusedParens {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
use syntax::ast::ExprKind::*;
let (value, msg, followed_by_block, left_pos, right_pos) = match e.node {
Let(ref pats, ..) => {
for p in pats {
self.check_unused_parens_pat(cx, p, false, false);
}
Let(ref pat, ..) => {
self.check_unused_parens_pat(cx, pat, false, false);
return;
}

Expand Down Expand Up @@ -594,9 +592,7 @@ impl EarlyLintPass for UnusedParens {
}

fn check_arm(&mut self, cx: &EarlyContext<'_>, arm: &ast::Arm) {
for p in &arm.pats {
self.check_unused_parens_pat(cx, p, false, false);
}
self.check_unused_parens_pat(cx, &arm.pat, false, false);
}
}

Expand Down
Loading

0 comments on commit a24934b

Please sign in to comment.