static let bindings will fail unexpectedly if that binding uses a no-arg case in a Discriminating Union (DU) type

[patrickallensimpson]

Issue

static let bindings will fail unexpectedly if that binding uses a no-arg case in a Discriminating Union (DU) type. This happens because the static constructor for the DU and the static constructor for the DU's let bindings which has $ prefixed to the name call each other in a particular order and before the backing fields of the DU have been initialized.

This can only be observed if you have at least two compilation units and the DU is inside a module and you then reference the static let binding from some other static member of the DU.

Here is a example:

Test.fs

module Test

type ABC =
    | A
    | B
    | C of int

    static let ab' = [ A; B ]
    static member ab = ab'

    static let c0' = ABC.C 0
    static member c0 = c0'

Program.fs

open Test
printfn "ABC.c0: %A" ABC.c0
printfn "ABC.ab: %A" ABC.ab

This will print:

ABC.c0: C 0
ABC.ab: [null; null]

We expect of course that the result should have been:

ABC.c0: C 0
ABC.ab: [A; B]

This is occurring due to following code being generated in ABC's static constructor and it's interaction with $Test modules static constructor.

    static ABC()
    {
      $Test.init@ = 0;              // this line is to make sure that the parent        
                                    // module's static initializer is executed prior to 
                                    // running the rest of this code.
      int init = $Test.init@;
      Test.ABC._unique_A = (Test.ABC) new Test.ABC._A();
      Test.ABC._unique_B = (Test.ABC) new Test.ABC._B();
    }

and here is the static constructor for the Test module:

    static $Test()
    {
      Test.ABC.ab' = FSharpList<Test.ABC>.Cons(Test.ABC.get_A(), FSharpList<Test.ABC>.Cons(Test.ABC.get_B(), FSharpList<Test.ABC>.get_Empty()));
      Test.ABC.init@3 = 0;
      Test.ABC.c0' = Test.ABC.NewC(0);
      Test.ABC.init@03 = 1;
    }

You can now see the issue that arises, the ABC static constructor causes the parent Test modules static constructor to execute before initializing the backing fields for the no-arg Union cases. The static constructor for the Test module in turn uses the ABC get methods that access those uninitialized backending fields returning null.

I understand why the module initializer is setting the fields, because that way accessing any other module elements will cause all the module contained types to initialize together. But I would then propose that the module's init@ reference in the DU static constructor should happen after backing fields for no-arg cases are initialized.

Workaround

To handle this issue I used a lazy expression instead for the initialization of the static let binding this means that accessing of the backing field is delayed until this binding is actually accessed after the static constructors complete. The use of lazy also means I'm caching the value so we don't re-compute the values again and again as a normal static member would do. The only downside is that we are now re-directing through a Lazy value which is also heap allocated. It might make sense to change Lazy into a value type so that we can avoid the indirection. If I understand the VM correctly if the Lazy type is a value type then the cached value inside would simply be an additional offset from the fields offset inside the containing type.

Example:

module Test

type ABC =
    | A
    | B
    | C of int

    static let ab' = lazy [ ABC.get_A(); ABC.get_B() ] // use of lazy here to workaround a recursive initialization issue between the parent module and this DU.
    static member ab = ab'.Value

    static let c0' = ABC.C 0
    static member c0 = c0'

I'd be happy to help make this change if someone would point to the section of the compiler that handles the generation of DU static constructors.

[Martin521]

I cannot completely oversee this quickly, but it might be that this is the expected behavior as defined in the (4.1) FSharp spec, section 12.5.1

[patrickallensimpson]

If that is so, then we definitely need a warning from the compiler when someone uses the no-arg constructors in static let bindings!

[patrickallensimpson]

I just checked the 12.5.1 section and it doesn't explicitly say anything about ordering of the static initializer calls, in relation to union no-arg cases. The important thing is that the static initializer must be called on first access. This is a case where the order of operations implied by the language requires that the no-arg DU cases need to exist prior to running of the initializer. The new case functions for any DU cases that have arguments already exist prior to static initializer being executed and thus can be used in the static initializer.

The fact that no-arg cases are represented by a backing field and a singleton value is actually just a compiler implementation detail. We could have just as easily return new instances of the DU case rather than re-using the field singleton and then this issue would not happen. The spec doesn't address the actual form of the no-arg case structure in the underlying il (at least not in that section). I think my solution is consistent with the specification as it ensures that those implementation specific backing fields are initialized correctly before executing any associated members.

Of course, if any other static bound entity in the parent module is access first then this issue doesn't happen either.

Let assume we add an addition let binding to the Test module from before, (it must be something that will force its assignment to be run in the static initializer):
Test.fs

module Test
    let anotherStaticBinding = System.Random.Shared.Next() // Note, a constant WILL NOT be compiled to force the execution of the static initializer.

    type ABC ... // rest of definition above.

Program.fs

open Test
printfn "anotherStaticBinding: %A" anotherStaticBinding // this will not cause the modules static binding to execute, which immediately causes ABC's to execute
printfn "ABC.c0: %A" ABC.c0
printfn "ABC.ab: %A" ABC.ab

This works because we hit Test module static initializer first, which then touches ABC's static initializer because of ABC.init@3 being referenced which in turn, then touches $Test.init@. But we're ok since we already executing the Test module static initializer so nothing extra has to happen, so we complete ABC static constructor which sets the backing field and then returns to the Test module initialization which now can call the getters on ABC correctly with the expected values.

Order of execution

• reference Test.anotherStaticBinding
• property references $Test.anotherStaticBinding
• this begins execution of $Test static initializer
• which first assigns that field, but then access the A and B members of ABC
• this executes the ABC static constructor
• which touches $Test,init@ which since we are already executing the $Test static initializer just - references the existing field
• completes the assignment of the ABC backing fields and returns
• the $Test static initializer then completes the calls to get_A() and get_B() which now return actual values
• static initializer is complete
• and the rest of the code works as expected.

[patrickallensimpson]

I think by moving ONLY the no-arg case backing field assignment to before the parent module static initializer init@ reference we can solve this issue, and without changing anything related to expected order of initialization.

[patrickallensimpson]

By the way what is the motivation for putting static bindings in $ prefixed classes in an entirely different namespace rather than encoding them into the module static classes that represent the module? I'm not understanding the advantage to this.

[patrickallensimpson]

I was examining the IlxGen.fs source file and noticed this comment around line 11364 from GenTypeDef:

...
            // If a non-generic type is written with "static let" and "static do" (i.e. it has a ".cctor")
            // then the code for the .cctor is placed into .cctor for the backing static class for the file.
            // It is not placed in its own .cctor as there is no feasible way for this to be given a coherent
            // order in the sequential initialization of the file.
            //
            // In this case, the .cctor for this type must force the .cctor of the backing static class for the file.
            if
                tycon.TyparsNoRange.IsEmpty
                && tycon.MembersOfFSharpTyconSorted
                   |> List.exists (fun vref -> vref.Deref.IsClassConstructor)
            then
                GenForceWholeFileInitializationAsPartOfCCtor cenv mgbuf lazyInitInfo tref eenv.imports m
...

It seems that there is an understanding that initialization with static's is an issue. I would suggest that we move the backing fields for union cases into the backing static class's .cctor instead when we detect this condition. It seems that we are inserting the reference and then assuming we can have the backing fields still in the DU's .cctor but we really can't.

If you really think about it the backing field optimization is really a file initialization issue and should be moved into that same location as all the other explicit static let and static do code that is going to be created. This probably something that should be considered anywhere else we are generating hidden static fields for other F# types as well.

[pirrmann]

I've faced the same issue with no-arg cases DUs. When testing various cases, I've tried to get a simple case to trigger the issue... and it turns out that for me, the sample from the Announcing F# 8 blog post does not work and fails with a NullReferenceException during the type initialization.

open FSharp.Reflection

type AbcDU = A | B | C
    with   
        static let namesAndValues = 
            FSharpType.GetUnionCases(typeof<AbcDU>) 
            |> Array.map (fun c -> c.Name, FSharpValue.MakeUnion (c,[||]) :?> AbcDU)
        static let stringMap = namesAndValues |> dict
        static let mutable cnt = 0

        static do printfn "Init done! We have %i cases" stringMap.Count
        static member TryParse text = 
            let cnt = Interlocked.Increment(&cnt)
            stringMap.TryGetValue text, sprintf "Parsed %i" cnt