[F# 5.x] better warnings on mutable struct #3968

[dsyme]

New version of #3968 targeting

As described in #3923 in certain cases, the F# compiler assumes that .NET struct types are shallow-immutable when the user forgets to declare a struct value mutable using let mutable x = ....

• This assumption is made because there is no way to analyze whether a .NET struct type is mutable, and
  performing a defensive copy

• This applies to method calls like x.SetFoo(3) but not to property setters x.Foo <- 3. In the latter case the operation is assumed mutating and an error is given at compile time.

• This can lead to differences in optimized and non-optimized code when let x = .... is used for a .NET struct value and a subsequent mutating operation is performed.

The following adjustment is proposed for F# 4.x:

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

2. Add a heuristic to give an improved diagnostic in some cases. Specifically if a method call has void/unit return type and starts with name "set" then give a warning (on by default) when used on a .NET struct value that has not been declared mutable.

Part (1) is fine, but part (2) of the change may be contentious: it is only a heuristic, but any name-based heuristic is unfortunate.

I'm slating this for F# 4.2 rather than pushing in the rule as a bug fix.

[cartermp]

Just as a note - I've slated the issue that tracks what this fixes (#3923) as dev16

[KevinRansom]

@dsyme, @TIHan is this still necessary, if so should we spend time on it?

Thanks

kevin

[cartermp]

This would also be good to re-assess now that we have all of the struct-based work (byref-like, readonly) in with F# 4.5.

[TIHan]

Yes, we should re-asses this.

[TIHan]

The error message numbers will need to be updated since we added more error messages since this was created.

[TIHan]

I think we should look at this again. I was messing with structs today and noticed the behavior described. We always assume .NET struct types are immutable. However, with readonly structs being a thing in C#, we can say that those are immutable. This will not cover cases where a struct is not readonly; the methods and getters technically don't change the values, but nothing is marked to indicate that it won't.

@tannergooding didn't you have a C# proposal for Properties/Methods on structs to have some sort of annotation to mark it as readonly? If C# were to add something like that, then F# could have enough information to understand that this call to a method or property doesn't mutate a value underneath.

[tannergooding]

@TIHan, yup and it is already championed and approved by LDM: dotnet/csharplang#1710

Last checked, it was still slated for C# 8.0, but as with most language features well have to wait and see which release it is actually done for/included with. Cc. @jaredpar

[dsyme]

Closing as this is just too stale. Can use this as a guide if necessary to implement this in the future