From aa3173d8848339ba1e2fa0d6bfe81ea85f0411f5 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 29 Sep 2022 13:02:15 +0200 Subject: [PATCH 1/6] Finding if a struct is readonly on IlTypeInfo --- src/Compiler/Checking/infos.fs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Compiler/Checking/infos.fs b/src/Compiler/Checking/infos.fs index 1c0612eb611..6c8e5499f5a 100644 --- a/src/Compiler/Checking/infos.fs +++ b/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 [] 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,17 @@ type MethInfo = member x.IsStruct = isStructTy x.TcGlobals x.ApparentEnclosingType - /// Indicates if this method is read-only; usually by the [] attribute. + /// Indicates if this method is read-only; usually by the [] 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 //|| ilMethInfo.DeclaringTyconRef.type | FSMeth _ -> false // F# defined methods not supported yet. Must be a language feature. | _ -> false From e9a52e52c81d7d406f9af661de69e64fdf4feb18 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 29 Sep 2022 13:20:33 +0200 Subject: [PATCH 2/6] Method is considered readonly if it is on a readonly type --- src/Compiler/Checking/infos.fs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Checking/infos.fs b/src/Compiler/Checking/infos.fs index 6c8e5499f5a..1dc35e9ab09 100644 --- a/src/Compiler/Checking/infos.fs +++ b/src/Compiler/Checking/infos.fs @@ -998,17 +998,22 @@ type MethInfo = member x.IsStruct = isStructTy x.TcGlobals x.ApparentEnclosingType + 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 [] 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 //|| ilMethInfo.DeclaringTyconRef.type + ilMethInfo.IsReadOnly g || x.IsOnReadOnlyType | FSMeth _ -> false // F# defined methods not supported yet. Must be a language feature. | _ -> false From 9ac58bac14d690271f8394cec6489065224b8f91 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 30 Sep 2022 11:35:04 +0200 Subject: [PATCH 3/6] Tests demonstrating that readonly struct does not need a defensive copy --- .../CompilerOptions/fsc/warn.fs | 9 +++++++++ .../fsc/warn/nowarn_readonlystruct.fs | 9 +++++++++ .../tests/CompilerOptions/fsc/warn/warn_level5.fs | 12 ++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/nowarn_readonlystruct.fs diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs b/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs index 1ba362eb86a..056ede3e77a 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs @@ -70,6 +70,15 @@ 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 + [] + let ``no error 52 with readonly struct`` compilation = + compilation + |> asExe + |> withOptions ["--warn:5"; "--warnaserror:52"] + |> compile + |> shouldSucceed + |> ignore + [] let ``warn_level6_fs --warn:6`` compilation = compilation diff --git a/tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/nowarn_readonlystruct.fs b/tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/nowarn_readonlystruct.fs new file mode 100644 index 00000000000..2a4f0152709 --- /dev/null +++ b/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> = failwith "" +printfn "%A" x.Current.Key // no defensive copy needed, because KeyValuePair is a "readonly struct" + +let y : list> = failwith "" // KeyValuePair +printfn "%A" y.[0].Key // no defensive copy needed, because KeyValuePair is a "readonly struct" diff --git a/tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/warn_level5.fs b/tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/warn_level5.fs index d33dfcf9eec..2b6115e5718 100644 --- a/tests/FSharp.Compiler.ComponentTests/resources/tests/CompilerOptions/fsc/warn/warn_level5.fs +++ b/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> = failwith "" -printfn "%A" x.Current.Key // defensive copy +[] +type NonReadOnlyStruct= + member val Property = "" with get, set -let y : list> = failwith "" -printfn "%A" y.[0].Key // defensive copy +let x : IEnumerator = failwith "" +printfn "%A" x.Current.Property // defensive copy + +let y : list = failwith "" // KeyValuePair +printfn "%A" y.[0].Property // defensive copy From ed70394d5a93a14b9317220efc4fff8da658e598 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 3 Oct 2022 14:49:56 +0200 Subject: [PATCH 4/6] IL tests for struct-defensive copy added --- .../CompilerOptions/fsc/warn.fs | 3 + .../StructDefensiveCopy.fs | 59 +++++++++++++++++++ .../FSharp.Compiler.ComponentTests.fsproj | 1 + 3 files changed, 63 insertions(+) create mode 100644 tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs b/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs index 056ede3e77a..4e98ff92545 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs @@ -70,6 +70,8 @@ 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 [] let ``no error 52 with readonly struct`` compilation = compilation @@ -78,6 +80,7 @@ module TestCompilerWarningLevel = |> compile |> shouldSucceed |> ignore +#endif [] let ``warn_level6_fs --warn:6`` compilation = diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs new file mode 100644 index 00000000000..b5270e299ea --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs @@ -0,0 +1,59 @@ +module FSharp.Compiler.ComponentTests.EmittedIL.StructDefensiveCopy + +open Xunit +open System.IO +open FSharp.Test +open FSharp.Test.Compiler + +let verifyCompilation expectedIl = + FSharp """ +module StructUnion01 +open System.Runtime.CompilerServices +open System.Collections.Generic + +let doWork(kvp1:inref>) = + kvp1.ToString() + """ + |> ignoreWarnings + |> compile + |> shouldSucceed + |> verifyIL expectedIl + +#if NETSTANDARD +// KeyValuePair defined as a readonly struct (in C#) +[] +let ``Defensive copy can be skipped on read-only structs``() = + verifyCompilation [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& 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 + IL_0007: callvirt instance string [runtime]System.Object::ToString() + IL_000c: ret + } + +} """] + +#else +// KeyValuePair just a regular struct. Notice the "ldobj" instruction +[] +let ``Non readonly struct needs a defensive copy``() = + verifyCompilation [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& 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 V_0) + IL_0000: ldarg.0 + IL_0001: ldobj valuetype [runtime]System.Collections.Generic.KeyValuePair`2 + IL_0006: stloc.0 + IL_0007: ldloca.s V_0 + IL_0009: constrained. valuetype [runtime]System.Collections.Generic.KeyValuePair`2 + IL_000f: callvirt instance string [runtime]System.Object::ToString() + IL_0014: ret + } """] +#endif diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 3ef7efa769f..0b5e005d7b7 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -134,6 +134,7 @@ + From 537f5fd728e77da6dd7f783850e4858545f4df74 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 3 Oct 2022 16:20:01 +0200 Subject: [PATCH 5/6] Optimizing for readonly structs also within extension methods --- .../StructDefensiveCopy.fs | 67 ++++++++++++++++++- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs index b5270e299ea..61f9055b9ef 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs @@ -5,7 +5,7 @@ open System.IO open FSharp.Test open FSharp.Test.Compiler -let verifyCompilation expectedIl = +let verifyKeyValuePairInstanceMethodCall expectedIl = FSharp """ module StructUnion01 open System.Runtime.CompilerServices @@ -23,7 +23,7 @@ let doWork(kvp1:inref>) = // KeyValuePair defined as a readonly struct (in C#) [] let ``Defensive copy can be skipped on read-only structs``() = - verifyCompilation [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& kvp1) cil managed + verifyKeyValuePairInstanceMethodCall [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& kvp1) cil managed { .param [1] .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) @@ -41,7 +41,7 @@ let ``Defensive copy can be skipped on read-only structs``() = // KeyValuePair just a regular struct. Notice the "ldobj" instruction [] let ``Non readonly struct needs a defensive copy``() = - verifyCompilation [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& kvp1) cil managed + verifyKeyValuePairInstanceMethodCall [""" .method public static string doWork([in] valuetype [runtime]System.Collections.Generic.KeyValuePair`2& kvp1) cil managed { .param [1] .custom instance void [runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) @@ -57,3 +57,64 @@ let ``Non readonly struct needs a defensive copy``() = IL_0014: ret } """] #endif + +let verifyDateTimeExtensionMethodCall expectedIl = + FSharp """ +module DateTimeExtensionMethod + +open System +open System.Collections.Generic +open System.Runtime.CompilerServices + +[] +type DateTimeExtensions = + [] + static member PrintDate(d: inref) = d.ToString() + +let doWork(dt:inref) = + dt.PrintDate() + """ + |> ignoreWarnings + |> compile + |> shouldSucceed + |> verifyIL expectedIl + +#if NETSTANDARD +// DateTime defined as a readonly struct (in C#) +[] +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 +[] +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 \ No newline at end of file From cfbca1fb706132cb221d75db6fef58db3425c531 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 3 Oct 2022 16:37:32 +0200 Subject: [PATCH 6/6] Combined test of defining extension method on readonly struct in C# and then calling it from F# --- .../StructDefensiveCopy.fs | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs index 61f9055b9ef..fe7f88ff74a 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/StructDefensiveCopy/StructDefensiveCopy.fs @@ -117,4 +117,42 @@ let ``Non readonly struct needs a defensive copy when its extension method is ca IL_0011: callvirt instance string [runtime]System.Object::ToString() IL_0016: ret } """] -#endif \ No newline at end of file +#endif + + +#if NETSTANDARD +[] +#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) = + 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 + } """] +