Feature nullness :: Cleanups, Test reorg, fix incrementalbuild cache behavior

VisualFSharp.sln

@@ -187,7 +187,6 @@ Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "FSharp.Editor.Tests", "vsin
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "FSharp.Editor.IntegrationTests", "vsintegration\tests\FSharp.Editor.IntegrationTests\FSharp.Editor.IntegrationTests.csproj", "{E31F9B59-FCF1-4D04-8762-C7BB60285A7B}"
 EndProject
-Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "nullness", "tests\adhoc\nullness\nullness.fsproj", "{6992D926-AB1C-4CD4-94D5-0319D14DB54B}"
 Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "FSharp.Benchmarks.Common", "tests\benchmarks\FSharp.Benchmarks.Common\FSharp.Benchmarks.Common.fsproj", "{6734FC6F-B5F3-45E1-9A72-720378BB49C9}"
 EndProject
 Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "MicroPerf", "tests\benchmarks\CompiledCodeBenchmarks\MicroPerf\MicroPerf.fsproj", "{601CD5C1-EAFA-4AE3-8FB9-F667B5728213}"
@@ -984,18 +983,6 @@ Global
 		{E31F9B59-FCF1-4D04-8762-C7BB60285A7B}.Release|Any CPU.Build.0 = Release|Any CPU
 		{E31F9B59-FCF1-4D04-8762-C7BB60285A7B}.Release|x86.ActiveCfg = Release|Any CPU
 		{E31F9B59-FCF1-4D04-8762-C7BB60285A7B}.Release|x86.Build.0 = Release|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Debug|Any CPU.Build.0 = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Debug|x86.ActiveCfg = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Debug|x86.Build.0 = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Proto|Any CPU.ActiveCfg = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Proto|Any CPU.Build.0 = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Proto|x86.ActiveCfg = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Proto|x86.Build.0 = Debug|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Release|Any CPU.ActiveCfg = Release|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Release|Any CPU.Build.0 = Release|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Release|x86.ActiveCfg = Release|Any CPU
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B}.Release|x86.Build.0 = Release|Any CPU
 		{6734FC6F-B5F3-45E1-9A72-720378BB49C9}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
 		{6734FC6F-B5F3-45E1-9A72-720378BB49C9}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{6734FC6F-B5F3-45E1-9A72-720378BB49C9}.Debug|x86.ActiveCfg = Debug|Any CPU
@@ -1109,7 +1096,6 @@ Global
 		{39CDF34B-FB23-49AE-AB27-0975DA379BB5} = {DFB6ADD7-3149-43D9-AFA0-FC4A818B472B}
 		{CBC96CC7-65AB-46EA-A82E-F6A788DABF80} = {F7876C9B-FB6A-4EFB-B058-D6967DB75FB2}
 		{E31F9B59-FCF1-4D04-8762-C7BB60285A7B} = {F7876C9B-FB6A-4EFB-B058-D6967DB75FB2}
-		{6992D926-AB1C-4CD4-94D5-0319D14DB54B} = {CFE3259A-2D30-4EB0-80D5-E8B5F3D01449}
 		{6734FC6F-B5F3-45E1-9A72-720378BB49C9} = {DFB6ADD7-3149-43D9-AFA0-FC4A818B472B}
 		{601CD5C1-EAFA-4AE3-8FB9-F667B5728213} = {DFB6ADD7-3149-43D9-AFA0-FC4A818B472B}
 		{9F9DD315-37DA-4413-928E-1CFC6924B64F} = {DFB6ADD7-3149-43D9-AFA0-FC4A818B472B}

src/Compiler/AbstractIL/ilread.fs

@@ -930,7 +930,7 @@ let mkCacheGeneric lowMem _inbase _nm (sz: int) =
     if lowMem then
         (fun f x -> f x)
     else
-        let mutable cache: ConcurrentDictionary<_, _> MaybeNull = null // TODO NULLNESS: this explicit annotation should not be needed
+        let mutable cache = null
 #if STATISTICS
 
         addReport (fun oc ->

src/Compiler/Checking/ConstraintSolver.fs

@@ -1062,8 +1062,7 @@ and SolveNullnessEquiv (csenv: ConstraintSolverEnv) m2 (trace: OptionalTrace) ty
         // Warn for 'strict "must pass null"` APIs like Option.ofObj
         | NullnessInfo.WithNull, NullnessInfo.WithoutNull when shouldWarnUselessNullCheck csenv -> 
             WarnD(Error(FSComp.SR.tcPassingWithoutNullToANullableExpectingFunc (csenv.SolverState.WarnWhenUsingWithoutNullOnAWithNullTarget.Value),m2))    
-        // Allow expected of WithNull and actual of WithoutNull
-        // TODO NULLNESS:  this is not sound in contravariant cases etc. It is assuming covariance.
+        // Allow expected of WithNull and actual of WithoutNull except for specially marked APIs (handled above)        
         | NullnessInfo.WithNull, NullnessInfo.WithoutNull -> CompleteD
         | _ -> 
             if csenv.g.checkNullness then 
@@ -1414,8 +1413,6 @@ and SolveTypeEqualsTypeEqns csenv ndeep m2 trace cxsln origl1 origl2 =
 
 and SolveFunTypeEqn csenv ndeep m2 trace cxsln domainTy1 domainTy2 rangeTy1 rangeTy2 =
     trackErrors {
-        // TODO NULLNESS: consider whether flipping the actual and expected in argument position
-        // causes other problems, e.g. better/worse diagnostics
         let g = csenv.g
         let domainTy2 = reqTyForArgumentNullnessInference g domainTy1 domainTy2
         do! SolveTypeEqualsTypeKeepAbbrevsWithCxsln csenv ndeep m2 trace cxsln domainTy2 domainTy1

src/Compiler/Checking/PostInferenceChecks.fs

@@ -2555,7 +2555,7 @@ let CheckEntityDefn cenv env (tycon: Entity) =
                 // Check if it's marked unsafe
                 let zeroInitUnsafe = TryFindFSharpBoolAttribute g g.attrib_DefaultValueAttribute f.FieldAttribs
                 if zeroInitUnsafe = Some true then
-                    if not (TypeHasDefaultValue g m ty) then
+                    if not (TypeHasDefaultValue g m f.FormalType) then
                         errorR(Error(FSComp.SR.chkValueWithDefaultValueMustHaveDefaultValue(), m))
 
         // Check type abbreviations

src/Compiler/Checking/import.fs

@@ -266,15 +266,18 @@ For value types, a value is passed even though it is always 0
                 | n when n > this.Idx -> this.Data[this.Idx] |> mapping
                 // This is an errornous case, we need more nullnessinfo then the metadata contains
                 | _ -> 
-                    failwithf "Length of Nullable metadata and needs of its processing do not match:  %A" this // TODO nullness - once being confident that our bugs are solved and what remains are incoming metadata bugs, remove failwith and replace with dprintfn
+                    // TODO nullness - once being confident that our bugs are solved and what remains are incoming metadata bugs, remove failwith and replace with dprintfn
+                    // Testing with .NET compilers other then Roslyn producing nullness metadata?
+                    failwithf "Length of Nullable metadata and needs of its processing do not match:  %A" this
                     knownAmbivalent
 
             member this.Advance() = {Data = this.Data; Idx = this.Idx + 1}
 
     let inline evaluateFirstOrderNullnessAndAdvance (ilt:ILType) (flags:NullableFlags) = 
         match ilt with
         | ILType.Value tspec when tspec.GenericArgs.IsEmpty -> KnownWithoutNull, flags
-        // TODO nullness - System.Nullable might be tricky, since you CAN assign 'null' to it, and when boxed, it CAN be boxed to 'null'.
+        // System.Nullable is special-cased in C# spec for nullnes metadata.
+        // You CAN assign 'null' to it, and when boxed, it CAN be boxed to 'null'.
         | ILType.Value tspec when tspec.Name = "Nullable`1" && tspec.Enclosing = ["System"] -> KnownWithoutNull, flags
         | ILType.Value _  -> KnownWithoutNull, flags.Advance()
         | _ -> flags.GetNullness(), flags.Advance()
@@ -436,7 +439,8 @@ let rec ImportProvidedType (env: ImportMap) (m: range) (* (tinst: TypeInst) *) (
     let g = env.g
     if st.PUntaint((fun st -> st.IsArray), m) then 
         let elemTy = ImportProvidedType env m (* tinst *) (st.PApply((fun st -> st.GetElementType()), m))
-        let nullness = Nullness.knownAmbivalent // TODO nullness import :: type providers  Nullness.ImportNullness env.g
+        // TODO Nullness - integration into type providers as a separate feature for later.
+        let nullness = Nullness.knownAmbivalent
         mkArrayTy g (st.PUntaint((fun st -> st.GetArrayRank()), m)) nullness elemTy m
     elif st.PUntaint((fun st -> st.IsByRef), m) then 
         let elemTy = ImportProvidedType env m (* tinst *) (st.PApply((fun st -> st.GetElementType()), m))
@@ -509,7 +513,8 @@ let rec ImportProvidedType (env: ImportMap) (m: range) (* (tinst: TypeInst) *) (
                 else
                     genericArg)
 
-        let nullness = Nullness.knownAmbivalent // TODO nullness import ::  type providers Nullness.ImportNullnessForTyconRef env.g m tcref
+        // TODO Nullness - integration into type providers as a separate feature for later.
+        let nullness = Nullness.knownAmbivalent
 
         ImportTyconRefApp env tcref genericArgs nullness
 
@@ -562,7 +567,7 @@ let ImportProvidedMethodBaseAsILMethodRef (env: ImportMap) (m: range) (mbase: Ta
                        let formalParamTysAfterInst = 
                            [ for p in ctor.PApplyArray((fun x -> x.GetParameters()), "GetParameters", m) do
                                 let ilFormalTy = ImportProvidedTypeAsILType env m (p.PApply((fun p -> p.ParameterType), m))
-                                // TODO nullness import ::  of Nullness in type providers
+                                // TODO Nullness - integration into type providers as a separate feature for later.
                                 yield ImportILType env m actualGenericArgs ilFormalTy ]
 
                        (formalParamTysAfterInst, actualParamTys) ||>  List.lengthsEqAndForall2 (typeEquiv env.g))

src/Compiler/Checking/infos.fs

@@ -396,7 +396,7 @@ let OptionalArgInfoOfProvidedParameter (amap: ImportMap) m (provParam : Tainted<
 /// Compute the ILFieldInit for the given provided constant value for a provided enum type.
 let GetAndSanityCheckProviderMethod m (mi: Tainted<'T :> ProvidedMemberInfo>) (get : 'T -> ProvidedMethodInfo MaybeNull) err = 
     match mi.PApply((fun mi -> (get mi :> ProvidedMethodBase MaybeNull)),m) with 
-    | Tainted.Null -> error(Error(err(mi.PUntaint((fun mi -> mi.Name),m),mi.PUntaint((fun mi -> (nonNull<ProvidedType> mi.DeclaringType).Name), m)), m))   // TODO NULLNESS: type isntantiation should not be needed
+    | Tainted.Null -> error(Error(err(mi.PUntaint((fun mi -> mi.Name),m),mi.PUntaint((fun mi -> (nonNull mi.DeclaringType).Name), m)), m))
     | Tainted.NonNull meth -> meth
 
 /// Try to get an arbitrary ProvidedMethodInfo associated with a property.

src/Compiler/Service/IncrementalBuild.fs

@@ -487,7 +487,7 @@ type BoundModel private (
 
 /// Global service state
 type FrameworkImportsCacheKey = 
-    | FrameworkImportsCacheKey of resolvedpath: string list * assemblyName: string * targetFrameworkDirectories: string list * fsharpBinaries: string * langVersion: decimal
+    | FrameworkImportsCacheKey of resolvedpath: string list * assemblyName: string * targetFrameworkDirectories: string list * fsharpBinaries: string * langVersion: decimal * checkNulls: bool
 
     interface ICacheKey<string, FrameworkImportsCacheKey> with
         member this.GetKey() =
@@ -529,7 +529,8 @@ type FrameworkImportsCache(size) =
                     tcConfig.primaryAssembly.Name,
                     tcConfig.GetTargetFrameworkDirectories(),
                     tcConfig.fsharpBinariesDir,
-                    tcConfig.langVersion.SpecifiedVersion)
+                    tcConfig.langVersion.SpecifiedVersion,
+                    tcConfig.checkNullness)
 
         let node =
             lock gate (fun () ->

src/Compiler/Service/IncrementalBuild.fsi

@@ -30,7 +30,8 @@ type internal FrameworkImportsCacheKey =
         assemblyName: string *
         targetFrameworkDirectories: string list *
         fsharpBinaries: string *
-        langVersion: decimal
+        langVersion: decimal *
+        checkNulls: bool
 
     interface ICacheKey<string, FrameworkImportsCacheKey>
 

src/Compiler/Service/TransparentCompiler.fs

@@ -510,7 +510,8 @@ type internal TransparentCompiler
                 tcConfig.primaryAssembly.Name,
                 tcConfig.GetTargetFrameworkDirectories(),
                 tcConfig.fsharpBinariesDir,
-                tcConfig.langVersion.SpecifiedVersion
+                tcConfig.langVersion.SpecifiedVersion,
+                tcConfig.checkNullness
             )
 
         caches.FrameworkImports.Get(

src/Compiler/TypedTree/TcGlobals.fs

@@ -663,12 +663,12 @@ type TcGlobals(
       | [_] -> None
       | _ -> TType_tuple (tupInfo, l)  |> Some
 
-  let decodeTupleTyAndNullness tupInfo tinst _nullness = // TODO nullness
+  let decodeTupleTyAndNullness tupInfo tinst _nullness =
       match tryDecodeTupleTy tupInfo tinst with
       | Some ty -> ty
       | None -> failwith "couldn't decode tuple ty"
 
-  let decodeTupleTyAndNullnessIfPossible tcref tupInfo tinst nullness = // TODO nullness
+  let decodeTupleTyAndNullnessIfPossible tcref tupInfo tinst nullness =
       match tryDecodeTupleTy tupInfo tinst with
       | Some ty -> ty
       | None -> TType_app(tcref, tinst, nullness)

src/Compiler/TypedTree/TypedTreeBasics.fs

@@ -192,7 +192,7 @@ let KnownWithoutNull = Nullness.Known NullnessInfo.WithoutNull
 
 let mkTyparTy (tp:Typar) = 
     match tp.Kind with 
-    | TyparKind.Type -> tp.AsType KnownWithoutNull // TODO NULLNESS: check various callers
+    | TyparKind.Type -> tp.AsType KnownWithoutNull
     | TyparKind.Measure -> TType_measure (Measure.Var tp)
 
 // For fresh type variables clear the StaticReq when copying because the requirement will be re-established through the
@@ -269,9 +269,9 @@ let tryAddNullnessToTy nullnessNew (ty:TType) =
             Some ty
         else 
             Some (TType_app (tcr, tinst, nullnessAfter))
-    | TType_ucase _ -> None // TODO NULLNESS
-    | TType_tuple _ -> None // TODO NULLNESS
-    | TType_anon _ -> None // TODO NULLNESS
+    | TType_ucase _ -> None
+    | TType_tuple _ -> None
+    | TType_anon _ -> None
     | TType_fun (d, r, nullnessOrig) ->
         let nullnessAfter = combineNullness nullnessOrig nullnessNew
         if nullnessEquiv nullnessAfter nullnessOrig then
@@ -295,9 +295,6 @@ let addNullnessToTy (nullness: Nullness) (ty:TType) =
         else 
             TType_app (tcr, tinst, combineNullness nullnessOrig nullness)
     | TType_fun (d, r, nullnessOrig) -> TType_fun (d, r, combineNullness nullnessOrig nullness)
-    //| TType_ucase _ -> None // TODO NULLNESS
-    //| TType_tuple _ -> None // TODO NULLNESS
-    //| TType_anon _ -> None // TODO NULLNESS
     | _ -> ty
 
 let rec stripTyparEqnsAux nullness0 canShortcut ty = 
@@ -336,9 +333,6 @@ let replaceNullnessOfTy nullness (ty:TType) =
     | TType_var (tp, _) -> TType_var (tp, nullness)
     | TType_app (tcr, tinst, _) -> TType_app (tcr, tinst, nullness)
     | TType_fun (d, r, _) -> TType_fun (d, r, nullness)
-    //| TType_ucase _ -> None // TODO NULLNESS
-    //| TType_tuple _ -> None // TODO NULLNESS
-    //| TType_anon _ -> None // TODO NULLNESS
     | sty -> sty
 
 /// Detect a use of a nominal type, including type abbreviations.

src/Compiler/TypedTree/TypedTreeOps.fs

@@ -1165,7 +1165,7 @@ let rec getErasedTypes g ty checkForNullness =
 
     | TType_var (tp, nullness) -> 
         match checkForNullness, nullness.Evaluate() with
-        | true, NullnessInfo.WithNull -> [ty] // with-null annotations can't be tested at runtime (TODO NULLNESS: for value types Nullable<_> they can be)
+        | true, NullnessInfo.WithNull -> [ty] // with-null annotations can't be tested at runtime, Nullabe<> is not part of Nullness feature as of now.
         | _ -> if tp.IsErased then [ty] else []
 
     | TType_app (_, b, nullness) ->

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

@@ -235,9 +235,10 @@
     <Compile Include="Language\CopyAndUpdateTests.fs" />
     <Compile Include="Language\ConstraintIntersectionTests.fs" />
     <Compile Include="Language\BooleanReturningAndReturnTypeDirectedPartialActivePatternTests.fs" />
-    <Compile Include="Language\NullableLibraryConstructsTests.fs" />
-    <Compile Include="Language\NullableCsharpImportTests.fs" />
-    <Compile Include="Language\NullableReferenceTypesTests.fs" />
+    <Compile Include="Language\Nullness\NullableLibraryConstructsTests.fs" />
+    <Compile Include="Language\Nullness\NullableCsharpImportTests.fs" />
+    <Compile Include="Language\Nullness\NullableRegressionTests.fs" />
+    <Compile Include="Language\Nullness\NullableReferenceTypesTests.fs" />
     <Compile Include="Language\IndexedSetTests.fs" />
     <Compile Include="ConstraintSolver\PrimitiveConstraints.fs" />
     <Compile Include="ConstraintSolver\MemberConstraints.fs" />

tests/FSharp.Compiler.ComponentTests/Language/NullableCsharpImportTests.fs → tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableCsharpImportTests.fs

@@ -4,13 +4,17 @@ open Xunit
 open FSharp.Test
 open FSharp.Test.Compiler
 
-let typeCheckWithStrictNullness cu =
+let withStrictNullness cu =
     cu
     |> withLangVersionPreview
     |> withCheckNulls
     |> withWarnOn 3261
     |> withOptions ["--warnaserror+"]
-    |> compile
+
+let typeCheckWithStrictNullness cu =
+    cu
+    |> withStrictNullness
+    |> typecheck
 
 
 [<FactForNETCOREAPP>]
@@ -211,7 +215,8 @@ let ``Consumption of nullable C# - no generics, just strings in methods and fiel
     """
     |> asLibrary
     |> withReferences [csharpLib]
-    |> typeCheckWithStrictNullness
+    |> withStrictNullness
+    |> compile
     |> shouldFail
     |> withDiagnostics [
             Error 3261, Line 5, Col 40, Line 5, Col 85, "Nullness warning: The types 'string' and 'string | null' do not have compatible nullability."

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

@@ -14,7 +14,7 @@ let withNullnessOptions cu =
 let typeCheckWithStrictNullness cu =
     cu
     |> withNullnessOptions
-    |> compile
+    |> typecheck
 
 [<Fact>]
 let ``Cannot pass possibly null value to a strict function``() =