Optimisation failure with structs in referenced projects

saul commented 6 years ago

Structs referenced from other projects have a significant optimisation failure whereby an instance of the struct cannot be passed to functions - it is always re-instantiated to empty just before the function call.

Repro steps

Imagine you have the following C# project with a single file:

namespace ClassLibrary1
{
    public struct SomeStruct
    {
        private string _value;
        public string Value => _value;

        public void Set(string value)
        {
            _value = value;
        }
    }
}

The above project is referenced from F#:

open ClassLibrary1

let someFunc (s : SomeStruct) =
    printfn "From inner: %A" s.Value

[<EntryPoint>]
let main argv = 

    let s = SomeStruct()
    s.Set("there")
    printfn "Pre call:   %A" s.Value
    someFunc s
    printfn "Post call:  %A" s.Value

    0

Expected behavior

Output of the program is:

Pre call:   "there"
From inner: "there"
Post call:  "there"

Actual behavior

With optimisations enabled (e.g. Release mode), the output is:

Pre call:   "there"
From inner: <null>
Post call:  "there"

Known workarounds

No known workaround.

Related information

Severe bug, stopping me from using a C# library that uses structs.

The IL for the main function looks like:

.method public static int32  main(string[] argv) cil managed
{
  .entrypoint
  .custom instance void [FSharp.Core]Microsoft.FSharp.Core.EntryPointAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       40 (0x28)
  .maxstack  4
  .locals init ([0] valuetype [ClassLibrary1]ClassLibrary1.SomeStruct s,
           [1] valuetype [ClassLibrary1]ClassLibrary1.SomeStruct V_1)
  IL_0000:  nop
  IL_0001:  ldloca.s   V_1
  IL_0003:  initobj    [ClassLibrary1]ClassLibrary1.SomeStruct
  IL_0009:  ldloc.1
  IL_000a:  stloc.0
  IL_000b:  ldloca.s   s
  IL_000d:  ldstr      "there"
  IL_0012:  call       instance void [ClassLibrary1]ClassLibrary1.SomeStruct::Set(string)
  IL_0017:  ldloca.s   V_1 // WHY IS THIS HERE?
  IL_0019:  initobj    [ClassLibrary1]ClassLibrary1.SomeStruct // WHY IS THIS HERE?
  IL_001f:  ldloc.1
  IL_0020:  call       void Program::someFunc(valuetype [ClassLibrary1]ClassLibrary1.SomeStruct)
  IL_0025:  nop
  IL_0026:  ldc.i4.0
  IL_0027:  ret
} // end of method Program::main

IL_0017 and IL_0019 have been added by the optimiser - in Debug mode these instructions are not here, and the bad behaviour is not present.

Thanks

dsyme commented 6 years ago

@saul Yes agreed, the behaviour shouldn't change under optimization

However the correct code to mutate the struct is this:

let mutable s = SomeStruct()

My first question is why we are not getting a "struct is being mutated" warning/error.

dsyme commented 6 years ago

@saul OK, I've looked into this more.

First, if your struct types are mutable then the right way to code with them is like this:

let mutable x = SomeStruct()
x.PropertyAccessOnAStruct
x.MethodOnAStruct()

etc. That's just the right thing to do.

Now, let's consider

let x = SomeStruct()
x.PropertyAccessOnAStruct

As a value-based language where let x = ... declares an immutable value, F# has special rules time with mutable structs. F# does the following:

If SomeStruct is declared in F# then it can be analyzed to determine if it is mutable or immutable (it's nearly always the latter in F# code, being quite hard to declare a mutable struct). In that case, we know that the property access doesn't mutate the struct, and we don't need to take a defensive copy before taking its address.
However, if SomeStruct is declared in .NET, we currently always assume it is immutable, or at least that the property access doesn't mutate the struct. (As an aside, this wasn't the case in early versions of F# (1.x) but was the case from F# 2.0 onwards. I can't recall the exact reasoning but I'm assuming the change was made for performance reasons - otherwise too many defensive copies would be taken)
A couple of operations such as property setters are inferred to be as "definitely mutates" (DefinitelyMutates) and an outright error will be given if they are used on let x = SomeStruct()
There is a level 5 optional warning you can turn on to show when defensive copies are made

Now, that leaves us in an awkward situation where we are falsely assuming your .NET struct type to be immutable. As a result, I propose to make the following adjustments:

If a method call has void/unit return type then we will label it a "likely mutates" and give a warning (on by default) when used on an .NET let-declared struct value. In your code, this would now give the following warning at your call to x.Set(...):

This is only a warning since it possible the user has I/O operations such as x.Dump() on a struct type. They would need to turn the warning off. But the case should be rare enough that we are doing the right thing here. For stability reasons we won't change the execution behaviour of "likely mutates" operations

We give an optional warning (off by default) whenever a .NET struct type has been assumed to be immutable. However this is very noisy, as all property getter accessors are assumes to be immutable

This is implemented in https://github.com/Microsoft/visualfsharp/pull/3968

dotnet / fsharp