Infer isreadonly from structs ("readonly struct" in C#) to avoid struct-copies just like we do with readonly methods

src/Compiler/Checking/infos.fs

@@ -438,6 +438,11 @@ type ILTypeInfo =
 
     member x.IsValueType = x.RawMetadata.IsStructOrEnum
 
+    /// Indicates if the type is marked with the [<IsReadOnly>] attribute.
+    member x.IsReadOnly (g: TcGlobals) =
+        x.RawMetadata.CustomAttrs
+        |> TryFindILAttribute g.attrib_IsReadOnlyAttribute
+
     member x.Instantiate inst =
         let (ILTypeInfo(g, ty, tref, tdef)) = x
         ILTypeInfo(g, instType inst ty, tref, tdef)
@@ -993,15 +998,22 @@ type MethInfo =
     member x.IsStruct =
         isStructTy x.TcGlobals x.ApparentEnclosingType
 
-    /// Indicates if this method is read-only; usually by the [<IsReadOnly>] attribute.
+    member x.IsOnReadOnlyType = 
+        let g = x.TcGlobals
+        let typeInfo = ILTypeInfo.FromType g x.ApparentEnclosingType
+        typeInfo.IsReadOnly g
+
+    /// Indicates if this method is read-only; usually by the [<IsReadOnly>] attribute on method or struct level.
     /// Must be an instance method.
     /// Receiver must be a struct type.
     member x.IsReadOnly =
-        // Perf Review: Is there a way we can cache this result?
+        // Perf Review: Is there a way we can cache this result?        
+
         x.IsInstance &&
         x.IsStruct &&
         match x with
-        | ILMeth (g, ilMethInfo, _) -> ilMethInfo.IsReadOnly g
+        | ILMeth (g, ilMethInfo, _) -> 
+             ilMethInfo.IsReadOnly g || x.IsOnReadOnlyType
         | FSMeth _ -> false // F# defined methods not supported yet. Must be a language feature.
         | _ -> false
 

tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs

@@ -70,6 +70,18 @@ module TestCompilerWarningLevel =
         |> withDiagnosticMessageMatches "The value has been copied to ensure the original is not mutated by this operation or because the copy is implicit when returning a struct from a member and another member is then accessed$"
         |> ignore
 
+#if NETSTANDARD 
+// This test works with KeyValuePair, which is not  a 'readonly struct' in net472
+    [<Theory; Directory(__SOURCE_DIRECTORY__ + "/../../resources/tests/CompilerOptions/fsc/warn", Includes=[|"nowarn_readonlystruct.fs"|])>]
+    let ``no error 52 with readonly struct`` compilation =
+        compilation
+        |> asExe
+        |> withOptions ["--warn:5"; "--warnaserror:52"]
+        |> compile
+        |> shouldSucceed
+        |> ignore
+#endif
+
     [<Theory; Directory(__SOURCE_DIRECTORY__ + "/../../resources/tests/CompilerOptions/fsc/warn", Includes=[|"warn_level6.fs"|])>]
     let ``warn_level6_fs --warn:6`` compilation =
         compilation

tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs

@@ -0,0 +1,158 @@
+module FSharp.Compiler.ComponentTests.EmittedIL.StructDefensiveCopy
+
+open Xunit
+open System.IO
+open FSharp.Test
+open FSharp.Test.Compiler
+
+let verifyKeyValuePairInstanceMethodCall expectedIl =
+    FSharp """
+module StructUnion01
+open System.Runtime.CompilerServices
+open System.Collections.Generic
+
+let doWork(kvp1:inref<KeyValuePair<int,int>>) =
+    kvp1.ToString()
+    """
+    |> ignoreWarnings
+    |> compile
+    |> shouldSucceed
+    |> verifyIL expectedIl
+
+#if NETSTANDARD 
+// KeyValuePair defined as a readonly struct (in C#)
+[<Fact>]
+let ``Defensive copy can be skipped on read-only structs``() =
+    verifyKeyValuePairInstanceMethodCall ["""      .method public static string  doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32>& kvp1) cil managed
+  {
+    .param [1]
+    .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
+
+    .maxstack  8
+    IL_0000:  ldarg.0
+    IL_0001:  constrained. valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32>
+    IL_0007:  callvirt   instance string [runtime]System.Object::ToString()
+    IL_000c:  ret
+  } 
+
+} """]
+
+#else 
+// KeyValuePair just a regular struct. Notice the "ldobj" instruction
+[<Fact>]
+let ``Non readonly struct needs a defensive copy``() =
+    verifyKeyValuePairInstanceMethodCall ["""      .method public static string  doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32>& kvp1) cil managed
+      {
+        .param [1]
+        .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
+
+        .maxstack  3
+        .locals init (valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32> V_0)
+        IL_0000:  ldarg.0
+        IL_0001:  ldobj      valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32>
+        IL_0006:  stloc.0
+        IL_0007:  ldloca.s   V_0
+        IL_0009:  constrained. valuetype [runtime]System.Collections.Generic.KeyValuePair`2<int32,int32>
+        IL_000f:  callvirt   instance string [runtime]System.Object::ToString()
+        IL_0014:  ret
+      } """]
+#endif
+
+let verifyDateTimeExtensionMethodCall expectedIl =
+    FSharp """
+module DateTimeExtensionMethod
+
+open System
+open System.Collections.Generic
+open System.Runtime.CompilerServices
+
+[<Extension>]
+type DateTimeExtensions =
+    [<Extension>]
+    static member PrintDate(d: inref<DateTime>) = d.ToString()
+
+let doWork(dt:inref<DateTime>) =
+    dt.PrintDate()
+    """
+    |> ignoreWarnings
+    |> compile
+    |> shouldSucceed
+    |> verifyIL expectedIl
+
+#if NETSTANDARD 
+// DateTime defined as a readonly struct (in C#)
+[<Fact>]
+let ``Defensive copy can be skipped for extension methods on read-only structs``() =
+    verifyDateTimeExtensionMethodCall ["""      .method public static string  doWork([in] valuetype [runtime]System.DateTime& dt) cil managed
+      {
+        .param [1]
+        .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
+
+        .maxstack  8
+        IL_0000:  ldarg.0
+        IL_0001:  constrained. [runtime]System.DateTime
+        IL_0007:  callvirt   instance string [runtime]System.Object::ToString()
+        IL_000c:  ret
+      } """]
+
+#else 
+// DateTime just a regular struct. Notice the "ldobj" instruction
+[<Fact>]
+let ``Non readonly struct needs a defensive copy when its extension method is called``() =
+    verifyDateTimeExtensionMethodCall ["""      .method public static string  doWork([in] valuetype [runtime]System.DateTime& dt) cil managed
+      {
+        .param [1]
+        .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
+
+        .maxstack  3
+        .locals init (valuetype [runtime]System.DateTime& V_0,
+                 valuetype [runtime]System.DateTime V_1)
+        IL_0000:  ldarg.0
+        IL_0001:  stloc.0
+        IL_0002:  ldloc.0
+        IL_0003:  ldobj      [runtime]System.DateTime
+        IL_0008:  stloc.1
+        IL_0009:  ldloca.s   V_1
+        IL_000b:  constrained. [runtime]System.DateTime
+        IL_0011:  callvirt   instance string [runtime]System.Object::ToString()
+        IL_0016:  ret
+      } """]
+#endif
+
+
+#if NETSTANDARD 
+[<Fact>]
+#endif
+let ``Csharp extension method on a readonly struct does not need defensive copy``() =
+    let csLib = 
+        CSharp """
+using System;
+public static class DateTimeExtensionMethod
+{
+        public static string CustomPrintDate(this in DateTime d)
+        {
+            return d.Date.ToShortDateString();
+        }
+}"""    |> withName "CsLib"
+
+    FSharp """
+module DateTimeDefinedInCsharpUsage
+open System
+let doWork(dt:inref<DateTime>) =
+    dt.CustomPrintDate()
+    """
+    |> withReferences [csLib]
+    |> ignoreWarnings
+    |> compile
+    |> shouldSucceed
+    |> verifyIL ["""      .method public static string  doWork([in] valuetype [runtime]System.DateTime& dt) cil managed
+      {
+        .param [1]
+        .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
+
+        .maxstack  8
+        IL_0000:  ldarg.0
+        IL_0001:  call       string [CsLib]DateTimeExtensionMethod::CustomPrintDate(valuetype [runtime]System.DateTime&)
+        IL_0006:  ret
+      } """]
+

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

@@ -134,6 +134,7 @@
     <Compile Include="EmittedIL\Structure\Structure.fs" />
     <Compile Include="EmittedIL\TestFunctions\TestFunctions.fs" />
     <Compile Include="EmittedIL\Tuples\Tuples.fs" />
+	<Compile Include="EmittedIL\StructDefensiveCopy\StructDefensiveCopy.fs" />
     <Compile Include="ErrorMessages\UnsupportedAttributes.fs" />
     <Compile Include="ErrorMessages\TypeEqualsMissingTests.fs" />
     <Compile Include="ErrorMessages\AccessOfTypeAbbreviationTests.fs" />

tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/nowarn_readonlystruct.fs

@@ -0,0 +1,9 @@
+// #Regression #NoMT #CompilerOptions 
+// See DevDiv:364238
+open System.Collections.Generic
+
+let x : IEnumerator<KeyValuePair<int, int>> = failwith ""
+printfn "%A" x.Current.Key // no defensive copy needed, because KeyValuePair is a "readonly struct"
+
+let y : list<KeyValuePair<int, int>> = failwith ""  // KeyValuePair<int, int>
+printfn "%A" y.[0].Key // no defensive copy needed, because KeyValuePair is a "readonly struct"

tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/warn_level5.fs

@@ -2,8 +2,12 @@
 // See DevDiv:364238
 open System.Collections.Generic
 
-let x : IEnumerator<KeyValuePair<int, int>> = failwith ""
-printfn "%A" x.Current.Key // defensive copy
+[<Struct>]
+type NonReadOnlyStruct=
+    member val Property = "" with get, set
 
-let y : list<KeyValuePair<int, int>> = failwith ""
-printfn "%A" y.[0].Key // defensive copy
+let x : IEnumerator<NonReadOnlyStruct> = failwith ""
+printfn "%A" x.Current.Property // defensive copy
+
+let y : list<NonReadOnlyStruct> = failwith ""  // KeyValuePair<int, int>
+printfn "%A" y.[0].Property // defensive copy