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

Restrict use of constants in patterns (RFC 1445) #31434

Closed
4 of 6 tasks
nikomatsakis opened this issue Feb 5, 2016 · 17 comments
Closed
4 of 6 tasks

Restrict use of constants in patterns (RFC 1445) #31434

nikomatsakis opened this issue Feb 5, 2016 · 17 comments
Assignees
Labels
A-patterns Relating to patterns and pattern matching B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-design-concerns Status: There are blocking design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 5, 2016

Tracking issue for rust-lang/rfcs#1445.

Implementation steps:

@nikomatsakis nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 5, 2016
@nikomatsakis nikomatsakis self-assigned this Feb 5, 2016
@durka
Copy link
Contributor

durka commented Feb 5, 2016

Why #[derive(Eq)] and not #[derive(PartialEq)]? I see this was brought up in the RFC thread but I didn't really see any reasons given.

@nikomatsakis
Copy link
Contributor Author

See the PR #32199

@nikomatsakis
Copy link
Contributor Author

@durka

Why #[derive(Eq)] and not #[derive(PartialEq)]? I see this was brought up in the RFC thread but I didn't really see any reasons given.

The primary reason is that types which only implement PartialEq are not really compatible with a "structural" interpretation. For example, imagine you have a match that tests against the floating point value NaN -- should this match, or not? Consider that NaN != NaN (and hence floating point types are not Eq). Currently, we sidestep this by disallowing a constant of NaN, but we won't be able to do that in the future. And there are other weird examples. Consider 0 vs -0 -- these are distinct values (different bitpatterns) and yet they compare equal. So should match foo { 0 => ... } trigger if foo is -0? Today, it does, but that is a special case for floating point values, and we don't afford user-defined types the same courtesy (that is, we use a strict structural match for user-defined types).

If we wind up adopting a "semantic" interpretation, then we could consider loosening to PartialEq.

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 26, 2016
…patterns-2, r=pnkfelix

Restrict constants in patterns

This implements [RFC 1445](https://github.com/rust-lang/rfcs/blob/master/text/1445-restrict-constants-in-patterns.md). The primary change is to limit the types of constants used in patterns to those that *derive* `Eq` (note that implementing `Eq` is not sufficient). This has two main effects:

1. Floating point constants are linted, and will eventually be disallowed. This is because floating point constants do not implement `Eq` but only `PartialEq`. This check replaces the existing special case code that aimed to detect the use of `NaN`.
2. Structs and enums must derive `Eq` to be usable within a match.

This is a [breaking-change]: if you encounter a problem, you are most likely using a constant in an expression where the type of the constant is some struct that does not currently implement
`Eq`. Something like the following:

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

The easiest and most future compatible fix is to annotate the type in question with `#[derive(Eq)]` (note that merely *implementing* `Eq` is not enough, it must be *derived*):

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

Another good option is to rewrite the match arm to use an `if` condition (this is also particularly good for floating point types, which implement `PartialEq` but not `Eq`):

```rust
match foo {
    c if c == SOME_CONST => ...
}
```

Finally, a third alternative is to tag the type with `#[structural_match]`; but this is not recommended, as the attribute is never expected to be stabilized. Please see RFC rust-lang#1445 for more details.

cc rust-lang#31434

r? @pnkfelix
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 26, 2016
…patterns-2, r=pnkfelix

Restrict constants in patterns

This implements [RFC 1445](https://github.com/rust-lang/rfcs/blob/master/text/1445-restrict-constants-in-patterns.md). The primary change is to limit the types of constants used in patterns to those that *derive* `Eq` (note that implementing `Eq` is not sufficient). This has two main effects:

1. Floating point constants are linted, and will eventually be disallowed. This is because floating point constants do not implement `Eq` but only `PartialEq`. This check replaces the existing special case code that aimed to detect the use of `NaN`.
2. Structs and enums must derive `Eq` to be usable within a match.

This is a [breaking-change]: if you encounter a problem, you are most likely using a constant in an expression where the type of the constant is some struct that does not currently implement
`Eq`. Something like the following:

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

The easiest and most future compatible fix is to annotate the type in question with `#[derive(Eq)]` (note that merely *implementing* `Eq` is not enough, it must be *derived*):

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

Another good option is to rewrite the match arm to use an `if` condition (this is also particularly good for floating point types, which implement `PartialEq` but not `Eq`):

```rust
match foo {
    c if c == SOME_CONST => ...
}
```

Finally, a third alternative is to tag the type with `#[structural_match]`; but this is not recommended, as the attribute is never expected to be stabilized. Please see RFC rust-lang#1445 for more details.

cc rust-lang#31434

r? @pnkfelix
@brson
Copy link
Contributor

brson commented Jul 14, 2016

Nominating for status update.

@brson brson removed the I-nominated label Jul 14, 2016
@brson
Copy link
Contributor

brson commented Jul 14, 2016

@nikomatsakis status update?

@nikomatsakis
Copy link
Contributor Author

@brson this is basically implemented, except that I don't think I did anything clever for the exhaustiveness check. I updated the check marks. The semantics are still (in my mind) basically a kind of temporary hack.

@Mark-Simulacrum Mark-Simulacrum added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jul 24, 2017
@Centril
Copy link
Contributor

Centril commented Sep 15, 2018

Triage: @nikomatsakis any movement here?

@jethrogb
Copy link
Contributor

jethrogb commented Jul 12, 2019

Came here via #62339/#62411/#62614

use std::rc::Rc;
use std::error::Error;

const OK: Result<(), Rc<dyn Error>> = Ok(());

fn main() {
    let is_ok = match OK {
        OK => true,
        _ => false,
    };
}

On the current nightly this complains about not being able to structurally match Rc. I'm never doing that. I feel like this code should just work (as it does on stable)?

@Aaron1011
Copy link
Member

What is still left to be done for this issue?

@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

What is still left to be done for this issue?

@crlf0710
Copy link
Member

#63438 is closed. However on rustdoc page it seems the primitive types are missing...: https://doc.rust-lang.org/nightly/std/marker/trait.StructuralEq.html

@edwardw
Copy link
Contributor

edwardw commented Dec 28, 2019

One original goal of RFC 1445 is to address the weakened abstraction boundary. But looks like it doesn't consider the leak through error messages.

mod state {
    use std::borrow::Cow;

    #[derive(PartialEq, Eq)]
    pub struct State(Cow<'static, str>);
    impl State {
        pub fn new(s: &'static str) -> Self {
            State(Cow::Borrowed(s))
        }
    }

    pub const COLD: State = State(Cow::Borrowed("cold"));
}

fn main() {
    let s = state::State::new("hot");
    match s {
        state::COLD => (),
        _ => (),
    }
}

The error message:

error: to use a constant of type std::borrow::Cow in a pattern, std::borrow::Cow must be annotated with #[derive(PartialEq, Eq)]

exposes the fact that std::borrow::Cow is used under the hood, which should be mere implementation detail that doesn't concern the user. Diagnostically it is also confusing.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 14, 2020

CTFE forbids comparing pointers for equality, since two pointers being unequal may arbitrarily change to the two pointers being equal depending on various factors (e.g. different constants sharing the same memory or not depending on whether they were instantiated in the same crate).

Should we thus also exclude raw pointers from #[structural_match]?

@TheNewJavaman
Copy link

I apologize if I missed something, but why isn't this trait implemented for CStr? It seems to be implemented for CString just fine

@RalfJung
Copy link
Member

Which trait?
Please open a new issue with example code demonstrating what you want to do and what doesn't work.

@RalfJung
Copy link
Member

For this issue, is there anything left to do here? I think it is entirely superseded by #120362.

@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

Closing in favor of #120362.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-patterns Relating to patterns and pattern matching B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-design-concerns Status: There are blocking design concerns. 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