Property and method calls from a inref<'T> where 'T is a .NET struct type does not copy the value

[TIHan]

Consider the following F# code using the C# code:

    public struct S
    {
        public int X { get; set; }

        public void DoSomething()
        {
            X = 1;
        }
    }

let f (x: inref<S>) =
    x.DoSomething()

[<EntryPoint>]
let main argv =
    let s = S()
    printfn "%A" s.X
    f &s
    printfn "%A" s.X
    0

Prints:

0
1

But it should print:

0
0

The current behavior on the call site x.DoSomething() is not producing a defensive copy of x when x is of type inref<_>. Currently works for F# struct types; it makes a defensive copy.

The reason this was missed is because we did not have a very specific test for this, namely a .NET struct that could mutate its contents on a property or method call.

While fixing this is technically a breaking change, it is a bug and violates the F# 4.5 spec for inref:
inref<'T> also implies "the holder of the byref pointer may not modify the immediate contents of the memory pointed to".

I imagine a lot of developers are not writing this sort of code, or encountering it, as it is very specific. We have made breaking changes to byref last year and it seems to be doing fine; so I think a fix for this will be alright. It should not be part of a language version, F# 4.6+ should get this fix.

The reason why this occurs on .NET structs and not F# structs is because F# assumes that .NET struct types are immutable, even when they may not be:

/// This is an incomplete check for .NET struct types. Returning 'false' doesn't mean the thing is immutable.
let isRecdOrUnionOrStructTyconRefDefinitelyMutable (tcref: TyconRef) = 
    let tycon = tcref.Deref
    if tycon.IsUnionTycon then 
        tycon.UnionCasesArray |> Array.exists isUnionCaseDefinitelyMutable
    elif tycon.IsRecordTycon || tycon.IsStructOrEnumTycon then 
        // Note: This only looks at the F# fields, causing oddities.
        // See https://github.com/Microsoft/visualfsharp/pull/4576
        tycon.AllFieldsArray |> Array.exists isRecdOrStructFieldDefinitelyMutable
    else
        false

...

let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) =
    tcref.CanDeref &&
    not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) ||
    tyconRefEq g tcref g.decimal_tcr ||
    tyconRefEq g tcref g.date_tcr

Because of this assumption, we do not get the defensive copy when calling on the inref.

Why are .NET structs assumed immutable?
I think it was that F# wanted to have immutable struct values, even if the struct had mutable fields. However, when adding methods + properties to the struct, calling them, and then in order to ensure they truly didn't mutate the contents, it would produce a defensive copy. Only way to opt-out of the defensive copy was to have a mutable value.
While you could build your F# defined structs with this in mind, using .NET structs was probably very painful especially when the backing fields were only accessible via a property; you actually don't know if calling the property would mutate the contents. This means every call to a property would produce a copy; not great for performance. While you could make the value mutable to avoid copying, it isn't a very natural thing to do. So, a decision was made specifically with .NET struct types and therefore, we treat all of them as if they are immutable and that their properties/methods would not mutate its contents, even if it would.
This behavior cannot be changed, as it is a far reaching breaking change to do so otherwise. But for inref, it seems reasonable to me to not assume immutability and should take defensive copies.

.NET added the IsReadOnlyAttribute which currently allows to decorate a struct definition and will provide the assumption that the struct does not have fields, properties or methods that mutate its contents. F# already respects this for both F# and .NET struct types, when it thinks it needs to make a defensive copy, it won't because of this attribute.

Update: Currently F# does not respect IsReadOnlyAttribute on methods, which include the backing methods for properties. So, there is room for improvement in this area.

[TIHan]

Closing as this has been addressed.