Nullness :: Improvement :: Overrides of generic methods with nullable type instantiations (#18337)

* issue analysis

* No nullness warning if override passes in explicit nullable type argument

* notes

Author: T-Gro

docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@@ -11,6 +11,7 @@
 * Cancellable: set token in more places ([PR #18283](https://github.com/dotnet/fsharp/pull/18283))
 * Cancellable: fix leaking cancellation token ([PR #18295](https://github.com/dotnet/fsharp/pull/18295))
 * Fix NRE when accessing nullable fields of types within their equals/hash/compare methods ([PR #18296](https://github.com/dotnet/fsharp/pull/18296))
+* Fix nullness warning for overrides of generic code with nullable type instance ([Issue #17988](https://github.com/dotnet/fsharp/issues/17988), [PR #18337](https://github.com/dotnet/fsharp/pull/18337))
 
 ### Added
 * Added missing type constraints in FCS. ([PR #18241](https://github.com/dotnet/fsharp/pull/18241))

src/Compiler/Checking/Expressions/CheckExpressions.fs

@@ -1890,7 +1890,11 @@ let FreshenAbstractSlot g amap m synTyparDecls absMethInfo =
 
     // Work out the required type of the member
     let argTysFromAbsSlot = argTys |> List.mapSquared (instType typarInstFromAbsSlot)
-    let retTyFromAbsSlot = retTy |> GetFSharpViewOfReturnType g |> instType typarInstFromAbsSlot
+
+    let retTyFromAbsSlot = 
+        retTy 
+        |> GetFSharpViewOfReturnType g        
+        |> instType typarInstFromAbsSlot
     typarsFromAbsSlotAreRigid, typarsFromAbsSlot, argTysFromAbsSlot, retTyFromAbsSlot
 
 let CheckRecdExprDuplicateFields (elems: Ident list) =
@@ -11774,16 +11778,26 @@ and ApplyAbstractSlotInference (cenv: cenv) (envinner: TcEnv) (_: Val option) (a
                  match uniqueAbstractMethSigs with
                  | uniqueAbstractMeth :: _ ->
 
+                    // Overrides can narrow the retTy from nullable to not-null.
+                     // By changing nullness to be variable we do not get in the way of eliminating nullness (=good).
+                     // We only keep a WithNull nullness if it was part of an explicit type instantiation
+                     let canChangeNullableRetTy = 
+                        match g.checkNullness, renaming with
+                        | false, _ -> false
+                        | true, [] -> true
+                        | true, _ -> not(uniqueAbstractMeth.HasGenericRetTy())
+
                      let uniqueAbstractMeth = uniqueAbstractMeth.Instantiate(cenv.amap, m, renaming)
 
                      let typarsFromAbsSlotAreRigid, typarsFromAbsSlot, argTysFromAbsSlot, retTyFromAbsSlot =
                          FreshenAbstractSlot g cenv.amap m synTyparDecls uniqueAbstractMeth
 
                      let declaredTypars = (if typarsFromAbsSlotAreRigid then typarsFromAbsSlot else declaredTypars)
 
-                     // Overrides can narrow the retTy from nullable to not-null.
-                     // By changing nullness to be variable we do not get in the way of eliminating nullness (=good).
-                     let retTyFromAbsSlot = retTyFromAbsSlot |> changeWithNullReqTyToVariable g
+                     let retTyFromAbsSlot = 
+                        if canChangeNullableRetTy then
+                            retTyFromAbsSlot |> changeWithNullReqTyToVariable g
+                        else retTyFromAbsSlot 
 
                      let absSlotTy = mkMethodTy g argTysFromAbsSlot retTyFromAbsSlot
 

src/Compiler/Checking/infos.fs

@@ -1430,6 +1430,20 @@ type MethInfo =
                  let (ParamAttribs(isParamArrayArg, isInArg, isOutArg, optArgInfo, callerInfo, reflArgInfo)) = info
                  ParamData(isParamArrayArg, isInArg, isOutArg, optArgInfo, callerInfo, nmOpt, reflArgInfo, pty)))
 
+    member x.HasGenericRetTy() =
+        match x with
+        | ILMeth(_g, ilminfo, _) -> ilminfo.RawMetadata.Return.Type.IsTypeVar
+        | FSMeth(g, _, vref, _) ->
+            let _, _, _, retTy, _ = GetTypeOfMemberInMemberForm g vref
+            match retTy with
+            | Some retTy -> isTyparTy g retTy
+            | None -> false
+        | MethInfoWithModifiedReturnType(_,retTy) -> false
+        | DefaultStructCtor _ -> false
+#if !NO_TYPEPROVIDERS
+        | ProvidedMeth(amap, mi, _, m) -> false
+#endif
+
     /// Get the ParamData objects for the parameters of a MethInfo
     member x.HasParamArrayArg(amap, m, minst) =
         x.GetParamDatas(amap, m, minst) |> List.existsSquared (fun (ParamData(isParamArrayArg, _, _, _, _, _, _, _)) -> isParamArrayArg)

src/Compiler/Checking/infos.fsi

@@ -538,6 +538,8 @@ type MethInfo =
     /// Get the signature of an abstract method slot.
     member GetSlotSig: amap: ImportMap * m: range -> SlotSig
 
+    member HasGenericRetTy: unit -> bool
+
     /// Get the ParamData objects for the parameters of a MethInfo
     member HasParamArrayArg: amap: ImportMap * m: range * minst: TType list -> bool
 

tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs

@@ -1382,6 +1382,32 @@ dict["ok"] <- 42
     |> shouldSucceed
 
 
+
+[<InlineData("null")>]
+[<InlineData("if false then null else []")>]
+[<InlineData("if false then [] else null")>]
+[<InlineData("(if false then [] else null) : (_ list | null)")>]
+[<InlineData("[] : (_ list | null)")>]
+[<Theory>]
+let ``Nullness in inheritance chain`` (returnExp:string) = 
+    
+    FSharp $"""module MyLibrary
+
+[<AbstractClass>]
+type Generator<'T>() =
+    abstract Values: unit -> 'T
+
+[<Sealed>]
+type ListGenerator<'T>() =
+    inherit Generator<List<'T> | null>()
+
+    override _.Values() = {returnExp}
+"""
+    |> asLibrary
+    |> typeCheckWithStrictNullness
+    |> shouldSucceed
+
+
 [<Fact>]
 let ``Notnull constraint and inline annotated value`` () = 
     FSharp """module MyLibrary