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

attribute enforcing explicit matching of DU cases #731

Open
smoothdeveloper opened this Issue Apr 5, 2019 · 8 comments

Comments

Projects
None yet
6 participants
@smoothdeveloper
Copy link

smoothdeveloper commented Apr 5, 2019

EnforceTotalMatch attribute for DU (naming TBD)

The safety around exhaustive pattern matching is one of the strong point of F# type system,

For F# users, discarding some cases of a DU in a match is always a fine balance between future changes and state of the code using the DU.

I'm sure many F# users face situations while adding cases to the DU or reviewing / adjusting the existing matches or those to add asking self:

  • should I just put a wild card?
  • is this existing wild card correct?

In specific cases, it would be relevant to make usage of the wildcard discouraged more than morally, but with the tools of software engineers: the compiler.

sample

type NormalDU = Normal1 | Normal2 | Normal3

[<EnforceTotalMatch>]
type StrictDU = Enforced1 | Enforced2 | Enforced3

let test normal strict =
    let interesting =
        match normal with 
        | Normal1 -> true
        | _ -> false

    let moreInteresting =
        match strict with
        | Enforced1 -> true
        | _ -> false // compile error

    (interesting, moreInteresting)

What:

Case Enforced2 has not been matched explicitly.

Why:

StrictDU is declared with [<EnforceTotalMatch>] disallowing usage of wildcard in pattern matching.

How To Fix:

All cases need to be matched explicitly.

Where:

    | _ -> false
      ^ 

NB: An error is produced opposed to the warning we usually get if there is no wildcard and missing cases.

Usage in other conditional constructs is an area of investigation / suggestions, maybe usage outside match and function should lead to a new warning.

This could be refined to apply to individual cases in a later revision, if the feature is relevant and doable.

Using this attribute could lead to a design time technique, where F# user would flip the attribute on while adding cases and removing it once all code changes and tests are done or to keep only in debug build; bringing some extra confidence of having reviewed all the matches.

Pros and Cons

The advantages of making this adjustment to F# are:

  • correctness better enforced in type modeling
  • express coding standards around matching / conditionals
  • reinforces "compiler is helpful" feeling

The disadvantages of making this adjustment to F# are:

  • the costs to integrate in compiler codebase, logic handling pattern matches is assumed to be large
  • compile time will increase a bit whenever a _ is used (many places...)
  • some doubts around potential false positives or misses, pattern matching is key area, requires thourough testing
  • reinforces "compiler is slow" feeling

Extra information

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

rfc: small
compiler: medium
tests: medium-large
documentation / guidelines: medium

Related suggestions: #414

Affidavit

  • 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

@smoothdeveloper smoothdeveloper changed the title Add an attribute enforcing explicit matching of DU cases attribute enforcing explicit matching of DU cases Apr 5, 2019

@xperiandri

This comment has been minimized.

Copy link

xperiandri commented Apr 6, 2019

Great idea!

@cartermp

This comment has been minimized.

Copy link
Member

cartermp commented Apr 6, 2019

Another con to this is that you can achieve this today by setting warnings as errors, though I can see someone wanting compile errors for failing to be exhaustive while not wanting an error on all warnings.

Some other considerations:

  • Wouldn't be added to any types defined in FSharp.Core as this would break existing code
  • Cost in documentation + guidance, as _ is considered exhaustive as it's "everything else", meaning there's an additional property - total match vs. exhaustiveness - to consider

I'm in favor of this in theory, though I'd probably have to think about it more

@smoothdeveloper

This comment has been minimized.

Copy link
Author

smoothdeveloper commented Apr 7, 2019

Another con to this is that you can achieve this today by setting warnings as errors,

Actually, I'm not able to achieve this today, if matches use wildcards, adding a case will result in warnings (that are great) in places there are no wildcards.

The design of that optional restriction feels similar to [<RequiresQualifiedAccess>] which also dictates how the matching is done.

Wouldn't be added to any types defined in FSharp.Core as this would break existing codeWouldn't be added to any types defined in FSharp.Core as this would break existing code

Agreed, this is really a feature to allow optional strictness on specific DUs, and I don't believe it applies to general libraries, but more to domain specific code; where having some extra explicitness on how matching should be done for selected entities seems like a win / great tool.

Cost in documentation + guidance, as _ is considered exhaustive as it's "everything else", meaning there's an additional property - total match vs. exhaustiveness - to consider

Agreed, there would probably be need to enrich few places in the documentation, language specification, guildelines, maybe most of it could remain concentrated on the attribute documentation, and would go along well with existing content touching on [<RequiresQualifiedAccess>].

Thanks for feedback guys!

@theprash

This comment has been minimized.

Copy link

theprash commented Apr 7, 2019

What about nested pattern matching on these types? Consider these examples:

[<EnforceTotalMatch>]
type StrictDU = Enforced1 | Enforced2 | Enforced3
let strict = Enforced1

match Some strict with
| None -> "nothing"
| Some _ -> "something" // error?

match [ strict ] with
| [] -> "empty"
| _ -> "not empty" // error?

If the error happens for nested patterns, then I think it would be too impractical to use. If it doesn't happen on nested patterns, then the restriction quite easy to work around, limiting the usefulness of the feature. Maybe it would only actually be enforced if at least one case was explicitly mentioned?

@smoothdeveloper

This comment has been minimized.

Copy link
Author

smoothdeveloper commented Apr 7, 2019

@theprash great question!

I'm eager to see what other think about potential issues / inconsistencies that would make the feature useless or if it can be made sound / respecting the principle of least surprise for most F# users:

In both of the matches you shown, if the type of strict is not resolved to a concrete DU type having the attribute, then it should compile fine (generic code).

If the compiler knows that strict has for type a DU with the attribute, then it should fail in your sample, as you are using an explicit wildcard on the value in the first match.

The second match is ambiguous and it would definitely be more expensive for the compiler to figure out if a wildcard is OK or should fail than it is now; since the second match you are using is not binding the DU instance at all, I think that one should succeed the compilation.

then I think it would be too impractical to use

I agree that the second example is showing the kind of edge cases that would need to be nailed down, and more consistency/use cases considerations required for a RFC.

If it doesn't happen on nested patterns, then the restriction quite easy to work around

I don't think it should be let to happen; so long a _ or named/unrestricted binding is used as a binding for DU with this attribute, I'd expect a compile error.

@charlesroddie

This comment has been minimized.

Copy link

charlesroddie commented Apr 7, 2019

I think this can be implemented consistently if, for the purpose of warnings or compilation, _ is not allowed to match DU types with [<EnforceTotalMatch>].

So in @theprash 's example,

match Some strict with
| None -> "nothing"
| Some _ -> "something" // Error: _ is not allowed to match a StrictDU

match [ strict ] with
| [] -> "empty"
| _ -> "not empty" // OK: _ is allowed to match a List<StrictDU> because List<_> does not have [<EnforceTotalMatch>]

In codebases that you control, I think advice to minimize use of _ is adequate and this suggestion is not needed. It could be useful when creating APIs that expose F# DUs for F# consumers.

@Tarmil

This comment has been minimized.

Copy link

Tarmil commented Apr 8, 2019

I think there's a big difference between a _ that matches absolutely all cases, and a _ that matches cases not previously matched. ie between this:

match Some strict with
| None -> "nothing"
| Some _ -> "something" // matches everything

and this:

match Some strict with
| None -> "nothing"
| Some Enforced1 -> "something1"
| Some _ -> "something2" // matches remaining cases

To me, the first case is just a general ignore and should not cause an error. The second one is the case where an error would be useful.

@cartermp

This comment has been minimized.

Copy link
Member

cartermp commented Apr 8, 2019

Great discussion. Thinking about this at a bit more of a high level, there are two axes by which to grade a feature like this:

  • Does the enforcement feel consistent; i.e., will the use of _ in certain contexts surprise you?
  • Does the enforcement feel predictable; i.e., can you use it without feeling like you need to read the docs?

I'm curious how folks here feel about @charlesroddie's example and @Tarmil's example on these two points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.