Unions with 4 or more cases silently fail to utilize UseNullAsTrueValue.

[bryanedds]

Repro. case in F# 4.4.0.0 -

[<CompilationRepresentation (CompilationRepresentationFlags.UseNullAsTrueValue)>]
type T = A | B of int | C of int | D of int;;
A;;

In F# interactive, here you'll see that A is a proper value, which is not desired.

[<CompilationRepresentation (CompilationRepresentationFlags.UseNullAsTrueValue)>]
type T = A | B of int | C of int;;
A;;

In F# interactive, here you'll see that A is a null, which is desired.

Would be a nice optimization to have this work for 4 cases as here - https://github.com/bryanedds/Prime/blob/master/Prime/Prime/Prime/Vmap.fs#L25-L30

NOTE: Issue moved from here - fsharp/fsharp#510

Cheers!

[enricosada]

About this issue, there is a constant UnionReprDecisions.TaggingThresholdFixedConstant = 4, below this value the UseNullAsTrueValue is applied.

This is used here

I dont know the history about of this decision, @dsyme ? we need to add a warning (error?) or remove this check?

There is already an error if the attribute cannot be applied, but the UnionReprDecisions.OptimizeAlternativeToNull is not aligned with CanHaveUseNullAsTrueValueAttribute, the only difference is this check where the constant TaggingThresholdFixedConstant is evaluated

[bryanedds]

@dsyme: At the very minimum, I think we should increase the counter to 5, as the case I show above demonstrates a real need for it to work on 4 cases in practice.

[dsyme]

First we batten down tha' hatches. The minimal approach for Update 2 is to give an error on the use of this attribute when there are 4 or more cases and note this in the language specification. This is the minimum change to make the feature sound.

Adjusting the point where we switch to a different discrimination technique (or more likely not switching at all if UseNullAsTrueValue is used) is possible. Please by all means submit a PR with testing. Add a tracking entry to http://fslang.uservoice.com and we can discuss on those threads. It's possible we could take this for Update 2 but since it's effectively a language extension it's less likely.

[enricosada]

@dsyme that's more a bug than a language extension or a missing optimization.
The code compile but doesn't work as expected (no error, no warning).
UseNullAsTrueValue doesn't say nothing abount max 4 union cases.

The surface area of this bug is small, we really need a compile warning now and remove the 4 union case limit ( why is there a limit? the history is too short on git ) next version?

About update 2, we are already in code freeze? ( /cc @KevinRansom @NumberByColors )
i mean, cannot we use a stabilization (v4.1) branch and a master branch? it'is really difficult with this workflow to do less than trivial changes. And no, coreclr branch doesnt count, is unstable.
Leave a pr open for a long time is really bad too (require rebase/sync, we cannot use the new code to further changes, etc )

[bryanedds]

Okay, I'm glad this is being treated as a bug so we can just patch it by 4.1 or 4.2, ya?

[dsyme]

I suppose the best way to expedite things is to go ahead and submit PRs implementing first the warning and then the fix, as described here: #711 (comment)

That then gives Kevin and others on the Microsoft Visual F# team the choice about when to pull them.

[enricosada]

I removed the threshold, but i am not going to implement the warning.
Next version is 4.1, we can fix this bug and add a release note in changelog about this change ( but is a bug )

[bryanedds]

I personally strongly think it makes sense to just remove the threshold and proceed without implementing the warning in this case.

Thanks Enrico for pushing this through!

[enricosada]

me too, compiler optimizations shouldn't change the behaviour the developer chooses to use in the code.
But i dont know if i understood @dsyme's comment in my reply