未验证 提交 8dd81c64 编写于 作者: W Will Smith 提交者: GitHub

Added inref immutability assumption removal (#7407)

* Added inref immutability assumption fix. Aware of IsReadOnly attribute, only on ILMethods.

* Added isILStructTy

* targeting assumption

* Fixed tests

* renaming

* Better IsReadOnly check

* Added tests

* Splitting read-only assumption and read-only attribute check

* Func removal

* Better tests

* Trying to fix test

* More tests

* Fixed extension member inref check

* small cleanup

* Added mkDereferencedByrefExpr

* Minor cleanup

* More tests. Changed how we deref addresses

* Update comment

* Being more specific on dereference addr work

* Drastically simplified the solution

* Added tests

* Verifying current behavior
上级 650a166e
......@@ -938,6 +938,19 @@ let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f =
let hasCallInfo = ccallInfo.IsSome
let mustTakeAddress = hasCallInfo || minfo.ObjArgNeedsAddress(amap, m)
let objArgTy = tyOfExpr g objArgExpr
let isMutable =
match isMutable with
| DefinitelyMutates
| NeverMutates
| AddressOfOp -> isMutable
| PossiblyMutates ->
// Check to see if the method is read-only. Perf optimization.
// If there is an extension member whose first arg is an inref, we must return NeverMutates.
if mustTakeAddress && (minfo.IsReadOnly || minfo.IsReadOnlyExtensionMember (amap, m)) then
NeverMutates
else
isMutable
let wrap, objArgExprAddr, isReadOnly, _isWriteOnly =
mkExprAddrOfExpr g mustTakeAddress hasCallInfo isMutable objArgExpr None m
......
......@@ -263,7 +263,8 @@ let GetLimitValByRef cenv env m v =
{ scope = scope; flags = flags }
let LimitVal cenv (v: Val) limit =
cenv.limitVals.[v.Stamp] <- limit
if not v.IgnoresByrefScope then
cenv.limitVals.[v.Stamp] <- limit
let BindVal cenv env (v: Val) =
//printfn "binding %s..." v.DisplayName
......
......@@ -3004,8 +3004,8 @@ let isByrefTyconRef (g: TcGlobals) (tcref: TyconRef) =
let isByrefLikeTyconRef (g: TcGlobals) m (tcref: TyconRef) =
tcref.CanDeref &&
match tcref.TryIsByRefLike with
| Some res -> res
| None ->
| ValueSome res -> res
| _ ->
let res =
isByrefTyconRef g tcref ||
(isStructTyconRef tcref && TyconRefHasAttribute g m g.attrib_IsByRefLikeAttribute tcref)
......@@ -5941,34 +5941,53 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets =
//-------------------------------------------------------------------------
type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates
exception DefensiveCopyWarning of string * range
exception DefensiveCopyWarning of string * range
let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) =
tcref.CanDeref &&
not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) ||
tyconRefEq g tcref g.decimal_tcr ||
tyconRefEq g tcref g.decimal_tcr ||
tyconRefEq g tcref g.date_tcr
let isRecdOrStructTyconRefReadOnly (g: TcGlobals) m (tcref: TyconRef) =
let isTyconRefReadOnly g m (tcref: TyconRef) =
tcref.CanDeref &&
match tcref.TryIsReadOnly with
| Some res -> res
| None ->
let isImmutable = isRecdOrStructTyconRefAssumedImmutable g tcref
let hasAttrib = TyconRefHasAttribute g m g.attrib_IsReadOnlyAttribute tcref
let res = isImmutable || hasAttrib
| ValueSome res -> res
| _ ->
let res = TyconRefHasAttribute g m g.attrib_IsReadOnlyAttribute tcref
tcref.SetIsReadOnly res
res
let isRecdOrStructTyReadOnly (g: TcGlobals) m ty =
let isTyconRefAssumedReadOnly g (tcref: TyconRef) =
tcref.CanDeref &&
match tcref.TryIsAssumedReadOnly with
| ValueSome res -> res
| _ ->
let res = isRecdOrStructTyconRefAssumedImmutable g tcref
tcref.SetIsAssumedReadOnly res
res
let isRecdOrStructTyconRefReadOnlyAux g m isInref (tcref: TyconRef) =
if isInref && tcref.IsILStructOrEnumTycon then
isTyconRefReadOnly g m tcref
else
isTyconRefReadOnly g m tcref || isTyconRefAssumedReadOnly g tcref
let isRecdOrStructTyconRefReadOnly g m tcref =
isRecdOrStructTyconRefReadOnlyAux g m false tcref
let isRecdOrStructTyReadOnlyAux (g: TcGlobals) m isInref ty =
match tryDestAppTy g ty with
| ValueNone -> false
| ValueSome tcref -> isRecdOrStructTyconRefReadOnly g m tcref
| ValueSome tcref -> isRecdOrStructTyconRefReadOnlyAux g m isInref tcref
let isRecdOrStructTyReadOnly g m ty =
isRecdOrStructTyReadOnlyAux g m false ty
let CanTakeAddressOf g m ty mut =
let CanTakeAddressOf g m isInref ty mut =
match mut with
| NeverMutates -> true
| PossiblyMutates -> isRecdOrStructTyReadOnly g m ty
| PossiblyMutates -> isRecdOrStructTyReadOnlyAux g m isInref ty
| DefinitelyMutates -> false
| AddressOfOp -> true // you can take the address but you might get a (readonly) inref<T> as a result
......@@ -5996,7 +6015,7 @@ let CanTakeAddressOfImmutableVal (g: TcGlobals) m (vref: ValRef) mut =
// || valRefInThisAssembly g.compilingFslib vref
// This is because we don't actually guarantee to generate static backing fields for all values like these, e.g. simple constants "let x = 1".
// We always generate a static property but there is no field to take an address of
CanTakeAddressOf g m vref.Type mut
CanTakeAddressOf g m false vref.Type mut
let MustTakeAddressOfVal (g: TcGlobals) (vref: ValRef) =
vref.IsMutable &&
......@@ -6008,7 +6027,7 @@ let MustTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) =
let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut =
isInByrefTy g vref.Type &&
CanTakeAddressOf g vref.Range (destByrefTy g vref.Type) mut
CanTakeAddressOf g vref.Range true (destByrefTy g vref.Type) mut
let MustTakeAddressOfRecdField (rfref: RecdField) =
// Static mutable fields must be private, hence we don't have to take their address
......@@ -6021,14 +6040,18 @@ let CanTakeAddressOfRecdFieldRef (g: TcGlobals) m (rfref: RecdFieldRef) tinst mu
// We only do this if the field is defined in this assembly because we can't take addresses across assemblies for immutable fields
entityRefInThisAssembly g.compilingFslib rfref.TyconRef &&
not rfref.RecdField.IsMutable &&
CanTakeAddressOf g m (actualTyOfRecdFieldRef rfref tinst) mut
CanTakeAddressOf g m false (actualTyOfRecdFieldRef rfref tinst) mut
let CanTakeAddressOfUnionFieldRef (g: TcGlobals) m (uref: UnionCaseRef) cidx tinst mut =
// We only do this if the field is defined in this assembly because we can't take addresses across assemblies for immutable fields
entityRefInThisAssembly g.compilingFslib uref.TyconRef &&
let rfref = uref.FieldByIndex cidx
not rfref.IsMutable &&
CanTakeAddressOf g m (actualTyOfUnionFieldRef uref cidx tinst) mut
CanTakeAddressOf g m false (actualTyOfUnionFieldRef uref cidx tinst) mut
let mkDerefAddrExpr mAddrGet expr mExpr exprTy =
let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy
mkCompGenLet mExpr v expr (mkAddrGet mAddrGet (mkLocalValRef v))
/// Make the address-of expression and return a wrapper that adds any allocated locals at an appropriate scope.
/// Also return a flag that indicates if the resulting pointer is a not a pointer where writing is allowed and will
......@@ -6166,8 +6189,12 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress
// Take a defensive copy
let tmp, _ =
match mut with
| NeverMutates -> mkCompGenLocal m "copyOfStruct" ty
| NeverMutates -> mkCompGenLocal m "copyOfStruct" ty
| _ -> mkMutableCompGenLocal m "copyOfStruct" ty
// This local is special in that it ignore byref scoping rules.
tmp.SetIgnoresByrefScope()
let readonly = true
let writeonly = false
Some (tmp, expr), (mkValAddr m readonly (mkLocalValRef tmp)), readonly, writeonly
......
......@@ -369,6 +369,9 @@ exception DefensiveCopyWarning of string * range
type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates
/// Helper to create an expression that dereferences an address.
val mkDerefAddrExpr: mAddrGet: range -> expr: Expr -> mExpr: range -> exprTy: TType -> Expr
/// Helper to take the address of an expression
val mkExprAddrOfExprAux : TcGlobals -> bool -> bool -> Mutates -> Expr -> ValRef option -> range -> (Val * Expr) option * Expr * bool * bool
......
......@@ -3394,8 +3394,7 @@ let AnalyzeArbitraryExprAsEnumerable cenv (env: TcEnv) localAlloc m exprty expr
let currentExpr, enumElemTy =
// Implicitly dereference byref for expr 'for x in ...'
if isByrefTy cenv.g enumElemTy then
let v, _ = mkCompGenLocal m "byrefReturn" enumElemTy
let expr = mkCompGenLet currentExpr.Range v currentExpr (mkAddrGet m (mkLocalValRef v))
let expr = mkDerefAddrExpr m currentExpr currentExpr.Range enumElemTy
expr, destByrefTy cenv.g enumElemTy
else
currentExpr, enumElemTy
......@@ -4139,9 +4138,8 @@ let buildApp cenv expr resultTy arg m =
| _ when isByrefTy g resultTy ->
// Handle byref returns, byref-typed returns get implicitly dereferenced
let v, _ = mkCompGenLocal m "byrefReturn" resultTy
let expr = expr.SupplyArgument (arg, m)
let expr = mkCompGenLet m v expr.Expr (mkAddrGet m (mkLocalValRef v))
let expr = mkDerefAddrExpr m expr.Expr m resultTy
let resultTy = destByrefTy g resultTy
MakeApplicableExprNoFlex cenv expr, resultTy
......@@ -10214,8 +10212,7 @@ and TcMethodApplication
// byref-typed returns get implicitly dereferenced
let vty = tyOfExpr cenv.g callExpr0
if isByrefTy cenv.g vty then
let v, _ = mkCompGenLocal mMethExpr "byrefReturn" vty
mkCompGenLet mMethExpr v callExpr0 (mkAddrGet mMethExpr (mkLocalValRef v))
mkDerefAddrExpr mMethExpr callExpr0 mMethExpr vty
else
callExpr0
......
......@@ -835,7 +835,15 @@ type ILMethInfo =
member x.IsDllImport (g: TcGlobals) =
match g.attrib_DllImportAttribute with
| None -> false
| Some (AttribInfo(tref, _)) ->x.RawMetadata.CustomAttrs |> TryDecodeILAttribute g tref |> Option.isSome
| Some attr ->
x.RawMetadata.CustomAttrs
|> TryFindILAttribute attr
/// Indicates if the method is marked with the [<IsReadOnly>] attribute. This is done by looking at the IL custom attributes on
/// the method.
member x.IsReadOnly (g: TcGlobals) =
x.RawMetadata.CustomAttrs
|> TryFindILAttribute g.attrib_IsReadOnlyAttribute
/// Get the (zero or one) 'self'/'this'/'object' arguments associated with an IL method.
/// An instance extension method returns one object argument.
......@@ -1238,6 +1246,25 @@ type MethInfo =
member x.IsStruct =
isStructTy x.TcGlobals x.ApparentEnclosingType
/// Indicates if this method is read-only; usually by the [<IsReadOnly>] attribute.
/// 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?
x.IsInstance &&
x.IsStruct &&
match x with
| ILMeth (g, ilMethInfo, _) -> ilMethInfo.IsReadOnly g
| FSMeth _ -> false // F# defined methods not supported yet. Must be a language feature.
| _ -> false
/// Indicates if this method is an extension member that is read-only.
/// An extension member is considered read-only if the first argument is a read-only byref (inref) type.
member x.IsReadOnlyExtensionMember (amap: Import.ImportMap, m) =
x.IsExtensionMember &&
x.TryObjArgByrefType(amap, m, x.FormalMethodInst)
|> Option.exists (isInByrefTy amap.g)
/// Build IL method infos.
static member CreateILMeth (amap: Import.ImportMap, m, ty: TType, md: ILMethodDef) =
let tinfo = ILTypeInfo.FromType amap.g ty
......
此差异已折叠。
......@@ -108,11 +108,11 @@ let main argv = 0"""
ProjectId = None
SourceFiles = [|"test.fs"|]
#if !NETCOREAPP
OtherOptions = [|"--preferreduilang:en-US";|]
OtherOptions = [|"--preferreduilang:en-US";"--warn:5"|]
#else
OtherOptions =
let assemblies = getNetCoreAppReferences |> Array.map (fun x -> sprintf "-r:%s" x)
Array.append [|"--preferreduilang:en-US"; "--targetprofile:netcore"; "--noframework"|] assemblies
Array.append [|"--preferreduilang:en-US"; "--targetprofile:netcore"; "--noframework";"--warn:5"|] assemblies
#endif
ReferencedProjects = [||]
IsIncompleteTypeCheckEnvironment = false
......
// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information.
namespace FSharp.Compiler.UnitTests
open NUnit.Framework
open FSharp.Compiler.SourceCodeServices
[<TestFixture>]
module ByrefTests =
[<Test>]
let ``No defensive copy on .NET struct`` () =
CompilerAssert.Pass
"""
open System
open System.Runtime.CompilerServices
let f (x: DateTime) = x.ToLocalTime()
let f2 () =
let x = DateTime.Now
x.ToLocalTime()
[<Extension; AbstractClass; Sealed>]
type Extensions =
[<Extension>]
static member Test(x: inref<DateTime>) = &x
[<Extension>]
static member Test2(x: byref<DateTime>) = &x
let test (x: inref<DateTime>) =
x.Test()
let test2 (x: byref<DateTime>) =
x.Test2()
let test3 (x: byref<DateTime>) =
x.Test()
let test4 () =
DateTime.Now.Test()
let test5 (x: inref<DateTime>) =
&x.Test()
let test6 () =
DateTime.Now.Test().Test().Test()
"""
[<Test>]
let ``Extension method scope errors`` () =
CompilerAssert.TypeCheckWithErrors
"""
open System
open System.Runtime.CompilerServices
[<Extension; AbstractClass; Sealed>]
type Extensions =
[<Extension>]
static member Test(x: inref<DateTime>) = &x
let f1 () =
&DateTime.Now.Test()
let f2 () =
let result =
let dt = DateTime.Now
&dt.Test()
result
let f3 () =
Extensions.Test(let dt = DateTime.Now in &dt)
let f4 () =
let dt = DateTime.Now
&Extensions.Test(&dt)
let f5 () =
&Extensions.Test(let dt = DateTime.Now in &dt)
"""
[|
(
FSharpErrorSeverity.Error,
3228,
(12, 6, 12, 25),
"The address of a value returned from the expression cannot be used at this point. This is to ensure the address of the local value does not escape its scope."
)
(
FSharpErrorSeverity.Error,
3228,
(17, 10, 17, 19),
"The address of a value returned from the expression cannot be used at this point. This is to ensure the address of the local value does not escape its scope."
)
(
FSharpErrorSeverity.Error,
3228,
(21, 5, 21, 50),
"The address of a value returned from the expression cannot be used at this point. This is to ensure the address of the local value does not escape its scope."
)
(
FSharpErrorSeverity.Error,
3228,
(25, 6, 25, 26),
"The address of a value returned from the expression cannot be used at this point. This is to ensure the address of the local value does not escape its scope."
)
(
FSharpErrorSeverity.Error,
3228,
(28, 6, 28, 51),
"The address of a value returned from the expression cannot be used at this point. This is to ensure the address of the local value does not escape its scope."
)
|]
// TODO: A better way to test the ones below are to use a custom struct in C# code that contains explicit use of their "readonly" keyword.
#if NETCOREAPP
// NETCORE makes DateTime a readonly struct; therefore, it should not error.
[<Test>]
let ``No defensive copy on .NET struct - netcore`` () =
CompilerAssert.Pass
"""
open System
let f (x: inref<DateTime>) = x.ToLocalTime()
let f2 () =
let x = DateTime.Now
let y = &x
y.ToLocalTime()
let f3 (x: inref<DateTime>) = &x
let f4 (x: inref<DateTime>) =
(f3 &x).ToLocalTime()
open System.Runtime.CompilerServices
[<Extension; AbstractClass; Sealed>]
type Extensions =
[<Extension>]
static member Test(x: inref<DateTime>) = &x
let test1 () =
DateTime.Now.Test().Date
let test2 () =
DateTime.Now.Test().Test().Date.Test().Test().Date.Test()
"""
#else
// Note: Currently this is assuming NET472. That may change which might break these tests. Consider using custom C# code.
[<Test>]
let ``Defensive copy on .NET struct for inref`` () =
CompilerAssert.TypeCheckWithErrors
"""
open System
let f (x: inref<DateTime>) = x.ToLocalTime()
let f2 () =
let x = DateTime.Now
let y = &x
y.ToLocalTime()
let f3 (x: inref<DateTime>) = &x
let f4 (x: inref<DateTime>) =
(f3 &x).ToLocalTime()
open System.Runtime.CompilerServices
[<Extension; AbstractClass; Sealed>]
type Extensions =
[<Extension>]
static member Test(x: inref<DateTime>) = &x
let test1 () =
DateTime.Now.Test().Date
"""
[|
(
FSharpErrorSeverity.Warning,
52,
(3, 30, 3, 45),
"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"
)
(
FSharpErrorSeverity.Warning,
52,
(7, 5, 7, 20),
"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"
)
(
FSharpErrorSeverity.Warning,
52,
(10, 5, 10, 26),
"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"
)
(
FSharpErrorSeverity.Warning,
52,
(20, 5, 20, 29),
"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"
)
|]
#endif
\ No newline at end of file
......@@ -51,6 +51,7 @@
<Compile Include="Compiler\ErrorMessages\DontSuggestTests.fs" />
<Compile Include="Compiler\Warnings\AssignmentWarningTests.fs" />
<Compile Include="Compiler\SourceTextTests.fs" />
<Compile Include="Compiler\Language\ByrefTests.fs" />
<Compile Include="Compiler\Language\AnonRecordTests.fs" />
<Compile Include="Compiler\Language\OpenStaticClasses.fs" />
<Compile Include="Compiler\Language\SpanOptimizationTests.fs" />
......
......@@ -124,4 +124,17 @@ public class ApiWrapper
public static Func<int, string, byte, sbyte, int> f4 = new Func<int, string, byte, sbyte, int>((int arg1, string arg2, byte arg3, sbyte arg4) => arg1 + arg2.Length + 1 + arg3 + arg4);
public static Func<int, string, byte, sbyte, Int16, int> f5 = new Func<int, string, byte, sbyte, Int16, int>((int arg1, string arg2, byte arg3, sbyte arg4, Int16 arg5) => arg1 + arg2.Length + 1 + arg3 + arg4 + arg5);
}
}
namespace StructTests
{
public struct NonReadOnlyStruct
{
public int X { get; set; }
public void M(int x)
{
X = x;
}
}
}
\ No newline at end of file
......@@ -210,6 +210,75 @@ let ToFSharpFunc() =
test "vkejhwew904" (FuncConvert.FromFunc(FSharpFuncTests.ApiWrapper.f4)(3)("a")(6uy)(7y) = FSharpFuncTests.ApiWrapper.f4.Invoke(3, "a", 6uy, 7y))
test "vkejhwew905" (FuncConvert.FromFunc(FSharpFuncTests.ApiWrapper.f5)(3)("a")(6uy)(7y)(7s) = FSharpFuncTests.ApiWrapper.f5.Invoke(3, "a", 6uy, 7y, 7s))
module TestStructs =
open StructTests
let someFunc (s: NonReadOnlyStruct) =
s.M(456)
s.X
let someByrefFunc (s: byref<NonReadOnlyStruct>) =
s.M(456)
s.X
let someInrefFunc (s: inref<NonReadOnlyStruct>) =
s.M(456)
s.X
let someFuncReturn (s: NonReadOnlyStruct) =
s.X
let someInrefFuncReturn (s: inref<NonReadOnlyStruct>) =
s.X
let test1 () =
let s = NonReadOnlyStruct()
check "hdlcjiklhen1" s.X 0
s.M(123)
check "hdlcjiklhen2" s.X 123
check "hdlcjiklhen3" (someFunc s) 456
check "hdlcjiklhen4" s.X 123
let test2 () =
let mutable s = NonReadOnlyStruct()
check "hdlcjiklhen5" s.X 0
s.M(123)
check "hdlcjiklhen6" s.X 123
check "hdlcjiklhen7" (someByrefFunc &s) 456
check "hdlcjiklhen8" s.X 456
let test3 () =
let s = NonReadOnlyStruct()
check "hdlcjiklhen9" s.X 0
s.M(123)
check "hdlcjiklhen10" s.X 123
check "hdlcjiklhen11" (someInrefFunc &s) 123
check "hdlcjiklhen12" s.X 123
let test4 () =
let s = NonReadOnlyStruct()
check "hdlcjiklhen13" s.X 0
s.M(123)
check "hdlcjiklhen14" s.X 123
check "hdlcjiklhen15" (someFuncReturn s) 0 // Technically a bug today, but test is to verify current behavior.
check "hdlcjiklhen16" s.X 123
let test5 () =
let s = NonReadOnlyStruct()
check "hdlcjiklhen17" s.X 0
s.M(123)
check "hdlcjiklhen18" s.X 123
check "hdlcjiklhen19" (someInrefFuncReturn &s) 123
check "hdlcjiklhen20" s.X 123
TestStructs.test1 ()
TestStructs.test2 ()
TestStructs.test3 ()
TestStructs.test4 ()
TestStructs.test5 ()
#endif
#if TESTS_AS_APP
......
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册