diff --git a/src/Compiler/Checking/infos.fs b/src/Compiler/Checking/infos.fs index 1c0612eb611..1dc35e9ab09 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,22 @@ type MethInfo = member x.IsStruct = isStructTy x.TcGlobals x.ApparentEnclosingType - /// Indicates if this method is read-only; usually by the [] 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 [] 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 diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs b/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs index 1ba362eb86a..4e98ff92545 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerOptions/fsc/warn.fs +++ b/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 + [] + let ``no error 52 with readonly struct`` compilation = + compilation + |> asExe + |> withOptions ["--warn:5"; "--warnaserror:52"] + |> compile + |> shouldSucceed + |> ignore +#endif + [] let ``warn_level6_fs --warn:6`` compilation = 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..fe7f88ff74a --- /dev/null +++ b/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>) = + 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``() = + 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 ) + + .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``() = + 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 ) + + .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 + +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 + + +#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 + } """] + 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 @@ + 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