Tracking issue for bindings in sub-patterns after `@`s (`feature(bindings_after_at)`)

[pnkfelix]

This is the tracking issue for #![feature(bindings_after_at)], which allows patterns of form binding @ pat to have bindings in pat, for example ref x @ Some(y).

Implementation history

• On 2019-12-24, Initial implementation of #![feature(bindings_after_at)] #66296, containing the initial implementation, landed. The PR was written by @Centril and reviewed by @pnkfelix and @matthewjasper.

• On 2020-02-10, Correctly parse mut a @ b #68992 landed. The PR was written by @matthewjasper and reviewed by @Centril. The PR fixed a parser recovery bug wherein mut x @ y would make y a mutable binding, manifesting as issues Wrong bindings_after_at warnings #67861 and bindings_after_at: outer mutability annotation affects inner bindings #67926.

• On 2020-02-19, parse: recover mut (x @ y) as (mut x @ mut y). #69236 landed. The PR was written by @Centril and reviewed by @estebank. The PR adjusted the parser changes in Correctly parse mut a @ b #68992 to retain good error messages on e.g. mut (x @ y) patterns, suggesting mut x @ mut y as opposed to mut x @ y.

• On 2020-03-08, test(pattern): add tests for combinations of pattern features #69690 landed. The PR was written by @thekuom and reviewed by @Centril and added run-pass tests for the combination of bindings-after-at, or-patterns, box-patterns, slice-patterns.

• On 2020-03-08, test(bindings_after_at): add dynamic drop tests for bindings_after_at #69810 landed. The PR was written by @thekuom and reviewed by @Centril and added dynamic-rop run-pass tests for bindings-after-at.

• On 2020-03-10, test(patterns): add patterns feature tests to borrowck test suite #69817, written by @thekuom and reviewed by @Centril, landed. The PR added borrow checker tests in combination with or-patterns & other pattern features.

• On 2020-03-26, borrowck: prefer "value" over "_" in diagnostics #70389, written by @Centril and reviewed by @mark-i-m, landed. The PR fixed some bugs in diagnostics where we would end up using _ as the name of the place when matching on a temporary. Now it says "value" in the diagnostic instead.

Old description

Pre-1.0 Rust used to support writing code like:

match x {
    y @ z => { ... }
}

match a {
    b @ Some(c) => { ... }
}

PR #16053 disabled the ability to write such code. More precisely, it disabled the ability to introduce any kind of binding in subpattern P in the context of any binding-pattern ident @ P

The reason we did this was because it was too hard to handle them soundly in the AST-borrowck.

However, now that we have migrated 100% to NLL and thus MIR-borrowck, we should be able to soundly re-enable this feature, without too much effort (I hope).

• Back when the feature was removed, we made an explicit note that the long term intention was to reenable it.
• And this is a useful coding pattern at times. (The whole reason I went looking for this issue, and then decided to file it after discovering it did not seem to exist, was because I wanted to write code that required this feature, namely a pattern like: ty::Ref(_, adt_ty @ ty::TyS { kind: ty::Adt(adt_def, _), .. }, _))

Note that any attempt to re-enable the feature should do a careful survey of the various cases that can arise (copying, moving, mixing ref and non-ref, match ergonomics, ...)

Also, any attempt to re-enable the feature should also attempt to re-add all the tests that were removed with PR #16053

[pnkfelix]

I don't think implementing this requires an RFC, but I suppose its worth double-checking with the lang team. Nominating for discussion at lang team meeting on that point.

(And removing from T-compiler for now; no reason to bother T-compiler about this until after T-lang has confirmed that we want this feature re-added.)

[estebank]

I see the potential for trivial bugs to be enabled and diagnostic text confusion exhacerbated by doing this, which is why I would like to have a very clear plan to extend the pattern checking code to suggest things like b @ Some(c) when clearly Some(c) @ b was meant.

[Centril]

@estebank That has already been fixed in #65410?

[Centril]

The language team discussed this on 2019-10-17. We agreed with @pnkfelix's assessment that an RFC is not necessary in this case. Moreover, we agreed that this was desirable to remove unnecessary and annoying restrictions ("franken-feature" was used). We did note however that, as with all other features, it is important to have the tests in place, and that a to-be-FCPed stabilization should be filed.

---

The main implementation work consists of (now done):

• Introducing a feature gate bindings_after_at; follow https://rust-lang.github.io/rustc-guide/implementing_new_features.html#stability-in-code to do so.

• Removing the check that bans this (under a feature gate):

  rust/src/librustc_mir/hair/pattern/check_match.rs

  Lines 648 to 685 in e5b8c11

  	/// Forbids bindings in `@` patterns. This is necessary for memory safety,
  	/// because of the way rvalues are handled in the borrow check. (See issue
  	/// #14587.)
  	fn check_legality_of_bindings_in_at_patterns(cx: &MatchVisitor<'_, '_>, pat: &Pat) {
  	AtBindingPatternVisitor { cx, bindings_allowed: true }.visit_pat(pat);
  	}
  	struct AtBindingPatternVisitor<'a, 'b, 'tcx> {
  	cx: &'a MatchVisitor<'b, 'tcx>,
  	bindings_allowed: bool
  	}
  	impl<'v> Visitor<'v> for AtBindingPatternVisitor<'_, '_, '_> {
  	fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'v> {
  	NestedVisitorMap::None
  	}
  	fn visit_pat(&mut self, pat: &Pat) {
  	match pat.kind {
  	hir::PatKind::Binding(.., ref subpat) => {
  	if !self.bindings_allowed {
  	struct_span_err!(self.cx.tcx.sess, pat.span, E0303,
  	"pattern bindings are not allowed after an `@`")
  	.span_label(pat.span, "not allowed after `@`")
  	.emit();
  	}
  	if subpat.is_some() {
  	let bindings_were_allowed = self.bindings_allowed;
  	self.bindings_allowed = false;
  	intravisit::walk_pat(self, pat);
  	self.bindings_allowed = bindings_were_allowed;
  	}
  	}
  	_ => intravisit::walk_pat(self, pat),
  	}
  	}
  	}

• Removing (under a feature gate) (since bindings are now allowed it always is MachineApplicable)

  rust/src/libsyntax/parse/parser/pat.rs

  Lines 405 to 412 in e5b8c11

  	lhs.walk(&mut |p| match p.kind {
  	// `check_match` is unhappy if the subpattern has a binding anywhere.
  	PatKind::Ident(..) => {
  	applicability = Applicability::MaybeIncorrect;
  	false // Short-circuit.
  	},
  	_ => true,
  	});

• Writing a barrage of tests to make sure the behavior is both allowed and sound. This will be the harder part of the implementation and where most time should be spent!

If you have questions, please hop into https://rust-lang.zulipchat.com/#narrow/stream/122657-t-compiler.2Fwg-nll/topic/.2365490.20-.20x.20.40.20Some(y) where we can discuss things.