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

Implement the illegal_floating_point_literal_pattern compat lint #41293

Merged
merged 3 commits into from
May 9, 2017

Conversation

est31
Copy link
Member

@est31 est31 commented Apr 14, 2017

Adds a future-compatibility lint for the [breaking-change] introduced by issue #41620 . cc issue #41255 .

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@est31 est31 changed the title Floating literal match Disallow floating point literals in match Apr 14, 2017
@bors
Copy link
Contributor

bors commented Apr 14, 2017

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

@nikomatsakis nikomatsakis added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 14, 2017
@nikomatsakis
Copy link
Contributor

I'll spin up a crater run. Thanks @est31!

format!("floating point constants cannot be used in patterns"));
}
_ => {}
}
match const_cx.eval(expr) {
Copy link
Member

Choose a reason for hiding this comment

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

This branch may be dead now.


match 0.0 {
0.0 ... FOO => (),
0.0 ... FOOFL => (),
Copy link
Member

Choose a reason for hiding this comment

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

This, i feel, should keep working just fine. It is just a range and testing that floats fall into some range is encouraged even.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm neutral about ranges, will adopt whatever the consensus is.

@@ -29,7 +29,7 @@ fn is_zero(v: Value) -> bool {
unsafe {
match v {
Value { tag: Tag::I, u: U { i: 0 } } => true,
Value { tag: Tag::F, u: U { f: 0.0 } } => true,
Value { tag: Tag::F, u: U { u: 0 } } => true,
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here. This code is already annotated with unsafe and all.

Copy link
Member

Choose a reason for hiding this comment

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

Although i guess one could argue that people should just match on the bit pattern (i.e. The integer variant)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is annotated with unsafe, but unsafe doesn't allow everything, so IMO its okay to disallow it. Especially as making unsafe feel more comfortable to write than non unsafe code is not really a priority of Rust.

@shepmaster shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 14, 2017
@nikomatsakis
Copy link
Contributor

Crater report: https://gist.github.com/nikomatsakis/b482d33f058a8dbccc6a0def6545ed7f

  • 8600 crates tested: 6541 working / 1929 broken / 35 regressed / 11 fixed / 84 unknown.

But the 11 fixed sounds a bit suspicious. I haven't looked in detail at the results, could well be a lot of false positives etc.

@leoyvens
Copy link
Contributor

leoyvens commented Apr 15, 2017

I count 15 legit regressions out of the 35. Those are:
fann-0.1.6 - Use of -1.0.
libstat-0.1.1 - Use of 0..
image-0.12.3 - Use of 0.0.
tfidf-0.3.0 - Use of 0.0f32.
clippy-lints-0.0.123 - Use of let Ok(0.0) = foo.
wankel-0.0.1 - Use of 0.0 and 1.0.
cargo-incremental-0.1.13 - Use of 0.0.
geodate-0.2.1 - Big user of matching float ranges, 22 errors in total.
capnpc-0.8.5 - Use of 0.0.
liquid-0.9.1 - Use of 1f32.
humansize-1.0.1 - Use of 0.0.
rand-0.3.15 - Use of 0.0 ... 1.0 range.
probability-0.15.6 - Use of 1.0.
rust-3d-0.5.4 - Use of 0.0.
basichll-0.3.1 -Use of 0.0.

@arielb1 arielb1 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 15, 2017
@nikomatsakis
Copy link
Contributor

@leodasvacas thanks for that! The majority, then, are "equality" matches, right?

@nikomatsakis
Copy link
Contributor

i.e., as opposed to 0.0 ... 1.0-style ranges -- although I guess those have an equality component to them too.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang -- note that @est31 has kindly quantified the breakage here. I'm not sure how I feel about this. In some ways, I'd like to permit matches over floating points, which clearly have some utility, but definitely it raises some thorny questions (e.g., matching on 0.0 -- what should happen with -0?). (Once we have matching around associated constants and the like, we won't be able to rule out the ambiguous cases like NaN or 0.0 statically.)

Still, I kind of feel like maybe we should carve out special-case rules here around floats and deal with it, even if that introduces a measure of inconsistency. I guess we have to think about how this may interact with constant evaluation in general.

@leoyvens
Copy link
Contributor

@nikomatsakis Yes, I think only geodate and rand used ranges.

@aidanhs
Copy link
Member

aidanhs commented Apr 18, 2017

Just throwing out that this will also regress ayzim which matches on 0f64 and 1f64 (I really need to keep that updated so I benefit from crater runs).

@nagisa
Copy link
Member

nagisa commented Apr 24, 2017

The crater was run and the results posted, so removing the needs-crater tag.

@nagisa nagisa removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 24, 2017
@aturon
Copy link
Member

aturon commented Apr 25, 2017

Nominating for discussion in team meeting.

@nikomatsakis nikomatsakis removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 27, 2017
@nikomatsakis
Copy link
Contributor

@est31 after some discussion in the @rust-lang/lang meeting, I think we decided we'd like to start issuing warnings (not deny-by-default) for this case. For that, we'll need a separate tracking issue etc. Do you think you can update this PR to do that?

@withoutboats
Copy link
Contributor

withoutboats commented May 4, 2017

It seems like we should have the same rules for now (no floats) and if its a problem we can expedite finding a permanent solution.

Imagining that we allow float consts in ranges, if we use PartialOrd NaN doesn't fall into NaN ... NaN, right? I guess you wouldn't be able to construct that range because its ends wouldn't be properly ordered?

These can all be transformed to conditionals right? Not saying its nice but it does work.

@nikomatsakis
Copy link
Contributor

@withoutboats

Imagining that we allow float consts in ranges, if we use PartialOrd NaN doesn't fall into NaN ... NaN, right?

Yes. The main question is, is there any reason that this is bad? (I don't see one.)

@withoutboats
Copy link
Contributor

withoutboats commented May 4, 2017

just that float equality is a foot gun. I don't really have a strong opinion about what we do here. I guess procedurally I'm a bit worried we are capriciously reversing course on an RFC.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 4, 2017

@withoutboats

Well, I don't think the RFC addressed range literals at all. The focus was purely on equality and nobody brought up ... patterns. I guess the main question is whether one should be able to expect that C...C is the same as C.

(The RFC also reported 6 regressions, because it was based on flawed data, since literals were accidentally overlooked.)

(My view is that in such cases, procedurally, the @rust-lang/lang team can make minor adjustments, within the spirit of the original consensus. If you don't feel this is such a case, I'd not necessarily be opposed to issuing the warnings, and perhaps opening another RFC to explicitly allow .. and ... patterns for floating point types -- and maybe any PartialOrd type?)

@withoutboats
Copy link
Contributor

I guess the main question is whether one should be able to expect that C...C is the same as C.

A misguided user might also forget to handle NaN and write something like:

match x {
    0.0 ... INF => { ... }
    NEG_INF ... 0.0 => { ... }
    _ => unreachable!()
}

Of course depending on the context panicking might be better than whatever would happen if they instead wrote this conditional:

if x >= 0.0 { ... }
else { ... }

@withoutboats
Copy link
Contributor

@nikomatsakis I guess one thing I do feel kind of strongly about is that we have the same policy for floats in ranges as we do for match in general (to make this easier to teach), so if we're going to allow those I'd prefer to reach a decision on all floats.

@carols10cents
Copy link
Member

What are the next steps on this PR? @withoutboats @nikomatsakis

@nikomatsakis
Copy link
Contributor

I think we should go forward with the PR for now. I'm still vaguely unhappy about the whole situation, but warnings seem ok and are a decent way to gain feedback too.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2017

📌 Commit 37fb676 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 8, 2017

⌛ Testing commit 37fb676 with merge 497d628...

@bors
Copy link
Contributor

bors commented May 8, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 9, 2017
…omatsakis

Implement the illegal_floating_point_literal_pattern compat lint

Adds a future-compatibility lint for the [breaking-change] introduced by issue rust-lang#41620 . cc issue rust-lang#41255 .
bors added a commit that referenced this pull request May 9, 2017
Rollup of 8 pull requests

- Successful merges: #41293, #41520, #41827, #41828, #41833, #41836, #41838, #41842
- Failed merges:
@bors
Copy link
Contributor

bors commented May 9, 2017

⌛ Testing commit 37fb676 with merge bedd7da...

@bors bors merged commit 37fb676 into rust-lang:master May 9, 2017
@est31 est31 mentioned this pull request May 10, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
Remove debug message

It was added by me in rust-lang#41293. Sorry about that!

Thanks goes to @alexbool for finding it.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
Remove debug message

It was added by me in rust-lang#41293. Sorry about that!

Thanks goes to @alexbool for finding it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.