[RFC FS-1060] Nullness checking

[dsyme]

Continuation of #5790

This is a prototype implementation of RFC FS-1060 nullable reference types

See tests\fsharp\core\nullness\test.fsx for testing and samples.

TODO:

• [ ] implement import of .NET metadata
• [ ] implement emit of .NET metadata
• [ ] resolve all unresolved issues in RFC
• [ ] implement match x with null -> ... | x -> ... implied use of NonNull pattern
• [ ] make null and NonNull be considered disjunctive discriminators in pattern matching
• [ ] resolve all // TODO NULLNESS
• [ ] ambivalent/oblivious annotations are not being shown at all in visual output. Consider what to do about these. They should likely be dropped but give a supplementary sentence "Some types shown are ambivalent to null checking for compatibility reasons".
• [ ] add testing
• [ ] get it green
• [ ] performance testing
• [ ] should nullness be supported on ref tuples and anon tuples
• [ ] check signature compatibility. While integrating master I noticed a case in the compiler where a signature file had a non-nullable string type for ther return of a function and an implementation had a nullable string type (and a later soundness problem arose)
• [ ] Work out what to do about compat of Option.ofObj and others
• [ ] Fix and re-enable test Test ProjectForWitnesses4 GetWitnessPassingInfo

DONE:

• [x] allow Foo<string?>, currently Foo< string? > with a space between ? and > is needed, need to token smash ?> token in tyargs

[dsyme]

Failing tests:

CodeGen\EmittedIL\Misc (AnonRecd.fs) -- failed
CodeGen\EmittedIL\Tuples (OptionalArg01.fs - test optimizatons) -- failed

[dsyme]

@brettfo This has some weird error now for BuildFromSource on Linux, any ideas? I've not seen this before

.../InitializeSourceControlInformation.targets(3,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid. [/home/vsts/work/1/s/src/fsharp/fsi/fsi.fsproj]

[dsyme]

There's an annoying failure in the Windows source build.

D:\a\1\s\src\fsharp\fsi\fsi.fsproj : error MSB4057: The target "UpdateXlf" does not exist in the project. ##[error]src\fsharp\fsi\fsi.fsproj(0,0): error MSB4057: (NETCORE_ENGINEERING_TELEMETRY=Build) The target "UpdateXlf" does not exist in the project.

It doesn't reproduce for me on my windows machine when running this:

C:\GitHub\dsyme\visualfsharp>eng\CIBuild.cmd -configuration Release -noSign /p:DotNetBuildFromSource=true /p:FSharpSourceBuild=true

[dsyme]

This PR has two failures

CodeGen\EmittedIL\LiteralValue -- failed
CodeGen\EmittedIL\Mutation -- failed

but I can't debug them. When I run these unit tests from the IDE they don't fail

[brettfo]

@dsyme I saw a similar failure in another merge, and from looking at it, those directories aren't on disk, so they may just need to be removed from a *.lst file.

[dsyme]

@dsyme I saw a similar failure in another merge, and from looking at it, those directories aren't on disk, so they may just need to be removed from a *.lst file.

Ah of course, thank you!! Yes They are in test.lst, trialling removing the now

[dsyme]

@KevinRansom I assume you want this targeted at release/fsharp5?

[dsyme]

Heisenerror during testing

  X Automation.EnumDUInterfacefromFSBrowse.InsideMatch [210ms]
  Error Message:
   System.IO.IOException : The process cannot access the file 'C:\Users\VssAdministrator\AppData\Local\Temp\2e1002c3-895b-45e2-a4da-075b4c929043\testFile.fs' because it is being used by another process.
  Stack Trace:
     at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
   at System.IO.File.InternalDelete(String path, Boolean checkHost)
   at System.IO.File.Delete(String path)
   at VisualFSharp.UnitTests.Roslyn.FSharpProject.System-IDisposable-Dispose() in D:\a\1\s\vsintegration\tests\UnitTests\ProjectOptionsBuilder.fs:line 31
   at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.QuickInfo.GetQuickInfoTextFromCode(String code) in D:\a\1\s\vsintegration\tests\UnitTests\QuickInfoTests.fs:line 45
   at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.QuickInfo.Automation.EnumDUInterfacefromFSBrowse.InsideMatch() in D:\a\1\s\vsintegration\tests\UnitTests\QuickInfoTests.fs:line 95

[dsyme]

Three remaining failures:

fsharpqa:

2020-10-26T22:24:26.3260107Z Diagnostics\General (E_Quotation_UnresolvedGenericConstruct01.fs) -- failed

fsharp:

2020-10-26T23:03:01.7500148Z   X type check neg37 [1s 485ms]
2020-10-26T23:03:01.7502823Z   Error Message:
2020-10-26T23:03:01.7509023Z    System.Exception : neg37.err neg37.bsl differ; "diff between [D:\a\1\s\tests\fsharp\typecheck\sigs\neg37.bsl] and [D:\a\1\s\tests\fsharp\typecheck\sigs\neg37.err]
2020-10-26T23:03:01.7511353Z line 8
2020-10-26T23:03:01.7513565Z  - neg37.fs(24,10,24,37): typecheck error FS0331: The implicit instantiation of a generic construct at or near this point could not be resolved because it could resolve to multiple unrelated types, e.g. 'IComparable <'T>' and 'IEvent <'T>'. Consider using type annotations to resolve the ambiguity
2020-10-26T23:03:01.7515818Z  + neg37.fs(24,10,24,37): typecheck error FS0331: The implicit instantiation of a generic construct at or near this point could not be resolved because it could resolve to multiple unrelated types, e.g. 'IComparable <'T> %' and 'IEvent <'T> %'. Consider using type annotations to resolve the ambiguity

2020-10-26T22:47:38.1184825Z   X nullness_no_checknulls [1s 10ms]
2020-10-26T22:47:38.1193233Z   Error Message:
2020-10-26T22:47:38.1197331Z    Error running command 'D:\a\1\s\tests\FSharp.Test.Utilities\..\..\artifacts\bin\fsc\Release\net472\fsc.exe' with args '-r:System.Core.dll --nowarn:20 --define:COMPILED -o:test-no-checknulls.exe -g --define:NO_CHECKNULLS test.fsx' in directory 'D:\a\1\s\tests\fsharp\core\nullness'.
2020-10-26T22:47:38.1199836Z ---- stdout below --- 
2020-10-26T22:47:38.1200622Z Microsoft (R) F# Compiler version 11.0.0.0 for F# 5.0
2020-10-26T22:47:38.1201974Z Copyright (c) Microsoft Corporation. All Rights Reserved.
2020-10-26T22:47:38.1202615Z 
2020-10-26T22:47:38.1203151Z ---- stderr below --- 
2020-10-26T22:47:38.1203622Z 
2020-10-26T22:47:38.1204446Z test.fsx(29,14): warning FS3271: The 'nullness checking' language feature is not enabled. This use of a nullness checking construct will be ignored.
2020-10-26T22:47:38.1206024Z 
2020-10-26T22:47:38.1208435Z test.fsx(31,14): warning FS3271: The 'nullness checking' language feature is not enabled. This use of a nullness checking construct will be ignored.

[dsyme]

Very happy to see this is now green again (which gives us opportunity to to iterate on it, instead of just maintaining it)

[dsyme]

On integrating main there's a test failure in Test ProjectForWitnesses4 GetWitnessPassingInfo

I'll disable the test in this branch and add it to the list of things in the PR description

[dsyme]

@brettfo Random test failure, we takled about this one

The active test run was aborted. Reason: Test host process crashed : go
Unhandled exception. System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at [email protected](ExceptionDispatchInfo edi)
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 104
   at Microsoft.FSharp.Control.AsyncPrimitives.continuation@951-2(AsyncActivation`1 ctxt, Boolean useCcontForTaskCancellation, Task completedTask) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 952
   at Microsoft.FSharp.Control.AsyncPrimitives.taskContinueWithUnit(Task task, AsyncActivation`1 ctxt, Boolean useCcontForTaskCancellation) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 966
   at <StartupCode$FSharp-Core>[email protected](AsyncActivation`1 ctxt) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 1708
   at Microsoft.FSharp.Control.AsyncPrimitives.ProtectedCode[T](AsyncActivation`1 ctxt, FSharpFunc`2 userCode) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 448
   at <StartupCode$FSharp-Core>[email protected](AsyncActivation`1 ctxt) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 1690
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 104
   at <StartupCode$FSharp-Core>[email protected](Object o) in /Users/runner/work/1/s/src/fsharp/FSharp.Core/async.fs:line 157
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

[dsyme]

Failing test

CodeGenRenamings01

[smoothdeveloper]

@dsyme, in some of the code, it looks like | null turned into a new | Null .

Is there some background information on how they are identical? and is it an idiom which is internal to the compiler implementation?

It seems to be defined in the RFC as an active pattern though, just trying to understand better.

[vzarytovskii]

@dsyme, in some of the code, it looks like | null turned into a new | Null .

Is there some background information on how they are identical? and is it an idiom which is internal to the compiler implementation?

It seems to be defined in the RFC as an active pattern though, just trying to understand better.

Yeah, it is pretty much a (|Null|NonNull|) active pattern now

[dsyme]

@dsyme, in some of the code, it looks like | null turned into a new | Null .

Is there some background information on how they are identical? and is it an idiom which is internal to the compiler implementation?

It seems to be defined in the RFC as an active pattern though, just trying to understand better.

Within the existing compiler codebase it is just this:

    let inline (|Null|NonNull|) (x: 'T MaybeNull) : Choice<unit,'T> = match x with null -> Null | v -> NonNull v

used as in

match x with 
| Null -> ()
| NonNull x -> ...

Note the non-nullness of z on the positive branch is not "known" to the existing compiler - so this is just an unchecked idiom - for the existing compiler this is entirely equivalent to

match x with 
| null -> ()
| x -> ....

In this PR/RFC, the definition is removed from illib.fs and uses get replaced by the following definition in FSharp.Core

let inline (|Null|NonNull|) (value : 'T? when 'T : not null) = 
    match value with 
    | null -> Null () 
    | _ -> NonNull (# "" value : 'T #)

with signature:

val inline (|Null|NonNull|) : value: 'T? -> Choice<unit, 'T>  when 'T : not null

Note that here the value bound on the positive branch is known to be non-null, e.g.

match x with 
| Null -> ()
| NonNull x -> ... // x known to be non-null

The construct is not optimized in any way for either the existing compiler or this branch - so using this construct will allocate - I've been careful to only use it in places where that doesn't matter.

It remains to be decided if this construct will remain in the final design for nullness in F#, or if we will do the extra work to allow

match x with 
| null -> ()
| x -> ...

to "tighten" the type known for x. I expect we will but we need to check if it's feasible - we don't currently have any constructs where the pattern matching rules before affect the typing of patterns after.

[dsyme]

I merged main into this but it is going to take quite a lot of work to get it compiling and green again :)

[dsyme]

I'll open a new PR for this