remove 'illegal_floating_point_literal_pattern' future-compat lint

[RalfJung]

In #84045 the lang team rejected making this a hard error. So clearly we shouldn't have a lint saying "this will be a hard error in the future".

TODO:

• Remove the references to the lint in https://github.com/rust-lang/reference/blob/master/src/patterns.md

Fixes #41620

[rustbot]

r? @WaffleLapkin

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

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

[Centri3]

rust-lang/rust-clippy#11305 should be reverted if this lint is removed.

I haven't really read up on why this won't be a hard error, but would this have a place in clippy, perhaps as a subset of the current lint? I do personally find matching on floats a bit peculiar, albeit prettier, but I have the same issues with == as well 🤔

Are there any use cases for such a clippy lint?

[Centri3]

Now that I think about it, float_cmp already exists, but doesn't catch this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f095e0b4d52d956c872b9d11410d9501

This would probably be a good enhancement to it, suggesting a guard instead

[ehuss]

Can you also open a PR to the reference to remove the two references to 41620 in https://github.com/rust-lang/reference/blob/master/src/patterns.md?

[RalfJung]

rustc does not currently lint against '==' on floats, so I don't think it makes sense to lint against matching on floats in general. I did make sure that the NaN comparison lint works for match as well. I don't really have an opinion on what Clippy should do.

[WaffleLapkin]

@RalfJung #84045 was 2 years ago, do we perhaps need to re-nominate this for t-lang discussion?

Personally I do find existing behavior very confusing, error prone and wrong, although t-lang might disagree... I've read the discussion there and don't see justification for the current behavior...

[RalfJung]

The current behavior is "behave like ==". What's confusing/wrong about that?

I agree that this PR needs to go through t-lang discussion.

[RalfJung]

I tried to keep this the same as the other copies of the file, but the clippy test is somehow different, ad that doesn't work. It says rustfix failed. I didn't see any other useful information, I just did random mutation of the test file until the clippy test suite passed...

[flip1995]

Looks fine to me. The purpose of this test is to check the expect attribute, which lint the test expects shouldn't really matter.

[RalfJung]

@rust-lang/lang in #84045 you decided to not turn "match on float" into a hard error. However, we kept emitting a lint which said that we would eventually make this a hard error. Clearly that's not consistent, so this PR proposes to remove the lint.

However, there's not a ton of rationale discussed in that thread. We have people saying they are matching on floats and see no reason why that would be rejected; we have the point that == works on floats and has all the same issues so why would we complain only about match-based comparison.

OTOH, RFC 1445 is fairly clear about rejecting floats in patterns due to them being "not structural" (e.g., 0.0 and -0.0 compare equal but can make a difference when used with copysign).

So would be good to see if you are still feeling the same way about making this a hard error or not, and why. I don't personally have a preference either way, I just don't want to have a lint that says "will be made a hard error" when at the same time a PR that makes it a hard error gets rejected saying "actually we don't want to make this a hard error". :)

[WaffleLapkin]

The current behavior is "behave like ==". What's confusing/wrong about that?

For reference, I've answered this in zulip: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/matching.20and.20.60Eq.60/near/392838566

OTOH, RFC 1445 is fairly clear about rejecting floats in patterns due to them being "not structural" (e.g., 0.0 and -0.0 compare equal but can make a difference when used with copysign).

(emph. mine)
Indeed this is my main point 😅

[...], I just don't want to have a lint that says "will be made a hard error" when at the same time a PR that makes it a hard error gets rejected saying "actually we don't want to make this a hard error". :)

Totally agree, we should make some decision here :)

[bors]

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

[WaffleLapkin]

Marking as S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). , since this mainly awaits t-lang opinion on whatever we should do.

[RalfJung]

The proposed design meeting rust-lang/lang-team#220 is also relevant (though all of the designs proposed there can make floats work, some might want to restrict matching to non-NaN floats or something like that).

[pitaj]

The following is based on my zulip message, where Ralf asked me to expand on those thoughts here.

f32::NaN == f32::NaN must be false for two reasons:

• We explicitly define it as "some NaN" not "this specific NaN". Equality between two undefined values is undefined (which gets mapped to false in PartialEq terms)
• It was decided that PartialEq and PartialOrd for floats follow the IEEE standard for comparison

Neither must apply to match semantics, because match can compare set-wise. We can express v ∈ S with match, but not with PartialEq or PartialOrd alone (both operate on values).

Match maps a value to a branch based on an ordered list of set specifiers (patterns): sometimes the set is a single value, multiple discrete values, a range, a union of those, etc.

That's all to say that pattern matching and equivalence are quite different concepts and don't need to behave identically. In fact, sometimes we want or need them to behave differently in order to achieve the desired PartialEq semantics for a type. In my opinion, Rust should not mix these concepts.

In my opinion, the ideal version of float matching would be:

match some_float {
    f32::INFINITY => "positive infinity",
    f32::NEG_INFINITY => "negative infinity",
    +0.0 => "positive zero",
    -0.0 => "negative zero",
    f32::MIN_POSITIVE.. => "positive normal",
    ..=-f32::MIN_POSITIVE => "negative normal",
    +0.0..f32::MIN_POSITIVE => "positive subnormal",
    -f32::MIN_POSITIVE..-0.0 => "negative subnormal",
    _ => "not a number",
}

Just 0.0 would match both signs of zero (equivalent to the pattern -0.0 | +0.0).

And forbid matching individual NaN values:

// error: matching on individual NaN values is not allowed
f32::NAN => "this particular NaN",
// error: matching on individual NaN values is not allowed
f32::NAN..=f32::NAN => "this particular NaN",

One hitch with the above is that float matching currently uses PartialEq semantics, so -0.0 matches both signs of zero. This may not be a breaking change in practice (a crater run may decide that). But if it must be avoided, then one possible workaround is the following:

• add f32::NEG_ZERO = -0.0
• allow f32::NEG_ZERO to be used in patterns and match only negative zero
• -0.0 continues to mean the same thing as 0.0
• forbid -0.0 in patterns in the next edition
• several editions later perhaps, re-enable -0.0 matching only negative zero

One possibility for the future is enabling some way to create a pattern that matches NaNs. A few possibilities:

• with "not patterns": not f32::NEG_INFINITY..=f32::INFINITY
• with bounding consts: f32::MIN_NAN..=f32::MAX_NAN | -f32::MAX_NAN..=-f32::MIN_NAN
  And pattern types may compose well to give the "matches any NaN" pattern a name:
  type AnyNaN = f32 is not f32::NEG_INFINITY..=f32::INFINITY

[RalfJung]

Thanks for writing this up!

Neither must apply to match semantics

That's a very strong statement. IMO it is totally reasonable to apply this to match semantics, it just doesn't match your mental model.

[RalfJung]

One hitch with the above is that float matching currently uses PartialEq semantics, so -0.0 matches both signs of zero. This may not be a breaking change in practice (a crater run may decide that). But if it must be avoided, then one possible workaround is the following:

I don't think we ever want to have a situation where there is a const C = <literal> and matching on C vs matching directly on the literal has different behavior.

I also think most people that don't know much about floats (and don't even know that 0 has a sign) would be very surprised if matches!(-1.0f32 * 0.0, 0.0..=1.0) was false. Returning false here is arguably nonsense, clearly -1*0 is contained in the range 0..=1. Expecting false here is a case of "expert syndrome" -- you need to be really deep into the details and quite far removed from the way most people think about floats to arrive at that conclusion.

[RalfJung]

Also marking as "blocked", I think we want to make matching on NaN a hard error (and we've future-compat linted against it for a long time so we should be able to do this reasonably fast).

[pitaj]

That's a very strong statement. IMO it is totally reasonable to apply this to match semantics, it just doesn't match your mental model.

To be clear, I did not mean "both must NOT apply to match semantics" but rather "these don't necessarily have to apply to match semantics as well".

For instance, my proposal does not allow matching on individual NaNs - just like how PartialEq does not find them equal.

I don't think we ever want to have a situation where there is a const C = and matching on C vs matching directly on the literal has different behavior.

I agree that it's not pretty. But under this option, putting -0.0 in a pattern is linted or forbidden period, so you can't/shouldn't match on the literal directly.

I also think most people that don't know much about floats (and don't even know that 0 has a sign) would be very surprised if matches!(-1.0f32 * 0.0, 0.0..=1.0) was false. Returning false here is arguably nonsense, clearly -1*0 is contained in the range 0..=1.

To be clear, the pattern 0.0 in my proposal matches both positive and negative zero for this very reason. Only the patterns -0.0/f32::NEG_ZERO and +0.0 would match specific zeros.

[RalfJung]

To be clear, I did not mean "both must NOT apply to match semantics" but rather "these don't necessarily have to apply to match semantics as well".

Ah I see, thanks for clarifying.

To be clear, the pattern 0.0 in my proposal matches both positive and negative zero for this very reason. Only the patterns -0.0/f32::NEG_ZERO and +0.0 would match specific zeros.

But if I have const ZERO: f32 = 0.0/const ZERO: f32 = +0.0, what it will it do when matching on ZERO or on ZERO ..= 1.0?

Also, you're still taking for granted that 0.0 and +0.0 are not the same thing. That's already a totally outlandish idea if you're not well-versed in the binary representation of floats. If a programmer wrote +0.0 I do not think we can conclude that they meant that to not match -1.0 * 0.0. Nobody will expect matches!(-1.0f32 * 0.0, +0.0) to be different from matches!(-1.0f32 * 0.0, 0.0).

[pitaj]

I think it should eventually be possible to match every float value individually. Positive and negative zero are fundamental different values and will have different divide-by-zero results. So I think being able to match on them separately is valuable.

That said, it's not super important to me. If it's far more practical to have -0.0 match 0.0 and vice-versa, I guess that's fine.

But if I have const ZERO: f32 = 0.0/const ZERO: f32 = +0.0, what it will it do when matching on ZERO or on ZERO ..= 1.0?

I think matching a certain const should only ever match that exact value. So ZERO would only match positive zero, because we say that 0.0 is the literal for positive zero. But I recognize that is also a breaking change. I doubt that changing that will break anything in practice tho.

One benefit of that behavior is that you get the practical benefits of the pattern 0.0 matching both signs of zero, while also being able to match either sign separately with a little more verbosity (and without special syntax like +0.0). It just means special-casing the 0.0 pattern.

That would mean that the pattern is slightly inconsistent from the literal value, but since zero is a special case anyways, and the only case where this happens, I think that's acceptable.

[tmandry]

Un-nominating for now in lieu of the upcoming lang team meeting, rust-lang/lang-team#222.

@rustbot label -I-lang-nominated

[RalfJung]

If we follow the proposal in rust-lang/rfcs#3535, we should first land #116284 and then remove the lint.

[RalfJung]

I have folded this PR into #116284.