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

or-patterns: Push PatKind/PatternKind::Or at top level to HIR & HAIR #64508

Merged
merged 22 commits into from
Sep 25, 2019
Merged

or-patterns: Push PatKind/PatternKind::Or at top level to HIR & HAIR #64508

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
146cb8e
or-patterns: remove hack from lowering.
Centril Sep 7, 2019
b2f903c
or-patterns: `hir::Arm::pats` -> `::pat` + `Arm::top_pats_hack`.
Centril Sep 7, 2019
6579d13
or-patterns: normalize HIR pretty priting.
Centril Sep 7, 2019
75fb42a
or-patterns: use `top_pats_hack` to make things compile.
Centril Sep 7, 2019
89bbef3
check_match: misc cleanup.
Centril Sep 15, 2019
4c34693
or-patterns: fix problems in typeck.
Centril Sep 15, 2019
9d1c3c9
simplify `hir::Pat::walk_`.
Centril Sep 15, 2019
cc5fe6d
or-patterns: liveness/`visit_arm`: remove `top_pats_hack`.
Centril Sep 15, 2019
9ca42a5
or-patterns: liveness: generalize + remove `top_pats_hack`.
Centril Sep 15, 2019
57eeb61
or-patterns: remove `Arm::contains_explicit_ref_binding`.
Centril Sep 15, 2019
fb2cfec
or-patterns: euv/`arm_move_mode`: remove `top_pats_hack`.
Centril Sep 15, 2019
d7139f3
or-patterns: euv/`walk_arm`: remove `top_pats_hack`.
Centril Sep 15, 2019
07deb93
or-patterns: middle/dead: make a hack less hacky.
Centril Sep 15, 2019
0dfd706
or-patterns: rvalue_promotion: remove `top_pats_hack`.
Centril Sep 15, 2019
38a5ae9
or-patterns: regionck/visit_arm: remove `top_pats_hack`.
Centril Sep 15, 2019
9b406f1
or-patterns: regionck/`link_match`: remove `top_pats_hack`.
Centril Sep 15, 2019
6bd8c6d
or-patterns: check_match: remove `top_pats_hack` for `check_for_bindi…
Centril Sep 15, 2019
549756b
or-patterns: check_match: nix `top_pats_hack` passed to `check_patter…
Centril Sep 15, 2019
56b055a
or-patterns: HAIR: `Arm.patterns: Vec<Pattern<'_>>` -> `.pattern: Pat…
Centril Sep 16, 2019
05cc3c0
or-patterns: liveness: `is_argument` -> `is_param`.
Centril Sep 16, 2019
370fbcc
or-patterns: #47390: we rely on names to exercise `IndexMap`.
Centril Sep 16, 2019
0918dc4
or-patterns: middle/dead: remove `top_pats_hack`.
Centril Sep 21, 2019
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
2 changes: 1 addition & 1 deletion src/librustc/hir/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,7 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) {

pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm) {
visitor.visit_id(arm.hir_id);
walk_list!(visitor, visit_pat, &arm.pats);
visitor.visit_pat(&arm.pat);
if let Some(ref g) = arm.guard {
match g {
Guard::If(ref e) => visitor.visit_expr(e),
Expand Down
29 changes: 0 additions & 29 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,35 +434,6 @@ impl<'a> LoweringContext<'a> {
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
51 changes: 19 additions & 32 deletions src/librustc/hir/lowering/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,14 +250,14 @@ impl LoweringContext<'_> {
// 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_top_hack(pat);
let pat = self.lower_pat(pat);
let expr = self.expr_bool(span, true);
self.arm(pat, P(expr))
};
let else_arm = {
let pat = self.pat_wild(span);
let expr = self.expr_bool(span, false);
self.arm(hir_vec![pat], P(expr))
self.arm(pat, P(expr))
};
hir::ExprKind::Match(
P(scrutinee),
Expand All @@ -281,7 +281,7 @@ impl LoweringContext<'_> {
None => (self.expr_block_empty(span), false),
Some(els) => (self.lower_expr(els), true),
};
let else_arm = self.arm(hir_vec![else_pat], P(else_expr));
let else_arm = self.arm(else_pat, P(else_expr));

// Handle then + scrutinee:
let then_blk = self.lower_block(then, false);
Expand All @@ -290,7 +290,7 @@ impl LoweringContext<'_> {
// `<pat> => <then>`:
ExprKind::Let(ref pat, ref scrutinee) => {
let scrutinee = self.lower_expr(scrutinee);
let pat = self.lower_pat_top_hack(pat);
let pat = self.lower_pat(pat);
(pat, scrutinee, hir::MatchSource::IfLetDesugar { contains_else_clause })
}
// `true => <then>`:
Expand All @@ -307,7 +307,7 @@ impl LoweringContext<'_> {
// let temporaries live outside of `cond`.
let cond = self.expr_drop_temps(span_block, P(cond), ThinVec::new());
let pat = self.pat_bool(span, true);
(hir_vec![pat], cond, hir::MatchSource::IfDesugar { contains_else_clause })
(pat, cond, hir::MatchSource::IfDesugar { contains_else_clause })
}
};
let then_arm = self.arm(then_pat, P(then_expr));
Expand All @@ -331,7 +331,7 @@ impl LoweringContext<'_> {
let else_arm = {
let else_pat = self.pat_wild(span);
let else_expr = self.expr_break(span, ThinVec::new());
self.arm(hir_vec![else_pat], else_expr)
self.arm(else_pat, else_expr)
};

// Handle then + scrutinee:
Expand All @@ -348,7 +348,7 @@ impl LoweringContext<'_> {
// }
// }
let scrutinee = self.with_loop_condition_scope(|t| t.lower_expr(scrutinee));
let pat = self.lower_pat_top_hack(pat);
let pat = self.lower_pat(pat);
(pat, scrutinee, hir::MatchSource::WhileLetDesugar, hir::LoopSource::WhileLet)
}
_ => {
Expand Down Expand Up @@ -376,7 +376,7 @@ impl LoweringContext<'_> {
let cond = self.expr_drop_temps(span_block, P(cond), ThinVec::new());
// `true => <then>`:
let pat = self.pat_bool(span, true);
(hir_vec![pat], cond, hir::MatchSource::WhileDesugar, hir::LoopSource::While)
(pat, cond, hir::MatchSource::WhileDesugar, hir::LoopSource::While)
}
};
let then_arm = self.arm(then_pat, P(then_expr));
Expand Down Expand Up @@ -429,7 +429,7 @@ impl LoweringContext<'_> {
hir::Arm {
hir_id: self.next_id(),
attrs: self.lower_attrs(&arm.attrs),
pats: self.lower_pat_top_hack(&arm.pat),
pat: self.lower_pat(&arm.pat),
guard: match arm.guard {
Some(ref x) => Some(hir::Guard::If(P(self.lower_expr(x)))),
_ => None,
Expand All @@ -439,16 +439,6 @@ 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 @@ -597,7 +587,7 @@ impl LoweringContext<'_> {
);
P(this.expr(await_span, expr_break, ThinVec::new()))
});
self.arm(hir_vec![ready_pat], break_x)
self.arm(ready_pat, break_x)
};

// `::std::task::Poll::Pending => {}`
Expand All @@ -608,7 +598,7 @@ impl LoweringContext<'_> {
hir_vec![],
);
let empty_block = P(self.expr_block_empty(span));
self.arm(hir_vec![pending_pat], empty_block)
self.arm(pending_pat, empty_block)
};

let inner_match_stmt = {
Expand Down Expand Up @@ -650,7 +640,7 @@ impl LoweringContext<'_> {
});

// mut pinned => loop { ... }
let pinned_arm = self.arm(hir_vec![pinned_pat], loop_expr);
let pinned_arm = self.arm(pinned_pat, loop_expr);

// match <expr> {
// mut pinned => loop { .. }
Expand Down Expand Up @@ -1084,15 +1074,15 @@ impl LoweringContext<'_> {
ThinVec::new(),
));
let some_pat = self.pat_some(pat.span, val_pat);
self.arm(hir_vec![some_pat], assign)
self.arm(some_pat, assign)
};

// `::std::option::Option::None => break`
let break_arm = {
let break_expr =
self.with_loop_scope(e.id, |this| this.expr_break(e.span, ThinVec::new()));
let pat = self.pat_none(e.span);
self.arm(hir_vec![pat], break_expr)
self.arm(pat, break_expr)
};

// `mut iter`
Expand Down Expand Up @@ -1163,7 +1153,7 @@ impl LoweringContext<'_> {
});

// `mut iter => { ... }`
let iter_arm = self.arm(hir_vec![iter_pat], loop_expr);
let iter_arm = self.arm(iter_pat, loop_expr);

// `match ::std::iter::IntoIterator::into_iter(<head>) { ... }`
let into_iter_expr = {
Expand Down Expand Up @@ -1249,7 +1239,7 @@ impl LoweringContext<'_> {
ThinVec::from(attrs.clone()),
));
let ok_pat = self.pat_ok(span, val_pat);
self.arm(hir_vec![ok_pat], val_expr)
self.arm(ok_pat, val_expr)
};

// `Err(err) => #[allow(unreachable_code)]
Expand Down Expand Up @@ -1284,7 +1274,7 @@ impl LoweringContext<'_> {
};

let err_pat = self.pat_err(try_span, err_local);
self.arm(hir_vec![err_pat], ret_expr)
self.arm(err_pat, ret_expr)
};

hir::ExprKind::Match(
Expand Down Expand Up @@ -1479,14 +1469,11 @@ 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 arm(&mut self, pats: HirVec<P<hir::Pat>>, expr: P<hir::Expr>) -> hir::Arm {
fn arm(&mut self, pat: P<hir::Pat>, expr: P<hir::Expr>) -> hir::Arm {
hir::Arm {
hir_id: self.next_id(),
attrs: hir_vec![],
pats,
pat,
guard: None,
span: expr.span,
body: expr,
Expand Down
34 changes: 20 additions & 14 deletions src/librustc/hir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,19 +889,14 @@ impl Pat {
return false;
}

match self.node {
PatKind::Binding(.., Some(ref p)) => p.walk_(it),
PatKind::Struct(_, ref fields, _) => {
fields.iter().all(|field| field.pat.walk_(it))
}
PatKind::TupleStruct(_, ref s, _) | PatKind::Tuple(ref s, _) => {
match &self.node {
PatKind::Binding(.., Some(p)) => p.walk_(it),
PatKind::Struct(_, fields, _) => fields.iter().all(|field| field.pat.walk_(it)),
PatKind::TupleStruct(_, s, _) | PatKind::Tuple(s, _) | PatKind::Or(s) => {
s.iter().all(|p| p.walk_(it))
}
PatKind::Or(ref pats) => pats.iter().all(|p| p.walk_(it)),
PatKind::Box(ref s) | PatKind::Ref(ref s, _) => {
s.walk_(it)
}
PatKind::Slice(ref before, ref slice, ref after) => {
PatKind::Box(s) | PatKind::Ref(s, _) => s.walk_(it),
PatKind::Slice(before, slice, after) => {
before.iter()
.chain(slice.iter())
.chain(after.iter())
Expand Down Expand Up @@ -1259,21 +1254,32 @@ pub struct Local {
}

/// Represents a single arm of a `match` expression, e.g.
/// `<pats> (if <guard>) => <body>`.
/// `<pat> (if <guard>) => <body>`.
#[derive(RustcEncodable, RustcDecodable, Debug, HashStable)]
pub struct Arm {
#[stable_hasher(ignore)]
pub hir_id: HirId,
pub span: Span,
pub attrs: HirVec<Attribute>,
/// Multiple patterns can be combined with `|`
pub pats: HirVec<P<Pat>>,
/// If this pattern and the optional guard matches, then `body` is evaluated.
pub pat: P<Pat>,
/// Optional guard clause.
pub guard: Option<Guard>,
/// The expression the arm evaluates to if this arm matches.
pub body: P<Expr>,
}

impl Arm {
// HACK(or_patterns; Centril | dlrobertson): Remove this and
// correctly handle each case in which this method is used.
pub fn top_pats_hack(&self) -> &[P<Pat>] {
match &self.pat.node {
PatKind::Or(pats) => pats,
_ => std::slice::from_ref(&self.pat),
}
}
}

#[derive(RustcEncodable, RustcDecodable, Debug, HashStable)]
pub enum Guard {
If(P<Expr>),
Expand Down
45 changes: 28 additions & 17 deletions src/librustc/hir/pat_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,34 @@ impl hir::Pat {
});
}

/// Call `f` on every "binding" in a pattern, e.g., on `a` in
/// `match foo() { Some(a) => (), None => () }`.
///
/// When encountering an or-pattern `p_0 | ... | p_n` only `p_0` will be visited.
pub fn each_binding_or_first<F>(&self, c: &mut F)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn each_binding_or_first<F>(&self, c: &mut F)
pub fn each_binding_or_first<F>(&self, f: &mut F)

Copy link
Member

Choose a reason for hiding this comment

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

Is calling each_binding_or_first recursively going to have the right behaviour for nested Or patterns? Shouldn't it pick the first pattern only at the top level? (In which case, you can use each_binding instead and just check the top level at the call site.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Using c for callback as f is used for FieldPat; could switch to g for the callback instead.)

(We discussed this a bit privately on Discord; The existing commentary in define_bindings_in_pat states that we only consider the first pattern since any later patterns must have the same bindings (and the first pattern is considered to have the authoritative set of ids). This suggests that we should do the same in nested positions as there should be no intrinsic semantic difference between Some(a @ 1) | Some(a @ 2) and Some(a @ (1 | 2)). That said, I don't really know why we only consider the first pattern and if we just use .each_binding(...) uniformly for top/nested then the UI tests pass at least.)

Copy link
Contributor Author

@Centril Centril Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at blame, it seems the comment was left by @eddyb 5 years ago in b062128. Maybe they still know. =P

Copy link
Member

Choose a reason for hiding this comment

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

It's actually due to @nikomatsakis here: e47d2f6#diff-97c8d3b79a36724da452cf176a6a42b5R661-R663.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't even understand what the question is here. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis So your comment to a call of .define_bindings_in_pat(...) in liveness.rs regarding ExprKind::Match(...) => states that:

                    // only consider the first pattern; any later patterns must have
                    // the same bindings, and we also consider the first pattern to be
                    // the "authoritative" set of ids
                    let arm_succ =
                        self.define_bindings_in_arm_pats(arm.pats.first().map(|p| &**p),
                                                         guard_succ);

Now, I don't exactly understand why this is, but my intuition says that if we want to extend this logic to nested or-patterns, e.g. Some(A(x) | B(x)) then we should do the same think there. That is, in any PatKind::Or(pats) we will just visit pats[0] and not pats[1..]. That's what the code in this PR does now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, so here's the problem. Presently we use the id of the AST node for a pattern binding as the variable's id -- but when you have e.g. Foo(x) | Bar(x), you wind up with two id's for x! Clearly all the alternatives must have the same set of bindings (i.e., they must all define x), so yeah I dimly recall deciding that we would just use the id's from the first set of patterns to define the variables, and then check the remaining patterns against that first set.

I think another viable approach would be to use some kind of fresh identifiers for the variables (not the id from the pattern node) and then be able to "resolve" each pattern node to that. Or perhaps to have a table of "redirects" where for most bindings the redirect is the identity, but for | patterns they point to the bindings from the first set.

In any case I think you are correct that if we are moving | patterns further down, and we want to try and use the same strategy, then we only want to take the left-most branches (but check the other branches for consistency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the elaboration!

Clearly all the alternatives must have the same set of bindings (i.e., they must all define x), so yeah I dimly recall deciding that we would just use the id's from the first set of patterns to define the variables, and then check the remaining patterns against that first set.

(but check the other branches for consistency).

Yea, that's what we do; I recently tweaked this logic in resolve here:

rust/src/librustc_resolve/late.rs

Lines 1121 to 1475 in 9b9d2af

fn resolve_params(&mut self, params: &[Param]) {
let mut bindings = smallvec![(PatBoundCtx::Product, Default::default())];
for Param { pat, ty, .. } in params {
self.resolve_pattern(pat, PatternSource::FnParam, &mut bindings);
self.visit_ty(ty);
debug!("(resolving function / closure) recorded parameter");
}
}
fn resolve_local(&mut self, local: &Local) {
// Resolve the type.
walk_list!(self, visit_ty, &local.ty);
// Resolve the initializer.
walk_list!(self, visit_expr, &local.init);
// Resolve the pattern.
self.resolve_pattern_top(&local.pat, PatternSource::Let);
}
/// build a map from pattern identifiers to binding-info's.
/// this is done hygienically. This could arise for a macro
/// that expands into an or-pattern where one 'x' was from the
/// user and one 'x' came from the macro.
fn binding_mode_map(&mut self, pat: &Pat) -> BindingMap {
let mut binding_map = FxHashMap::default();
pat.walk(&mut |pat| {
match pat.node {
PatKind::Ident(binding_mode, ident, ref sub_pat)
if sub_pat.is_some() || self.is_base_res_local(pat.id) =>
{
binding_map.insert(ident, BindingInfo { span: ident.span, binding_mode });
}
PatKind::Or(ref ps) => {
// Check the consistency of this or-pattern and
// then add all bindings to the larger map.
for bm in self.check_consistent_bindings(ps) {
binding_map.extend(bm);
}
return false;
}
_ => {}
}
true
});
binding_map
}
fn is_base_res_local(&self, nid: NodeId) -> bool {
match self.r.partial_res_map.get(&nid).map(|res| res.base_res()) {
Some(Res::Local(..)) => true,
_ => false,
}
}
/// Checks that all of the arms in an or-pattern have exactly the
/// same set of bindings, with the same binding modes for each.
fn check_consistent_bindings(&mut self, pats: &[P<Pat>]) -> Vec<BindingMap> {
let mut missing_vars = FxHashMap::default();
let mut inconsistent_vars = FxHashMap::default();
// 1) Compute the binding maps of all arms.
let maps = pats.iter()
.map(|pat| self.binding_mode_map(pat))
.collect::<Vec<_>>();
// 2) Record any missing bindings or binding mode inconsistencies.
for (map_outer, pat_outer) in pats.iter().enumerate().map(|(idx, pat)| (&maps[idx], pat)) {
// Check against all arms except for the same pattern which is always self-consistent.
let inners = pats.iter().enumerate()
.filter(|(_, pat)| pat.id != pat_outer.id)
.flat_map(|(idx, _)| maps[idx].iter())
.map(|(key, binding)| (key.name, map_outer.get(&key), binding));
for (name, info, &binding_inner) in inners {
match info {
None => { // The inner binding is missing in the outer.
let binding_error = missing_vars
.entry(name)
.or_insert_with(|| BindingError {
name,
origin: BTreeSet::new(),
target: BTreeSet::new(),
could_be_path: name.as_str().starts_with(char::is_uppercase),
});
binding_error.origin.insert(binding_inner.span);
binding_error.target.insert(pat_outer.span);
}
Some(binding_outer) => {
if binding_outer.binding_mode != binding_inner.binding_mode {
// The binding modes in the outer and inner bindings differ.
inconsistent_vars
.entry(name)
.or_insert((binding_inner.span, binding_outer.span));
}
}
}
}
}
// 3) Report all missing variables we found.
let mut missing_vars = missing_vars.iter_mut().collect::<Vec<_>>();
missing_vars.sort();
for (name, mut v) in missing_vars {
if inconsistent_vars.contains_key(name) {
v.could_be_path = false;
}
self.r.report_error(
*v.origin.iter().next().unwrap(),
ResolutionError::VariableNotBoundInPattern(v));
}
// 4) Report all inconsistencies in binding modes we found.
let mut inconsistent_vars = inconsistent_vars.iter().collect::<Vec<_>>();
inconsistent_vars.sort();
for (name, v) in inconsistent_vars {
self.r.report_error(v.0, ResolutionError::VariableBoundWithDifferentMode(*name, v.1));
}
// 5) Finally bubble up all the binding maps.
maps
}
/// Check the consistency of the outermost or-patterns.
fn check_consistent_bindings_top(&mut self, pat: &Pat) {
pat.walk(&mut |pat| match pat.node {
PatKind::Or(ref ps) => {
self.check_consistent_bindings(ps);
false
},
_ => true,
})
}
fn resolve_arm(&mut self, arm: &Arm) {
self.with_rib(ValueNS, NormalRibKind, |this| {
this.resolve_pattern_top(&arm.pat, PatternSource::Match);
walk_list!(this, visit_expr, &arm.guard);
this.visit_expr(&arm.body);
});
}
/// Arising from `source`, resolve a top level pattern.
fn resolve_pattern_top(&mut self, pat: &Pat, pat_src: PatternSource) {
let mut bindings = smallvec![(PatBoundCtx::Product, Default::default())];
self.resolve_pattern(pat, pat_src, &mut bindings);
}
fn resolve_pattern(
&mut self,
pat: &Pat,
pat_src: PatternSource,
bindings: &mut SmallVec<[(PatBoundCtx, FxHashSet<Ident>); 1]>,
) {
self.resolve_pattern_inner(pat, pat_src, bindings);
// This has to happen *after* we determine which pat_idents are variants:
self.check_consistent_bindings_top(pat);
visit::walk_pat(self, pat);
}
/// Resolve bindings in a pattern. This is a helper to `resolve_pattern`.
///
/// ### `bindings`
///
/// A stack of sets of bindings accumulated.
///
/// In each set, `PatBoundCtx::Product` denotes that a found binding in it should
/// be interpreted as re-binding an already bound binding. This results in an error.
/// Meanwhile, `PatBound::Or` denotes that a found binding in the set should result
/// in reusing this binding rather than creating a fresh one.
///
/// When called at the top level, the stack must have a single element
/// with `PatBound::Product`. Otherwise, pushing to the stack happens as
/// or-patterns (`p_0 | ... | p_n`) are encountered and the context needs
/// to be switched to `PatBoundCtx::Or` and then `PatBoundCtx::Product` for each `p_i`.
/// When each `p_i` has been dealt with, the top set is merged with its parent.
/// When a whole or-pattern has been dealt with, the thing happens.
///
/// See the implementation and `fresh_binding` for more details.
fn resolve_pattern_inner(
&mut self,
pat: &Pat,
pat_src: PatternSource,
bindings: &mut SmallVec<[(PatBoundCtx, FxHashSet<Ident>); 1]>,
) {
// Visit all direct subpatterns of this pattern.
pat.walk(&mut |pat| {
debug!("resolve_pattern pat={:?} node={:?}", pat, pat.node);
match pat.node {
PatKind::Ident(bmode, ident, ref sub) => {
// First try to resolve the identifier as some existing entity,
// then fall back to a fresh binding.
let has_sub = sub.is_some();
let res = self.try_resolve_as_non_binding(pat_src, pat, bmode, ident, has_sub)
.unwrap_or_else(|| self.fresh_binding(ident, pat.id, pat_src, bindings));
self.r.record_partial_res(pat.id, PartialRes::new(res));
}
PatKind::TupleStruct(ref path, ..) => {
self.smart_resolve_path(pat.id, None, path, PathSource::TupleStruct);
}
PatKind::Path(ref qself, ref path) => {
self.smart_resolve_path(pat.id, qself.as_ref(), path, PathSource::Pat);
}
PatKind::Struct(ref path, ..) => {
self.smart_resolve_path(pat.id, None, path, PathSource::Struct);
}
PatKind::Or(ref ps) => {
// Add a new set of bindings to the stack. `Or` here records that when a
// binding already exists in this set, it should not result in an error because
// `V1(a) | V2(a)` must be allowed and are checked for consistency later.
bindings.push((PatBoundCtx::Or, Default::default()));
for p in ps {
// Now we need to switch back to a product context so that each
// part of the or-pattern internally rejects already bound names.
// For example, `V1(a) | V2(a, a)` and `V1(a, a) | V2(a)` are bad.
bindings.push((PatBoundCtx::Product, Default::default()));
self.resolve_pattern_inner(p, pat_src, bindings);
// Move up the non-overlapping bindings to the or-pattern.
// Existing bindings just get "merged".
let collected = bindings.pop().unwrap().1;
bindings.last_mut().unwrap().1.extend(collected);
}
// This or-pattern itself can itself be part of a product,
// e.g. `(V1(a) | V2(a), a)` or `(a, V1(a) | V2(a))`.
// Both cases bind `a` again in a product pattern and must be rejected.
let collected = bindings.pop().unwrap().1;
bindings.last_mut().unwrap().1.extend(collected);
// Prevent visiting `ps` as we've already done so above.
return false;
}
_ => {}
}
true
});
}
fn fresh_binding(
&mut self,
ident: Ident,
pat_id: NodeId,
pat_src: PatternSource,
bindings: &mut SmallVec<[(PatBoundCtx, FxHashSet<Ident>); 1]>,
) -> Res {
// Add the binding to the local ribs, if it doesn't already exist in the bindings map.
// (We must not add it if it's in the bindings map because that breaks the assumptions
// later passes make about or-patterns.)
let ident = ident.modern_and_legacy();
// Walk outwards the stack of products / or-patterns and
// find out if the identifier has been bound in any of these.
let mut already_bound_and = false;
let mut already_bound_or = false;
for (is_sum, set) in bindings.iter_mut().rev() {
match (is_sum, set.get(&ident).cloned()) {
// Already bound in a product pattern, e.g. `(a, a)` which is not allowed.
(PatBoundCtx::Product, Some(..)) => already_bound_and = true,
// Already bound in an or-pattern, e.g. `V1(a) | V2(a)`.
// This is *required* for consistency which is checked later.
(PatBoundCtx::Or, Some(..)) => already_bound_or = true,
// Not already bound here.
_ => {}
}
}
if already_bound_and {
// Overlap in a product pattern somewhere; report an error.
use ResolutionError::*;
let error = match pat_src {
// `fn f(a: u8, a: u8)`:
PatternSource::FnParam => IdentifierBoundMoreThanOnceInParameterList,
// `Variant(a, a)`:
_ => IdentifierBoundMoreThanOnceInSamePattern,
};
self.r.report_error(ident.span, error(&ident.as_str()));
}
// Record as bound if it's valid:
let ident_valid = ident.name != kw::Invalid;
if ident_valid {
bindings.last_mut().unwrap().1.insert(ident);
}
if already_bound_or {
// `Variant1(a) | Variant2(a)`, ok
// Reuse definition from the first `a`.
self.innermost_rib_bindings(ValueNS)[&ident]
} else {
let res = Res::Local(pat_id);
if ident_valid {
// A completely fresh binding add to the set if it's valid.
self.innermost_rib_bindings(ValueNS).insert(ident, res);
}
res
}
}
fn innermost_rib_bindings(&mut self, ns: Namespace) -> &mut IdentMap<Res> {
&mut self.ribs[ns].last_mut().unwrap().bindings
}
fn try_resolve_as_non_binding(
&mut self,
pat_src: PatternSource,
pat: &Pat,
bm: BindingMode,
ident: Ident,
has_sub: bool,
) -> Option<Res> {
let binding = self.resolve_ident_in_lexical_scope(ident, ValueNS, None, pat.span)?.item()?;
let res = binding.res();
// An immutable (no `mut`) by-value (no `ref`) binding pattern without
// a sub pattern (no `@ $pat`) is syntactically ambiguous as it could
// also be interpreted as a path to e.g. a constant, variant, etc.
let is_syntactic_ambiguity = !has_sub && bm == BindingMode::ByValue(Mutability::Immutable);
match res {
Res::Def(DefKind::Ctor(_, CtorKind::Const), _) |
Res::Def(DefKind::Const, _) if is_syntactic_ambiguity => {
// Disambiguate in favor of a unit struct/variant or constant pattern.
self.r.record_use(ident, ValueNS, binding, false);
Some(res)
}
Res::Def(DefKind::Ctor(..), _)
| Res::Def(DefKind::Const, _)
| Res::Def(DefKind::Static, _) => {
// This is unambiguously a fresh binding, either syntactically
// (e.g., `IDENT @ PAT` or `ref IDENT`) or because `IDENT` resolves
// to something unusable as a pattern (e.g., constructor function),
// but we still conservatively report an error, see
// issues/33118#issuecomment-233962221 for one reason why.
self.r.report_error(
ident.span,
ResolutionError::BindingShadowsSomethingUnacceptable(
pat_src.descr(),
ident.name,
binding,
),
);
None
}
Res::Def(DefKind::Fn, _) | Res::Err => {
// These entities are explicitly allowed to be shadowed by fresh bindings.
None
}
res => {
span_bug!(ident.span, "unexpected resolution for an \
identifier in pattern: {:?}", res);
}
}
}

I think another viable approach would be to use some kind of fresh identifiers for the variables (not the id from the pattern node) and then be able to "resolve" each pattern node to that. Or perhaps to have a table of "redirects" where for most bindings the redirect is the identity, but for | patterns they point to the bindings from the first set.

Sounds interesting (but should be done in a follow-up if so). cc @petrochenkov may be interested.

where F: FnMut(hir::BindingAnnotation, HirId, Span, ast::Ident),
{
match &self.node {
PatKind::Binding(bm, _, ident, sub) => {
c(*bm, self.hir_id, self.span, *ident);
sub.iter().for_each(|p| p.each_binding_or_first(c));
}
PatKind::Or(ps) => ps[0].each_binding_or_first(c),
PatKind::Struct(_, fs, _) => fs.iter().for_each(|f| f.pat.each_binding_or_first(c)),
PatKind::TupleStruct(_, ps, _) | PatKind::Tuple(ps, _) => {
ps.iter().for_each(|p| p.each_binding_or_first(c));
}
PatKind::Box(p) | PatKind::Ref(p, _) => p.each_binding_or_first(c),
PatKind::Slice(before, slice, after) => {
before.iter()
.chain(slice.iter())
.chain(after.iter())
.for_each(|p| p.each_binding_or_first(c));
}
PatKind::Wild | PatKind::Lit(_) | PatKind::Range(..) | PatKind::Path(_) => {}
}
}

/// Checks if the pattern contains any patterns that bind something to
/// an ident, e.g., `foo`, or `Foo(foo)` or `foo @ Bar(..)`.
pub fn contains_bindings(&self) -> bool {
Expand Down Expand Up @@ -161,20 +189,3 @@ impl hir::Pat {
result
}
}

impl hir::Arm {
/// Checks if the patterns for this arm contain any `ref` or `ref mut`
/// bindings, and if yes whether its containing mutable ones or just immutables ones.
pub fn contains_explicit_ref_binding(&self) -> Option<hir::Mutability> {
// FIXME(tschottdorf): contains_explicit_ref_binding() must be removed
// for #42640 (default match binding modes).
//
// See #44848.
self.pats.iter()
.filter_map(|pat| pat.contains_explicit_ref_binding())
.max_by_key(|m| match *m {
hir::MutMutable => 1,
hir::MutImmutable => 0,
})
}
}
11 changes: 1 addition & 10 deletions src/librustc/hir/print.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1790,16 +1790,7 @@ impl<'a> State<'a> {
self.ann.pre(self, AnnNode::Arm(arm));
self.ibox(0);
self.print_outer_attributes(&arm.attrs);
let mut first = true;
for p in &arm.pats {
if first {
first = false;
} else {
self.s.space();
self.word_space("|");
}
self.print_pat(&p);
}
self.print_pat(&arm.pat);
self.s.space();
if let Some(ref g) = arm.guard {
match g {
Expand Down
5 changes: 3 additions & 2 deletions src/librustc/middle/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,9 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> {
}

fn visit_arm(&mut self, arm: &'tcx hir::Arm) {
if arm.pats.len() == 1 {
let variants = arm.pats[0].necessary_variants();
let pats = arm.top_pats_hack();
Centril marked this conversation as resolved.
Show resolved Hide resolved
if let [pat] = pats {
let variants = pat.necessary_variants();

// Inside the body, ignore constructions of variants
// necessary for the pattern to match. Those construction sites
Expand Down
8 changes: 2 additions & 6 deletions src/librustc/middle/expr_use_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,16 +779,12 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {

fn arm_move_mode(&mut self, discr_cmt: mc::cmt<'tcx>, arm: &hir::Arm) -> TrackMatchMode {
let mut mode = Unknown;
for pat in &arm.pats {
self.determine_pat_move_mode(discr_cmt.clone(), &pat, &mut mode);
}
self.determine_pat_move_mode(discr_cmt.clone(), &arm.pat, &mut mode);
mode
}

fn walk_arm(&mut self, discr_cmt: mc::cmt<'tcx>, arm: &hir::Arm, mode: MatchMode) {
for pat in &arm.pats {
self.walk_pat(discr_cmt.clone(), &pat, mode);
}
self.walk_pat(discr_cmt.clone(), &arm.pat, mode);

if let Some(hir::Guard::If(ref e)) = arm.guard {
self.consume_expr(e)
Expand Down
Loading