Implement BindingMode for pattern matching. #982

mjkillough · 2019-03-16T18:26:35Z

Implement BindingMode for pattern matching, so that types can be
correctly inferred using match ergonomics. The binding mode defaults to
Move (referred to as 'BindingMode::BindByValue` in rustc), and is
updated by automatic dereferencing of the value being matched.

Fixes #888.

Implement `BindingMode` for pattern matching, so that types can be correctly inferred using match ergonomics. The binding mode defaults to `Move` (referred to as 'BindingMode::BindByValue` in rustc), and is updated by automatic dereferencing of the value being matched.

mjkillough · 2019-03-16T18:28:22Z

crates/ra_hir/src/ty/tests.rs

+    let A(n) = &mut A(1);
+
+    let v = &(1, &2);
+    let (_, &w) = v;


This tests the else if let Pat::Ref { .. }, which is rust-lang/rust#46688.

maybe a separate test for that, plus a marker (tested_by!), would be nice :)

Also, in general, I'd prefer adding the new test cases in a separate test -- it's already not so easy what's what here

I hadn't come across tested_by!. Very neat! 🙂

I separated this out into two test-cases, as I think it makes the snapshots easier to read, and makes it clear what is being tested.

mjkillough · 2019-03-16T18:28:56Z

crates/ra_hir/src/ty/infer.rs

+/// Binding modes inferred for patterns.
+/// https://doc.rust-lang.org/reference/patterns.html#binding-modes
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum BindingMode {


I wasn't sure where to put this new enum. As it is only used by inference, I left it here for now.

Yeah, if it's only used here I think it's the right place.

mjkillough · 2019-03-16T18:30:38Z

crates/ra_hir/src/ty/infer.rs

+/// https://doc.rust-lang.org/reference/patterns.html#binding-modes
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum BindingMode {
+    Move,


rustc calls this BindByValue (and takes a mutability argument).

I chose to call it Move to match the reference documentation, and the match ergonomics RFC which refer to it that way. I'm very happy to change it if consistency with rustc is more important.

I think Move is good :)

mjkillough · 2019-03-16T18:31:51Z

crates/ra_hir/src/ty/infer.rs

+            | Pat::Range { .. }
+            | Pat::Slice { .. } => true,
+            // TODO: Path/Lit might actually evaluate to ref, but inference is unimplemented.
+            Pat::Path(..) | Pat::Lit(..) => true,


These are the relevant bits of the rustc implementation. Though I am not sure we're able to implement those here yet?

hm. somehow I was sure we supported literal patterns, but apparently we don't yet 😄 those should be easy to add, though. Path patterns to consts are supported. We'll probably need to extract the 'resolving' part from infer_path_expr to do the check. I think it'd be good to leave that for a separate PR though.

matklad · 2019-03-16T20:01:26Z

crates/ra_hir/src/ty/infer.rs

+            default_bm = BindingMode::Move;
+        }
+
+        // Lose mutability.


matklad · 2019-03-16T20:03:06Z

crates/ra_hir/src/ty/infer.rs

@@ -700,7 +761,7 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
            }
            Expr::For { iterable, body, pat } => {
                let _iterable_ty = self.infer_expr(*iterable, &Expectation::none());
-                self.infer_pat(*pat, &Ty::Unknown);
+                self.infer_pat(*pat, &Ty::Unknown, BindingMode::Move);


Micro nit, but this seems like a good place for Default ?

flodiebold

LGTM apart from a few nits (mainly, separating the test would be nicer), thanks!

flodiebold · 2019-03-17T09:58:28Z

crates/ra_hir/src/ty/infer.rs

+/// Binding modes inferred for patterns.
+/// https://doc.rust-lang.org/reference/patterns.html#binding-modes
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum BindingMode {


Yeah, if it's only used here I think it's the right place.

flodiebold · 2019-03-17T09:59:40Z

crates/ra_hir/src/ty/infer.rs

+/// https://doc.rust-lang.org/reference/patterns.html#binding-modes
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum BindingMode {
+    Move,


I think Move is good :)

flodiebold · 2019-03-17T10:03:26Z

crates/ra_hir/src/ty/infer.rs

+                }
+            }
+        } else if let Pat::Ref { .. } = &body[pat] {
+            default_bm = BindingMode::Move;


a similar comment as in rustc might be appropriate here?

flodiebold · 2019-03-17T10:09:18Z

crates/ra_hir/src/ty/infer.rs

+            | Pat::Range { .. }
+            | Pat::Slice { .. } => true,
+            // TODO: Path/Lit might actually evaluate to ref, but inference is unimplemented.
+            Pat::Path(..) | Pat::Lit(..) => true,


hm. somehow I was sure we supported literal patterns, but apparently we don't yet 😄 those should be easy to add, though. Path patterns to consts are supported. We'll probably need to extract the 'resolving' part from infer_path_expr to do the check. I think it'd be good to leave that for a separate PR though.

flodiebold · 2019-03-17T10:12:22Z

crates/ra_hir/src/ty/infer.rs

+                    }
+                    BindingMode::Ref(Mutability::Mut) => {
+                        Ty::Ref(inner_ty.clone().into(), Mutability::Mut)
+                    }


these two could just be written as

BindingMode::Ref(m) => Ty::ref(inner_ty.clone().into(), m)

or am I missing something?

You're right! That'd be much simpler.

flodiebold · 2019-03-17T10:13:33Z

crates/ra_hir/src/ty/tests.rs

+    let A(n) = &mut A(1);
+
+    let v = &(1, &2);
+    let (_, &w) = v;


maybe a separate test for that, plus a marker (tested_by!), would be nice :)

flodiebold · 2019-03-17T10:13:59Z

crates/ra_hir/src/ty/tests.rs

+    let A(n) = &mut A(1);
+
+    let v = &(1, &2);
+    let (_, &w) = v;


Also, in general, I'd prefer adding the new test cases in a separate test -- it's already not so easy what's what here

This decouples callers from knowing what the default binding mode of pattern matching is.

flodiebold · 2019-03-17T21:41:23Z

bors r+

982: Implement BindingMode for pattern matching. r=flodiebold a=mjkillough Implement `BindingMode` for pattern matching, so that types can be correctly inferred using match ergonomics. The binding mode defaults to `Move` (referred to as 'BindingMode::BindByValue` in rustc), and is updated by automatic dereferencing of the value being matched. Fixes #888. - [Binding modes in The Reference](https://doc.rust-lang.org/reference/patterns.html#binding-modes) - [`rustc` implementation](https://github.com/rust-lang/rust/blob/e17c48e2f21eefd59748e364234efc7037a3ec96/src/librustc_typeck/check/_match.rs#L77) (and [definition of `BindingMode`](https://github.com/rust-lang/rust/blob/e957ed9d10ec589bdd523b88b4b44c41b1ecf763/src/librustc/ty/binding.rs)) - [Match Ergonomics RFC](https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md#binding-mode-rules) Co-authored-by: Michael Killough <michaeljkillough@gmail.com>

bors · 2019-03-17T21:48:36Z

Build succeeded

continuous-integration/travis-ci/push

mjkillough commented Mar 16, 2019

View reviewed changes

matklad requested a review from flodiebold March 16, 2019 19:55

matklad approved these changes Mar 16, 2019

View reviewed changes

flodiebold reviewed Mar 17, 2019

View reviewed changes

mjkillough added 3 commits March 17, 2019 18:46

impl Default for BindingMode.

354134f

This decouples callers from knowing what the default binding mode of pattern matching is.

Simplify match statement.

33add0e

Split test case and use tested_by!.

6299ccd

bors bot merged commit 6299ccd into rust-lang:master Mar 17, 2019

mjkillough deleted the binding_modes branch March 17, 2019 21:51

mjkillough mentioned this pull request Mar 17, 2019

Forbid TODO markers on master branch #990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BindingMode for pattern matching. #982

Implement BindingMode for pattern matching. #982

mjkillough commented Mar 16, 2019

mjkillough Mar 16, 2019

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

mjkillough Mar 17, 2019

mjkillough Mar 16, 2019

flodiebold Mar 17, 2019

mjkillough Mar 16, 2019

flodiebold Mar 17, 2019

mjkillough Mar 16, 2019

flodiebold Mar 17, 2019

matklad Mar 16, 2019

matklad Mar 16, 2019

flodiebold left a comment

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

mjkillough Mar 17, 2019

flodiebold Mar 17, 2019

flodiebold Mar 17, 2019

flodiebold commented Mar 17, 2019

bors bot commented Mar 17, 2019

Implement BindingMode for pattern matching. #982

Implement BindingMode for pattern matching. #982

Conversation

mjkillough commented Mar 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flodiebold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flodiebold commented Mar 17, 2019

bors bot commented Mar 17, 2019

Build succeeded