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

[RFC FS-1060] Nullness checking #6804

Closed
wants to merge 312 commits into from
Closed

[RFC FS-1060] Nullness checking #6804

wants to merge 312 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 22, 2019

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:

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

@dsyme
Copy link
Contributor Author

dsyme commented May 22, 2019

Failing tests:

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

@dsyme dsyme changed the title [RFC FS-1060] Nullness checking [WIP, RFC FS-1060] Nullness checking Jun 5, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Jul 3, 2019

@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
Copy link
Contributor Author

dsyme commented Jul 5, 2019

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 dsyme changed the title [WIP, RFC FS-1060] Nullness checking [RFC FS-1060] Nullness checking Jul 8, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Oct 15, 2019

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
Copy link
Member

brettfo commented Oct 15, 2019

@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
Copy link
Contributor Author

dsyme commented Oct 16, 2019

@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
Copy link
Contributor Author

dsyme commented Dec 16, 2019

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

@dsyme
Copy link
Contributor Author

dsyme commented Jan 17, 2020

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

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

A merge brough back fcs, src/utils/reshapredmsbuild, net40 Fsharp.Core surface area tests, and likely more

@dsyme
Copy link
Contributor Author

dsyme commented Oct 27, 2020

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
Copy link
Contributor Author

dsyme commented Oct 27, 2020

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

@dsyme
Copy link
Contributor Author

dsyme commented Oct 25, 2022

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

@dsyme
Copy link
Contributor Author

dsyme commented May 3, 2023

I'll open a new PR for this

@dsyme dsyme closed this May 3, 2023
@dsyme dsyme mentioned this pull request May 3, 2023
23 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants