Match Ergonomics Using Default Binding Modes #2005

Merged
merged 2 commits into from Jun 13, 2017

Conversation

Projects
None yet
@cramertj
Member

cramertj commented May 22, 2017

Rendered.

[edited to link to final rendered version]

@cramertj cramertj changed the title from Match Ergonomics Take 2 to Match Ergonomics Using Default Binding Modes May 22, 2017

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov May 22, 2017

Contributor

There are examples of nested patterns, but no examples with nested/multiple references.
Is something like this work supposed to work?

match &&&Some(0) {
    Some(x)
}

=>

match &&&Some(0) {
    &&&Some(ref x) // typeof(x) == &i32
}

or

match &&&Some(0) {
    &&&Some(/* equivalent to */ ref ref ref x) // typeof(x) == &&&i32
}

From some details in the text (e.g. "we don't know whether x[0] is Option<_> or &Option<_>") it seems that only one layer of references is permitted (seems reasonable).

Contributor

petrochenkov commented May 22, 2017

There are examples of nested patterns, but no examples with nested/multiple references.
Is something like this work supposed to work?

match &&&Some(0) {
    Some(x)
}

=>

match &&&Some(0) {
    &&&Some(ref x) // typeof(x) == &i32
}

or

match &&&Some(0) {
    &&&Some(/* equivalent to */ ref ref ref x) // typeof(x) == &&&i32
}

From some details in the text (e.g. "we don't know whether x[0] is Option<_> or &Option<_>") it seems that only one layer of references is permitted (seems reasonable).

@pnkfelix pnkfelix added the T-lang label May 22, 2017

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

There are examples of nested patterns, but no examples with nested/multiple references.

My assumption is the following:

match &&&Some(0) {
    Some(x)
}

// Equivalent to:

match &&&Some(0) {
    &&&Some(ref x)
}

That is, we will dereference any number of &T or &mut T values during pattern matching, but we do not "reproduce" those references on the other side by creating a corresponding number of indirections. (That doesn't seem like a particularly useful behavior, and in fact would be quite limiting in terms of the maximum lifetime of those bindings, since they may have to point into the stack in the general case.)

Contributor

nikomatsakis commented May 22, 2017

There are examples of nested patterns, but no examples with nested/multiple references.

My assumption is the following:

match &&&Some(0) {
    Some(x)
}

// Equivalent to:

match &&&Some(0) {
    &&&Some(ref x)
}

That is, we will dereference any number of &T or &mut T values during pattern matching, but we do not "reproduce" those references on the other side by creating a corresponding number of indirections. (That doesn't seem like a particularly useful behavior, and in fact would be quite limiting in terms of the maximum lifetime of those bindings, since they may have to point into the stack in the general case.)

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 22, 2017

Member

@petrochenkov If the compiler can determine that the value being matched is a reference, and it is being matched by a non-reference pattern, it will dereference it and shift the default binding mode. This process should repeat until there are no references left, or the type is successfully matched.

Note that this does not mean that it will pile up refs, ala ref ref ref.... Dereferencing a shared reference switches the binding mode to ref, regardless of the existing binding mode.

Example:

match &&&Some(0) {
    Some(x) => {},
    None => {},
}

// will desugar to
match &&&Some(0) {
    &&&Some(ref x) => {},
    &&&None => {},
}

Similarly, I would expect a pattern with too few references to also compile:

// If the user writes this `match`:
match &&&Some(0) {
    &Some(ref x) => {},
    &None => {},
}

// the compiler sees this:
match &(&(&(Some(0)))) {
    &(Some(ref x)) => {},
    &(None) => {},
}

// and will desugar to
match &&&Some(0) {
    &(&&Some(ref x)) => {},
    &(&&None) => {},
}

When it sees the first & in the pattern, it dereferences the value being matched and continues inwards where it finds a non-reference pattern (Some(ref x)). The value being matched is still a reference (&&Some(0)), so it is dereferenced and the default binding mode is shifted to ref. This process repeats once more until the value is successfully matched.

Member

cramertj commented May 22, 2017

@petrochenkov If the compiler can determine that the value being matched is a reference, and it is being matched by a non-reference pattern, it will dereference it and shift the default binding mode. This process should repeat until there are no references left, or the type is successfully matched.

Note that this does not mean that it will pile up refs, ala ref ref ref.... Dereferencing a shared reference switches the binding mode to ref, regardless of the existing binding mode.

Example:

match &&&Some(0) {
    Some(x) => {},
    None => {},
}

// will desugar to
match &&&Some(0) {
    &&&Some(ref x) => {},
    &&&None => {},
}

Similarly, I would expect a pattern with too few references to also compile:

// If the user writes this `match`:
match &&&Some(0) {
    &Some(ref x) => {},
    &None => {},
}

// the compiler sees this:
match &(&(&(Some(0)))) {
    &(Some(ref x)) => {},
    &(None) => {},
}

// and will desugar to
match &&&Some(0) {
    &(&&Some(ref x)) => {},
    &(&&None) => {},
}

When it sees the first & in the pattern, it dereferences the value being matched and continues inwards where it finds a non-reference pattern (Some(ref x)). The value being matched is still a reference (&&Some(0)), so it is dereferenced and the default binding mode is shifted to ref. This process repeats once more until the value is successfully matched.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

@cramertj

This reads great! I think there is one sort of "ambiguity" in the RFC text, and the examples don't cover it. In particular, what sort of types are "auto-dereferenceable"?

I see two interesting examples; one where the auto-deref occurs at the outermost level, and another with nested cases.

// EXAMPLE A -- deref at outermost level
//
// This could be made to work relatively easily, I think.

// If this works, I would expect `y` to be
// a `ref` binding.
let x: Rc<i32>;
match &x {
    Some(y) => ...,
    None => ...,
}

// More explicit form:
match &*x {
    &Some(ref y) => ...,
    &None => ...,
}

and:

// EXAMPLE B -- deref within
//
// I do not expect this to work.

let x: Option<Rc<Option<i32>>>;
match x {
    Some(Some(y)) => ...,
    None => ...,
}

My expectation is that we could support the first, but trying to support the second will be controversial and challenging. For one thing, it would require executing user-defined code (the Deref impl) during pattern matching; this may have side-effects (via Cell, println!) and there have been some concerns about that.

I think I'd be in favor of supporting the first but not the second, but it does introduce a sort of asymmetry.

Contributor

nikomatsakis commented May 22, 2017

@cramertj

This reads great! I think there is one sort of "ambiguity" in the RFC text, and the examples don't cover it. In particular, what sort of types are "auto-dereferenceable"?

I see two interesting examples; one where the auto-deref occurs at the outermost level, and another with nested cases.

// EXAMPLE A -- deref at outermost level
//
// This could be made to work relatively easily, I think.

// If this works, I would expect `y` to be
// a `ref` binding.
let x: Rc<i32>;
match &x {
    Some(y) => ...,
    None => ...,
}

// More explicit form:
match &*x {
    &Some(ref y) => ...,
    &None => ...,
}

and:

// EXAMPLE B -- deref within
//
// I do not expect this to work.

let x: Option<Rc<Option<i32>>>;
match x {
    Some(Some(y)) => ...,
    None => ...,
}

My expectation is that we could support the first, but trying to support the second will be controversial and challenging. For one thing, it would require executing user-defined code (the Deref impl) during pattern matching; this may have side-effects (via Cell, println!) and there have been some concerns about that.

I think I'd be in favor of supporting the first but not the second, but it does introduce a sort of asymmetry.

@phaylon

This comment has been minimized.

Show comment
Hide comment
@phaylon

phaylon May 22, 2017

I'd like to make a case for allowing explicitly specifying move: The RFC talks about this being only useful with Copy types and thus "not ... particularly useful in practice". However, this is quite common in my code, for primitives, shared references, and my own Copy types. As far as I understand, to get at the normal copying behavior I'd have to specify "Foo(x) => *x" even when the variant type is "Foo(i32)".

phaylon commented May 22, 2017

I'd like to make a case for allowing explicitly specifying move: The RFC talks about this being only useful with Copy types and thus "not ... particularly useful in practice". However, this is quite common in my code, for primitives, shared references, and my own Copy types. As far as I understand, to get at the normal copying behavior I'd have to specify "Foo(x) => *x" even when the variant type is "Foo(i32)".

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 22, 2017

Member

@phaylon For copying inner Copy types, you could manually dereference them, call clone(), or write exactly what you do today:

struct Foo<T>(T);
let foo = Foo(10);
let foo_ref = &foo;
let &Foo(foo_inner_copy) = foo_ref; // Copies `10`
// or
let Foo(foo_inner_copy) = *foo_ref;
Member

cramertj commented May 22, 2017

@phaylon For copying inner Copy types, you could manually dereference them, call clone(), or write exactly what you do today:

struct Foo<T>(T);
let foo = Foo(10);
let foo_ref = &foo;
let &Foo(foo_inner_copy) = foo_ref; // Copies `10`
// or
let Foo(foo_inner_copy) = *foo_ref;
@phaylon

This comment has been minimized.

Show comment
Hide comment
@phaylon

phaylon May 22, 2017

Ah, so as long as I don't leave off the * on the topic or & on the cases the match topic I won't trigger the auto-binding.

phaylon commented May 22, 2017

Ah, so as long as I don't leave off the * on the topic or & on the cases the match topic I won't trigger the auto-binding.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 22, 2017

Member

@phaylon Correct, and that's an important point-- this RFC doesn't change the behavior of any existing code. It only allows for code that would not have compiled previously.

Member

cramertj commented May 22, 2017

@phaylon Correct, and that's an important point-- this RFC doesn't change the behavior of any existing code. It only allows for code that would not have compiled previously.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

I think that @withoutboats in the past specifically wanted to include move as something that the user could type. I think that @nrc, in contast, was not keen. @cramertj, quickly skimming the RFC, I realized I wasn't sure whether you were proposing that the following would or would not be legal:

fn main() {
    match &Some(3) {
        Some(move v) => // v is copied out
        None => ...,
    }
}

I think that I personally favor allowing move on bindings, specifically for pedagogical purposes. I'd like to be able to explain the binding modes just as you did, and show code with all modes fully elaborated, and then show how the defaults work when they are elided.

Contributor

nikomatsakis commented May 22, 2017

I think that @withoutboats in the past specifically wanted to include move as something that the user could type. I think that @nrc, in contast, was not keen. @cramertj, quickly skimming the RFC, I realized I wasn't sure whether you were proposing that the following would or would not be legal:

fn main() {
    match &Some(3) {
        Some(move v) => // v is copied out
        None => ...,
    }
}

I think that I personally favor allowing move on bindings, specifically for pedagogical purposes. I'd like to be able to explain the binding modes just as you did, and show code with all modes fully elaborated, and then show how the defaults work when they are elided.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

(FWIW, I still think move is a non-perfect keyword, since "moving" a copy type is kind of a "copy", but I tend to teach this as 'when you reference a copy type, it is implicitly copied and everything is relative to that, so you are "moving the copy"', and I find that works "ok". Still it seems a bit more unfortunate than normal here in that move is only useful (in patterns) on copy types, since everywhere else that it is legal, it's also the default.)

Contributor

nikomatsakis commented May 22, 2017

(FWIW, I still think move is a non-perfect keyword, since "moving" a copy type is kind of a "copy", but I tend to teach this as 'when you reference a copy type, it is implicitly copied and everything is relative to that, so you are "moving the copy"', and I find that works "ok". Still it seems a bit more unfortunate than normal here in that move is only useful (in patterns) on copy types, since everywhere else that it is legal, it's also the default.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

Beyond pedagogy, I think I find Some(move v) clearer than &Some(v) in terms of communicating that it is important that I am copying v out from the option here. (That said, it's often not that important, it's just needed to make types work out.)

Contributor

nikomatsakis commented May 22, 2017

Beyond pedagogy, I think I find Some(move v) clearer than &Some(v) in terms of communicating that it is important that I am copying v out from the option here. (That said, it's often not that important, it's just needed to make types work out.)

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 22, 2017

Member

@nikomatsakis I left a note about this at the very end of the RFC under alternatives. Personally, I think that move is more confusing than helpful because it only works for Copy types, which are already a point of confusion for newcomers to Rust.

I might be amenable to a different keyword. None of the entries on the existing keyword list seem good to me. In a Rust 2.0/epoch world where we can add new keywords, I think copy would work nicely.

Member

cramertj commented May 22, 2017

@nikomatsakis I left a note about this at the very end of the RFC under alternatives. Personally, I think that move is more confusing than helpful because it only works for Copy types, which are already a point of confusion for newcomers to Rust.

I might be amenable to a different keyword. None of the entries on the existing keyword list seem good to me. In a Rust 2.0/epoch world where we can add new keywords, I think copy would work nicely.

@nrc nrc self-assigned this May 22, 2017

@nrc nrc referenced this pull request May 22, 2017

Closed

Improve match ergonomics #1944

text/0000-match-ergonomics.md
+match &Some(3) {
+ p => {
+ // `p` is a variable binding. Hence, this is **not** a ref-defaulting
+ // match, and `p` is bound with `move` semantics (and has type `&i32`).

This comment has been minimized.

@rkruppe

rkruppe May 22, 2017

Contributor

Should this say "and has type Option<&i32>"?

@rkruppe

rkruppe May 22, 2017

Contributor

Should this say "and has type Option<&i32>"?

This comment has been minimized.

@cramertj

cramertj May 22, 2017

Member

Yes, it should.

@cramertj

cramertj May 22, 2017

Member

Yes, it should.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 22, 2017

Contributor

Personally, I think that move is more confusing than helpful because it only works for Copy types, which are already a point of confusion for newcomers to Rust.

I think by "only works for Copy types" you mean "if you have something where the default mode is not move, then using move will only work for copy types", right? This seems true, though one could also use it (redundantly) in cases where you don't need it, for pedagogical purposes.

I agree to some uncertainty. I think the argument in favor of it is that I would rather teach people about Copy types, and what a "move" means in that context, than to teach them about & patterns. The former has independent value: they have to learn about copy types anyway, and closure syntax means that they will encounter this move keyword in other places, where it has the same meaning ("move a copy"). But & patterns are just a way to get "move" mode by default under this proposal.

Contributor

nikomatsakis commented May 22, 2017

Personally, I think that move is more confusing than helpful because it only works for Copy types, which are already a point of confusion for newcomers to Rust.

I think by "only works for Copy types" you mean "if you have something where the default mode is not move, then using move will only work for copy types", right? This seems true, though one could also use it (redundantly) in cases where you don't need it, for pedagogical purposes.

I agree to some uncertainty. I think the argument in favor of it is that I would rather teach people about Copy types, and what a "move" means in that context, than to teach them about & patterns. The former has independent value: they have to learn about copy types anyway, and closure syntax means that they will encounter this move keyword in other places, where it has the same meaning ("move a copy"). But & patterns are just a way to get "move" mode by default under this proposal.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 22, 2017

Member

@nikomatsakis

I would rather teach people about Copy types, and what a "move" means in that context, than to teach them about & patterns.

That seems reasonable, but I probably wouldn't do either-- I'd just have them dereference the value:

struct Foo<T>(T);
let foo_ref = &Foo(10);
let Foo(inner) = foo_ref; // This gives an `&i32`, they want an `i32`.

// In simple, non-nested cases, just do this:
let Foo(inner) = *foo_ref;

// In complex, nested cases, this will always work:
let Foo(inner_ref) = foo_ref;
let inner = *inner_ref;
Member

cramertj commented May 22, 2017

@nikomatsakis

I would rather teach people about Copy types, and what a "move" means in that context, than to teach them about & patterns.

That seems reasonable, but I probably wouldn't do either-- I'd just have them dereference the value:

struct Foo<T>(T);
let foo_ref = &Foo(10);
let Foo(inner) = foo_ref; // This gives an `&i32`, they want an `i32`.

// In simple, non-nested cases, just do this:
let Foo(inner) = *foo_ref;

// In complex, nested cases, this will always work:
let Foo(inner_ref) = foo_ref;
let inner = *inner_ref;
@repax

This comment has been minimized.

Show comment
Hide comment
@repax

repax May 22, 2017

I suppose, and hope, that auto-dereferencing could work on user Deref-impls at some point in the future if they could be marked as being pure, or free of side effects.

repax commented May 22, 2017

I suppose, and hope, that auto-dereferencing could work on user Deref-impls at some point in the future if they could be marked as being pure, or free of side effects.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc May 23, 2017

Member

I suppose, and hope, that auto-dereferencing could work on user Deref-impls at some point in the future

This is an interesting question. I think the answer is that we should work with Deref as well as built-in references and there doesn't need to be a purity requirement, e.g., the following should work:

fn foo(x: Rc<Option<T>>) {
    match &x {
        Some(x) => ..., // x: &T
        None => {}
    }
}

In the previous RFC I proposed this should follow the rules for deref coercions and only deref if the discriminant is already borrowed. Some people felt we could follow the dot operator and deref all types.

Member

nrc commented May 23, 2017

I suppose, and hope, that auto-dereferencing could work on user Deref-impls at some point in the future

This is an interesting question. I think the answer is that we should work with Deref as well as built-in references and there doesn't need to be a purity requirement, e.g., the following should work:

fn foo(x: Rc<Option<T>>) {
    match &x {
        Some(x) => ..., // x: &T
        None => {}
    }
}

In the previous RFC I proposed this should follow the rules for deref coercions and only deref if the discriminant is already borrowed. Some people felt we could follow the dot operator and deref all types.

@burdges

This comment has been minimized.

Show comment
Hide comment
@burdges

burdges May 23, 2017

I'm slightly nervous about the new mutable reference behavior, but it's what for does already, so consistency wins out.

I do think any book that covers style should explain that, due to this convenience, functions that return mutable references, and data structures that contain mutable references, should indicate this fact with their suffix, like _mut or Mut. It's reasonable that Rust should only be responsible for making mutability explicit with basic operations, and in std, but if the programmers starts creating data structures that involve mutable references then they become responsible for making mutability explicit. It's good to tell people though.

burdges commented May 23, 2017

I'm slightly nervous about the new mutable reference behavior, but it's what for does already, so consistency wins out.

I do think any book that covers style should explain that, due to this convenience, functions that return mutable references, and data structures that contain mutable references, should indicate this fact with their suffix, like _mut or Mut. It's reasonable that Rust should only be responsible for making mutability explicit with basic operations, and in std, but if the programmers starts creating data structures that involve mutable references then they become responsible for making mutability explicit. It's good to tell people though.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 23, 2017

Member

I'm working on gathering my thoughts with respect to the custom Deref question, and I'll post something in more detail soon. However, here's one interesting question I came across: how do we want to support types that implement both Deref and DerefMut? The obvious answer seems to be to prefer DerefMut since it allows for more flexibility, but I'm not so sure what that would actually mean in practice.

The following code seams reasonable:

let mut x = Box::new(Some(5));
match x {
    // The `DerefMut` impl of `Box` is used to get `&mut Option<i32>`,
    // which is then matched against `Some(5)`
    Some(5) => {},

    // Here, the `DerefMut` impl of `Box` is used to get `&mut Option<i32>`
    Some(y) => { 
        *y = 6;
    },

    // The `DerefMut` impl of `Box` is used to get `&mut Option<i32>`,
    // which is then matched against `None`
    None => {},
}

But what should happen in the case when x (our Box<Option<i32>>) isn't declared as mut? It can't be mutably borrowed for the DerefMut call, so it uses Deref instead.

let x = Box::new(Some(5)); // no `mut`
match x {
    // What happens here? We would use the `DerefMut` impl, but `x` is immutable.
    // So instead we could infer to use `Deref` here based on the immutability of `x`.
    Some(5) => {},

    // Here, we again see that `x` is immutable, and so use the `Deref` impl of `Box`
    // to get `&Option<i32>`
    Some(_) => {},

    // The `Deref` impl of `Box` is used to get `&Option<i32>`,
    // which is then matched against `None`
    None => {},
}

This obviously isn't an insurmountable issue, I just thought it was interesting that we would potentially have to dispatch to different methods (Deref vs. DerefMut) based on the mutability of the value being matched.

Member

cramertj commented May 23, 2017

I'm working on gathering my thoughts with respect to the custom Deref question, and I'll post something in more detail soon. However, here's one interesting question I came across: how do we want to support types that implement both Deref and DerefMut? The obvious answer seems to be to prefer DerefMut since it allows for more flexibility, but I'm not so sure what that would actually mean in practice.

The following code seams reasonable:

let mut x = Box::new(Some(5));
match x {
    // The `DerefMut` impl of `Box` is used to get `&mut Option<i32>`,
    // which is then matched against `Some(5)`
    Some(5) => {},

    // Here, the `DerefMut` impl of `Box` is used to get `&mut Option<i32>`
    Some(y) => { 
        *y = 6;
    },

    // The `DerefMut` impl of `Box` is used to get `&mut Option<i32>`,
    // which is then matched against `None`
    None => {},
}

But what should happen in the case when x (our Box<Option<i32>>) isn't declared as mut? It can't be mutably borrowed for the DerefMut call, so it uses Deref instead.

let x = Box::new(Some(5)); // no `mut`
match x {
    // What happens here? We would use the `DerefMut` impl, but `x` is immutable.
    // So instead we could infer to use `Deref` here based on the immutability of `x`.
    Some(5) => {},

    // Here, we again see that `x` is immutable, and so use the `Deref` impl of `Box`
    // to get `&Option<i32>`
    Some(_) => {},

    // The `Deref` impl of `Box` is used to get `&Option<i32>`,
    // which is then matched against `None`
    None => {},
}

This obviously isn't an insurmountable issue, I just thought it was interesting that we would potentially have to dispatch to different methods (Deref vs. DerefMut) based on the mutability of the value being matched.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 23, 2017

Contributor

@cramertj

That seems reasonable, but I probably wouldn't do either-- I'd just have them dereference the value:

That's an option, but it does lead to longer than necessary borrows, which in the general case can be a problem (although with NLL, less so). I sort of prefer to move the deref into the pattern match myself.

Contributor

nikomatsakis commented May 23, 2017

@cramertj

That seems reasonable, but I probably wouldn't do either-- I'd just have them dereference the value:

That's an option, but it does lead to longer than necessary borrows, which in the general case can be a problem (although with NLL, less so). I sort of prefer to move the deref into the pattern match myself.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 23, 2017

Contributor

@cramertj

Hmm, the Box example you showed didn't work like I would have expected. That is:

let mut x = Box::new(Some(5));
match x {
    Some(y) => ... 
    None => ...
}

I do not expect y to be a reference here, as we have not passed through any & patterns. I would expect this to work only once we have some solution for DerefMove. For now, my assumption is that to trigger a Deref impl of any kind, the outermost type must be a reference, just as with a deref coercion. Therefore, I would expect this to work:

let mut x = Box::new(Some(5));
match &mut x {
    Some(y) => ... // y: &mut i32
    None => ...
}

This obviously isn't an insurmountable issue, I just thought it was interesting that we would potentially have to dispatch to different methods (Deref vs. DerefMut) based on the mutability of the value being matched.

Basically I think we would pick based on the default binding mode, more or less.

Contributor

nikomatsakis commented May 23, 2017

@cramertj

Hmm, the Box example you showed didn't work like I would have expected. That is:

let mut x = Box::new(Some(5));
match x {
    Some(y) => ... 
    None => ...
}

I do not expect y to be a reference here, as we have not passed through any & patterns. I would expect this to work only once we have some solution for DerefMove. For now, my assumption is that to trigger a Deref impl of any kind, the outermost type must be a reference, just as with a deref coercion. Therefore, I would expect this to work:

let mut x = Box::new(Some(5));
match &mut x {
    Some(y) => ... // y: &mut i32
    None => ...
}

This obviously isn't an insurmountable issue, I just thought it was interesting that we would potentially have to dispatch to different methods (Deref vs. DerefMut) based on the mutability of the value being matched.

Basically I think we would pick based on the default binding mode, more or less.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 24, 2017

Member

@nikomatsakis Oh, I see. I was thinking that you were suggesting that Deref<Target=T> and &T be treated as equivalent for the purposes of pattern matching. That is, I was thinking that dereferencing a Box<T> would shift the default binding mode to ref or ref mut.

Your exact question was:

In particular, what sort of types are "auto-dereferenceable"?

But I had mentally linked "auto-dereference" with "auto-dereference and shift the binding mode."

If we only auto-dereference custom types without shifting the default binding mode, then yes, we can choose DerefMove vs. Deref vs. DerefMut based on the binding mode.

Member

cramertj commented May 24, 2017

@nikomatsakis Oh, I see. I was thinking that you were suggesting that Deref<Target=T> and &T be treated as equivalent for the purposes of pattern matching. That is, I was thinking that dereferencing a Box<T> would shift the default binding mode to ref or ref mut.

Your exact question was:

In particular, what sort of types are "auto-dereferenceable"?

But I had mentally linked "auto-dereference" with "auto-dereference and shift the binding mode."

If we only auto-dereference custom types without shifting the default binding mode, then yes, we can choose DerefMove vs. Deref vs. DerefMut based on the binding mode.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2017

Contributor

@cramertj so, thinking about it more, I think we could leave the "autoderef" questions for another RFC. I'd really like to see the "core" ideas land. If we leave that out, do you feel like there are major unresolved questions for this RFC? I feel like it's ready to go to FCP, myself.

Contributor

nikomatsakis commented May 30, 2017

@cramertj so, thinking about it more, I think we could leave the "autoderef" questions for another RFC. I'd really like to see the "core" ideas land. If we leave that out, do you feel like there are major unresolved questions for this RFC? I feel like it's ready to go to FCP, myself.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 30, 2017

Member

@nikomatsakis I agree, after thinking about it for a while I have a lot of questions about exactly how autoderef for custom types should work, and I think it'd be better to leave that feature for a future RFC.

The other major unresolved question is whether or not to introduce move patterns. As I said above, I'd prefer to leave them out, at least for the time being. move feels nice from a conceptual perspective, but its usefulness is limited and IMO not worth the additional complexity it brings to the language.

Member

cramertj commented May 30, 2017

@nikomatsakis I agree, after thinking about it for a while I have a lot of questions about exactly how autoderef for custom types should work, and I think it'd be better to leave that feature for a future RFC.

The other major unresolved question is whether or not to introduce move patterns. As I said above, I'd prefer to leave them out, at least for the time being. move feels nice from a conceptual perspective, but its usefulness is limited and IMO not worth the additional complexity it brings to the language.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2017

Contributor

@cramertj

The other major unresolved question is whether or not to introduce move patterns. As I said above, I'd prefer to leave them out, at least for the time being. move feels nice from a conceptual perspective, but its usefulness is limited and IMO not worth the additional complexity it brings to the language.

Ah, yes, that's important to resolve. I think that I could go either way, but I lean towards having them. I think that it will be helpful for didactic purposes, and it doesn't really add any particular complexity to the language (in some sense, I feel like it removes complexity, in that there is no special form). But we already had this back-and-forth, I think, so I guess I'm just re-treading ground.

I'm curious what the rest of the @rust-lang/lang team thinks. I am pretty sure that @pnkfelix would also want them, since they always favor an explicit form -- @withoutboats is also on record as being in favor. @nrc, I think, would prefer to leave them out. That just leaves @aturon and @eddyb to weigh in. =)

I suppose an alternative would be to seek for a different keyword or something like that, but it seems like move is really the obvious one here, given closures.

Contributor

nikomatsakis commented May 30, 2017

@cramertj

The other major unresolved question is whether or not to introduce move patterns. As I said above, I'd prefer to leave them out, at least for the time being. move feels nice from a conceptual perspective, but its usefulness is limited and IMO not worth the additional complexity it brings to the language.

Ah, yes, that's important to resolve. I think that I could go either way, but I lean towards having them. I think that it will be helpful for didactic purposes, and it doesn't really add any particular complexity to the language (in some sense, I feel like it removes complexity, in that there is no special form). But we already had this back-and-forth, I think, so I guess I'm just re-treading ground.

I'm curious what the rest of the @rust-lang/lang team thinks. I am pretty sure that @pnkfelix would also want them, since they always favor an explicit form -- @withoutboats is also on record as being in favor. @nrc, I think, would prefer to leave them out. That just leaves @aturon and @eddyb to weigh in. =)

I suppose an alternative would be to seek for a different keyword or something like that, but it seems like move is really the obvious one here, given closures.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2017

Contributor

(It's also true that if we do not include move, we can always add it at a later date.)

Contributor

nikomatsakis commented May 30, 2017

(It's also true that if we do not include move, we can always add it at a later date.)

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats May 30, 2017

Contributor

I'd like to include move, but I'd also like to include explicit ref and ref mut on closures. Then a parallel can be made between these.

Contributor

withoutboats commented May 30, 2017

I'd like to include move, but I'd also like to include explicit ref and ref mut on closures. Then a parallel can be made between these.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2017

Contributor

@withoutboats

I'd also like to include explicit ref and ref mut on closures. Then a parallel can be made between these.

As part of this RFC, do you mean? Or just in general?

(Any ideas as to the syntax?)

Contributor

nikomatsakis commented May 30, 2017

@withoutboats

I'd also like to include explicit ref and ref mut on closures. Then a parallel can be made between these.

As part of this RFC, do you mean? Or just in general?

(Any ideas as to the syntax?)

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats May 30, 2017

Contributor

I don't mean as part of this RFC. As to the syntax, I just mean ref |x| x + y and ref mut |x| y += x, the same way that move can be applied today (ref |x| y += x would be an error).

Contributor

withoutboats commented May 30, 2017

I don't mean as part of this RFC. As to the syntax, I just mean ref |x| x + y and ref mut |x| y += x, the same way that move can be applied today (ref |x| y += x would be an error).

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon May 30, 2017

Member

@cramertj, I finally had a chance to carefully read the RFC and the comment thread -- really fantastic work! This is basically how my mind always wants pattern matching to work, before I remind myself to go sprinkle in refs.

Regarding some of the open questions:

  • I'm in favor of punting user-defined Deref implementations and thus only supporting & and &mut to start. That's already a huge step, and we'll surely learn a lot along the way.

  • I prefer not to introduce move ever, but certainly not at the outset. To restate what's been said: the only time you would need move is to reset the default binding mode to owned, which means you're moving out of a reference, which means you must have a Copy type. Of course, there are cases where this can be annoying (&Option<i32> giving you &i32), but I'd prefer to resolve these cases by making Copy types more ergonomic in general. That is, I'd like to pursue something like a &T to T coercion for Copy types.

    • re: @pnkfelix and having an explicit form, I'd consider the ability to dereference in the body of the arm a sufficient explicit form for "moving out" Copy data.

    • re: @withoutboats, can you elaborate on why you see the need for move?

I think that basically means I'm happy with the RFC as-is. I'm going to go ahead and propose to go to FCP with the current state, just so we can get the ball rolling, but of course those who strongly want to see move or Deref at the outset should withhold consent :-)

@rfcbot fcp merge

Member

aturon commented May 30, 2017

@cramertj, I finally had a chance to carefully read the RFC and the comment thread -- really fantastic work! This is basically how my mind always wants pattern matching to work, before I remind myself to go sprinkle in refs.

Regarding some of the open questions:

  • I'm in favor of punting user-defined Deref implementations and thus only supporting & and &mut to start. That's already a huge step, and we'll surely learn a lot along the way.

  • I prefer not to introduce move ever, but certainly not at the outset. To restate what's been said: the only time you would need move is to reset the default binding mode to owned, which means you're moving out of a reference, which means you must have a Copy type. Of course, there are cases where this can be annoying (&Option<i32> giving you &i32), but I'd prefer to resolve these cases by making Copy types more ergonomic in general. That is, I'd like to pursue something like a &T to T coercion for Copy types.

    • re: @pnkfelix and having an explicit form, I'd consider the ability to dereference in the body of the arm a sufficient explicit form for "moving out" Copy data.

    • re: @withoutboats, can you elaborate on why you see the need for move?

I think that basically means I'm happy with the RFC as-is. I'm going to go ahead and propose to go to FCP with the current state, just so we can get the ball rolling, but of course those who strongly want to see move or Deref at the outset should withhold consent :-)

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot May 30, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented May 30, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2017

Contributor

So I agree that & suffices as an explicit form (in fact @cramertj and I were saying the same thing to each other over IRC just now). I am satisfied with this.

I am not entirely sure if I agree that just making &Copy more ergonomic removes the need for an explicit form, but it's a moot point, since we have one. The case I was thinking of was when using a ref binding would cause a loan that then fails the borrow checker (which would not be helped by &Copy ergonomics):

let mut x = Some(22);
loop {
    match &x {
        Some(p) => x = Some(p + 1), // illegal: `x` is borrowed here
        None => ...
    }
}

However, this is obviously artificial, since you could write it equally well as match x { Some(p) ... or match &x { &Some(p) .... I'm not sure whether this will arise in other cases -- I feel like it does, but I couldn't come up with a convincing example.

Contributor

nikomatsakis commented May 30, 2017

So I agree that & suffices as an explicit form (in fact @cramertj and I were saying the same thing to each other over IRC just now). I am satisfied with this.

I am not entirely sure if I agree that just making &Copy more ergonomic removes the need for an explicit form, but it's a moot point, since we have one. The case I was thinking of was when using a ref binding would cause a loan that then fails the borrow checker (which would not be helped by &Copy ergonomics):

let mut x = Some(22);
loop {
    match &x {
        Some(p) => x = Some(p + 1), // illegal: `x` is borrowed here
        None => ...
    }
}

However, this is obviously artificial, since you could write it equally well as match x { Some(p) ... or match &x { &Some(p) .... I'm not sure whether this will arise in other cases -- I feel like it does, but I couldn't come up with a convincing example.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats May 31, 2017

Contributor

re: @withoutboats, can you elaborate on why you see the need for move?

I don't see it as a need per se, more of an opportunity. :) So if other people don't feel its a simplifying force, I don't feel compelled to fight for it.

Right now we have ref and ref mut patterns on the one hands, and move closures on the other. Both of these are rather confusing features for new users in one way or another. My feeling is that we can unify these two concepts into a single notion of "ownership modes" which have certain defaults in different cases.

I also think &Some(x) is a less clear pattern than Some(move x) even considering that the move can only be applied to Copy types (I prefer a mental model in which copies are just moves you get to keep using, ymmv). I'm also unclear about a match statement like this:

x: &Option<i32>
match x {
    &Some(x) => { ... }
    None => { ... } // Is this okay, or does it have to be &None?
}

My belief is this is invalid, but I'm not sure. If it is, the use of references to get move mode seems to impact every arm of the match, not only this pattern. Seems suboptimal.

Contributor

withoutboats commented May 31, 2017

re: @withoutboats, can you elaborate on why you see the need for move?

I don't see it as a need per se, more of an opportunity. :) So if other people don't feel its a simplifying force, I don't feel compelled to fight for it.

Right now we have ref and ref mut patterns on the one hands, and move closures on the other. Both of these are rather confusing features for new users in one way or another. My feeling is that we can unify these two concepts into a single notion of "ownership modes" which have certain defaults in different cases.

I also think &Some(x) is a less clear pattern than Some(move x) even considering that the move can only be applied to Copy types (I prefer a mental model in which copies are just moves you get to keep using, ymmv). I'm also unclear about a match statement like this:

x: &Option<i32>
match x {
    &Some(x) => { ... }
    None => { ... } // Is this okay, or does it have to be &None?
}

My belief is this is invalid, but I'm not sure. If it is, the use of references to get move mode seems to impact every arm of the match, not only this pattern. Seems suboptimal.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj May 31, 2017

Member

My belief is this is invalid, but I'm not sure.

@withoutboats Yes, that is valid under the current design. Each match arm is analyzed independently.

Member

cramertj commented May 31, 2017

My belief is this is invalid, but I'm not sure.

@withoutboats Yes, that is valid under the current design. Each match arm is analyzed independently.

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 28, 2017

WIP: default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [][2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 28, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 28, 2017

WIP: default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [][2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 28, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 28, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

nikomatsakis added a commit to tschottdorf/rust that referenced this pull request Jul 29, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

bors added a commit to rust-lang/rust that referenced this pull request Jul 30, 2017

Auto merge of #43399 - tschottdorf:bndmode-pat-adjustments, r=nikomat…
…sakis

default binding modes: add pat_binding_modes

This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

cc @nikomatsakis

[1]: rust-lang/rfcs#2005
[2]: #42640 (comment)

bors added a commit to rust-lang/rust that referenced this pull request Jul 30, 2017

Auto merge of #43399 - tschottdorf:bndmode-pat-adjustments, r=nikomat…
…sakis

default binding modes: add pat_binding_modes

This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

cc @nikomatsakis

[1]: rust-lang/rfcs#2005
[2]: #42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 30, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

tschottdorf added a commit to tschottdorf/rust that referenced this pull request Jul 30, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)

bors added a commit to rust-lang/rust that referenced this pull request Jul 31, 2017

Auto merge of #43399 - tschottdorf:bndmode-pat-adjustments, r=nikomat…
…sakis

default binding modes: add pat_binding_modes

This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

cc @nikomatsakis

[1]: rust-lang/rfcs#2005
[2]: #42640 (comment)

bors added a commit to rust-lang/rust that referenced this pull request Jul 31, 2017

Auto merge of #43399 - tschottdorf:bndmode-pat-adjustments, r=nikomat…
…sakis

default binding modes: add pat_binding_modes

This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching `BindByRef|BindByValue` so that reviewers
are aware of all of them.

I will look into changing the HIR (as suggested in [2]) to not carry a
`BindingMode` unless one was explicitly specified, but this PR is good for
a first round of comments.

The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.

See #42640.

cc @nikomatsakis

[1]: rust-lang/rfcs#2005
[2]: #42640 (comment)

@nikomatsakis nikomatsakis referenced this pull request in nikomatsakis/nll-rfc Aug 2, 2017

Merged

Fix a bug and a few formatting errors #10

matthewhammer added a commit to matthewhammer/rust that referenced this pull request Aug 3, 2017

default binding modes: add pat_binding_modes
This PR kicks off the implementation of the [default binding modes RFC][1] by
introducing the `pat_binding_modes` typeck table mentioned in the [mentoring
instructions][2].

`pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and
used wherever the HIR would be scraped prior to this PR. Unfortunately, one
blemish, namely a two callers to `contains_explicit_ref_binding`, remains.
This will likely have to be removed when the second part of [1], the
`pat_adjustments` table, is tackled. Appropriate comments have been added.

See #42640.

[1]: rust-lang/rfcs#2005
[2]: rust-lang#42640 (comment)
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Aug 16, 2017

Member

When we first went through this RFC, I missed an interesting comment by @phaylon. The crux is that if you have an &mut SomeStruct, today you can use matching to destructure that into references to its fields, some of which are mutable, and some of which are not. Just like with the mut keyword, this can give you more insight into mutation in the code that follows.

@cramertj @nrc @nikomatsakis I'm curious what you think about this use case. Can you see another way to achieve a similar result if we eventually fully deprecate ref? Might this be a reason to keep ref around?

Member

aturon commented Aug 16, 2017

When we first went through this RFC, I missed an interesting comment by @phaylon. The crux is that if you have an &mut SomeStruct, today you can use matching to destructure that into references to its fields, some of which are mutable, and some of which are not. Just like with the mut keyword, this can give you more insight into mutation in the code that follows.

@cramertj @nrc @nikomatsakis I'm curious what you think about this use case. Can you see another way to achieve a similar result if we eventually fully deprecate ref? Might this be a reason to keep ref around?

@glaebhoerl

This comment has been minimized.

Show comment
Hide comment
@glaebhoerl

glaebhoerl Aug 16, 2017

Contributor

Copying @phaylon's example just for easy reference:

let Select {
    ref selector,
    ref stream,
    ref mut builder,
    ref mut state,
    ref mut restrict,
} = *self;

At worst, you could express it as:

let Select {
    selector,
    stream,
    builder,
    state,
    restrict,
} = self;
let (selector, stream) = (&*selector, &*stream);

So you're "explicitly discarding" your privileges on selector and stream. I wonder if there's a nicer way to write that than &*?

Contributor

glaebhoerl commented Aug 16, 2017

Copying @phaylon's example just for easy reference:

let Select {
    ref selector,
    ref stream,
    ref mut builder,
    ref mut state,
    ref mut restrict,
} = *self;

At worst, you could express it as:

let Select {
    selector,
    stream,
    builder,
    state,
    restrict,
} = self;
let (selector, stream) = (&*selector, &*stream);

So you're "explicitly discarding" your privileges on selector and stream. I wonder if there's a nicer way to write that than &*?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 16, 2017

Contributor

Hmm, to be honest, I think that the use case of "intermingled" ref and ref mut is less interesting than the use case of just using ref. If you only use ref bindings, then you wind up with a shared reborrow of the &mut, which you can do multiple times. So for example this would type-check:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match *pair { (ref x, _) => x };
let p: &(i32, i32) = &*pair;

while the following "erasure" does not:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match pair { (x, _) => x };
let p: &(i32, i32) = &*pair;

To do something equivalent, you would have to explicitly use a & borrow at the outermost level:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match &pair { (x, _) => x };
let p: &(i32, i32) = &*pair;

I'm not sure this is bad, but it is not something I had fully realized before. It seems related to the reasons that it is useful to have last() and last_mut() -- i.e., so that if you have an &mut reference, you can explicitly choose, via the name of the method, how you wish to be using it at a particular time. (In comparison to a hypothetical "generic" last that would be mut if self is mut.)

Contributor

nikomatsakis commented Aug 16, 2017

Hmm, to be honest, I think that the use case of "intermingled" ref and ref mut is less interesting than the use case of just using ref. If you only use ref bindings, then you wind up with a shared reborrow of the &mut, which you can do multiple times. So for example this would type-check:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match *pair { (ref x, _) => x };
let p: &(i32, i32) = &*pair;

while the following "erasure" does not:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match pair { (x, _) => x };
let p: &(i32, i32) = &*pair;

To do something equivalent, you would have to explicitly use a & borrow at the outermost level:

let pair: &mut (i32, i32) = &mut (0, 1);
let x = match &pair { (x, _) => x };
let p: &(i32, i32) = &*pair;

I'm not sure this is bad, but it is not something I had fully realized before. It seems related to the reasons that it is useful to have last() and last_mut() -- i.e., so that if you have an &mut reference, you can explicitly choose, via the name of the method, how you wish to be using it at a particular time. (In comparison to a hypothetical "generic" last that would be mut if self is mut.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 16, 2017

Contributor

One could imagine requiring the mut keyword to create the equivalent of today's ref mut:

let pair = &mut (0, 1);
match pair { (mut x, _) => { ... } }
// equivalent to: `match *pair { (ref mut x, _) => { ... } }`

Right now I think that mut x creates a variable that one can reassign (just as it does in any other context). This seems more "intuitive" to me than having it mean ref mut, but it's also something you cannot currently express (it would be like mut ref mut), and for which I've never really felt the need.

Contributor

nikomatsakis commented Aug 16, 2017

One could imagine requiring the mut keyword to create the equivalent of today's ref mut:

let pair = &mut (0, 1);
match pair { (mut x, _) => { ... } }
// equivalent to: `match *pair { (ref mut x, _) => { ... } }`

Right now I think that mut x creates a variable that one can reassign (just as it does in any other context). This seems more "intuitive" to me than having it mean ref mut, but it's also something you cannot currently express (it would be like mut ref mut), and for which I've never really felt the need.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 16, 2017

Member

@nikomatsakis The implicit modification of the original pair still seems very confusing to me. Having "mut" there says "this is modifiable", not "touching this will modify something else".

Member

joshtriplett commented Aug 16, 2017

@nikomatsakis The implicit modification of the original pair still seems very confusing to me. Having "mut" there says "this is modifiable", not "touching this will modify something else".

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 16, 2017

Contributor

@joshtriplett what do you mean by "implicit modification"? You mean that x has type &mut i32? This implies (then) that x could be used to modify the original by doing something like *x += 1. I think what you are saying is that, looking at match pair { (x, _) => ... }, you know that the code in the ... cannot use x to mutate pair? I can certainly see this POV; I'm not sure how confusing it would be in practice. I don't yet see the best way to signal that you want x to be a "mutable" borrow. As you say, mut x doesn't feel ideal to me, since it seems to be saying that x is mutable, not that *x is mutable.

Contributor

nikomatsakis commented Aug 16, 2017

@joshtriplett what do you mean by "implicit modification"? You mean that x has type &mut i32? This implies (then) that x could be used to modify the original by doing something like *x += 1. I think what you are saying is that, looking at match pair { (x, _) => ... }, you know that the code in the ... cannot use x to mutate pair? I can certainly see this POV; I'm not sure how confusing it would be in practice. I don't yet see the best way to signal that you want x to be a "mutable" borrow. As you say, mut x doesn't feel ideal to me, since it seems to be saying that x is mutable, not that *x is mutable.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 16, 2017

Member

As you say, mut x doesn't feel ideal to me, since it seems to be saying that x is mutable, not that *x is mutable.

This is an accurate summary of my concern, yes.

When I see something like match pair { (mut x, _) => ... }, I don't see anything that suggests that pair is modifiable. When I see a & or a ref, that possibility comes into play.

I have moderate concerns about making it harder to tell when something is referencing versus copying. I have much bigger concerns about making it harder to tell when something is mutably referencing.

Member

joshtriplett commented Aug 16, 2017

As you say, mut x doesn't feel ideal to me, since it seems to be saying that x is mutable, not that *x is mutable.

This is an accurate summary of my concern, yes.

When I see something like match pair { (mut x, _) => ... }, I don't see anything that suggests that pair is modifiable. When I see a & or a ref, that possibility comes into play.

I have moderate concerns about making it harder to tell when something is referencing versus copying. I have much bigger concerns about making it harder to tell when something is mutably referencing.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 16, 2017

Contributor

I think @phaylon's example is why I was personally in favor of requiring mut to make an &mut binding.

Contributor

withoutboats commented Aug 16, 2017

I think @phaylon's example is why I was personally in favor of requiring mut to make an &mut binding.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 16, 2017

Member

@withoutboats I definitely wouldn't want (x, _) to create a &mut binding where there wasn't one previously; I just also don't think (mut x, _) is enough to make that clear enough, either.

Member

joshtriplett commented Aug 16, 2017

@withoutboats I definitely wouldn't want (x, _) to create a &mut binding where there wasn't one previously; I just also don't think (mut x, _) is enough to make that clear enough, either.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 16, 2017

Member

@joshtriplett

I definitely wouldn't want (x, _) to create a &mut binding where there wasn't one previously

It doesn't create one where there wasn't one previously-- if you write if let Some(x) = &mut Some(5) { ... }, x will be an &mut i32. It's just matching the reference type of the value being matched.

Member

cramertj commented Aug 16, 2017

@joshtriplett

I definitely wouldn't want (x, _) to create a &mut binding where there wasn't one previously

It doesn't create one where there wasn't one previously-- if you write if let Some(x) = &mut Some(5) { ... }, x will be an &mut i32. It's just matching the reference type of the value being matched.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 16, 2017

Contributor

@joshtriplett I think your concerns were already expressed in the RFC thread above. rather than re-open general concerns about the RFC I'd like it if we could focus on the specific concerns @aturon bumped the thread about (originally raised by @phaylon) about splitting an object into mutable and immutable parts.

Is this something we want to support easily under this RFC? What are the ways to do that? To me it seems like "requiring mut to get an &mut" is the most obvious way. I don't really agree with @nikomatsakis's concern about this being surprising. I guess technically it means there isn't a way to support mutating the value without reassigning the reference, but those two things really seem to go hand in hand to me. In fact, it seems like a stumbling block to require mut to reassign when you don't require mut to call &mut self methods.

Contributor

withoutboats commented Aug 16, 2017

@joshtriplett I think your concerns were already expressed in the RFC thread above. rather than re-open general concerns about the RFC I'd like it if we could focus on the specific concerns @aturon bumped the thread about (originally raised by @phaylon) about splitting an object into mutable and immutable parts.

Is this something we want to support easily under this RFC? What are the ways to do that? To me it seems like "requiring mut to get an &mut" is the most obvious way. I don't really agree with @nikomatsakis's concern about this being surprising. I guess technically it means there isn't a way to support mutating the value without reassigning the reference, but those two things really seem to go hand in hand to me. In fact, it seems like a stumbling block to require mut to reassign when you don't require mut to call &mut self methods.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 16, 2017

Member

IMO requiring mut here is inconsistent with other parts of the language:

let x = &mut 5; // `x` is `&mut`
let (x, y, z) = (&mut 1, &mut 2, &mut 3); // `x, y, z` are all `&mut`
for i in &mut vec![1, 2, 3] { ... } // `i` is `&mut`

It's probably worth discussing if we want to interpret the above examples differently in a new epoch/version, but for the time being, I think it's better to stay consistent.

I personally don't think the mixed &mut/& case is very compelling-- I can't imagine a scenario where it would cause a change in behavior to mark some bindings as immutable (where they would have otherwise defaulted to &mut). Marking one or two references as immutable seems like more of a defensive move to ensure you're not mutating some items. It'd be nice to enable this (as it aligns with general best practices WRT "immutable by default") but, as I said above, I don't think it's consistent with our current handling of mutable references.

Member

cramertj commented Aug 16, 2017

IMO requiring mut here is inconsistent with other parts of the language:

let x = &mut 5; // `x` is `&mut`
let (x, y, z) = (&mut 1, &mut 2, &mut 3); // `x, y, z` are all `&mut`
for i in &mut vec![1, 2, 3] { ... } // `i` is `&mut`

It's probably worth discussing if we want to interpret the above examples differently in a new epoch/version, but for the time being, I think it's better to stay consistent.

I personally don't think the mixed &mut/& case is very compelling-- I can't imagine a scenario where it would cause a change in behavior to mark some bindings as immutable (where they would have otherwise defaulted to &mut). Marking one or two references as immutable seems like more of a defensive move to ensure you're not mutating some items. It'd be nice to enable this (as it aligns with general best practices WRT "immutable by default") but, as I said above, I don't think it's consistent with our current handling of mutable references.

@burdges

This comment has been minimized.

Show comment
Hide comment
@burdges

burdges Aug 16, 2017

I think this RFC creates only the second place where rustc plus std produce a named &mut binding without either some composite data type involving a &mut or some function returning one.

There is a convention throughout most of std that _mut appear on functions that directly produce such objects. I believe the only current exception are the impl<'a, T> IntoIterator for &'a mut F<T> where F is [], etc. You need iter_mut() if you want an Iterator<Item=&mut ..> but do not have &mut [T] or [&mut T] or similar. See: http://play.integer32.com/?gist=8a3899b7ab57fee0198755b04ecb6bfc&version=undefined

Avoiding the mut here is only aiming for consistency with those IntoIterators for &muts. It's looks incomparable to anything rustc itself requires.

burdges commented Aug 16, 2017

I think this RFC creates only the second place where rustc plus std produce a named &mut binding without either some composite data type involving a &mut or some function returning one.

There is a convention throughout most of std that _mut appear on functions that directly produce such objects. I believe the only current exception are the impl<'a, T> IntoIterator for &'a mut F<T> where F is [], etc. You need iter_mut() if you want an Iterator<Item=&mut ..> but do not have &mut [T] or [&mut T] or similar. See: http://play.integer32.com/?gist=8a3899b7ab57fee0198755b04ecb6bfc&version=undefined

Avoiding the mut here is only aiming for consistency with those IntoIterators for &muts. It's looks incomparable to anything rustc itself requires.

@phaylon

This comment has been minimized.

Show comment
Hide comment
@phaylon

phaylon Sep 20, 2017

I believe the ability to bind parts of &mut references as immutable is valuable enough to at least keep it around. Having to do explicit reborrows to protect myself is easy to forget with the result being mutation being able to happen, compared to forgetting a mut with the result of the compiler not allowing mutation.

The big reduction in ergonomics for me without this feature would be that instead of a pattern arm like Foo { ref a, mut ref b } => b.perform(a) I'd have to Foo { a, b } => { let a = &*a; b.perform(a) } to get the same benefits.

phaylon commented Sep 20, 2017

I believe the ability to bind parts of &mut references as immutable is valuable enough to at least keep it around. Having to do explicit reborrows to protect myself is easy to forget with the result being mutation being able to happen, compared to forgetting a mut with the result of the compiler not allowing mutation.

The big reduction in ergonomics for me without this feature would be that instead of a pattern arm like Foo { ref a, mut ref b } => b.perform(a) I'd have to Foo { a, b } => { let a = &*a; b.perform(a) } to get the same benefits.

@glaebhoerl

This comment has been minimized.

Show comment
Hide comment
@glaebhoerl

glaebhoerl Sep 20, 2017

Contributor

I can recognize the value there; at the same time, I also feel inclined to ask: if the language had started out with "default binding modes", and without the ability to express that, would we consider it worthwhile to add ref and ref mut to the language in order to cover that use case? Does the feature carry its weight?

Contributor

glaebhoerl commented Sep 20, 2017

I can recognize the value there; at the same time, I also feel inclined to ask: if the language had started out with "default binding modes", and without the ability to express that, would we consider it worthwhile to add ref and ref mut to the language in order to cover that use case? Does the feature carry its weight?

@phaylon

This comment has been minimized.

Show comment
Hide comment
@phaylon

phaylon Sep 20, 2017

@glaebhoerl Yes, I would. Similar to if let were always mut I'd welcome an immutable/mutable separation, with immutable being the default.

phaylon commented Sep 20, 2017

@glaebhoerl Yes, I would. Similar to if let were always mut I'd welcome an immutable/mutable separation, with immutable being the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment