Deny float matches

[workingjubilee]

This upgrades the "warn on float literals in patterns" lint tracked in #41620 to "deny". It's been quite enough time since rustc started issuing the warning.

I believe this probably should be run against crater first. Though it's not like it hasn't been an entire edition since the warning went up.

[rust-highfive]

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

[Dylan-DPC-zz]

r? @Mark-Simulacrum

[Mark-Simulacrum]

Nominating for T-lang discussion. I think we may want a crater run to assess to the scope of the breakage here; re-reading the tracking issue #41620 does not suggest to me that we have a particular consensus on this lint.

Some questions I have:

• Does this affect float range patterns, too? (It seems there is some particular contention whether those make sense. It's not really clear to me that they would differ from precise points in the float space from a pattern matching perspective).
• @workingjubilee you argue in Tracking issue for illegal_floating_point_literal_pattern compatibility lint #41620 (comment) that we should be wary of differing behavior due to float parsing. I'm sort of in agreement, but it seems like if we have different float parsing that affects literals regardless of whether they're in a pattern match or in if comparisons, or so.

[workingjubilee]

Allow me to be more precise: Most of the problem is a literal is a literal, and therefore can be interpreted as either an f32 or an f64. Consider the following code:

    let x = 1.0000001;
    assert!(match x {
        1.00000011 => true,
        _ => false,
    });

As is, x is by default implicitly an f64, so the other literal is also read as an f64. This assert fails.
However, if x is annotated as an f32 as

   let x: f32 = 1.0000001;

The other literal is also read as an f32, and the assert succeeds.
This may seem contrived, but I have spoken directly to highly experienced professional programmers who are used to working with numerical code. They literally use raw bitpatterns in Rust, at the extreme cost of legibility, rather than run this sort of risk of depending on the parse of floats.

Now, I was able to get this working, of course:

    let z = 1.00000011;
    let x: f32 = 1.0000001;
    assert!(z == x);

But by contrast, however, I tried to get this working with, say, matching on a const value, or using a const comparison, since match is by all rights against a const value in a pattern and therefore should be compared against that, and what I found was that I kept running into errors like this:

error[E0308]: mismatched types
 --> src/main.rs:5:9
  |
2 |     const Z: f64 = 1.00000011;
  |     -------------------------- constant defined here
3 |     let x: f32 = 1.0000001;
4 |     let y = match x {
  |                   - this expression has type `f32`
5 |         Z => true,
  |         ^
  |         |
  |         expected `f32`, found `f64`

or this:

error[E0308]: mismatched types
 --> src/main.rs:5:9
  |
4 |     assert!(match x {
  |                   - this expression has type `f32`
5 |         const { 1.00000011 } => true,
  |         ^^^^^^^^^^^^^^^^^^^^ expected `f32`, found `f64`

Type errors! Type errors everywhere! There's an obvious (cursed) golden path to successfully misusing a literal here, but it's the kind of thing that is easy to lint against and should instinctively make one's eyebrow raise. Even a slight deviation from that golden path slams the programmer against a wall of problems, which is just enough friction, I would argue, to make it much harder to accidentally introduce this kind of error while refactoring, and much less of a concern in any other context.

As for matches on ranges using float literals, they are included in the same lint, so would also be denied. My feeling is that comparing against values differing by some epsilon is worthwhile but that people should ideally not be working with literal ranges if the ranges are themselves so... fuzzily bounded.

Also, the entire problem with "consts are not functionally equivalent to literals" again rears its head here, when logically they arguably are supposed to be equivalent in patterns, as has been argued before!

[robinbernon]

Please don't merge this change! I've looked through the relevant threads and feel that as long as a compiler warning is thrown with a link to this issue it should be enough for developers to see and acknowledge the risks. It seems from feedback on this issue that a fair amount of people think the same way as I do and want to keep things the way they are. Can this please remain as a compiler warning?

[workingjubilee]

Keeping it as a warning does not mean it requires an acknowledgement. As-is, someone can ignore the warning outright, never reading it, and many developers do use workflows that abstract away the actual compiler output, such as in IDEs. Likewise, upgrading something to deny-by-default does not prevent allowing the lint. By requiring an explicit allowing of the lint instead, a programmer who wishes to use this will be forced to consider whether they truly do understand the risks involved and issue a programmatic acknowledgement. This is the standard the Rust project actually uses when it wants someone to opt-in to something considered a risky interface, e.g. nightly features.

This would not be the case if it was a hard error. A hard error would actually prevent usage. It has been proposed to make this a hard error, but I am not entirely sure I agree with that. I am, however, absolutely confident in upgrading this to deny-by-default.

This is because in order to make floating point literals in patterns make sense, I believe it would require an extensive proposal reconciling them in relation to other evolving features of the Rust language, including StructuralEq / #[structural_match], which these are most in conflict with, but also probably addressing other concerns about floating point in const fn / const eval (which permits match, and thus is an intersecting concern), and including anything that is required to actually settle up Rust with IEEE754-{2008,2019}... which probably will require attention to the dec2float parser.

I genuinely believe that, until such an event has occurred, they should be treated with at least as much forewarning as any other unstable interface in the Rust compiler.

[nikomatsakis]

We discussed this in our @rust-lang/lang meeting on 2021-05-04. The plan was for me to try and write-up a proposal of next steps and float it by the const eval or const generics group.

I have to say that my current inclination is that we should effectively adopt the current behavior around floating point literals (which I believe is that, for example, NaN never matches anything). The behavior seems sensible enough and I wouldn't want to risk existing code. I would consider keeping the warning but never moving it to a hard error; we might also limit it to not apply to range patterns like 1.0 .. 2.0.

I don't think there is any real blocker here. I believe it's forwards compatible with all variants we might use for matching on constants, since floats are a "leaf value" (the disagreement that we had was how to manage matching on constants of aggregate type: should we try to use PartialEq or what). I think at this point I'd be ready to settle that dispute as well (in favor of "unrolling" into match patterns and not using PartialEq at all), but we don't have to settle that to agree on how to manage floats.

However we land on this, I do want to thank @workingjubilee for binding up the issue.

[nikomatsakis]

cc @rust-lang/wg-const-eval and @rust-lang/project-const-generics on this comment

[oli-obk]

(which I believe is that, for example, NaN never matches anything).

as long as that is true (float matching thus essentially being equal to invoking PartialEq), I am in favor of just keeping the current behaviour, and doing as @workingjubilee suggests: deny lint, never hard error, possibly no lint in the future.

[nikomatsakis]

I'm with you right up until deny lint =) I have not yet fully reconciled myself to even the idea of a deny lint, I think, but the idea of having a deny lint that we might possibly remove in the future seems odd. The main reason to have deny lints that I have heard is basically "something that is almost certainly a bug and so obscure that you would want an allow anyway". I guess matching on floats outside of a range might fall into that category -- the other I've hard that seems convincing is overflowing literals like 296_u8.

[workingjubilee]

I do not believe floating point range patterns should be treated specially compared to "singleton" literals because

    let x: f32 = 1.0000001;
    assert!(match x {
        1.00000011..=2.0 => true,
        _ => false,
    });

still flies.

[RalfJung]

This is because in order to make floating point literals in patterns make sense, I believe it would require an extensive proposal reconciling them in relation to other evolving features of the Rust language, including StructuralEq / #[structural_match],

IMO, f32/f64 should not be StructuralEq. Their equality is not just sufficiently well-behaved for that.

This has some direct consequences on their use in patterns:

• Float patterns can never be exhaustive, they always need a _ fallback arm.
• Float pattern tests must behave exactly like PartialEq. (We might actually literally want to translate them to that, in general for all non-SturcturalEq matches and in particular for f32/f64.)

I think this corresponds to the current implementation, but it might be good to check. (This will be easier to do/see when @oli-obk finishes porting the pattern compilation to valtrees.)

On top of all that, I think the question should be whether we want to allow any non-StructuralEq types in matches. The purpose of StructuralEq was to prevent some constants from being used as patterns, which indicates the answer to this question is "no", which to me indicates we should also forbid floats (and function pointers and raw pointers). However, IIRC there's a hole in that StructuralEq restriction (something involving reference types?). If we decide we're okay with that and just allow match on all constants of PartialEq type (with the match behavior being equivalent to PartialEq), then IMO we should do the same with floats.

But I don't think it makes sense to discuss this in isolation, this should be seen in the greater context of the StructuralEq discussion. Cc #74446

(in favor of "unrolling" into match patterns and not using PartialEq at all)

That is only possible for StructuralEq types (and for those types, it is equivalent to using PartialEq, at least under my interpretation of what StructuralEq means). But what about match on non-StructuralEq types?

[nikomatsakis]

@RalfJung I would indeed like to spend time discussing structural equality in detail! Perhaps we could allocate some time to talk this over and draw up a proposal to help move things forward? (ideally, @oli-obk or others like @varkor, @lcnr could join)

[RalfJung]

Yes, sounds good. Some random links related to the subject:

• My notes / plans for a sane future including valtrees: https://hackmd.io/Qvrj_eOFTkCHZrhJ7f1ItA
• Pattern-matching related soundness concerns const-eval#42

Here's how to pattern-match on a non-StructuralEq type (example by @oli-obk).

[Mark-Simulacrum]

Dropping nomination in favor of the (already proposed, though not currently scheduled) lang design meeting.

[bors]

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

[JohnCSimon]

@workingjubilee
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive