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

Author: TIHan

src/fsharp/MethodCalls.fs

@@ -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

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

src/fsharp/TastOps.fs

@@ -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

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
 

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 = 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
 

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 [<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