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 constants in patterns #32199

Merged
merged 10 commits into from
Mar 26, 2016

Conversation

nikomatsakis
Copy link
Contributor

This implements RFC 1445. 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:

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):

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):

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 #1445 for more details.

cc #31434

r? @pnkfelix

@nikomatsakis
Copy link
Contributor Author

cc @alexcrichton @huonw -- I'd like to get some eyes on the commit "do not overwrite spans as eagerly" in particular, because I am really unsure if what I am doing there makes sense. Not sure who is best to cc -- the logic there that I am modifying is indeed very old.

@alexcrichton
Copy link
Member

Oh dear, anyone else's guess is as good as mine as to what's going on there... I basically have no idea :(

Some other random thoughts though:

  • The warnings/errors here are going through lines, which means that by default I won't get warned about my dependencies that are using this (due to Cargo's use of --cap-lints=allow). Other "this will break in the future" warnings in the past have gone through unconditional warnings (so we see it in dependencies), and maybe we want to do that here as well?
  • Having not read the RFC too closely, nor followed the discussion much, this may already be taken care of (but I didn't see a test). I'm curious what happens here?
#[derive(Eq)]
struct A { a: i32 }

impl A {
    fn eq(&self, other: &A) -> bool { true }
}

#[derive(Eq, PartialEq)]
struct B { a: A }

const CONST_B1: B = B { a: A { a: 1 } };
const CONST_B2: B = B { a: A { a: 2 } };

fn main() {
    match CONST_B1 {
        CONST_B2 => println!("b2"),
        CONST_B1 => println!("b1"),
        _ => {}
     }
}

By equality, in theory that should print b2, but it sounds like the way match is implemented it'll print b1?

(if this is covered in the RFC or discussions, please disregard me!)

};
debug!("new_span({:?}) = {:?}", sp, sp1);
if sp.expn_id.into_u32() == 0 && env::var_os("NDM").is_some() {
panic!("NDM");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, oops -- that was a debugging remnant, obviously :) should have left a // TODO, like I usually do

@nikomatsakis
Copy link
Contributor Author

@alexcrichton

The warnings/errors here are going through lines, which means that by default I won't get warned about my dependencies that are using this (due to Cargo's use of --cap-lints=allow). Other "this will break in the future" warnings in the past have gone through unconditional warnings (so we see it in dependencies), and maybe we want to do that here as well?

This is not so -- all of our "this will break in the future" code goes through lints now, precisely so that it plays nicely with -A cap-lints. That said, this is a special category of lint, so if you wanted to enable some special behavior (like: prints unconditionally) we could do that.

By equality, in theory that should print b2, but it sounds like the way match is implemented it'll print b1?

Heh, good point. I probably about to require that both PartialEq and Eq are derived.

@bors
Copy link
Contributor

bors commented Mar 14, 2016

☔ The latest upstream changes (presumably #30587) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -24,6 +24,7 @@
#![feature(str_char)]

extern crate fmt_macros;
#[macro_use] extern crate log;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs corresponding dependency edges in both Makefiles and rustbuild.

@pnkfelix
Copy link
Member

(I used to be able to leave comments on individual commits to mark them as "lgtm" while I did an incremental review of a PR... but it seems like github may have removed this capability ... so I will leave comments in the PR comment thread that reference the commit's via their hash instead...)

Update: I realized after the fact that leaving comments in this thread wont accomplish the main goal, which was to use each commits marker of the little "i have comments" icon as a way of track progress. So maybe I'll try something else.

@pnkfelix
Copy link
Member

commits lgtm:

  • bc8945072208033cd19ce407fbcc54d8d8de918e
  • 01b45e831ba3159cebf63738efbbe3d147f1eb1c (though nagisa's note about the makefiles does need to be addressed)
  • 39997ec59adf6eae2a44b2e86c4df5b1935d803b (though arielb1's note obviously needed to be addressed)
  • 4ed617f0d35840c9d255ca895f5ba3412e346d61

if self == NEG_INFINITY {
NEG_INFINITY
} else {
(self + ((self * self) + 1.0).sqrt()).ln()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I personally would have left this as a match and just changed its first clause to x if x == NEG_INFINITY => x)

@pnkfelix
Copy link
Member

@nikomatsakis okay PR looks good; r=me after you address notes above (including the bit about PartialEq) and rebase.

to careful use of the span from deriving, we
can permit it in stable code if it derives from
deriving (not-even-a-pun intended)
this was required to preserve the span from
the #[structural_match] attribute -- but honestly
I am not 100% sure if it makes sense.
This is a [breaking-change]: according to RFC rust-lang#1445, constants used as
patterns must be of a type that *derives* `Eq`. 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.
@nikomatsakis nikomatsakis force-pushed the limiting-constants-in-patterns-2 branch from afc4c70 to 93e4443 Compare March 25, 2016 14:05
@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 25, 2016

📌 Commit 93e4443 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 25, 2016

⌛ Testing commit 93e4443 with merge 4b59084...

@bors
Copy link
Contributor

bors commented Mar 25, 2016

💔 Test failed - auto-linux-64-x-armhf

@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 25, 2016

📌 Commit 944dc4a has been approved by pnkfelix

@nikomatsakis
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 25, 2016

📌 Commit 2536ae5 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 25, 2016

⌛ Testing commit 2536ae5 with merge 49e312b...

@bors
Copy link
Contributor

bors commented Mar 25, 2016

⛄ The build was interrupted to prioritize another pull request.

Manishearth added a commit to Manishearth/rust that referenced this pull request 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
bors added a commit that referenced this pull request Mar 26, 2016
Manishearth added a commit to Manishearth/rust that referenced this pull request 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
bors added a commit that referenced this pull request Mar 26, 2016
Rollup of 11 pull requests

- Successful merges: #32131, #32199, #32257, #32325, #32435, #32447, #32448, #32456, #32469, #32476, #32482
- Failed merges: #32240
@bors bors merged commit 2536ae5 into rust-lang:master Mar 26, 2016
@nikomatsakis nikomatsakis deleted the limiting-constants-in-patterns-2 branch March 30, 2016 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants