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

[Centril]

Following up on work in #64111, #63693, and #61708, in this PR:

• We change hair::Arm.patterns: Vec<Pattern<'_>> into hir::Arm.pattern: Pattern<'_>.

  • fn hair::Arm::top_pats_hack is introduced as a temporary crutch in MIR building to avoid more changes.

• We change hir::Arm.pats: HirVec<P<Pat>> into hir::Arm.pat: P<Pat>.

  • The hacks in rustc::hir::lowering are removed since the representation hack is no longer necessary.

  • In some places, fn hir::Arm::top_pats_hack is introduced to leave some things as future work.

  • Misc changes: HIR pretty printing is adjusted to behave uniformly wrt. top/inner levels, rvalue promotion is adjusted, regionck, and dead_code is also.

  • Type checking is adjusted to uniformly handle or-patterns at top/inner levels.
    To make things compile, p_0 | ... | p_n is redefined as a "reference pattern" in fn is_non_ref_pat for now. This is done so that reference types are not eagerly stripped from the expected: Ty<'tcx>.

  • Liveness is adjusted wrt. the unused_variables and unused_assignments lints to handle top/inner levels uniformly and the handling of fn parameters, let locals, and match arms are unified in this respect. This is not tested for now as exhaustiveness checks are reachable and will ICE.

  • In check_match, checking @ and by-move bindings is adjusted. However, exhaustiveness checking is not adjusted the moment and is handled by @dlrobertson in WIP: Initial implementation of or-pattern handling in MIR #63688.

  • AST borrowck (construct.rs) is not adjusted as AST borrowck will be removed soon.

r? @matthewjasper
cc @dlrobertson @varkor @oli-obk

[Centril]

Not sure what appropriate tests for diffs in this module would be.

[varkor]

Suggested change

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

[varkor]

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.)

[Centril]

(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.)

[Centril]

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

[varkor]

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

[nikomatsakis]

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

[Centril]

@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.

[nikomatsakis]

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).

[Centril]

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.

[varkor]

Looks good overall. I want to check whether https://github.com/rust-lang/rust/pull/64508/files#r324706033 is intentional or not, though.

[varkor]

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.)

[Centril]

@matthewjasper @varkor Tweaked dead.rs + some walkers. Should be ready for final review now.

[matthewjasper]

@bors r+

[bors]

📌 Commit 0918dc4 has been approved by matthewjasper

[bors]

⌛ Testing commit 0918dc4 with merge 6c3d391...

[Centril]

Triage: Blocking on toolstate breakage-ban being lifted in a few days.

[Centril]

@bors retry can break toolstate again.