Enum match warning

[jwosty]

First attempt at fslang-suggestions #652. It seems to work correctly so far, but tests will be coming soon.

One question -- this can't possibly be the first piece of code to grab all the defined enum values from a Tycon; is there a function that does this already? Currently this fix just looks through all the enum type's static fields (see PatternMatchCompilation.fs changes)

[dsyme]

One question -- this can't possibly be the first piece of code to grab all the defined enum values from a Tycon; is there a function that does this already? Currently this fix just looks through all the enum type's static fields (see PatternMatchCompilation.fs changes)

:) I think it is. There is this in Symbols.fs:

        if entity.IsILEnumTycon then
            let (TILObjectReprData(_scoref, _enc, tdef)) = entity.ILTyconInfo
            let formalTypars = entity.Typars(range.Zero)
            let formalTypeInst = generalizeTypars formalTypars
            let ty = TType_app(entity, formalTypeInst)
            let formalTypeInfo = ILTypeInfo.FromType cenv.g ty
            tdef.Fields.AsList
            |> List.map (fun tdef -> let ilFieldInfo = ILFieldInfo(formalTypeInfo, tdef)
                                     FSharpField(cenv, FSharpFieldData.ILField(ilFieldInfo) ))
            |> makeReadOnlyCollection

and the IlxGen.fs just iterates fiels

[dhwed]

What should the behavior be for enums defined with FlagsAttribute?

Ideally the new warning would check for exhastivity using every possible combination of the defined enum values, but if this is too hard then the existing warning can be retained.

[jwosty]

@dhwed Yeah that could be done, and it probably should (the original issue specified that the warning should happen when "the pattern match is complete and ... all enums [values] are valid")

[dsyme]

@jwosty Don't forget you can use a task to update XLF, see https://github.com/Microsoft/visualfsharp/blob/master/DEVGUIDE.md#when-modifying-adding-or-removing-keywords-or-compiler-messages

[jwosty]

@dsyme right, only problem is I'm developing on OSX. So no /t:UpdateXlf for me :(

[dsyme]

What should the behavior be for enums defined with FlagsAttribute? Ideally the new warning would check for exhastivity using every possible combination of the defined enum values, but if this is too hard then the existing warning can be retained.

We don't tend to do anything specific for FlagsAttribute in the language or compiler, because you can combine enums that don't have FlagsAttribute (at least F# doesn't enforce the attribute be there).

[jwosty]

@dsyme so should this be the behavior for all of them then? As in, it would consider an enum match "complete" (in that it covers all known values) when all combinations are covered? Or should we just not do this at all?

[dsyme]

@dsyme so should this be the behavior for all of them then? As in, it would consider an enum match "complete" (in that it covers all known values) when all combinations are covered?

I see. Yes, perhaps the use of a different error message should use a different number and appropriate message if the enum has FlagsAttribute?

[jwosty]

@dsyme What do you have in mind? Maybe "Enums may take values outside of known cases. Enums marked with FlagsAttribute also may take any combination of values."?

[saul]

These shouldn't be in the 'translated' state - same for all of the non-English translations.

[jwosty]

@saul Should it be "new"?

[jwosty]

@dotnet-bot test this please

[jwosty]

Alright, undid the whitespace changes @dsyme

[dsyme]

A few more whitespace changes to undo

[dsyme]

more whitespace changes to undo in this file please

[dsyme]

more whitespace changes to undo in this file please

[jwosty]

This should satisfy all requested changes now.

EDIT: Wait, hold on. I realized I never did anything about the FlagsAttribute thing. I won't be able to get around to this for a little while, but I will at some point (within weeks?). I don't know if it would be best to just wait for me to get around to it in this PR, or accept this one and have me open another PR later (this one is complete in its own right; FlagsAttribute stuff would only add another warning)

[dsyme]

Great work! :)

[cartermp]

@dsyme should this be tagged as Candidate for F# 4.5?

[dsyme]

No, it's just an error/warning improvement not a language change. Warning numbers change in one case but I don't think we need wait for a new language version for that

[abelbraaksma]

As in, it would consider an enum match "complete" (in that it covers all known values) when all combinations are covered? Or should we just not do this at all?

and earlier you wrote:

Ideally the new warning would check for exhastivity using every possible combination of the defined enum values

I use the Flags attribute a lot, and only when I actually use enums as flags, to differentiate with other cases. F# is not very well suited to deal with this, but I created a bunch of active pattern recognizers which may actually be useful in Core, but I digress...

Point I wanted to make is: "testing exhaustively" for all possible combinations. When programmers write something like that, I usually tell them: consider the worst case scenario. Here that would be a 64-bit enum with Flags attribute set and each bit has a corresponding enum value (that is, there are 63 flags).

That is, simply put, 2^64 - 1 possible combinations. That will take longer than one or two cup of coffees ;).

Even in a minor case, say, 24 flags, and assuming only the low 24 bits are set, you would still need to check 2^24 -1 combinations, which is considerable.

Of course, you could do the reverse: simply count all the pattern match cases, if they do not equal the expected amount of tests, then search for a hint to use in the warning message. But if you do consider an addition to the PR like this, i.e., checking for all possible flags, you should probably create a reasonable upper limit.

Actually, I'm not sure it is worth the effort. Enums with Flags are not typically used in exhaustive pattern matching. They are used instead with exhaustive or exclusive bit-test matching.

So instead of extending this PR with testing all possible combinations, I'd suggest two things (probably to go into a separate PR):

• Create a BitSet / BitNotSet (name tbd) active pattern primitive, which can take multiple bits at once (OR'ed together)
• Enhance enum-patterns-check to patterns that use BitSet, which can receive a warning when you forget a "known bit"

[jwosty]

@abelbraaksma that's a good point, actually -- I think you're right and it would not be worth the effort to try to make something like this work.

As for workarounds, I'm don't feel that there needs to be any standard lib/ compiler work to be done for this. The user should be able to come up with a way to use the type system to produce an incomplete match warning somewhere, e.g. by splitting the number into its bits, mapping them into a set of flags, then pattern matching inside that. Or maybe some clever active pattern use, as you say.

[charlesroddie]

@jwosty This is working well for me for enums defined in F#. However when consuming standard .Net enums the error is always the new one "FS0104 Enums maytake values outside known cases", even when there are defined cases that are not covered.
E.g. let f = function System.DateTimeKind.Unspecified -> 0
Does your subsequent PR address this? (I am using VS15.7.0 and it's not in there yet.)

[jwosty]

@charlesroddie off the top of my head, my other PR probably doesn't address this. I was not aware of it and hadn't thought to test that out. F# must define enums slightly differently than other languages, as the code I wrote assumes that all the enum fields are static const fields. I will revisit this, thanks!