Feature nullness :: Bugfixes by T-Gro · Pull Request #17102 · dotnet/fsharp · GitHub
Skip to content

Feature nullness :: Bugfixes #17102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
T-Gro merged 20 commits into from
May 21, 2024
Merged

Feature nullness :: Bugfixes #17102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
08fa294
Ignore Nullness applied on structs (C# allows T? when when T is a str…
T-Gro Apr 26, 2024
b558890
Bigfix: Working with CLI events in Fsharp
T-Gro Apr 29, 2024
8c2fd6c
Bugfix: Mutable binding initially assigned to null should not need ty…
T-Gro Apr 29, 2024
44ad511
fantomas
T-Gro Apr 29, 2024
84d3588
Move stats keeping of caches into #IF STATISTICS blocks.
T-Gro Apr 30, 2024
29c408e
Solving `let mutable cache = null` via type inference
T-Gro May 7, 2024
744e1a9
fantomas
T-Gro May 7, 2024
901bf32
Enforcing TyparConstraint.IsReferenceType when WithNull type is used
T-Gro May 15, 2024
c50b5ad
"when T:not null" does not enforce reference type
T-Gro May 15, 2024
05b6b47
Nullness-related constraint consistency
T-Gro May 15, 2024
62780bc
Bugfix for emitting Nullable attrs for C#
T-Gro May 15, 2024
c186131
fantomas
T-Gro May 20, 2024
e0e5406
tests adjusted
T-Gro May 20, 2024
eb5b5ec
update micro tests
T-Gro May 20, 2024
1b59eb2
adjusting micro tests
T-Gro May 20, 2024
3191be2
trimmed
T-Gro May 20, 2024
98f4012
Merge branch 'feature/nullness' into feature-nullness-bugfixes2
T-Gro May 20, 2024
28be27d
nullness fsi update
T-Gro May 21, 2024
0821508
try compile instead of typecheck to get tests trough
T-Gro May 21, 2024
b153a8d
Merge branch 'feature/nullness' into feature-nullness-bugfixes2
T-Gro May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
10 changes: 8 additions & 2 deletions src/Compiler/AbstractIL/ilread.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,8 +931,9 @@ let mkCacheInt32 lowMem _inbase _nm _sz =
(fun f x -> f x)
else
let mutable cache: ConcurrentDictionary<int32, _> MaybeNull = null // TODO NULLNESS: this explicit annotation should not be needed
let mutable count = 0
#if STATISTICS
let mutable count = 0

addReport (fun oc ->
if count <> 0 then
oc.WriteLine((_inbase + string count + " " + _nm + " cache hits"): string))
Expand All @@ -948,7 +949,9 @@ let mkCacheInt32 lowMem _inbase _nm _sz =

match cache.TryGetValue idx with
| true, res ->
#if STATISTICS
count <- count + 1
#endif
res
| _ ->
let res = f idx
Expand All @@ -960,8 +963,9 @@ let mkCacheGeneric lowMem _inbase _nm _sz =
(fun f x -> f x)
else
let mutable cache: ConcurrentDictionary<_, _> MaybeNull = null // TODO NULLNESS: this explicit annotation should not be needed
let mutable count = 0
#if STATISTICS
let mutable count = 0

addReport (fun oc ->
if !count <> 0 then
oc.WriteLine((_inbase + string !count + " " + _nm + " cache hits"): string))
Expand All @@ -977,7 +981,9 @@ let mkCacheGeneric lowMem _inbase _nm _sz =

match cache.TryGetValue idx with
| true, v ->
#if STATISTICS
count <- count + 1
#endif
v
| _ ->
let res = f idx
Expand Down
5 changes: 4 additions & 1 deletion src/Compiler/Checking/CheckExpressions.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,7 @@ let TcAddNullnessToType (warn: bool) (cenv: cenv) (env: TcEnv) nullness innerTyC

if not g.compilingFSharpCore || not (isTyparTy g innerTyC) then
AddCxTypeDefnNotSupportsNull env.DisplayEnv cenv.css m NoTrace innerTyC
AddCxTypeIsReferenceType env.DisplayEnv cenv.css m NoTrace innerTyC

if not g.compilingFSharpCore && isTyparTy g innerTyC then
// A typar might be later infered into a type not supporting `| null|, like tuple or anon.
Expand Down Expand Up @@ -5314,6 +5315,7 @@ and TcExprFlex (cenv: cenv) flex compat (desiredTy: TType) (env: TcEnv) tpenv (s

if flex then
let argTy = NewInferenceType g
(destTyparTy g argTy).SetSupportsNullFlex(true)
if compat then
(destTyparTy g argTy).SetIsCompatFlex(true)

Expand Down Expand Up @@ -9601,7 +9603,8 @@ and TcEventItemThen (cenv: cenv) overallTy env tpenv mItem mExprAndItem objDetai
| None, false -> error (Error (FSComp.SR.tcEventIsNotStatic nm, mItem))
| _ -> ()

let delTy = einfo.GetDelegateType(cenv.amap, mItem)
// The F# wrappers around events are null safe (impl is in FSharp.Core). Therefore, from an F# perspective, the type of the delegate can be considered Not Null.
let delTy = einfo.GetDelegateType(cenv.amap, mItem) |> replaceNullnessOfTy KnownWithoutNull
let (SigOfFunctionForDelegate(delInvokeMeth, delArgTys, _, _)) = GetSigOfFunctionForDelegate cenv.infoReader delTy mItem ad
let objArgs = Option.toList (Option.map fst objDetails)
MethInfoChecks g cenv.amap true None objArgs env.eAccessRights mItem delInvokeMeth
Expand Down
47 changes: 41 additions & 6 deletions src/Compiler/Checking/ConstraintSolver.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,9 @@ type ConstraintSolverEnv =
// Is this speculative, with a trace allowing undo, and trial method overload resolution
IsSpeculativeForMethodOverloading: bool

// Can this ignore the 'must support null' constraint, e.g. in a mutable assignment scenario
IsSupportsNullFlex: bool

/// Indicates that when unifying ty1 = ty2, only type variables in ty1 may be solved. Constraints
/// can't be added to type variables in ty2
MatchingOnly: bool
Expand Down Expand Up @@ -344,6 +347,7 @@ let MakeConstraintSolverEnv contextInfo css m denv =
EquivEnv = TypeEquivEnv.Empty
DisplayEnv = denv
IsSpeculativeForMethodOverloading = false
IsSupportsNullFlex = false
ExtraRigidTypars = emptyFreeTypars
}

Expand Down Expand Up @@ -953,6 +957,13 @@ let rec SolveTyparEqualsTypePart1 (csenv: ConstraintSolverEnv) m2 (trace: Option
// Record the solution before we solve the constraints, since
// We may need to make use of the equation when solving the constraints.
// Record a entry in the undo trace if one is provided

//let ty1AllowsNull = r.Constraints |> List.exists (function | TyparConstraint.SupportsNull _ -> true | _ -> false )
//let tyAllowsNull() = TypeNullIsExtraValueNew csenv.g m2 ty
//if ty1AllowsNull && not (tyAllowsNull()) then
// trace.Exec (fun () -> r.typar_solution <- Some (ty |> replaceNullnessOfTy csenv.g.knownWithNull)) (fun () -> r.typar_solution <- None)
//else
// trace.Exec (fun () -> r.typar_solution <- Some ty) (fun () -> r.typar_solution <- None)
trace.Exec (fun () -> r.typar_solution <- Some ty) (fun () -> r.typar_solution <- None)
}

Expand Down Expand Up @@ -1218,6 +1229,12 @@ and SolveTypeEqualsType (csenv: ConstraintSolverEnv) ndeep m2 (trace: OptionalTr
let sty1 = stripTyEqnsA csenv.g canShortcut ty1
let sty2 = stripTyEqnsA csenv.g canShortcut ty2

let csenv =
match ty1 with
Comment thread
T-Gro marked this conversation as resolved.
| TType.TType_var(r,_) when r.typar_flags.IsSupportsNullFlex ->
{ csenv with IsSupportsNullFlex = true}
| _ -> csenv

match sty1, sty2 with
// type vars inside forall-types may be alpha-equivalent
| TType_var (tp1, nullness1), TType_var (tp2, nullness2) when typarEq tp1 tp2 || (match aenv.EquivTypars.TryFind tp1 with | Some tpTy1 when typeEquiv g tpTy1 ty2 -> true | _ -> false) ->
Expand Down Expand Up @@ -1273,6 +1290,15 @@ and SolveTypeEqualsType (csenv: ConstraintSolverEnv) ndeep m2 (trace: OptionalTr
// Unifying 'T1? and 'T2?
| ValueSome NullnessInfo.WithNull, ValueSome NullnessInfo.WithNull ->
SolveTyparEqualsType csenv ndeep m2 trace sty1 (replaceNullnessOfTy g.knownWithoutNull sty2)
| ValueSome NullnessInfo.WithoutNull, ValueSome NullnessInfo.WithoutNull when
csenv.IsSupportsNullFlex &&
isAppTy g sty2 &&
tp1.Constraints |> List.exists (function TyparConstraint.SupportsNull _ -> true | _ -> false) ->
let tpNew = NewCompGenTypar(TyparKind.Type, TyparRigidity.Flexible, TyparStaticReq.None, TyparDynamicReq.No, false)
trackErrors {
do! SolveTypeEqualsType csenv ndeep m2 trace cxsln (TType_var(tpNew, g.knownWithoutNull)) sty2
do! SolveTypeEqualsType csenv ndeep m2 trace cxsln ty1 (TType_var(tpNew, g.knownWithNull))
}
// Unifying 'T1 % and 'T2 %
//| ValueSome NullnessInfo.AmbivalentToNull, ValueSome NullnessInfo.AmbivalentToNull ->
// SolveTyparEqualsType csenv ndeep m2 trace sty1 (replaceNullnessOfTy g.knownWithoutNull sty2)
Expand Down Expand Up @@ -2354,6 +2380,10 @@ and EnforceConstraintConsistency (csenv: ConstraintSolverEnv) ndeep m2 trace ret
| TyparConstraint.IsNonNullableStruct _, TyparConstraint.IsReferenceType _
| TyparConstraint.IsReferenceType _, TyparConstraint.IsNonNullableStruct _ ->
return! ErrorD (Error(FSComp.SR.csStructConstraintInconsistent(), m))

| TyparConstraint.SupportsNull _, TyparConstraint.NotSupportsNull _
| TyparConstraint.NotSupportsNull _, TyparConstraint.SupportsNull _ ->
return! ErrorD (Error(FSComp.SR.csNullNotNullConstraintInconsistent(), m))

| TyparConstraint.IsUnmanaged _, TyparConstraint.IsReferenceType _
| TyparConstraint.IsReferenceType _, TyparConstraint.IsUnmanaged _ ->
Expand Down Expand Up @@ -2534,16 +2564,18 @@ and SolveTypeUseSupportsNull (csenv: ConstraintSolverEnv) ndeep m2 trace ty =
return! ErrorD (ConstraintSolverError(FSComp.SR.csNullableTypeDoesNotHaveNull(NicePrint.minimalStringOfType denv ty), m, m2))
else
match tryDestTyparTy g ty with
| ValueSome tp ->
| ValueSome tp ->
let nullness = nullnessOfTy g ty
match nullness.TryEvaluate() with
// NULLNESS TODO: This rule means turning on checkNullness changes type inference results for the cases
// mentioned in the comment above. THat's OK but needs to be documented in the RFC.
| ValueNone when not g.checkNullness ->
return! AddConstraint csenv ndeep m2 trace tp (TyparConstraint.SupportsNull m)
| ValueSome NullnessInfo.WithoutNull ->
return! AddConstraint csenv ndeep m2 trace tp (TyparConstraint.SupportsNull m)
| ValueSome NullnessInfo.WithoutNull ->
return! AddConstraint csenv ndeep m2 trace tp (TyparConstraint.SupportsNull m)
| _ ->
if tp.Constraints |> List.exists (function | TyparConstraint.IsReferenceType _ -> true | _ -> false) |> not then
do! AddConstraint csenv ndeep m2 trace tp (TyparConstraint.IsReferenceType m)
return! SolveNullnessSupportsNull csenv ndeep m2 trace ty nullness
| _ ->
let nullness = nullnessOfTy g ty
Expand Down Expand Up @@ -2580,9 +2612,10 @@ and SolveNullnessSupportsNull (csenv: ConstraintSolverEnv) ndeep m2 (trace: Opti
match n1 with
| NullnessInfo.AmbivalentToNull -> ()
| NullnessInfo.WithNull -> ()
| NullnessInfo.WithoutNull ->
| NullnessInfo.WithoutNull ->
if g.checkNullness then
return! WarnD(ConstraintSolverNullnessWarningWithType(denv, ty, n1, m, m2))
// TODO nullness: Shouldn't this be an error? We have a 'must support null' situation which is not being met.
return! WarnD(ConstraintSolverNullnessWarningWithType(denv, ty, n1, m, m2))
}

and SolveTypeUseNotSupportsNull (csenv: ConstraintSolverEnv) ndeep m2 trace ty =
Expand Down Expand Up @@ -2635,7 +2668,9 @@ and SolveTypeCanCarryNullness (csenv: ConstraintSolverEnv) ty nullness =
let m = csenv.m
let strippedTy = stripTyEqnsA g true ty
match tryAddNullnessToTy nullness strippedTy with
| Some _ -> ()
| Some _ ->
if isTyparTy g strippedTy && not (isReferenceTyparTy g strippedTy) then
return! AddConstraint csenv 0 m NoTrace (destTyparTy g strippedTy) (TyparConstraint.IsReferenceType m)
| None ->
let tyString = NicePrint.minimalStringOfType csenv.DisplayEnv strippedTy
return! ErrorD(Error(FSComp.SR.tcTypeDoesNotHaveAnyNull(tyString), m))
Expand Down
12 changes: 9 additions & 3 deletions src/Compiler/CodeGen/IlxGen.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5648,17 +5648,23 @@ and GenGenericParam cenv eenv (tp: Typar) =
| TyparConstraint.IsNonNullableStruct _ -> true
| _ -> false)

let notNullReferenceTypeConstraint =
let nullnessOfTypar =
if g.langFeatureNullness && g.checkNullness then
let hasNotSupportsNull =
tp.Constraints
|> List.exists (function
| TyparConstraint.NotSupportsNull _ -> true
| _ -> false)

let hasSupportsNull () =
tp.Constraints
|> List.exists (function
| TyparConstraint.SupportsNull _ -> true
| _ -> false)

if hasNotSupportsNull || notNullableValueTypeConstraint then
NullnessInfo.WithoutNull
elif hasNotSupportsNull || refTypeConstraint then
elif refTypeConstraint || hasSupportsNull () then
NullnessInfo.WithNull
else
NullnessInfo.AmbivalentToNull
Expand Down Expand Up @@ -5711,7 +5717,7 @@ and GenGenericParam cenv eenv (tp: Typar) =
yield! GenAttrs cenv eenv tp.Attribs
if emitUnmanagedInIlOutput then
yield (GetIsUnmanagedAttribute g)
match notNullReferenceTypeConstraint with
match nullnessOfTypar with
| Some nullInfo -> yield GetNullableAttribute g [ nullInfo ]
| _ -> ()
]
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/CodeGen/IlxGenSupport.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,8 @@ let rec GetNullnessFromTType (g: TcGlobals) ty =
]
| TType_forall _
| TType_ucase _
| TType_var _
| TType_measure _ -> []
| TType_var(nullness = nullness) -> [ nullness.Evaluate() ]

let GenNullnessIfNecessary (g: TcGlobals) ty =
if g.langFeatureNullness && g.checkNullness then
Expand Down
36 changes: 27 additions & 9 deletions src/Compiler/TypedTree/TypedTree.fs
View file
Open in desktop
Loading