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

Tracking issue for `illegal_floating_point_literal_pattern` compatibility lint #41620

Open
est31 opened this Issue Apr 29, 2017 · 20 comments

Comments

Projects
None yet
@est31
Contributor

est31 commented Apr 29, 2017

This is a tracking issue for the compatibility lint disallowing floating point literals in patterns.

The corresponding RFC is RFC 1445. Originally, #36890 was the only tracking issue for floats in patterns, but after it was found out that the implementation of that lint doesn't cover literals (issue #41255), another lint and tracking issue were required.

The goal of this lint is to make code like this a hard error:

fn main() {
    let x = 13.4;

    match x {
        1.0 => println!("one"),
        22.4 => println!("two"),
        3.67 => println!("three"),
        13.4 => println!("thirteen point four"),
        _ => println!("anything"),
    }
}
  • PR #41293 introduces the lint as warn by default
  • PR xxxx makes the lint deny by default
  • PR xxxx makes the lint a hard error

est31 added a commit to est31/rust that referenced this issue Apr 29, 2017

Add illegal_floating_point_literal_pattern compat lint
Adds a compatibility lint to disallow floating point literals in
patterns like in match.

See the tracking issue rust-lang#41620.

est31 added a commit to est31/rust that referenced this issue Apr 29, 2017

Add illegal_floating_point_literal_pattern compat lint
Adds a compatibility lint to disallow floating point literals in
patterns like in match.

See the tracking issue rust-lang#41620.

est31 added a commit to est31/rust that referenced this issue May 2, 2017

Add illegal_floating_point_literal_pattern compat lint
Adds a compatibility lint to disallow floating point literals in
patterns like in match.

See the tracking issue rust-lang#41620.

bors added a commit that referenced this issue May 8, 2017

Auto merge of #41293 - est31:floating_literal_match, r=nikomatsakis
Implement the illegal_floating_point_literal_pattern compat lint

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

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 9, 2017

Rollup merge of rust-lang#41293 - est31:floating_literal_match, r=nik…
…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 .

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 9, 2017

Rollup merge of rust-lang#41293 - est31:floating_literal_match, r=nik…
…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 .

hackeryarn added a commit to hackeryarn/rust that referenced this issue May 9, 2017

Add illegal_floating_point_literal_pattern compat lint
Adds a compatibility lint to disallow floating point literals in
patterns like in match.

See the tracking issue rust-lang#41620.
@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented May 10, 2017

RFC 1445 is about const constants. Why are float literals restricted as well?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented May 10, 2017

This is a breaking change (rejecting previously-accepted programs) but as far as I can tell this one is not part of an accepted RFC.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented May 10, 2017

Is this a duplicate of #36890?

@est31

This comment has been minimized.

Contributor

est31 commented May 10, 2017

RFC 1445 is about const constants. Why are float literals restricted as well?

Hmm a quick glance over the RFC seemed to confirm this. Let's discuss this in #36890, okay?

Is this a duplicate of #36890?

No, its separate. There are two separate lints. This one is about literals, the one tracked in that issue is about constants. I have opened the PR #41293 after finding out about #41255.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented May 10, 2017

RFC 1445 is about const constants. Why are float literals restricted as well?

From my perspective, primitive literals (as opposed to destructuring struct literals) are a kind of constant. The same questions raised by consts in that RFC are raised by primitive literals.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented May 10, 2017

No, its separate. There are two separate lints. This one is about literals, the one tracked in that issue is about constants.

Yet the example given in #36890 uses literals.

From my perspective, primitive literals (as opposed to destructuring struct literals) are a kind of constant.

Given that we have a language keyword named const, I think it is reasonable to assume that constants are cont items and nothing else. If a different definition of the term is intended, it should absolutely be defined explicitly. This is not the case in this RFC.

@est31

This comment has been minimized.

Contributor

est31 commented May 10, 2017

No, its separate. There are two separate lints. This one is about literals, the one tracked in that issue is about constants.

Yet the example given in #36890 uses literals.

The example quoted in the tracking issue you linked never was implemented that way. I have asked about whether to create a separate lint or add the check to the existing one, and it was decided to create a separate lint.

I guess the example should be removed from that issue's description.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 10, 2017

I would definitely say that the RFC was intended to apply equally to all constants, whether they are literals or not. Clearly there is room for other opinions (since @SimonSapin read it differently).

Personally, I am on the fence here. I somewhat agree with @SimonSapin that the number of regressions here feels too high. Part of the reason that I agree to go with a warning was to highlight this question so that we can have a wider discussion with more of the people who are affected.

For clarity, floating point literals at present match the same way as ==. Literals cannot be NaN. Therefore, I believe the only case where == can differ from "structural equality" with a floating point value has to do with 0 vs -0, which compare as equal but are clearly structurally unequal. (Does anyone have another example? I don't claim to be an expert on floating point edge cases.)

I take the following as a given (I'm curious if others disagree):

  • We should not change the runtime behavior of matching a floating point literal (e.g., to make 0 and -0 unequal) -- silent runtime changes seem to me to be almost never desirable if they can be avoided.
  • If we support floating point non-literal constants (NLC), they should behave the same as floating point literals.

This seems to imply that if we don't make things a hard error, we can't have floating point NLC that use a purely structural match. They would always match with ==. I don't see any fundamental problem here: plausibly we could say that all user-defined types use structural equality and built-in types have their own customized behaviors. We could even tweak the behavior for NaN if we wanted, though I think that having matching behave almost like == but different (i.e., 0 == -0 but NaN == NaN) feels even more confusing to me and I'm probably opposed.

This may or may not affect plans around constant generics. It's not obvious to me that dynamic match evaluation and equality for constant generics have to be particularly related, but I can see the appeal of trying to have one notion of "structural equality" that applies equally. Maybe someone wants to make the case (@eddyb? @withoutboats?) that the two are entangled? (I'd like to hear it again.)

In other words, I see a few options:

  • Make literals illegal.
  • Continue to allow literals, but not NLC.
    • but @withoutboats objects, and quite reasonably so, to the idea that we should draw this distinction. I agree it is surprising to me that a literal and a const would behave so differently.
  • Allow both literals and floating point constants to continue with current semantics
    • We used to check for NaN and error out; we could keep doing so for now, though if we ever support generics constants of floating point type that will no longer be viable -- or at least you couldn't use those constants in a match expression.
    • Personally, I don't see the point, though I wouldn't object to warning if we ever see a NaN constant in a match arm, since it is dead-code (and I would warn about it just as we try to always warn about dead code).

I am not sure whether an RFC ought to be required, whichever path we chose, but since there seems to be disagreement, it seems plausible to open an RFC amendment to finalize the decision.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented May 11, 2017

@nikomatsakis I don't think this needs to be entangled with const generics, we just need to transition to a solution for that as well as whatever we decide for match. Just renaming the attribute is probably fine.

I do feel that literals and consts should behave the same, but I don't have a strong opinion about how they ought to behave. Really whatever decision seems fine if it applies to both.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 11, 2017

I don't think this needs to be entangled with const generics, we just need to transition to a solution for that as well as whatever we decide for match.

Let me unpack this a bit to be sure I understand. You're saying:

  1. Currently, the idea for const generics equality would have been to use the same #[structural_equality] attribute to denote types that are "structurally comparable for equality".
  2. But if we want we could have distinct attributes for the match system and for const generics.

Right?

(That said, it's not entirely clear to me why const generics would even need an attribute at all; I guess it depends a lot on how we wind up defining equality. My expectation was that equality would be based more on where in the source the expression arose (i.e., we would treat constant expressions as kind of "uninterpreted functions") -- and we'd always be assuming that said functions are deterministic (because of the limits we place on const fns), and hence we consider two constant expressions "equal" if they are the same function applied to equal inputs, where this bottoms out with simple integers and other builtins. But I guess eventually we might want to extend that notion of structural equality to other kinds of expressions and types, and maybe we want some opt-in around that, unsure.)

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented May 11, 2017

@nikomatsakis the issue here isn't with unevaluable const expressions but with floating point numbers and other types for which equality is not reflexive because of how that would impact unification. We don't need an attribute but it does seem that we do need const values to have a guaranteed reflexive definition of equality (which is why just using PartialEq seems problematic for consts).

@darkwater

This comment has been minimized.

darkwater commented May 23, 2017

I might be missing something here, but I see this also applies to ranges, ie.

let color = match foo {
    0.0...0.1 => Color::Red,
    0.1...0.4 => Color::Yellow,
    0.4...0.8 => Color::Blue,
    _         => Color::Grey,
};

gives warning: floating-point literals cannot be used in patterns.

Is this intentional? I feel like this is a pretty common pattern.

@fschutt

This comment has been minimized.

Contributor

fschutt commented May 24, 2017

Yeah, I gotta ask, how am I supposed to do clean pattern matching with floating point ranges. I know floating points are hard, but isn't there a way to make this work in ranges? Example (real) code that gives a warning now:

/// Calculates the UTM zone this longitude falls in
/// Handles exceptions for Norway / Svalbard
/// For a visual representation: https://upload.wikimedia.org/wikipedia/commons/a/a5/UTM-Zone.svg
///
/// Inputs: Longitude, in degrees
///         Latitude, in degrees
///
/// Returns: UTM Zone
///
#[allow(non_snake_case)]
pub fn get_utm_zone(lon: f64, lat: f64) -> u8 {

    let mut zone = ((lon + 180.0) / 6.0).floor() as u8 + 1;

    match lat {
        56.0..64.0 => {
            /* Zone V, Norway */
            match lon {
                3.0..6.0 => {
                    zone += 1;
                }
                _ => {}
            }
        }

        72.0..84.0 => {
            /* Zone X, Svalbard */
            match lon {
                6.0..9.0 => {
                    zone -= 1;
                }
                9.0..12.0 => {
                    zone += 1;
                }
                18.0..21.0 => {
                    zone -= 1;
                }
                21.0..24.0 => {
                    zone += 1;
                }
                30.0..33.0 => {
                    zone -= 1;
                }
                33.0..36.0 => {
                    zone += 1;
                }
                _ => {}
            }
        }

        _ => {}
    }

    zone
}

How should I write this instead? Why is it that I can do comparisons of floats in if statements, but not pattern matching in range statements? I would have to write if value < 30.0 && value > 27.0 { }, which, in the end does the same thing, but less readable.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 24, 2017

@darkwater @sharazam

I might be missing something here, but I see this also applies to ranges, ie.

Yes, there has been some back and forth on whether we ought to apply the same thing to floating points.

@vks

This comment has been minimized.

Contributor

vks commented May 25, 2017

It would be very unfortunate if floating point ranges were disallowed by this. I don't see any problems with this particular pattern.

@kornelski

This comment has been minimized.

Contributor

kornelski commented May 29, 2017

I was caught by this as well.

My case is:

let fudge_factor = match magic_value {
            0. ... 0.8 => 9., 
            0. ... 1.0 => 6.,
            0. ... 1.2 => 4.,
            0. ... 2.5 => 3.67, 
            _ => 3.0,
        };

To work around float comparison woes I'm using a hack of starting every range with 0. ... and depending on the specific match order instead.

I don't particularly like the current hacky/unclear/error-prone way of matching non-overlapping ranges of numbers, but I'd rather have something working until Rust gets a better replacement.

bvssvni added a commit to bvssvni/image that referenced this issue Jun 12, 2017

@U007D

This comment has been minimized.

Contributor

U007D commented Jul 11, 2017

I waded through three or four threads to find the rationale for this (found it here: #20489, in a comment dated Jul 18, 2015).

Niko Matsakis wrote: "right now, you can match on types that don't implement PartialEq at all, and if they do implement it, the match code semantics do not align with what PartialEq implements. This seems bad. Having match x { FOO => ... } and match x { y if x == FOO => } both be accepted but different in subtle ways seems clearly suboptimal to me."

Now I can understand (and agree with) the motivation; I thought I'd post this here in case others were looking for it too.

tynril added a commit to tynril/capnpc-rust that referenced this issue Jul 30, 2017

Not using `match` on floating point values anymore.
The use of literal pattern matching on floating points has been
deprecated. It currently produces a warning, but will eventually
cause a compilation error.

See: rust-lang/rust#41620

pixelistik added a commit to pixelistik/aggregated_stats that referenced this issue Oct 8, 2017

pnkfelix added a commit to pnkfelix/rust that referenced this issue Aug 31, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue Aug 31, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 3, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 6, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 6, 2018

eliovir added a commit to eliovir/rust-examples that referenced this issue Oct 1, 2018

Changes to version 1.29.1 (b801ae664 2018-09-20)
The use of literal pattern matching on floating points has been
deprecated. It currently produces a warning, but will eventually
cause a compilation error.

See: rust-lang/rust#41620
@blakehawkins

This comment has been minimized.

blakehawkins commented Oct 9, 2018

Any update on this? Will float ranges ever get different treatment from matching over equality for float values?

@est31 est31 closed this Oct 19, 2018

@rust-lang rust-lang locked and limited conversation to collaborators Oct 19, 2018

@pietroalbini pietroalbini reopened this Oct 21, 2018

@rust-lang rust-lang unlocked this conversation Oct 21, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 30, 2018

@nikomatsakis How do you feel about crater running this to see if we can drive this to completion in the coming weeks or so?

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 30, 2018

cc #56362

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment