From d39a68053977b10307050e080405f94eeeed154e Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 15 Aug 2019 03:00:05 -0700 Subject: [PATCH 01/22] Added inref immutability assumption fix. Aware of IsReadOnly attribute, only on ILMethods. --- src/fsharp/MethodCalls.fs | 12 ++++++++++++ src/fsharp/TastOps.fs | 6 +++++- src/fsharp/infos.fs | 3 +++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/fsharp/MethodCalls.fs b/src/fsharp/MethodCalls.fs index 895f2d1f729..09906e62582 100644 --- a/src/fsharp/MethodCalls.fs +++ b/src/fsharp/MethodCalls.fs @@ -925,6 +925,12 @@ let ComputeConstrainedCallInfo g amap m (objArgs, minfo: MethInfo) = | _ -> None +let CanMethodNeverMutate g m (minfo: MethInfo) = + // Respect IsReadOnly attribute on IL methods for now. + minfo.IsILMethod && + minfo.IsStruct && + MethInfoHasAttribute g m g.attrib_IsReadOnlyAttribute minfo + /// Adjust the 'this' pointer before making a call /// Take the address of a struct, and coerce to an interface/base/constraint type if necessary let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = @@ -938,6 +944,12 @@ 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 = + if isMutable <> NeverMutates && CanMethodNeverMutate g m minfo then + NeverMutates + else + isMutable let wrap, objArgExprAddr, isReadOnly, _isWriteOnly = mkExprAddrOfExpr g mustTakeAddress hasCallInfo isMutable objArgExpr None m diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 18894044784..8656ae97c0e 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6008,7 +6008,11 @@ 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 + let destTy = destByrefTy g vref.Type + CanTakeAddressOf g vref.Range destTy mut && + // If we have a .NET defined struct, we return false. + // We do not want to use the same immutability assumption on .NET structs for inref. + (isFSharpStructTy g destTy || isEnumTy g destTy) let MustTakeAddressOfRecdField (rfref: RecdField) = // Static mutable fields must be private, hence we don't have to take their address diff --git a/src/fsharp/infos.fs b/src/fsharp/infos.fs index dd6f8b0e3f3..d7347eb8b7d 100755 --- a/src/fsharp/infos.fs +++ b/src/fsharp/infos.fs @@ -1238,6 +1238,9 @@ type MethInfo = member x.IsStruct = isStructTy x.TcGlobals x.ApparentEnclosingType + /// Indicates if this method is an IL method. + member x.IsILMethod = match x with ILMeth _ -> true | _ -> false + /// Build IL method infos. static member CreateILMeth (amap: Import.ImportMap, m, ty: TType, md: ILMethodDef) = let tinfo = ILTypeInfo.FromType amap.g ty From 72d1b9ec516fd69ce55939224743a83dd92ef71e Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 15 Aug 2019 03:16:30 -0700 Subject: [PATCH 02/22] Added isILStructTy --- src/fsharp/TastOps.fs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 8656ae97c0e..bc882c055a6 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -1679,6 +1679,11 @@ let isILReferenceTy g ty = | ILTypeMetadata (TILObjectReprData(_, _, td)) -> not td.IsStructOrEnum | FSharpOrArrayOrByrefOrTupleOrExnTypeMetadata -> isArrayTy g ty +let isILStructTy g ty = + match tryDestAppTy g ty with + | ValueSome tcref -> tcref.Deref.IsILStructOrEnumTycon + | _ -> false + let isILInterfaceTycon (tycon: Tycon) = match metadataOfTycon tycon with #if !NO_EXTENSIONTYPING @@ -6012,7 +6017,7 @@ let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = CanTakeAddressOf g vref.Range destTy mut && // If we have a .NET defined struct, we return false. // We do not want to use the same immutability assumption on .NET structs for inref. - (isFSharpStructTy g destTy || isEnumTy g destTy) + (not (isILStructTy g destTy) || isEnumTy g destTy) let MustTakeAddressOfRecdField (rfref: RecdField) = // Static mutable fields must be private, hence we don't have to take their address From 1866b330ccf6bc412dc55622fd94ba5f6be1586f Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 15 Aug 2019 22:38:40 -0700 Subject: [PATCH 03/22] targeting assumption --- src/fsharp/TastOps.fs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index bc882c055a6..1eb816ea657 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -5948,11 +5948,20 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets = type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates exception DefensiveCopyWarning of string * range +let canIgnoreILTyconRefImmutabilityAssumption g tcref = + tyconRefEq g tcref g.decimal_tcr || tyconRefEq g tcref g.date_tcr + +let hasILStructImmutabilityAssumption g ty = + if isILStructTy g ty then + let tcref, _ = destAppTy g ty + not (canIgnoreILTyconRefImmutabilityAssumption g tcref) + else + false + let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) = tcref.CanDeref && not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) || - tyconRefEq g tcref g.decimal_tcr || - tyconRefEq g tcref g.date_tcr + canIgnoreILTyconRefImmutabilityAssumption g tcref let isRecdOrStructTyconRefReadOnly (g: TcGlobals) m (tcref: TyconRef) = tcref.CanDeref && @@ -6013,11 +6022,7 @@ let MustTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) = let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = isInByrefTy g vref.Type && - let destTy = destByrefTy g vref.Type - CanTakeAddressOf g vref.Range destTy mut && - // If we have a .NET defined struct, we return false. - // We do not want to use the same immutability assumption on .NET structs for inref. - (not (isILStructTy g destTy) || isEnumTy g destTy) + CanTakeAddressOf g vref.Range (destByrefTy g vref.Type) mut let MustTakeAddressOfRecdField (rfref: RecdField) = // Static mutable fields must be private, hence we don't have to take their address @@ -6046,7 +6051,8 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress if mustTakeAddress then match expr with // LVALUE of "*x" where "x" is byref is just the byref itself - | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> + // Note: For this case, do not assume IL struct types are immutable. + | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || (CanTakeAddressOfByrefGet g vref mut && not (hasILStructImmutabilityAssumption g (destByrefTy g vref.Type))) -> let readonly = not (MustTakeAddressOfByrefGet g vref) let writeonly = isOutByrefTy g vref.Type None, exprForValRef m vref, readonly, writeonly From eef81f036f273f0b977cea9078e48139ce286898 Mon Sep 17 00:00:00 2001 From: TIHan Date: Fri, 16 Aug 2019 00:44:53 -0700 Subject: [PATCH 04/22] Fixed tests --- src/fsharp/MethodCalls.fs | 13 +++++++++---- src/fsharp/TastOps.fs | 33 ++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/fsharp/MethodCalls.fs b/src/fsharp/MethodCalls.fs index 09906e62582..02eb6917877 100644 --- a/src/fsharp/MethodCalls.fs +++ b/src/fsharp/MethodCalls.fs @@ -946,10 +946,15 @@ let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = let objArgTy = tyOfExpr g objArgExpr let isMutable = - if isMutable <> NeverMutates && CanMethodNeverMutate g m minfo then - NeverMutates - else - isMutable + match isMutable with + | DefinitelyMutates + | NeverMutates + | AddressOfOp -> isMutable + | PossiblyMutates -> + if CanMethodNeverMutate g m minfo then + NeverMutates + else + isMutable let wrap, objArgExprAddr, isReadOnly, _isWriteOnly = mkExprAddrOfExpr g mustTakeAddress hasCallInfo isMutable objArgExpr None m diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 1eb816ea657..20727b80ac6 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -5948,20 +5948,26 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets = type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates exception DefensiveCopyWarning of string * range -let canIgnoreILTyconRefImmutabilityAssumption g tcref = +/// Check if the tycon can ignore the immutability assumption. +let canILTyconRefIgnoreImmutabilityAssumption g tcref = tyconRefEq g tcref g.decimal_tcr || tyconRefEq g tcref g.date_tcr -let hasILStructImmutabilityAssumption g ty = - if isILStructTy g ty then +/// .NET struct types are assumed immutable, meaning none of their properties or methods mutate even if in reality they do. +/// This was from a decision made in F# 2.0, probably due to performance reasons from defensive copying. +/// Turning this assumption off will be a severe breaking change. +/// However, the assumption is turned off if we have an 'inref' of the struct type. +let isILStructTyWithImmutabilityAssumption g ty = + // Enums are not assumed immutable, they *are* immutable. + if isILStructTy g ty && not (isEnumTy g ty) then let tcref, _ = destAppTy g ty - not (canIgnoreILTyconRefImmutabilityAssumption g tcref) + not (canILTyconRefIgnoreImmutabilityAssumption g tcref) else false let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) = tcref.CanDeref && not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) || - canIgnoreILTyconRefImmutabilityAssumption g tcref + canILTyconRefIgnoreImmutabilityAssumption g tcref let isRecdOrStructTyconRefReadOnly (g: TcGlobals) m (tcref: TyconRef) = tcref.CanDeref && @@ -6021,8 +6027,18 @@ let MustTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) = isByrefTy g vref.Type && not (isInByrefTy g vref.Type) let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = - isInByrefTy g vref.Type && - CanTakeAddressOf g vref.Range (destByrefTy g vref.Type) mut + if isInByrefTy g vref.Type then + let destTy = destByrefTy g vref.Type + CanTakeAddressOf g vref.Range destTy mut && + match mut with + | DefinitelyMutates -> false + | NeverMutates + | AddressOfOp -> true + + // Disable immutability assumption on .NET structs when we have an 'inref'. + | PossiblyMutates -> not (isILStructTyWithImmutabilityAssumption g destTy) + else + false let MustTakeAddressOfRecdField (rfref: RecdField) = // Static mutable fields must be private, hence we don't have to take their address @@ -6051,8 +6067,7 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress if mustTakeAddress then match expr with // LVALUE of "*x" where "x" is byref is just the byref itself - // Note: For this case, do not assume IL struct types are immutable. - | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || (CanTakeAddressOfByrefGet g vref mut && not (hasILStructImmutabilityAssumption g (destByrefTy g vref.Type))) -> + | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> let readonly = not (MustTakeAddressOfByrefGet g vref) let writeonly = isOutByrefTy g vref.Type None, exprForValRef m vref, readonly, writeonly From d35ecf530536d7533f7df523b60e95867181abcd Mon Sep 17 00:00:00 2001 From: TIHan Date: Fri, 16 Aug 2019 01:02:26 -0700 Subject: [PATCH 05/22] renaming --- src/fsharp/TastOps.fs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 20727b80ac6..e8936060861 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -5948,26 +5948,26 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets = type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates exception DefensiveCopyWarning of string * range -/// Check if the tycon can ignore the immutability assumption. -let canILTyconRefIgnoreImmutabilityAssumption g tcref = +/// Check if the tycon is immutable but not assumed immutable. +let isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref = tyconRefEq g tcref g.decimal_tcr || tyconRefEq g tcref g.date_tcr /// .NET struct types are assumed immutable, meaning none of their properties or methods mutate even if in reality they do. /// This was from a decision made in F# 2.0, probably due to performance reasons from defensive copying. /// Turning this assumption off will be a severe breaking change. /// However, the assumption is turned off if we have an 'inref' of the struct type. -let isILStructTyWithImmutabilityAssumption g ty = +let isILStructTyAssumedImmutable g ty = // Enums are not assumed immutable, they *are* immutable. if isILStructTy g ty && not (isEnumTy g ty) then let tcref, _ = destAppTy g ty - not (canILTyconRefIgnoreImmutabilityAssumption g tcref) + not (isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref) else false let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) = tcref.CanDeref && not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) || - canILTyconRefIgnoreImmutabilityAssumption g tcref + isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref let isRecdOrStructTyconRefReadOnly (g: TcGlobals) m (tcref: TyconRef) = tcref.CanDeref && @@ -6035,8 +6035,8 @@ let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = | NeverMutates | AddressOfOp -> true - // Disable immutability assumption on .NET structs when we have an 'inref'. - | PossiblyMutates -> not (isILStructTyWithImmutabilityAssumption g destTy) + // Do not assume immutability on 'inref'. + | PossiblyMutates -> not (isILStructTyAssumedImmutable g destTy) else false From 07e2f3cc724bb2093ea21629a4246fc25a106a8d Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 03:00:11 -0700 Subject: [PATCH 06/22] Better IsReadOnly check --- src/fsharp/MethodCalls.fs | 9 ++------- src/fsharp/infos.fs | 21 ++++++++++++++++++--- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/fsharp/MethodCalls.fs b/src/fsharp/MethodCalls.fs index 02eb6917877..a3e94dc94d0 100644 --- a/src/fsharp/MethodCalls.fs +++ b/src/fsharp/MethodCalls.fs @@ -925,12 +925,6 @@ let ComputeConstrainedCallInfo g amap m (objArgs, minfo: MethInfo) = | _ -> None -let CanMethodNeverMutate g m (minfo: MethInfo) = - // Respect IsReadOnly attribute on IL methods for now. - minfo.IsILMethod && - minfo.IsStruct && - MethInfoHasAttribute g m g.attrib_IsReadOnlyAttribute minfo - /// Adjust the 'this' pointer before making a call /// Take the address of a struct, and coerce to an interface/base/constraint type if necessary let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = @@ -951,7 +945,8 @@ let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = | NeverMutates | AddressOfOp -> isMutable | PossiblyMutates -> - if CanMethodNeverMutate g m minfo then + // Check to see if the method is read-only. Perf optimization. + if mustTakeAddress && minfo.IsReadOnly then NeverMutates else isMutable diff --git a/src/fsharp/infos.fs b/src/fsharp/infos.fs index d7347eb8b7d..5d6cce0bc5b 100755 --- a/src/fsharp/infos.fs +++ b/src/fsharp/infos.fs @@ -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 [] 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,8 +1246,15 @@ type MethInfo = member x.IsStruct = isStructTy x.TcGlobals x.ApparentEnclosingType - /// Indicates if this method is an IL method. - member x.IsILMethod = match x with ILMeth _ -> true | _ -> false + /// Indicates if this method is read-only; usually by the [] attribute. + /// Method does not mutate the receiver's contents. + /// Receiver must be a struct type. + member x.IsReadOnly = + // Perf Review: Is there a way we can cache this result? + x.IsStruct && + match x with + | ILMeth (g, ilMethInfo, _) -> ilMethInfo.IsReadOnly g + | _ -> false /// Build IL method infos. static member CreateILMeth (amap: Import.ImportMap, m, ty: TType, md: ILMethodDef) = From 2a30b3e5b86b0bf393cf082ff04d61233fad9d41 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 03:32:25 -0700 Subject: [PATCH 07/22] Added tests --- src/fsharp/TastOps.fs | 15 ++----- src/fsharp/infos.fs | 1 + tests/fsharp/Compiler/CompilerAssert.fs | 4 +- tests/fsharp/Compiler/Language/ByrefTests.fs | 44 ++++++++++++++++++++ tests/fsharp/FSharpSuite.Tests.fsproj | 1 + 5 files changed, 52 insertions(+), 13 deletions(-) create mode 100644 tests/fsharp/Compiler/Language/ByrefTests.fs diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index e8936060861..0d7c3c3c7e1 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -5946,11 +5946,7 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets = //------------------------------------------------------------------------- type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates -exception DefensiveCopyWarning of string * range - -/// Check if the tycon is immutable but not assumed immutable. -let isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref = - tyconRefEq g tcref g.decimal_tcr || tyconRefEq g tcref g.date_tcr +exception DefensiveCopyWarning of string * range /// .NET struct types are assumed immutable, meaning none of their properties or methods mutate even if in reality they do. /// This was from a decision made in F# 2.0, probably due to performance reasons from defensive copying. @@ -5958,16 +5954,13 @@ let isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref = /// However, the assumption is turned off if we have an 'inref' of the struct type. let isILStructTyAssumedImmutable g ty = // Enums are not assumed immutable, they *are* immutable. - if isILStructTy g ty && not (isEnumTy g ty) then - let tcref, _ = destAppTy g ty - not (isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref) - else - false + isILStructTy g ty && not (isEnumTy g ty) let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) = tcref.CanDeref && not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) || - isSpecialTyconRefImmutableAndNotAssumedImmutable g tcref + tyconRefEq g tcref g.decimal_tcr || + tyconRefEq g tcref g.date_tcr let isRecdOrStructTyconRefReadOnly (g: TcGlobals) m (tcref: TyconRef) = tcref.CanDeref && diff --git a/src/fsharp/infos.fs b/src/fsharp/infos.fs index 5d6cce0bc5b..148e6eccedb 100755 --- a/src/fsharp/infos.fs +++ b/src/fsharp/infos.fs @@ -1254,6 +1254,7 @@ type MethInfo = 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 /// Build IL method infos. diff --git a/tests/fsharp/Compiler/CompilerAssert.fs b/tests/fsharp/Compiler/CompilerAssert.fs index 89bc15f5f2f..61058df1e5a 100644 --- a/tests/fsharp/Compiler/CompilerAssert.fs +++ b/tests/fsharp/Compiler/CompilerAssert.fs @@ -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 diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs new file mode 100644 index 00000000000..f53f9fc93c3 --- /dev/null +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -0,0 +1,44 @@ +// 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 + +[] +module ByrefTests = + + [] + let ``No defensive copy on .NET struct`` () = + CompilerAssert.Pass + """ +let f (x: int) = x.GetHashCode() +let f2 () = + let x = 1 + x.GetHashCode() + """ + + [] + let ``Defensive copy on .NET struct for inref`` () = + CompilerAssert.TypeCheckWithErrors + """ +let f (x: inref) = x.GetHashCode() +let f2 () = + let x = 1 + let y = &x + y.GetHashCode() + """ + [| + ( + FSharpErrorSeverity.Warning, + 52, + (2, 25, 2, 40), + "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, + (6, 5, 6, 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" + ) + |] diff --git a/tests/fsharp/FSharpSuite.Tests.fsproj b/tests/fsharp/FSharpSuite.Tests.fsproj index 1278cbe0be5..79adf613e01 100644 --- a/tests/fsharp/FSharpSuite.Tests.fsproj +++ b/tests/fsharp/FSharpSuite.Tests.fsproj @@ -51,6 +51,7 @@ + From 61dc6c3ae433b32117c5d6602d1e553e101b5ed1 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 04:43:00 -0700 Subject: [PATCH 08/22] Splitting read-only assumption and read-only attribute check --- src/fsharp/TastOps.fs | 42 ++++++++------- src/fsharp/tast.fs | 54 +++++++++++++++----- tests/fsharp/Compiler/Language/ByrefTests.fs | 1 + 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 0d7c3c3c7e1..5c03fc46201 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -3009,8 +3009,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) @@ -5948,31 +5948,33 @@ let mkAndSimplifyMatch spBind exprm matchm ty tree targets = type Mutates = AddressOfOp | DefinitelyMutates | PossiblyMutates | NeverMutates exception DefensiveCopyWarning of string * range -/// .NET struct types are assumed immutable, meaning none of their properties or methods mutate even if in reality they do. -/// This was from a decision made in F# 2.0, probably due to performance reasons from defensive copying. -/// Turning this assumption off will be a severe breaking change. -/// However, the assumption is turned off if we have an 'inref' of the struct type. -let isILStructTyAssumedImmutable g ty = - // Enums are not assumed immutable, they *are* immutable. - isILStructTy g ty && not (isEnumTy g ty) - let isRecdOrStructTyconRefAssumedImmutable (g: TcGlobals) (tcref: TyconRef) = tcref.CanDeref && not (isRecdOrUnionOrStructTyconRefDefinitelyMutable tcref) || 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 isTyconRefAssumedReadOnly g (tcref: TyconRef) = + tcref.CanDeref && + match tcref.TryIsAssumedReadOnly with + | ValueSome res -> res + | _ -> + let res = isRecdOrStructTyconRefAssumedImmutable g tcref + tcref.SetIsAssumedReadOnly res + res + +let isRecdOrStructTyconRefReadOnly g m tcref = + isTyconRefReadOnly g m tcref || isTyconRefAssumedReadOnly g tcref + let isRecdOrStructTyReadOnly (g: TcGlobals) m ty = match tryDestAppTy g ty with | ValueNone -> false @@ -6027,9 +6029,11 @@ let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = | DefinitelyMutates -> false | NeverMutates | AddressOfOp -> true - - // Do not assume immutability on 'inref'. - | PossiblyMutates -> not (isILStructTyAssumedImmutable g destTy) + | PossiblyMutates -> + // Do not assume immutability for 'inref'. + match tryDestAppTy g destTy with + | ValueSome tcref -> not (isTyconRefAssumedReadOnly g tcref) + | _ -> false else false diff --git a/src/fsharp/tast.fs b/src/fsharp/tast.fs index d59c33f700f..866b1704ef7 100644 --- a/src/fsharp/tast.fs +++ b/src/fsharp/tast.fs @@ -423,9 +423,9 @@ type EntityFlags(flags: int64) = /// These two bits represents the on-demand analysis about whether the entity has the IsByRefLike attribute member x.TryIsByRefLike = (flags &&& 0b000000011000000L) |> function - | 0b000000011000000L -> Some true - | 0b000000010000000L -> Some false - | _ -> None + | 0b000000011000000L -> ValueSome true + | 0b000000010000000L -> ValueSome false + | _ -> ValueNone /// Adjust the on-demand analysis about whether the entity has the IsByRefLike attribute member x.WithIsByRefLike flag = @@ -436,14 +436,14 @@ type EntityFlags(flags: int64) = | false -> 0b000000010000000L) EntityFlags flags - /// These two bits represents the on-demand analysis about whether the entity has the IsReadOnly attribute or is otherwise determined to be a readonly struct + /// These two bits represents the on-demand analysis about whether the entity has the IsReadOnly attribute member x.TryIsReadOnly = (flags &&& 0b000001100000000L) |> function - | 0b000001100000000L -> Some true - | 0b000001000000000L -> Some false - | _ -> None + | 0b000001100000000L -> ValueSome true + | 0b000001000000000L -> ValueSome false + | _ -> ValueNone - /// Adjust the on-demand analysis about whether the entity has the IsReadOnly attribute or is otherwise determined to be a readonly struct + /// Adjust the on-demand analysis about whether the entity has the IsReadOnly attribute member x.WithIsReadOnly flag = let flags = (flags &&& ~~~0b000001100000000L) ||| @@ -452,8 +452,24 @@ type EntityFlags(flags: int64) = | false -> 0b000001000000000L) EntityFlags flags + /// These two bits represents the on-demand analysis about whether the entity is assumed to be a readonly struct + member x.TryIsAssumedReadOnly = (flags &&& 0b000110000000000L) + |> function + | 0b000110000000000L -> ValueSome true + | 0b000100000000000L -> ValueSome false + | _ -> ValueNone + + /// Adjust the on-demand analysis about whether the entity is assumed to be a readonly struct + member x.WithIsAssumedReadOnly flag = + let flags = + (flags &&& ~~~0b000110000000000L) ||| + (match flag with + | true -> 0b000110000000000L + | false -> 0b000100000000000L) + EntityFlags flags + /// Get the flags as included in the F# binary metadata - member x.PickledBits = (flags &&& ~~~0b000001111000100L) + member x.PickledBits = (flags &&& ~~~0b000111111000100L) #if DEBUG @@ -1065,12 +1081,18 @@ and /// Represents a type definition, exception definition, module definition or /// Set the on-demand analysis about whether the entity has the IsByRefLike attribute member x.SetIsByRefLike b = x.entity_flags <- x.entity_flags.WithIsByRefLike b - /// These two bits represents the on-demand analysis about whether the entity has the IsReadOnly attribute or is otherwise determined to be a readonly struct + /// These two bits represents the on-demand analysis about whether the entity has the IsReadOnly attribute member x.TryIsReadOnly = x.entity_flags.TryIsReadOnly - /// Set the on-demand analysis about whether the entity has the IsReadOnly attribute or is otherwise determined to be a readonly struct + /// Set the on-demand analysis about whether the entity has the IsReadOnly attribute member x.SetIsReadOnly b = x.entity_flags <- x.entity_flags.WithIsReadOnly b + /// These two bits represents the on-demand analysis about whether the entity is assumed to be a readonly struct + member x.TryIsAssumedReadOnly = x.entity_flags.TryIsAssumedReadOnly + + /// Set the on-demand analysis about whether the entity is assumed to be a readonly struct + member x.SetIsAssumedReadOnly b = x.entity_flags <- x.entity_flags.WithIsAssumedReadOnly b + /// Indicates if this is an F# type definition whose r.h.s. is known to be some kind of F# object model definition member x.IsFSharpObjectModelTycon = match x.TypeReprInfo with | TFSharpObjectRepr _ -> true | _ -> false @@ -3551,12 +3573,18 @@ and /// Set the on-demand analysis about whether the entity has the IsByRefLike attribute member x.SetIsByRefLike b = x.Deref.SetIsByRefLike b - /// The on-demand analysis about whether the entity has the IsByRefLike attribute + /// The on-demand analysis about whether the entity has the IsReadOnly attribute member x.TryIsReadOnly = x.Deref.TryIsReadOnly - /// Set the on-demand analysis about whether the entity has the IsReadOnly attribute or is otherwise determined to be a readonly struct + /// Set the on-demand analysis about whether the entity has the IsReadOnly attribute member x.SetIsReadOnly b = x.Deref.SetIsReadOnly b + /// The on-demand analysis about whether the entity is assumed to be a readonly struct + member x.TryIsAssumedReadOnly = x.Deref.TryIsAssumedReadOnly + + /// Set the on-demand analysis about whether the entity is assumed to be a readonly struct + member x.SetIsAssumedReadOnly b = x.Deref.SetIsAssumedReadOnly b + /// Indicates if this is an F# type definition whose r.h.s. definition is unknown (i.e. a traditional ML 'abstract' type in a signature, /// which in F# is called a 'unknown representation' type). member x.IsHiddenReprTycon = x.Deref.IsHiddenReprTycon diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index f53f9fc93c3..fc215c8e036 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -20,6 +20,7 @@ let f2 () = [] let ``Defensive copy on .NET struct for inref`` () = + // Note: This particular test could fail in the future when 'int' is considered read-only. When that happens, consider a custom C# struct type as part of the test. CompilerAssert.TypeCheckWithErrors """ let f (x: inref) = x.GetHashCode() From 72499157f5bbcbe2f5ff4af702b79fbb7249b9fa Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 04:52:07 -0700 Subject: [PATCH 09/22] Func removal --- src/fsharp/TastOps.fs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 5c03fc46201..2cc0b9b5075 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -1679,11 +1679,6 @@ let isILReferenceTy g ty = | ILTypeMetadata (TILObjectReprData(_, _, td)) -> not td.IsStructOrEnum | FSharpOrArrayOrByrefOrTupleOrExnTypeMetadata -> isArrayTy g ty -let isILStructTy g ty = - match tryDestAppTy g ty with - | ValueSome tcref -> tcref.Deref.IsILStructOrEnumTycon - | _ -> false - let isILInterfaceTycon (tycon: Tycon) = match metadataOfTycon tycon with #if !NO_EXTENSIONTYPING From 687dbe55be20be84c64da14549ad6f1ab4dc4dd4 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 05:53:27 -0700 Subject: [PATCH 10/22] Better tests --- src/fsharp/TastOps.fs | 12 +++++-- tests/fsharp/Compiler/Language/ByrefTests.fs | 34 ++++++++++++++------ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 2cc0b9b5075..aa972f714aa 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -1679,6 +1679,11 @@ let isILReferenceTy g ty = | ILTypeMetadata (TILObjectReprData(_, _, td)) -> not td.IsStructOrEnum | FSharpOrArrayOrByrefOrTupleOrExnTypeMetadata -> isArrayTy g ty +let isILStructTy g ty = + match tryDestAppTy g ty with + | ValueSome tcref -> tcref.Deref.IsILStructOrEnumTycon + | _ -> false + let isILInterfaceTycon (tycon: Tycon) = match metadataOfTycon tycon with #if !NO_EXTENSIONTYPING @@ -6025,9 +6030,12 @@ let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = | NeverMutates | AddressOfOp -> true | PossiblyMutates -> - // Do not assume immutability for 'inref'. + // Do not assume immutability for IL types on 'inref'. match tryDestAppTy g destTy with - | ValueSome tcref -> not (isTyconRefAssumedReadOnly g tcref) + | ValueSome tcref -> + not (isILStructTy g destTy + && isTyconRefAssumedReadOnly g tcref + && not (isTyconRefReadOnly g vref.Range tcref)) | _ -> false else false diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index fc215c8e036..e94de29dc43 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -12,34 +12,50 @@ module ByrefTests = let ``No defensive copy on .NET struct`` () = CompilerAssert.Pass """ -let f (x: int) = x.GetHashCode() +open System +let f (x: DateTime) = x.ToLocalTime() let f2 () = - let x = 1 - x.GetHashCode() + let x = DateTime.Now + x.ToLocalTime() """ +#if NETCOREAPP + // NETCORE makes DateTime a readonly struct; therefore, it should not error. + [] + let ``No defensive copy on .NET struct - netcore`` () = + CompilerAssert.Pass + """ +open System +let f (x: inref) = x.ToLocalTime() +let f2 () = + let x = DateTime.Now + let y = &x + y.ToLocalTime() + """ +#else [] let ``Defensive copy on .NET struct for inref`` () = - // Note: This particular test could fail in the future when 'int' is considered read-only. When that happens, consider a custom C# struct type as part of the test. CompilerAssert.TypeCheckWithErrors """ -let f (x: inref) = x.GetHashCode() +open System +let f (x: inref) = x.ToLocalTime() let f2 () = - let x = 1 + let x = DateTime.Now let y = &x - y.GetHashCode() + y.ToLocalTime() """ [| ( FSharpErrorSeverity.Warning, 52, - (2, 25, 2, 40), + (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, - (6, 5, 6, 20), + (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" ) |] +#endif \ No newline at end of file From 761c7b6cc4af3542f6911053d1df18aec2438c7b Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 14:38:06 -0700 Subject: [PATCH 11/22] Trying to fix test --- src/fsharp/TastOps.fs | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index aa972f714aa..6c539be9fd5 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6022,23 +6022,24 @@ let MustTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) = isByrefTy g vref.Type && not (isInByrefTy g vref.Type) let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = - if isInByrefTy g vref.Type then - let destTy = destByrefTy g vref.Type - CanTakeAddressOf g vref.Range destTy mut && - match mut with - | DefinitelyMutates -> false - | NeverMutates - | AddressOfOp -> true - | PossiblyMutates -> - // Do not assume immutability for IL types on 'inref'. - match tryDestAppTy g destTy with - | ValueSome tcref -> - not (isILStructTy g destTy - && isTyconRefAssumedReadOnly g tcref - && not (isTyconRefReadOnly g vref.Range tcref)) - | _ -> false - else - false + isInByrefTy g vref.Type && + CanTakeAddressOf g vref.Range (destByrefTy g vref.Type) mut + +let NoDefensiveCopy g m ty mut = + isInByrefTy g ty && + match mut with + | DefinitelyMutates -> false + | NeverMutates + | AddressOfOp -> true + | PossiblyMutates -> + let destTy = destByrefTy g ty + // Do not assume immutability for IL types on 'inref'. + match tryDestAppTy g destTy with + | ValueSome tcref -> + not (isILStructTy g destTy && + isTyconRefAssumedReadOnly g tcref && + not (isTyconRefReadOnly g m tcref)) + | _ -> false let MustTakeAddressOfRecdField (rfref: RecdField) = // Static mutable fields must be private, hence we don't have to take their address @@ -6067,7 +6068,7 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress if mustTakeAddress then match expr with // LVALUE of "*x" where "x" is byref is just the byref itself - | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> + | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || (CanTakeAddressOfByrefGet g vref mut && NoDefensiveCopy g vref.Range vref.Type mut) -> let readonly = not (MustTakeAddressOfByrefGet g vref) let writeonly = isOutByrefTy g vref.Type None, exprForValRef m vref, readonly, writeonly From c6162620d0ce90ef2e0e58eec686f115a05432e2 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 21:16:41 -0700 Subject: [PATCH 12/22] More tests --- src/fsharp/TastOps.fs | 11 ++++------- tests/fsharp/Compiler/Language/ByrefTests.fs | 12 ++++++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 6c539be9fd5..2f15d863a83 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6023,22 +6023,19 @@ 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 - -let NoDefensiveCopy g m ty mut = - isInByrefTy g ty && + let destTy = destByrefTy g vref.Type + CanTakeAddressOf g vref.Range destTy mut && match mut with | DefinitelyMutates -> false | NeverMutates | AddressOfOp -> true | PossiblyMutates -> - let destTy = destByrefTy g ty // Do not assume immutability for IL types on 'inref'. match tryDestAppTy g destTy with | ValueSome tcref -> not (isILStructTy g destTy && isTyconRefAssumedReadOnly g tcref && - not (isTyconRefReadOnly g m tcref)) + not (isTyconRefReadOnly g vref.Range tcref)) | _ -> false let MustTakeAddressOfRecdField (rfref: RecdField) = @@ -6068,7 +6065,7 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress if mustTakeAddress then match expr with // LVALUE of "*x" where "x" is byref is just the byref itself - | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || (CanTakeAddressOfByrefGet g vref mut && NoDefensiveCopy g vref.Range vref.Type mut) -> + | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> let readonly = not (MustTakeAddressOfByrefGet g vref) let writeonly = isOutByrefTy g vref.Type None, exprForValRef m vref, readonly, writeonly diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index e94de29dc43..889516a8aed 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -31,6 +31,9 @@ let f2 () = let x = DateTime.Now let y = &x y.ToLocalTime() +let f3 (x: inref) = &x +let f4 (x: inref) = + (f3 &x).ToLocalTime() """ #else [] @@ -43,6 +46,9 @@ let f2 () = let x = DateTime.Now let y = &x y.ToLocalTime() +let f3 (x: inref) = &x +let f4 (x: inref) = + (f3 &x).ToLocalTime() """ [| ( @@ -57,5 +63,11 @@ let f2 () = (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" + ) |] #endif \ No newline at end of file From 071777e2b380f6ed77ab84185a1addb600be1908 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sat, 17 Aug 2019 22:43:09 -0700 Subject: [PATCH 13/22] Fixed extension member inref check --- src/fsharp/MethodCalls.fs | 8 +++++++- tests/fsharp/Compiler/Language/ByrefTests.fs | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/fsharp/MethodCalls.fs b/src/fsharp/MethodCalls.fs index a3e94dc94d0..6ee9c2e98fc 100644 --- a/src/fsharp/MethodCalls.fs +++ b/src/fsharp/MethodCalls.fs @@ -930,6 +930,11 @@ let ComputeConstrainedCallInfo g amap m (objArgs, minfo: MethInfo) = let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = let ccallInfo = ComputeConstrainedCallInfo g amap m (objArgs, minfo) + let inline isObjArgReadOnlyExtensionMember amap m (minfo: MethInfo) = + minfo.IsCSharpStyleExtensionMember && + minfo.TryObjArgByrefType(amap, m, minfo.FormalMethodInst) + |> Option.exists (isInByrefTy g) + let wrap, objArgs = match objArgs with @@ -946,7 +951,8 @@ let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = | AddressOfOp -> isMutable | PossiblyMutates -> // Check to see if the method is read-only. Perf optimization. - if mustTakeAddress && minfo.IsReadOnly then + // If there is an extension member whose obj arg (receiver) is an inref, we must return NeverMutates. + if mustTakeAddress && (minfo.IsReadOnly || isObjArgReadOnlyExtensionMember amap m minfo) then NeverMutates else isMutable diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index 889516a8aed..22935fda74a 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -13,10 +13,30 @@ module ByrefTests = CompilerAssert.Pass """ open System +open System.Runtime.CompilerServices + let f (x: DateTime) = x.ToLocalTime() let f2 () = let x = DateTime.Now x.ToLocalTime() + +[] +type Extensions = + + [] + static member Test(x: inref) = &x + + [] + static member Test2(x: byref) = &x + +let test (x: inref) = + x.Test() + +let test2 (x: byref) = + x.Test2() + +let test3 (x: byref) = + x.Test() """ #if NETCOREAPP From cad6369201f77582770154fdebf1887dfc3b5054 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sun, 18 Aug 2019 02:13:56 -0700 Subject: [PATCH 14/22] small cleanup --- src/fsharp/TastOps.fs | 49 ++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 2f15d863a83..f391084b731 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -1679,11 +1679,6 @@ let isILReferenceTy g ty = | ILTypeMetadata (TILObjectReprData(_, _, td)) -> not td.IsStructOrEnum | FSharpOrArrayOrByrefOrTupleOrExnTypeMetadata -> isArrayTy g ty -let isILStructTy g ty = - match tryDestAppTy g ty with - | ValueSome tcref -> tcref.Deref.IsILStructOrEnumTycon - | _ -> false - let isILInterfaceTycon (tycon: Tycon) = match metadataOfTycon tycon with #if !NO_EXTENSIONTYPING @@ -5972,18 +5967,27 @@ let isTyconRefAssumedReadOnly g (tcref: TyconRef) = 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 = - isTyconRefReadOnly g m tcref || isTyconRefAssumedReadOnly g tcref + isRecdOrStructTyconRefReadOnlyAux g m false tcref -let isRecdOrStructTyReadOnly (g: TcGlobals) m ty = +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 as a result @@ -6011,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 && @@ -6022,21 +6026,8 @@ let MustTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) = isByrefTy g vref.Type && not (isInByrefTy g vref.Type) let CanTakeAddressOfByrefGet (g: TcGlobals) (vref: ValRef) mut = - isInByrefTy g vref.Type && - let destTy = destByrefTy g vref.Type - CanTakeAddressOf g vref.Range destTy mut && - match mut with - | DefinitelyMutates -> false - | NeverMutates - | AddressOfOp -> true - | PossiblyMutates -> - // Do not assume immutability for IL types on 'inref'. - match tryDestAppTy g destTy with - | ValueSome tcref -> - not (isILStructTy g destTy && - isTyconRefAssumedReadOnly g tcref && - not (isTyconRefReadOnly g vref.Range tcref)) - | _ -> false + isInByrefTy g vref.Type && + 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 @@ -6049,14 +6040,14 @@ 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 /// 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 @@ -6194,7 +6185,7 @@ 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 let readonly = true let writeonly = false From f8b38131c8f6067d854938c4948cab7635c1bdeb Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sun, 18 Aug 2019 03:08:33 -0700 Subject: [PATCH 15/22] Added mkDereferencedByrefExpr --- src/fsharp/TastOps.fs | 4 ++++ src/fsharp/TastOps.fsi | 4 +++- src/fsharp/TypeChecker.fs | 9 +++------ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index f391084b731..39202771107 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -8991,3 +8991,7 @@ let isStaticClass (g:TcGlobals) (x: EntityRef) = #endif || (not x.IsILTycon && not x.IsProvided && HasFSharpAttribute g g.attrib_AbstractClassAttribute x.Attribs)) && not (HasFSharpAttribute g g.attrib_RequireQualifiedAccessAttribute x.Attribs) + +let mkDereferencedByrefExpr mAddrGet expr mExpr exprTy = + let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy + mkCompGenLet mExpr v expr (mkAddrGet mAddrGet (mkLocalValRef v)) diff --git a/src/fsharp/TastOps.fsi b/src/fsharp/TastOps.fsi index 1da22742300..716f2ea181b 100755 --- a/src/fsharp/TastOps.fsi +++ b/src/fsharp/TastOps.fsi @@ -2304,4 +2304,6 @@ val isThreadOrContextStatic: TcGlobals -> Attrib list -> bool val mkUnitDelayLambda: TcGlobals -> range -> Expr -> Expr -val isStaticClass: g: TcGlobals -> tcref: TyconRef -> bool \ No newline at end of file +val isStaticClass: g: TcGlobals -> tcref: TyconRef -> bool + +val mkDereferencedByrefExpr: mAddrGet: range -> expr: Expr -> mExpr: range -> exprTy: TType -> Expr \ No newline at end of file diff --git a/src/fsharp/TypeChecker.fs b/src/fsharp/TypeChecker.fs index bf70c63a178..1cc33891fc1 100644 --- a/src/fsharp/TypeChecker.fs +++ b/src/fsharp/TypeChecker.fs @@ -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 = mkDereferencedByrefExpr 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 = mkDereferencedByrefExpr 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)) + mkDereferencedByrefExpr mMethExpr callExpr0 mMethExpr vty else callExpr0 From c4f33dc3696e7ae2232fb6627e9c1a92a4cbeaad Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sun, 18 Aug 2019 16:05:04 -0700 Subject: [PATCH 16/22] Minor cleanup --- src/fsharp/MethodCalls.fs | 9 ++------- src/fsharp/infos.fs | 10 +++++++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/fsharp/MethodCalls.fs b/src/fsharp/MethodCalls.fs index 6ee9c2e98fc..cf590b69ee3 100644 --- a/src/fsharp/MethodCalls.fs +++ b/src/fsharp/MethodCalls.fs @@ -930,11 +930,6 @@ let ComputeConstrainedCallInfo g amap m (objArgs, minfo: MethInfo) = let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = let ccallInfo = ComputeConstrainedCallInfo g amap m (objArgs, minfo) - let inline isObjArgReadOnlyExtensionMember amap m (minfo: MethInfo) = - minfo.IsCSharpStyleExtensionMember && - minfo.TryObjArgByrefType(amap, m, minfo.FormalMethodInst) - |> Option.exists (isInByrefTy g) - let wrap, objArgs = match objArgs with @@ -951,8 +946,8 @@ let TakeObjAddrForMethodCall g amap (minfo: MethInfo) isMutable m objArgs f = | AddressOfOp -> isMutable | PossiblyMutates -> // Check to see if the method is read-only. Perf optimization. - // If there is an extension member whose obj arg (receiver) is an inref, we must return NeverMutates. - if mustTakeAddress && (minfo.IsReadOnly || isObjArgReadOnlyExtensionMember amap m minfo) then + // 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 diff --git a/src/fsharp/infos.fs b/src/fsharp/infos.fs index 148e6eccedb..4ca16f77cdd 100755 --- a/src/fsharp/infos.fs +++ b/src/fsharp/infos.fs @@ -1247,16 +1247,24 @@ type MethInfo = isStructTy x.TcGlobals x.ApparentEnclosingType /// Indicates if this method is read-only; usually by the [] attribute. - /// Method does not mutate the receiver's contents. + /// 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 From 477246efd7f45ae3f003c164828a923e129878d3 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sun, 18 Aug 2019 22:18:16 -0700 Subject: [PATCH 17/22] More tests. Changed how we deref addresses --- src/fsharp/TastOps.fs | 51 +++++++--- src/fsharp/TastOps.fsi | 7 +- src/fsharp/TypeChecker.fs | 6 +- tests/fsharp/Compiler/Language/ByrefTests.fs | 99 ++++++++++++++++++++ 4 files changed, 144 insertions(+), 19 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 39202771107..408440beca1 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6049,11 +6049,49 @@ let CanTakeAddressOfUnionFieldRef (g: TcGlobals) m (uref: UnionCaseRef) cidx tin not rfref.IsMutable && CanTakeAddressOf g m false (actualTyOfUnionFieldRef uref cidx tinst) mut +let mkDerefAddrExpr mAddrGet expr mExpr exprTy = + let rec remap mAddrGet expr mExpr exprTy contf = + match expr with + // This helps chained method calls by preventing defensive copies. + // To do this properly with the byref scoping rules, we need to dereference directly on the call, rather than the whole expression. + | Expr.Let (bind, bodyExpr, m, freeVars) -> + remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) + | _ -> + let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy + contf (mkCompGenLet mExpr v expr (mkAddrGet mAddrGet (mkLocalValRef v))) + remap mAddrGet expr mExpr exprTy id + +let tryEraseDerefAddr g expr mut = + let rec remap g expr mut contf = + match expr with + // LVALUE: "&meth(args)" where meth has a byref or inref return. Includes "&span.[idx]". + | Expr.Let (TBind(vref, e, _), Expr.Op (TOp.LValueOp (LByrefGet, vref2), _, _, _), _, _) + when (valRefEq g (mkLocalValRef vref) vref2) && + (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> + ValueSome (contf e) + + | Expr.Let (bind, bodyExpr, m, freeVars) -> + remap g bodyExpr mut (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) + + | _ -> + ValueNone + remap g expr mut id + /// 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 /// have intended effect (i.e. is a readonly pointer and/or a defensive copy). let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress mut expr addrExprVal m = if mustTakeAddress then + // Since we are trying to take an address of an expression, + // see if there is a dereferenced expression already. If so, just erase it and return. + match tryEraseDerefAddr g expr mut with + | ValueSome e -> + let ty = tyOfExpr g e + let readonly = isInByrefTy g ty + let writeonly = isOutByrefTy g ty + None, e, readonly, writeonly + | _ -> + match expr with // LVALUE of "*x" where "x" is byref is just the byref itself | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> @@ -6135,15 +6173,6 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress | _ -> false None, mkArrayElemAddress g (readonly, ilInstrReadOnlyAnnotation, isNativePtr, shape, elemTy, (aexpr :: args), m), readonly, writeonly - - // LVALUE: "&meth(args)" where meth has a byref or inref return. Includes "&span.[idx]". - | Expr.Let (TBind(vref, e, _), Expr.Op (TOp.LValueOp (LByrefGet, vref2), _, _, _), _, _) - when (valRefEq g (mkLocalValRef vref) vref2) && - (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> - let ty = tyOfExpr g e - let readonly = isInByrefTy g ty - let writeonly = isOutByrefTy g ty - None, e, readonly, writeonly // Give a nice error message for address-of-byref | Expr.Val (vref, _, m) when isByrefTy g vref.Type -> @@ -8991,7 +9020,3 @@ let isStaticClass (g:TcGlobals) (x: EntityRef) = #endif || (not x.IsILTycon && not x.IsProvided && HasFSharpAttribute g g.attrib_AbstractClassAttribute x.Attribs)) && not (HasFSharpAttribute g g.attrib_RequireQualifiedAccessAttribute x.Attribs) - -let mkDereferencedByrefExpr mAddrGet expr mExpr exprTy = - let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy - mkCompGenLet mExpr v expr (mkAddrGet mAddrGet (mkLocalValRef v)) diff --git a/src/fsharp/TastOps.fsi b/src/fsharp/TastOps.fsi index 716f2ea181b..6119007d52c 100755 --- a/src/fsharp/TastOps.fsi +++ b/src/fsharp/TastOps.fsi @@ -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 @@ -2304,6 +2307,4 @@ val isThreadOrContextStatic: TcGlobals -> Attrib list -> bool val mkUnitDelayLambda: TcGlobals -> range -> Expr -> Expr -val isStaticClass: g: TcGlobals -> tcref: TyconRef -> bool - -val mkDereferencedByrefExpr: mAddrGet: range -> expr: Expr -> mExpr: range -> exprTy: TType -> Expr \ No newline at end of file +val isStaticClass: g: TcGlobals -> tcref: TyconRef -> bool \ No newline at end of file diff --git a/src/fsharp/TypeChecker.fs b/src/fsharp/TypeChecker.fs index 1cc33891fc1..af828fd8228 100644 --- a/src/fsharp/TypeChecker.fs +++ b/src/fsharp/TypeChecker.fs @@ -3394,7 +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 expr = mkDereferencedByrefExpr m currentExpr currentExpr.Range enumElemTy + let expr = mkDerefAddrExpr m currentExpr currentExpr.Range enumElemTy expr, destByrefTy cenv.g enumElemTy else currentExpr, enumElemTy @@ -4139,7 +4139,7 @@ let buildApp cenv expr resultTy arg m = | _ when isByrefTy g resultTy -> // Handle byref returns, byref-typed returns get implicitly dereferenced let expr = expr.SupplyArgument (arg, m) - let expr = mkDereferencedByrefExpr m expr.Expr m resultTy + let expr = mkDerefAddrExpr m expr.Expr m resultTy let resultTy = destByrefTy g resultTy MakeApplicableExprNoFlex cenv expr, resultTy @@ -10212,7 +10212,7 @@ and TcMethodApplication // byref-typed returns get implicitly dereferenced let vty = tyOfExpr cenv.g callExpr0 if isByrefTy cenv.g vty then - mkDereferencedByrefExpr mMethExpr callExpr0 mMethExpr vty + mkDerefAddrExpr mMethExpr callExpr0 mMethExpr vty else callExpr0 diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index 22935fda74a..1b1490fc699 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -37,8 +37,80 @@ let test2 (x: byref) = let test3 (x: byref) = x.Test() + +let test4 () = + DateTime.Now.Test() + +let test5 (x: inref) = + &x.Test() """ + [] + let ``Extension method scope errors`` () = + CompilerAssert.TypeCheckWithErrors + """ +open System +open System.Runtime.CompilerServices + +[] +type Extensions = + + [] + static member Test(x: inref) = &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. [] @@ -54,8 +126,19 @@ let f2 () = let f3 (x: inref) = &x let f4 (x: inref) = (f3 &x).ToLocalTime() + +open System.Runtime.CompilerServices +[] +type Extensions = + + [] + static member Test(x: inref) = &x + +let test1 () = + DateTime.Now.Test().Date """ #else + // Note: Currently this is assuming NET472. That may change which might break these tests. Consider using custom C# code. [] let ``Defensive copy on .NET struct for inref`` () = CompilerAssert.TypeCheckWithErrors @@ -69,6 +152,16 @@ let f2 () = let f3 (x: inref) = &x let f4 (x: inref) = (f3 &x).ToLocalTime() + +open System.Runtime.CompilerServices +[] +type Extensions = + + [] + static member Test(x: inref) = &x + +let test1 () = + DateTime.Now.Test().Date """ [| ( @@ -89,5 +182,11 @@ let f4 (x: inref) = (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 From 120174f2300f978162ae95d7474ebbe2cf096d3a Mon Sep 17 00:00:00 2001 From: Will Smith Date: Sun, 18 Aug 2019 22:32:42 -0700 Subject: [PATCH 18/22] Update comment --- src/fsharp/TastOps.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 408440beca1..1639f6921ea 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6051,9 +6051,8 @@ let CanTakeAddressOfUnionFieldRef (g: TcGlobals) m (uref: UnionCaseRef) cidx tin let mkDerefAddrExpr mAddrGet expr mExpr exprTy = let rec remap mAddrGet expr mExpr exprTy contf = - match expr with - // This helps chained method calls by preventing defensive copies. // To do this properly with the byref scoping rules, we need to dereference directly on the call, rather than the whole expression. + match expr with | Expr.Let (bind, bodyExpr, m, freeVars) -> remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) | _ -> @@ -6070,6 +6069,7 @@ let tryEraseDerefAddr g expr mut = (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> ValueSome (contf e) + // This helps chained method calls by preventing defensive copies. | Expr.Let (bind, bodyExpr, m, freeVars) -> remap g bodyExpr mut (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) From 6e144eb822a5af1c75fe7d38bef1b3832e37f0a1 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Mon, 19 Aug 2019 01:40:29 -0700 Subject: [PATCH 19/22] Being more specific on dereference addr work --- src/fsharp/TastOps.fs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 1639f6921ea..959980a2621 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6053,7 +6053,9 @@ let mkDerefAddrExpr mAddrGet expr mExpr exprTy = let rec remap mAddrGet expr mExpr exprTy contf = // To do this properly with the byref scoping rules, we need to dereference directly on the call, rather than the whole expression. match expr with - | Expr.Let (bind, bodyExpr, m, freeVars) -> + | Expr.Let (bind, (Expr.App (Expr.Val _, _, _, _, _) as bodyExpr), m, freeVars) -> + remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) + | Expr.Let (bind, (Expr.Op (TOp.ILCall _, _, _, _) as bodyExpr), m, freeVars) -> remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) | _ -> let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy @@ -6069,8 +6071,7 @@ let tryEraseDerefAddr g expr mut = (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> ValueSome (contf e) - // This helps chained method calls by preventing defensive copies. - | Expr.Let (bind, bodyExpr, m, freeVars) -> + | Expr.Let (bind, (Expr.Let (_, Expr.Op (TOp.LValueOp (LByrefGet, _), _, _, _), _, _) as bodyExpr), m, freeVars) -> remap g bodyExpr mut (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) | _ -> From a633fc26af8b9547c7abbd1cee0345c83247d012 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Mon, 19 Aug 2019 03:55:58 -0700 Subject: [PATCH 20/22] Drastically simplified the solution --- src/fsharp/PostInferenceChecks.fs | 3 +- src/fsharp/TastOps.fs | 52 ++----- src/fsharp/tast.fs | 149 ++++++++++--------- tests/fsharp/Compiler/Language/ByrefTests.fs | 6 + 4 files changed, 102 insertions(+), 108 deletions(-) diff --git a/src/fsharp/PostInferenceChecks.fs b/src/fsharp/PostInferenceChecks.fs index dd1ec5c2af5..d97766ac1b9 100644 --- a/src/fsharp/PostInferenceChecks.fs +++ b/src/fsharp/PostInferenceChecks.fs @@ -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 diff --git a/src/fsharp/TastOps.fs b/src/fsharp/TastOps.fs index 959980a2621..55576c88d40 100644 --- a/src/fsharp/TastOps.fs +++ b/src/fsharp/TastOps.fs @@ -6050,49 +6050,14 @@ let CanTakeAddressOfUnionFieldRef (g: TcGlobals) m (uref: UnionCaseRef) cidx tin CanTakeAddressOf g m false (actualTyOfUnionFieldRef uref cidx tinst) mut let mkDerefAddrExpr mAddrGet expr mExpr exprTy = - let rec remap mAddrGet expr mExpr exprTy contf = - // To do this properly with the byref scoping rules, we need to dereference directly on the call, rather than the whole expression. - match expr with - | Expr.Let (bind, (Expr.App (Expr.Val _, _, _, _, _) as bodyExpr), m, freeVars) -> - remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) - | Expr.Let (bind, (Expr.Op (TOp.ILCall _, _, _, _) as bodyExpr), m, freeVars) -> - remap mAddrGet bodyExpr mExpr exprTy (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) - | _ -> - let v, _ = mkCompGenLocal mAddrGet "byrefReturn" exprTy - contf (mkCompGenLet mExpr v expr (mkAddrGet mAddrGet (mkLocalValRef v))) - remap mAddrGet expr mExpr exprTy id - -let tryEraseDerefAddr g expr mut = - let rec remap g expr mut contf = - match expr with - // LVALUE: "&meth(args)" where meth has a byref or inref return. Includes "&span.[idx]". - | Expr.Let (TBind(vref, e, _), Expr.Op (TOp.LValueOp (LByrefGet, vref2), _, _, _), _, _) - when (valRefEq g (mkLocalValRef vref) vref2) && - (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> - ValueSome (contf e) - - | Expr.Let (bind, (Expr.Let (_, Expr.Op (TOp.LValueOp (LByrefGet, _), _, _, _), _, _) as bodyExpr), m, freeVars) -> - remap g bodyExpr mut (contf << fun bodyExpr' -> Expr.Let (bind, bodyExpr', m, freeVars)) - - | _ -> - ValueNone - remap g expr mut id + 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 /// have intended effect (i.e. is a readonly pointer and/or a defensive copy). let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress mut expr addrExprVal m = if mustTakeAddress then - // Since we are trying to take an address of an expression, - // see if there is a dereferenced expression already. If so, just erase it and return. - match tryEraseDerefAddr g expr mut with - | ValueSome e -> - let ty = tyOfExpr g e - let readonly = isInByrefTy g ty - let writeonly = isOutByrefTy g ty - None, e, readonly, writeonly - | _ -> - match expr with // LVALUE of "*x" where "x" is byref is just the byref itself | Expr.Op (TOp.LValueOp (LByrefGet, vref), _, [], m) when MustTakeAddressOfByrefGet g vref || CanTakeAddressOfByrefGet g vref mut -> @@ -6174,6 +6139,15 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress | _ -> false None, mkArrayElemAddress g (readonly, ilInstrReadOnlyAnnotation, isNativePtr, shape, elemTy, (aexpr :: args), m), readonly, writeonly + + // LVALUE: "&meth(args)" where meth has a byref or inref return. Includes "&span.[idx]". + | Expr.Let (TBind(vref, e, _), Expr.Op (TOp.LValueOp (LByrefGet, vref2), _, _, _), _, _) + when (valRefEq g (mkLocalValRef vref) vref2) && + (MustTakeAddressOfByrefGet g vref2 || CanTakeAddressOfByrefGet g vref2 mut) -> + let ty = tyOfExpr g e + let readonly = isInByrefTy g ty + let writeonly = isOutByrefTy g ty + None, e, readonly, writeonly // Give a nice error message for address-of-byref | Expr.Val (vref, _, m) when isByrefTy g vref.Type -> @@ -6217,6 +6191,10 @@ let rec mkExprAddrOfExprAux g mustTakeAddress useReadonlyForGenericArrayAddress match mut with | 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 diff --git a/src/fsharp/tast.fs b/src/fsharp/tast.fs index 866b1704ef7..6e063cd8314 100644 --- a/src/fsharp/tast.fs +++ b/src/fsharp/tast.fs @@ -116,133 +116,137 @@ type ValFlags(flags: int64) = new (recValInfo, baseOrThis, isCompGen, inlineInfo, isMutable, isModuleOrMemberBinding, isExtensionMember, isIncrClassSpecialMember, isTyFunc, allowTypeInst, isGeneratedEventVal) = let flags = (match baseOrThis with - | BaseVal -> 0b0000000000000000000L - | CtorThisVal -> 0b0000000000000000010L - | NormalVal -> 0b0000000000000000100L - | MemberThisVal -> 0b0000000000000000110L) ||| - (if isCompGen then 0b0000000000000001000L - else 0b00000000000000000000L) ||| + | BaseVal -> 0b00000000000000000000L + | CtorThisVal -> 0b00000000000000000010L + | NormalVal -> 0b00000000000000000100L + | MemberThisVal -> 0b00000000000000000110L) ||| + (if isCompGen then 0b00000000000000001000L + else 0b000000000000000000000L) ||| (match inlineInfo with - | ValInline.PseudoVal -> 0b0000000000000000000L - | ValInline.Always -> 0b0000000000000010000L - | ValInline.Optional -> 0b0000000000000100000L - | ValInline.Never -> 0b0000000000000110000L) ||| + | ValInline.PseudoVal -> 0b00000000000000000000L + | ValInline.Always -> 0b00000000000000010000L + | ValInline.Optional -> 0b00000000000000100000L + | ValInline.Never -> 0b00000000000000110000L) ||| (match isMutable with - | Immutable -> 0b0000000000000000000L - | Mutable -> 0b0000000000001000000L) ||| + | Immutable -> 0b00000000000000000000L + | Mutable -> 0b00000000000001000000L) ||| (match isModuleOrMemberBinding with - | false -> 0b0000000000000000000L - | true -> 0b0000000000010000000L) ||| + | false -> 0b00000000000000000000L + | true -> 0b00000000000010000000L) ||| (match isExtensionMember with - | false -> 0b0000000000000000000L - | true -> 0b0000000000100000000L) ||| + | false -> 0b00000000000000000000L + | true -> 0b00000000000100000000L) ||| (match isIncrClassSpecialMember with - | false -> 0b0000000000000000000L - | true -> 0b0000000001000000000L) ||| + | false -> 0b00000000000000000000L + | true -> 0b00000000001000000000L) ||| (match isTyFunc with - | false -> 0b0000000000000000000L - | true -> 0b0000000010000000000L) ||| + | false -> 0b00000000000000000000L + | true -> 0b00000000010000000000L) ||| (match recValInfo with - | ValNotInRecScope -> 0b0000000000000000000L - | ValInRecScope true -> 0b0000000100000000000L - | ValInRecScope false -> 0b0000001000000000000L) ||| + | ValNotInRecScope -> 0b00000000000000000000L + | ValInRecScope true -> 0b00000000100000000000L + | ValInRecScope false -> 0b00000001000000000000L) ||| (match allowTypeInst with - | false -> 0b0000000000000000000L - | true -> 0b0000100000000000000L) ||| + | false -> 0b00000000000000000000L + | true -> 0b00000100000000000000L) ||| (match isGeneratedEventVal with - | false -> 0b0000000000000000000L - | true -> 0b0100000000000000000L) + | false -> 0b00000000000000000000L + | true -> 0b00100000000000000000L) ValFlags flags member x.BaseOrThisInfo = - match (flags &&& 0b0000000000000000110L) with - | 0b0000000000000000000L -> BaseVal - | 0b0000000000000000010L -> CtorThisVal - | 0b0000000000000000100L -> NormalVal - | 0b0000000000000000110L -> MemberThisVal + match (flags &&& 0b00000000000000000110L) with + | 0b00000000000000000000L -> BaseVal + | 0b00000000000000000010L -> CtorThisVal + | 0b00000000000000000100L -> NormalVal + | 0b00000000000000000110L -> MemberThisVal | _ -> failwith "unreachable" - member x.IsCompilerGenerated = (flags &&& 0b0000000000000001000L) <> 0x0L + member x.IsCompilerGenerated = (flags &&& 0b00000000000000001000L) <> 0x0L member x.SetIsCompilerGenerated isCompGen = - let flags = (flags &&& ~~~0b0000000000000001000L) ||| + let flags = (flags &&& ~~~0b00000000000000001000L) ||| (match isCompGen with - | false -> 0b0000000000000000000L - | true -> 0b0000000000000001000L) + | false -> 0b00000000000000000000L + | true -> 0b00000000000000001000L) ValFlags flags member x.InlineInfo = - match (flags &&& 0b0000000000000110000L) with - | 0b0000000000000000000L -> ValInline.PseudoVal - | 0b0000000000000010000L -> ValInline.Always - | 0b0000000000000100000L -> ValInline.Optional - | 0b0000000000000110000L -> ValInline.Never + match (flags &&& 0b00000000000000110000L) with + | 0b00000000000000000000L -> ValInline.PseudoVal + | 0b00000000000000010000L -> ValInline.Always + | 0b00000000000000100000L -> ValInline.Optional + | 0b00000000000000110000L -> ValInline.Never | _ -> failwith "unreachable" member x.MutabilityInfo = - match (flags &&& 0b0000000000001000000L) with - | 0b0000000000000000000L -> Immutable - | 0b0000000000001000000L -> Mutable + match (flags &&& 0b00000000000001000000L) with + | 0b00000000000000000000L -> Immutable + | 0b00000000000001000000L -> Mutable | _ -> failwith "unreachable" member x.IsMemberOrModuleBinding = - match (flags &&& 0b0000000000010000000L) with - | 0b0000000000000000000L -> false - | 0b0000000000010000000L -> true + match (flags &&& 0b00000000000010000000L) with + | 0b00000000000000000000L -> false + | 0b00000000000010000000L -> true | _ -> failwith "unreachable" - member x.WithIsMemberOrModuleBinding = ValFlags(flags ||| 0b0000000000010000000L) + member x.WithIsMemberOrModuleBinding = ValFlags(flags ||| 0b00000000000010000000L) - member x.IsExtensionMember = (flags &&& 0b0000000000100000000L) <> 0L + member x.IsExtensionMember = (flags &&& 0b00000000000100000000L) <> 0L - member x.IsIncrClassSpecialMember = (flags &&& 0b0000000001000000000L) <> 0L + member x.IsIncrClassSpecialMember = (flags &&& 0b00000000001000000000L) <> 0L - member x.IsTypeFunction = (flags &&& 0b0000000010000000000L) <> 0L + member x.IsTypeFunction = (flags &&& 0b00000000010000000000L) <> 0L - member x.RecursiveValInfo = match (flags &&& 0b0000001100000000000L) with - | 0b0000000000000000000L -> ValNotInRecScope - | 0b0000000100000000000L -> ValInRecScope true - | 0b0000001000000000000L -> ValInRecScope false + member x.RecursiveValInfo = match (flags &&& 0b00000001100000000000L) with + | 0b00000000000000000000L -> ValNotInRecScope + | 0b00000000100000000000L -> ValInRecScope true + | 0b00000001000000000000L -> ValInRecScope false | _ -> failwith "unreachable" member x.WithRecursiveValInfo recValInfo = let flags = - (flags &&& ~~~0b0000001100000000000L) ||| + (flags &&& ~~~0b00000001100000000000L) ||| (match recValInfo with - | ValNotInRecScope -> 0b0000000000000000000L - | ValInRecScope true -> 0b0000000100000000000L - | ValInRecScope false -> 0b0000001000000000000L) + | ValNotInRecScope -> 0b00000000000000000000L + | ValInRecScope true -> 0b00000000100000000000L + | ValInRecScope false -> 0b00000001000000000000L) ValFlags flags - member x.MakesNoCriticalTailcalls = (flags &&& 0b0000010000000000000L) <> 0L + member x.MakesNoCriticalTailcalls = (flags &&& 0b00000010000000000000L) <> 0L - member x.WithMakesNoCriticalTailcalls = ValFlags(flags ||| 0b0000010000000000000L) + member x.WithMakesNoCriticalTailcalls = ValFlags(flags ||| 0b00000010000000000000L) - member x.PermitsExplicitTypeInstantiation = (flags &&& 0b0000100000000000000L) <> 0L + member x.PermitsExplicitTypeInstantiation = (flags &&& 0b00000100000000000000L) <> 0L - member x.HasBeenReferenced = (flags &&& 0b0001000000000000000L) <> 0L + member x.HasBeenReferenced = (flags &&& 0b00001000000000000000L) <> 0L - member x.WithHasBeenReferenced = ValFlags(flags ||| 0b0001000000000000000L) + member x.WithHasBeenReferenced = ValFlags(flags ||| 0b00001000000000000000L) - member x.IsCompiledAsStaticPropertyWithoutField = (flags &&& 0b0010000000000000000L) <> 0L + member x.IsCompiledAsStaticPropertyWithoutField = (flags &&& 0b00010000000000000000L) <> 0L - member x.WithIsCompiledAsStaticPropertyWithoutField = ValFlags(flags ||| 0b0010000000000000000L) + member x.WithIsCompiledAsStaticPropertyWithoutField = ValFlags(flags ||| 0b00010000000000000000L) - member x.IsGeneratedEventVal = (flags &&& 0b0100000000000000000L) <> 0L + member x.IsGeneratedEventVal = (flags &&& 0b00100000000000000000L) <> 0L - member x.IsFixed = (flags &&& 0b1000000000000000000L) <> 0L + member x.IsFixed = (flags &&& 0b01000000000000000000L) <> 0L - member x.WithIsFixed = ValFlags(flags ||| 0b1000000000000000000L) + member x.WithIsFixed = ValFlags(flags ||| 0b01000000000000000000L) + + member x.IgnoresByrefScope = (flags &&& 0b10000000000000000000L) <> 0L + + member x.WithIgnoresByrefScope = ValFlags(flags ||| 0b10000000000000000000L) /// Get the flags as included in the F# binary metadata member x.PickledBits = @@ -250,7 +254,7 @@ type ValFlags(flags: int64) = // Clear the IsCompiledAsStaticPropertyWithoutField, only used to determine whether to use a true field for a value, and to eliminate the optimization info for observable bindings // Clear the HasBeenReferenced, only used to report "unreferenced variable" warnings and to help collect 'it' values in FSI.EXE // Clear the IsGeneratedEventVal, since there's no use in propagating specialname information for generated add/remove event vals - (flags &&& ~~~0b0011001100000000000L) + (flags &&& ~~~0b10011001100000000000L) /// Represents the kind of a type parameter [] @@ -2747,6 +2751,9 @@ and [] /// Indicates if the value is pinned/fixed member x.IsFixed = x.val_flags.IsFixed + /// Indicates if the value will ignore byref scoping rules + member x.IgnoresByrefScope = x.val_flags.IgnoresByrefScope + /// Indicates if this value allows the use of an explicit type instantiation (i.e. does it itself have explicit type arguments, /// or does it have a signature?) member x.PermitsExplicitTypeInstantiation = x.val_flags.PermitsExplicitTypeInstantiation @@ -2970,6 +2977,8 @@ and [] member x.SetIsFixed() = x.val_flags <- x.val_flags.WithIsFixed + member x.SetIgnoresByrefScope() = x.val_flags <- x.val_flags.WithIgnoresByrefScope + member x.SetValReprInfo info = match x.val_opt_data with | Some optData -> optData.val_repr_info <- info diff --git a/tests/fsharp/Compiler/Language/ByrefTests.fs b/tests/fsharp/Compiler/Language/ByrefTests.fs index 1b1490fc699..2797125d791 100644 --- a/tests/fsharp/Compiler/Language/ByrefTests.fs +++ b/tests/fsharp/Compiler/Language/ByrefTests.fs @@ -43,6 +43,9 @@ let test4 () = let test5 (x: inref) = &x.Test() + +let test6 () = + DateTime.Now.Test().Test().Test() """ [] @@ -136,6 +139,9 @@ type Extensions = 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. From 7225034bb8827d6b869dd1d4422f2e35e148edbb Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 19 Aug 2019 17:43:57 -0700 Subject: [PATCH 21/22] Added tests --- tests/fsharp/core/fsfromfsviacs/lib2.cs | 13 +++++ tests/fsharp/core/fsfromfsviacs/test.fsx | 69 ++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/tests/fsharp/core/fsfromfsviacs/lib2.cs b/tests/fsharp/core/fsfromfsviacs/lib2.cs index eb6d6acc141..449f926d7f4 100644 --- a/tests/fsharp/core/fsfromfsviacs/lib2.cs +++ b/tests/fsharp/core/fsfromfsviacs/lib2.cs @@ -124,4 +124,17 @@ public class ApiWrapper public static Func f4 = new Func((int arg1, string arg2, byte arg3, sbyte arg4) => arg1 + arg2.Length + 1 + arg3 + arg4); public static Func f5 = new Func((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 diff --git a/tests/fsharp/core/fsfromfsviacs/test.fsx b/tests/fsharp/core/fsfromfsviacs/test.fsx index 072a896c1eb..c429438882e 100644 --- a/tests/fsharp/core/fsfromfsviacs/test.fsx +++ b/tests/fsharp/core/fsfromfsviacs/test.fsx @@ -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) = + s.M(456) + s.X + + let someInrefFunc (s: inref) = + s.M(456) + s.X + + let someFuncReturn (s: NonReadOnlyStruct) = + s.X + + let someInrefFuncReturn (s: inref) = + 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) 123 + 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 From af76003e1215768c384d7596b811bc18bcf536c3 Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 19 Aug 2019 23:05:26 -0700 Subject: [PATCH 22/22] Verifying current behavior --- tests/fsharp/core/fsfromfsviacs/test.fsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fsharp/core/fsfromfsviacs/test.fsx b/tests/fsharp/core/fsfromfsviacs/test.fsx index c429438882e..47e2e359769 100644 --- a/tests/fsharp/core/fsfromfsviacs/test.fsx +++ b/tests/fsharp/core/fsfromfsviacs/test.fsx @@ -262,7 +262,7 @@ module TestStructs = check "hdlcjiklhen13" s.X 0 s.M(123) check "hdlcjiklhen14" s.X 123 - check "hdlcjiklhen15" (someFuncReturn s) 123 + check "hdlcjiklhen15" (someFuncReturn s) 0 // Technically a bug today, but test is to verify current behavior. check "hdlcjiklhen16" s.X 123 let test5 () =