Skip to content
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.

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

Bugfix :: Nullness in signature file is not considered by implementation and vice versa #18186

Open
wants to merge 61 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
c7295c2
type equivalence - add nullness support for signature conformance
T-Gro Dec 30, 2024
52bf00b
tests for nullness signature conformance
T-Gro Dec 30, 2024
0deae3d
softer whitespace handling for "withDiagnostics" check
T-Gro Dec 30, 2024
c0d4991
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Dec 30, 2024
307eaef
fantomasd
T-Gro Dec 30, 2024
3e1b650
Merge branch '18058-nullness-issue---nullness-in-signature-file-is-no…
T-Gro Dec 30, 2024
5bd7b8b
Apply changes to codebase
T-Gro Dec 31, 2024
09d0232
adjust code to meet signature conformance
T-Gro Dec 31, 2024
05db836
see errors with ambivalence turned on
T-Gro Dec 31, 2024
c758e17
allow (WithoutNull === Ambivalent) in signature conformance
T-Gro Jan 3, 2025
dd55064
conform impl to signature
T-Gro Jan 3, 2025
6123665
fix
T-Gro Jan 6, 2025
f87f1fb
a few more
T-Gro Jan 6, 2025
79e0e52
format
T-Gro Jan 6, 2025
b58290b
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Jan 6, 2025
582f9af
few more
T-Gro Jan 6, 2025
21f9e20
Can I get all errors at once if I replace them with warnings?
T-Gro Jan 6, 2025
c1ba534
some more
T-Gro Jan 6, 2025
3f2ae24
parsing
T-Gro Jan 6, 2025
9c300b2
one more
T-Gro Jan 7, 2025
703e821
try get more errors at once
T-Gro Jan 7, 2025
a0ef43e
That one should stay strict - this is checked per-phase (OK), not per…
T-Gro Jan 7, 2025
d30504c
Can I process more diags now?
T-Gro Jan 7, 2025
c67b453
Revisit this - this produced a "mismatch on accessibility" first
T-Gro Jan 7, 2025
8449fc4
another
T-Gro Jan 7, 2025
b1635c0
cannot see info warning in output
T-Gro Jan 8, 2025
d8fa0e6
i really want to see all warnings
T-Gro Jan 8, 2025
dc3e815
prettynaming conformant now
T-Gro Jan 8, 2025
9cc5227
ilreflect conformant
T-Gro Jan 8, 2025
f1454ea
format
T-Gro Jan 8, 2025
c1991ce
apply
T-Gro Jan 10, 2025
800c7e5
type provider nullness
T-Gro Jan 13, 2025
d4621bd
more errors resolved
T-Gro Jan 13, 2025
a22ee92
another wave
T-Gro Jan 13, 2025
ce8b02e
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Jan 13, 2025
0545025
messages redone
T-Gro Jan 13, 2025
8394e5c
another batch
T-Gro Jan 13, 2025
bbc3263
fix
T-Gro Jan 13, 2025
6b1c981
one more time
T-Gro Jan 13, 2025
3a0a07f
x
T-Gro Jan 13, 2025
148bde8
fantomas and fantomasignore because of nullness
T-Gro Jan 13, 2025
105f27a
show more warnings!
T-Gro Jan 13, 2025
a4a689e
Dont expose overrides in .fsi
T-Gro Jan 13, 2025
0c2739b
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Jan 20, 2025
311f415
fix
T-Gro Jan 20, 2025
ebe86b3
fix infinity recursion in SRTP typar equivalence resolutio
T-Gro Jan 23, 2025
07b9951
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Jan 23, 2025
e62ef15
Cosmetics
T-Gro Jan 23, 2025
ef535a1
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Feb 3, 2025
0976824
ilverify update
T-Gro Feb 3, 2025
b08f7e5
adjust test framework sensitivity to consequitive whitespace in diag …
T-Gro Feb 3, 2025
030b003
Fix TP ResolveTypeName nullness
T-Gro Feb 3, 2025
3cca025
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Feb 3, 2025
30fcf6f
Update .fantomasignore
T-Gro Feb 3, 2025
8cc5186
move tests back to fsharp suite
T-Gro Feb 3, 2025
4fda992
fix NRE reporting for type providers
T-Gro Feb 4, 2025
5c3dfbd
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
T-Gro Feb 4, 2025
9e05a4c
fix
T-Gro Feb 4, 2025
096c9e8
ilverify
T-Gro Feb 4, 2025
5974aa7
release notes
T-Gro Feb 4, 2025
bba18bc
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
KevinRansom Feb 4, 2025
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
5 changes: 5 additions & 0 deletions .fantomasignore
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ src/Compiler/Checking/TypeRelations.fs

# nullness-related problems
src/Compiler/DependencyManager/DependencyProvider.fs
src/FSharp.Core/fslib-extra-pervasives.fs
src/FSharp.Core/fslib-extra-pervasives.fsi

# Incorrectly formatted: https://github.com/dotnet/fsharp/pull/14645/commits/49443a67ea8a17670c8a7c80c8bdf91f82231e91 or https://github.com/fsprojects/fantomas/issues/2733
# This CompilerImports.fs behavior is not fixed yet, following up in https://github.com/fsprojects/fantomas/issues/2733
Expand Down Expand Up @@ -124,6 +126,9 @@ src/Compiler/SyntaxTree/LexerStore.fs
src/Compiler/Driver/GraphChecking/Graph.fsi
src/Compiler/Driver/GraphChecking/Graph.fs

src/Compiler/DependencyManager/NativeDllResolveHandler.fsi
src/Compiler/DependencyManager/AssemblyResolveHandler.fsi

# Fantomas limitations on implementation files (to investigate)

src/Compiler/AbstractIL/ilwrite.fs
Expand Down
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Compiler.Service/9.0.300.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
### Added
* Added missing type constraints in FCS. ([PR #18241](https://github.com/dotnet/fsharp/pull/18241))
* The 'use' keyword can be used on IDisposable|null without nullness warnings ([PR #18262](https://github.com/dotnet/fsharp/pull/18262))
* Nullness warnings are issued for signature<>implementation conformance ([PR #18186](https://github.com/dotnet/fsharp/pull/18186))

### Changed

Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/AbstractIL/ilreflect.fs
Original file line number Diff line number Diff line change
Expand Up @@ -2548,7 +2548,7 @@ let EmitDynamicAssemblyFragment
ignore (typB.InvokeMemberAndLog(methodName, BindingFlags.InvokeMethod ||| BindingFlags.Public ||| BindingFlags.Static, [||]))
None
with :? TargetInvocationException as exn ->
Some exn.InnerException
Option.ofObj exn.InnerException

let emEnv, entryPts = envPopEntryPts emEnv
let execs = List.map execEntryPtFun entryPts
Expand Down
4 changes: 2 additions & 2 deletions src/Compiler/Checking/AttributeChecking.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ type AttribInfo =
| FSAttribInfo of TcGlobals * Attrib
| ILAttribInfo of TcGlobals * Import.ImportMap * ILScopeRef * ILAttribute * range

member ConstructorArguments: (TType * obj) list
member NamedArguments: (TType * string * bool * obj) list
member ConstructorArguments: (TType * objnull) list
member NamedArguments: (TType * string * bool * objnull) list
member Range: range
member TyconRef: TyconRef

Expand Down
12 changes: 6 additions & 6 deletions src/Compiler/Checking/CheckDeclarations.fs
Original file line number Diff line number Diff line change
Expand Up @@ -3060,7 +3060,7 @@ module EstablishTypeDefinitionCores =

if not isRootGenerated then
let desig = theRootTypeWithRemapping.TypeProviderDesignation
let nm = theRootTypeWithRemapping.PUntaint((fun st -> st.FullName), m)
let nm = theRootTypeWithRemapping.PUntaint((fun st -> string st.FullName), m)
error(Error(FSComp.SR.etErasedTypeUsedInGeneration(desig, nm), m))

cenv.createsGeneratedProvidedTypes <- true
Expand Down Expand Up @@ -3101,7 +3101,7 @@ module EstablishTypeDefinitionCores =

if not isGenerated then
let desig = st.TypeProviderDesignation
let nm = st.PUntaint((fun st -> st.FullName), m)
let nm = st.PUntaint((fun st -> string st.FullName), m)
error(Error(FSComp.SR.etErasedTypeUsedInGeneration(desig, nm), m))

// Embed the type into the module we're compiling
Expand Down Expand Up @@ -4251,7 +4251,7 @@ module TcDeclarations =
// For historical reasons we only give a warning for incorrect type parameters on intrinsic extensions
if nReqTypars <> synTypars.Length then
errorR(Error(FSComp.SR.tcDeclaredTypeParametersForExtensionDoNotMatchOriginal(tcref.DisplayNameWithStaticParametersAndUnderscoreTypars), m))
if not (typarsAEquiv g TypeEquivEnv.Empty reqTypars declaredTypars) then
if not (typarsAEquiv g (TypeEquivEnv.EmptyWithNullChecks g) reqTypars declaredTypars) then
warning(Error(FSComp.SR.tcDeclaredTypeParametersForExtensionDoNotMatchOriginal(tcref.DisplayNameWithStaticParametersAndUnderscoreTypars), m))
// Note we return 'reqTypars' for intrinsic extensions since we may only have given warnings
IntrinsicExtensionBinding, reqTypars
Expand All @@ -4260,7 +4260,7 @@ module TcDeclarations =
errorR(Error(FSComp.SR.tcMembersThatExtendInterfaceMustBePlacedInSeparateModule(), tcref.Range))
if nReqTypars <> synTypars.Length then
error(Error(FSComp.SR.tcDeclaredTypeParametersForExtensionDoNotMatchOriginal(tcref.DisplayNameWithStaticParametersAndUnderscoreTypars), m))
if not (typarsAEquiv g TypeEquivEnv.Empty reqTypars declaredTypars) then
if not (typarsAEquiv g (TypeEquivEnv.EmptyWithNullChecks g) reqTypars declaredTypars) then
errorR(Error(FSComp.SR.tcDeclaredTypeParametersForExtensionDoNotMatchOriginal(tcref.DisplayNameWithStaticParametersAndUnderscoreTypars), m))
ExtrinsicExtensionBinding, declaredTypars

Expand Down Expand Up @@ -5702,7 +5702,7 @@ let CheckModuleSignature g (cenv: cenv) m denvAtEnd rootSigOpt implFileTypePrior
|]

// We want to show imperative type variables in any types in error messages at this late point
let denv = { denvAtEnd with showInferenceTyparAnnotations=true }
let denv = { denvAtEnd with showInferenceTyparAnnotations=true;showNullnessAnnotations=Some g.checkNullness }
try

// As typechecked the signature and implementation use different tycons etc.
Expand All @@ -5714,7 +5714,7 @@ let CheckModuleSignature g (cenv: cenv) m denvAtEnd rootSigOpt implFileTypePrior
// Compute the remapping from implementation to signature
let remapInfo, _ = ComputeRemappingFromInferredSignatureToExplicitSignature g implFileTypePriorToSig sigFileType

let aenv = { TypeEquivEnv.Empty with EquivTycons = TyconRefMap.OfList remapInfo.RepackagedEntities }
let aenv = { TypeEquivEnv.EmptyWithNullChecks g with EquivTycons = TyconRefMap.OfList remapInfo.RepackagedEntities }

if not (SignatureConformance.Checker(g, cenv.amap, denv, remapInfo, true).CheckSignature aenv cenv.infoReader (mkLocalModuleRef implFileSpecPriorToSig) sigFileType) then
// We can just raise 'ReportedError' since CheckModuleOrNamespace raises its own error
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/ConstraintSolver.fs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ let MakeConstraintSolverEnv contextInfo css m denv =
eContextInfo = contextInfo
MatchingOnly = false
ErrorOnFailedMemberConstraintResolution = false
EquivEnv = TypeEquivEnv.Empty
EquivEnv = TypeEquivEnv.EmptyIgnoreNulls
DisplayEnv = denv
IsSpeculativeForMethodOverloading = false
IsSupportsNullFlex = false
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/Expressions/CheckExpressions.fs
Original file line number Diff line number Diff line change
Expand Up @@ -12687,7 +12687,7 @@ and FixupLetrecBind (cenv: cenv) denv generalizedTyparsForRecursiveBlock (bind:
| Some _ ->
match PartitionValTyparsForApparentEnclosingType g vspec with
| Some(parentTypars, memberParentTypars, _, _, _) ->
ignore(SignatureConformance.Checker(g, cenv.amap, denv, SignatureRepackageInfo.Empty, false).CheckTypars vspec.Range TypeEquivEnv.Empty memberParentTypars parentTypars)
ignore(SignatureConformance.Checker(g, cenv.amap, denv, SignatureRepackageInfo.Empty, false).CheckTypars vspec.Range TypeEquivEnv.EmptyIgnoreNulls memberParentTypars parentTypars)
| None ->
errorR(Error(FSComp.SR.tcMemberIsNotSufficientlyGeneric(), vspec.Range))
| _ -> ()
Expand Down
3 changes: 2 additions & 1 deletion src/Compiler/Checking/InfoReader.fs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ let rec GetImmediateIntrinsicMethInfosOfTypeAux (optFilter, ad) g amap m withExp
let st = info.ProvidedType
let meths =
match optFilter with
| Some name -> st.PApplyArray ((fun st -> st.GetMethods() |> Array.filter (fun mi -> mi.Name = name) ), "GetMethods", m)
| Some name ->
st.PApplyFilteredArray ((fun st -> st.GetMethods()),(fun mi -> mi.Name = name), "GetMethods", m)
| None -> st.PApplyArray ((fun st -> st.GetMethods()), "GetMethods", m)
[ for mi in meths -> ProvidedMeth(amap, mi.Coerce(m), None, m) ]
#endif
Expand Down
14 changes: 7 additions & 7 deletions src/Compiler/Checking/MethodCalls.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ let TryImportProvidedMethodBaseAsLibraryIntrinsic (amap: Import.ImportMap, m: ra
match tryTcrefOfAppTy amap.g declaringType with
| ValueSome declaringEntity ->
if not declaringEntity.IsLocalRef && ccuEq declaringEntity.nlr.Ccu amap.g.fslibCcu then
let n = mbase.PUntaint((fun x -> x.GetParameters().Length), m)
let n = mbase.PApplyArray((fun x -> x.GetParameters()),"GetParameters", m).Length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nameof for "GetParameters"?

> let x = typeof<int>;;
    nameof x.ToString;;
val it: string = "ToString"

match amap.g.knownIntrinsics.TryGetValue ((declaringEntity.LogicalName, None, methodName, n)) with
| true, vref -> Some vref
| _ ->
Expand Down Expand Up @@ -1815,14 +1815,14 @@ module ProvidedMethodCalls =
let rec loop (st: Tainted<ProvidedType>) =
if st.PUntaint((fun st -> st.IsGenericParameter), m) then st
elif st.PUntaint((fun st -> st.IsArray), m) then
let et = st.PApply((fun st -> st.GetElementType()), m)
let et = st.PApply((fun st -> !! st.GetElementType()), m)
let rank = st.PUntaint((fun st -> st.GetArrayRank()), m)
(loop et).PApply((fun st -> if rank = 1 then st.MakeArrayType() else st.MakeArrayType(rank)), m)
elif st.PUntaint((fun st -> st.IsByRef), m) then
let et = st.PApply((fun st -> st.GetElementType()), m)
let et = st.PApply((fun st -> !! st.GetElementType()), m)
(loop et).PApply((fun st -> st.MakeByRefType()), m)
elif st.PUntaint((fun st -> st.IsPointer), m) then
let et = st.PApply((fun st -> st.GetElementType()), m)
let et = st.PApply((fun st -> !! st.GetElementType()), m)
(loop et).PApply((fun st -> st.MakePointerType()), m)
else
let isGeneric = st.PUntaint((fun st -> st.IsGenericType), m)
Expand Down Expand Up @@ -1863,7 +1863,7 @@ module ProvidedMethodCalls =
allArgs: Exprs,
paramVars: Tainted<ProvidedVar>[],
g, amap, mut, isProp, isSuperInit, m,
expr: Tainted<ProvidedExpr>) =
expr: Tainted<ProvidedExpr MaybeNull>) =

let varConv =
// note: Assuming the size based on paramVars
Expand All @@ -1873,7 +1873,7 @@ module ProvidedMethodCalls =
dict.Add(v, (None, e))
dict

let rec exprToExprAndWitness top (ea: Tainted<ProvidedExpr>) =
let rec exprToExprAndWitness top (ea: Tainted<ProvidedExpr MaybeNull>) =
let fail() = error(Error(FSComp.SR.etUnsupportedProvidedExpression(ea.PUntaint((fun etree -> etree.UnderlyingExpressionString), m)), m))
match ea with
| Tainted.Null -> error(Error(FSComp.SR.etNullProvidedExpression(ea.TypeProviderDesignation), m))
Expand Down Expand Up @@ -2115,7 +2115,7 @@ module ProvidedMethodCalls =
methInfoOpt, expr, exprTy
with
| :? TypeProviderError as tpe ->
let typeName = mi.PUntaint((fun mb -> (nonNull<ProvidedType> mb.DeclaringType).FullName), m)
let typeName = mi.PUntaint((fun mb -> (nonNull<ProvidedType> mb.DeclaringType).FullName |> string), m)
let methName = mi.PUntaint((fun mb -> mb.Name), m)
raise( tpe.WithContext(typeName, methName) ) // loses original stack trace
#endif
Expand Down
4 changes: 2 additions & 2 deletions src/Compiler/Checking/MethodOverrides.fs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ module DispatchSlotChecking =
// Compare the types. CompiledSigOfMeth, GetObjectExprOverrideInfo and GetTypeMemberOverrideInfo have already
// applied all relevant substitutions except the renamings from fvtmps <-> methTypars

let aenv = TypeEquivEnv.FromEquivTypars fvmethTypars methTypars
let aenv = (TypeEquivEnv.EmptyIgnoreNulls).FromEquivTypars fvmethTypars methTypars

List.forall2 (List.lengthsEqAndForall2 (typeAEquiv g aenv)) vargTys argTys &&
returnTypesAEquiv g aenv vrty retTy &&
Expand Down Expand Up @@ -305,7 +305,7 @@ module DispatchSlotChecking =
ComposeTyparInsts ttpinst (ReverseTyparRenaming g memberToParentInst)

// Compare under the composed substitutions
let aenv = TypeEquivEnv.FromTyparInst ttpinst
let aenv = (TypeEquivEnv.EmptyIgnoreNulls).FromTyparInst ttpinst

typarsAEquiv g aenv fvmethTypars methTypars

Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/NameResolution.fs
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ let ResolveProvidedTypeNameInEntity (amap, m, typeName, modref: ModuleOrNamespac
//if staticResInfo.NumStaticArgs > 0 then
// error(Error(FSComp.SR.etNestedProvidedTypesDoNotTakeStaticArgumentsOrGenericParameters(), m))
[]
| nestedSty ->
| Tainted.NonNull nestedSty ->
[AddEntityForProvidedType (amap, modref, resolutionEnvironment, nestedSty, m) ]
| _ -> []
#endif
Expand Down
6 changes: 3 additions & 3 deletions src/Compiler/Checking/NicePrint.fs
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@ module PrintTypes =
|> ListSet.setify (fun (_, cx1) (_, cx2) ->
match cx1, cx2 with
| TyparConstraint.MayResolveMember(traitInfo1, _),
TyparConstraint.MayResolveMember(traitInfo2, _) -> traitsAEquiv denv.g TypeEquivEnv.Empty traitInfo1 traitInfo2
TyparConstraint.MayResolveMember(traitInfo2, _) -> traitsAEquiv denv.g (TypeEquivEnv.EmptyWithNullChecks denv.g) traitInfo1 traitInfo2
| _ -> false)

let cxsL = List.collect (layoutConstraintWithInfo denv env) cxs
Expand Down Expand Up @@ -2177,7 +2177,7 @@ module TastDefinitionPrinting =
match tcref.TypeReprInfo with
| TProvidedTypeRepr info ->
[
for nestedType in info.ProvidedType.PApplyArray((fun sty -> sty.GetNestedTypes() |> Array.filter (fun t -> t.IsPublic || t.IsNestedPublic)), "GetNestedTypes", m) do
for nestedType in info.ProvidedType.PApplyFilteredArray((fun sty -> sty.GetNestedTypes()),(fun t -> t.IsPublic || t.IsNestedPublic), "GetNestedTypes", m) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps moar nameof

yield nestedType.PUntaint((fun t -> t.IsClass, t.Name), m)
]
|> List.sortBy snd
Expand Down Expand Up @@ -2930,7 +2930,7 @@ let minimalStringsOfTwoTypes denv ty1 ty2 =
let denv = denv.SetOpenPaths []
let denv = { denv with includeStaticParametersInTypeNames=true }
let makeName t =
let assemblyName = PrintTypes.layoutAssemblyName denv t |> function Null | NonNull "" -> "" | NonNull name -> sprintf " (%s)" name
let assemblyName = PrintTypes.layoutAssemblyName denv t |> function | "" -> "" | name -> sprintf " (%s)" name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely $" ({name})"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bonus kudos for using function match :-)

sprintf "%s%s" (stringOfTy denv t) assemblyName

(makeName ty1, makeName ty2, stringOfTyparConstraints denv tpcs)
Expand Down
Loading