Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any old type of constant can be used in a pattern #20489

Closed
nikomatsakis opened this issue Jan 3, 2015 · 15 comments
Closed

Any old type of constant can be used in a pattern #20489

nikomatsakis opened this issue Jan 3, 2015 · 15 comments
Labels
P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Example:

struct Foo<T> {
    value: T
}

const F: Foo<int> = Foo { value: 2 };

fn main() {
    let x = Foo { value: 2 };
    match x {
        F => { println!("Hi"); }
        _ => { println!("Ho"); }
    }
}

I feel pretty sure that we would want matching to use a PartialEq impl. For now constant patterns should be limited to uint/int/&str. I'll throw it into a patch I'm working on.

@Thiez
Copy link
Contributor

Thiez commented Jan 3, 2015

Surely if you wanted the comparison to use PartialEq, you would do

#[deriving(PartialEq)]
struct Foo<T> {
    value: T
}

const F: Foo<int> = Foo { value: 2 };

fn main() {
    let x = Foo { value: 2 };
    match x {
        _ if x == F => { println!("Hi"); }
        _ => { println!("Ho"); }
    }
}

Why remove the possibly useful behaviour of using constants as a pattern?

@ftxqxd
Copy link
Contributor

ftxqxd commented Jan 4, 2015

I was under the impression that consts basically have their values inserted into their use sites. That would make this a perfectly valid example. If we required PartialEq, then we might as well allow statics in patterns as well. I certainly don’t think we should implicitly run PartialEq::eq when pattern matching on constants, so it seems strange to require PartialEq to be implemented if the implementation’s never going to be used.

@nikomatsakis
Copy link
Contributor Author

So, all I'd like to do for this bug is to ensure that we limit the types of such constants to scalars (and maybe tuples of scalars), which I'm sure we can all agree ought to work. We can hash out our disagreements about what the correct semantics ought to be over an RFC, I imagine.

But for the record:

  1. I think that the current system breaks encapsulation. The fields of that structure are private, we shouldn't implicitly accessing and comparing them. We don't know what "equality" means for this struct.
  2. Put another way, I think that match foo { CONSTANT => { ... } } and match foo { _ if foo == CONSTANT => { ... } } ought to be equivalent.
  3. As a consequence, I do think we should invoke PartialEq from within the match code, which is why I would want to require that it be implemented.

@pythonesque
Copy link
Contributor

I really can't agree on this. I don't expect pattern matching to run arbitrary code (which is what running the PartialEq implementation would do). If the two things in your second bullet are supposed to be equivalent, I don't see why matching on a constant should even be allowed, since it doesn't provide any additional functionality over pattern guards.

@nikomatsakis
Copy link
Contributor Author

It appears this bug was closed accidentally. I still think this is something we'll have to resolve -- right now, you can match on types that don't implement PartialEq at all, and if they do implement it, the match code semantics do not align with what PartialEq implements. This seems bad. Having match x { FOO => ... } and match x { y if x == FOO => } both be accepted but different in subtle ways seems clearly suboptimal to me.

@nikomatsakis nikomatsakis reopened this Jul 17, 2015
@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 17, 2015
@nikomatsakis
Copy link
Contributor Author

I'll note that in the MIR desugaring branch this becomes much easier to handle.

@nikomatsakis
Copy link
Contributor Author

triage: P-medium

@nrc nrc added P-medium Medium priority and removed I-nominated labels Jul 31, 2015
@rust-highfive rust-highfive added the P-medium Medium priority label Jul 31, 2015
@bluss
Copy link
Member

bluss commented Oct 12, 2015

Pattern matching is always structural (as I know it from both Rust and other languages), this bug report is very surprising to me.

@petrochenkov
Copy link
Contributor

I really can't agree on this.

+1

Pattern matching is always structural (as I know it from both Rust and other languages), this bug report is very surprising to me.

+1

@nikomatsakis
AFAIR, it was your own idea to treat

strust S;

as

struct S {}
const S: S = S {}

Given that interpretation, in the next example S and None are not structure patterns, but constants of types S and Option<u8>.

struct S;

enum Option<T> {
    Some(T),
    None,
}

let s = S;
match s {
    S => {}
}

let opt = Some(0u8);
match opt {
    None => {}
    _ => {}
}

Should this matching involve PartialEq? No way! It means that these constants should be somehow special-cased and the interpretation above doesn't really hold. (Not mentioning the fact, that no one expects PartialEq to be run in matches, as @bluss mentioned)

@Thiez
Copy link
Contributor

Thiez commented Oct 24, 2015

I have a hard time imagining how one would even implement PartialEq for types such as Option without being able to use match.

@nikomatsakis
Copy link
Contributor Author

@petrochenkov yeah, but I no longer stand by that idea. I think it's a good guiding principle, but there is a definite distinction between an enum variant and a constant like:

struct Foo { x: i32 }
const fn make_foo(x: i32) -> Foo { Foo { x: x } }
...
const EXAMPLE: Foo = make_foo(22);
...
match something {
    EXAMPLE => ...
}

what we currently do now in cases like these is to treat that final match as if you wrote:

match something {
    Foo { x: 22 } => ...
}

This effectively introduces two forms of equality: normal equality (==) and structural-equality (match). I don't think this is a good idea. If you match on a constant (note: not an enum variant), I think it should have the same semantics as == (if both are accepted). Note that I think it might be ok to have it be an error to match on a constant (e.g., Haskell does not accept this, iiuc, as requires you to write match foo { x if x == EXAMPLE => instead).

In particular, there is something about taking a constant that is computed through an arbitrary complex compile-time expression and "boiling it down" to the particular result that feels like a violation of abstraction boundaries. Let me give you another example. Imagine I wrote:

mod foo {
    pub struct Foo { b: bool }
    pub const V1: Foo = Foo { b: true };
    pub const V2: Foo = Foo { b: false };
}

Note that there is an abstraction boundary here: b is a private field. But now if I wrote code from another module that matches on a value of type Foo, that abstraction boundary is pierced:

fn bar(f: x::Foo) {
    // rustc knows this is exhaustive because if expanded `V1` into equivalent
    // patterns; patterns you could not write by hand!
    match f {
        x::V1 => { }
        x::V2 => { }
    }
}

This seems bad to me. Now if I add new fields to Foo, or perhaps change that b field to an i32, suddenly those examples stop compiling.

(On play: http://is.gd/exTrhs)

Anyway, it seems clear that this topic is controversial enough that it merits a wider audience. I've been planning for some time (months now!) to write an RFC. Things are coming to a head with the MIR rewrite, which requires us to reimplement pattern matching anyhow, and thus makes a good chance to make sure it is doing what we want it to.

@petrochenkov
Copy link
Contributor

@nikomatsakis
Yesterday I almost wrote about prohibiting matching on constants with types containing private fields (possibly nested), but stopped after realizing that matching on None::<StructWithPrivateFields> would be prohibited as well.
But it is apparently solvable by a small tweak - instead of prohibiting private fields in constant types, we can prohibit them in constant values. This way a "matchable" constant is completely "open" - it's just a shortcut for a struct literal expression and abstraction boundaries are not violated. Also, enum-variant-constants never contain private fields, so the mental model about unit structs/variants and constants stays in place.

@arielb1
Copy link
Contributor

arielb1 commented Oct 26, 2015

+1 for "ban constants that assign a value to a private field". Maybe even make matching against NaN do the right thing.

@nikomatsakis
Copy link
Contributor Author

I opened a discussion thread here: https://internals.rust-lang.org/t/how-to-handle-pattern-matching-on-constants/2846 I also tried to lay out the pros/cons as I see them. Please add additional considerations I forgot about!

@brson
Copy link
Contributor

brson commented Jul 14, 2016

Closing in favor of #31434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants