fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

attribute enforcing explicit matching of DU cases

Open smoothdeveloper opened this issue 5 years ago • 11 comments

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

  • [x] 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 avatar Apr 05 '19 07:04 smoothdeveloper

Great idea!

xperiandri avatar Apr 06 '19 12:04 xperiandri

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

cartermp avatar Apr 06 '19 22:04 cartermp

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!

smoothdeveloper avatar Apr 07 '19 07:04 smoothdeveloper

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?

theprash avatar Apr 07 '19 11:04 theprash

@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.

smoothdeveloper avatar Apr 07 '19 14:04 smoothdeveloper

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.

charlesroddie avatar Apr 07 '19 19:04 charlesroddie

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.

Tarmil avatar Apr 08 '19 11:04 Tarmil

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.

cartermp avatar Apr 08 '19 15:04 cartermp

Not sure why exhaustive match is not always preferred? This is the approach rust goes with, if match is used as an expression.

I dislike sprinkling the language with attributes here and there.

Swoorup avatar May 28 '19 07:05 Swoorup

@Swoorup Exhaustive match is always preferred: F# gives a warning if a match isn't exhaustive. But this is specifically about failing when exhaustive match was achieved using a _ wildcard rather than explicitly listing all cases. That's why it only makes sense for some types but not others. Rust doesn't warn or fail about this either, nor should it by default:

enum MyEnum {
    Case1,
    Case2,
}

match myEnum {
    Case1 => 1,
    _ => 2, // No problem
}

Tarmil avatar May 28 '19 09:05 Tarmil

@Swoorup

I dislike sprinkling the language with attributes here and there.

That implementation detail doesn't impact at all the consumption side, so it is a minor concern to me.

If you like the suggestion, what would you do to mark the DU you want it to apply on?

I get used to the name of attributes I find useful in my design, and I prefer that to keyword soups for this type of features. (it is also much easier to not have to change the grammar and carry in the type checker for a apprentice compiler maintainer)

@Tarmil: do you feel there is a tension between #222, the pre #222 defaults of F# and this suggestion?

smoothdeveloper avatar Apr 20 '21 14:04 smoothdeveloper