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

Pattern matching enums: adjust warning #652

Closed
charlesroddie opened this Issue Mar 10, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@charlesroddie

charlesroddie commented Mar 10, 2018

Pattern matching enums: adjust warning

Enums are important for interop with non-F# .Net code. Currently, pattern matching an Enum gives a warning:

type E =
    | A = 0

let f =
    function // "FS0025 Incomplete pattern matches on this expression".
    | E.A -> "A"

I suggest that a different warning be given where:

  • The pattern match is complete, assuming that the code knows the definition of the enum and all enums are valid, AND
  • There are invalid enums (such as enum<E>(1) ) that the pattern match would not handle.

In this case the warning should be "FS**** Enums may take values outside of known cases."

Current problem

To avoid seeing the FS0025 warning, a user must reduce compile-time safety:

  • Disable FS0025 which is extremely unsafe.
  • Use wildcards which eliminate the benefits of the FS0025 warning locally, leading to runtime misbehavior if the pattern match is genuinely incomplete:
let g : E -> unit =
    function
    | _ -> "A"
  • Handle invalid cases explicitly, which requires extra code and again eliminates the benefits of FS0025 locally, causing runtime errors instead of a compile-time warning:
let h =
    function
    | E.A -> "A"
    | _ -> failwith "I didn't want to see a warning so I am failing at runtime."

Pros of change

The change will distinguish between incompleteness resulting from the user failing to match all known enums, which is highly likely to lead to runtime failure, and incompleteness resulting from invalid enums, which is much less likely and indicates a problem with other code, not the code that gives the warning.

With the change, to avoid seeing the warning, a user may simply disable the FS**** warning. This will preserve the safety of exhaustive pattern matching. The user will be warned about additions to the enum for example with an FS0025 warning.

This is with the proviso that you are relying on other code not to be using invalid Enums, and that you have the correct enumeration of the enum at compile time. Both of which are reasonable assumptions.

Cons of change

A small amount of extra code would be needed in the pattern match exhaustiveness check.

Extra information

There are people who think there should be a warning. This is why this suggestion is to change the warning rather than eliminate it (which would have been my personal preference).

Estimated cost (XS, S, M, L, XL, XXL):

M

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@jwosty

This comment has been minimized.

Contributor

jwosty commented Mar 11, 2018

Just going to say that I like this suggestion as-is.

@cartermp

This comment has been minimized.

Member

cartermp commented Mar 12, 2018

Yep, nothing much to say other than 👍 on a more descriptive warning for when we know we're pattern matching over an enum.

@realvictorprm

This comment has been minimized.

Member

realvictorprm commented Mar 12, 2018

@dsyme

This comment has been minimized.

Collaborator

dsyme commented Mar 13, 2018

Definitely ok

@dsyme

This comment has been minimized.

Collaborator

dsyme commented Mar 13, 2018

This one doesn't need an RFC, just submit a PR

@jwosty

This comment has been minimized.

Contributor

jwosty commented Apr 3, 2018

@charlesroddie this can be closed; it's completed and merged

@charlesroddie

This comment has been minimized.

charlesroddie commented Apr 3, 2018

@dsyme can you do that?
Nice work @jwosty !

@jwosty

This comment has been minimized.

Contributor

jwosty commented Apr 10, 2018

Thank you!
@charlesroddie You should be able to do that, since you're the original submitter

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