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 #5790

Closed
wants to merge 290 commits into from
Closed

[RFC FS-1060] Nullness checking #5790

wants to merge 290 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Oct 18, 2018

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

DONE:

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

@cartermp
Copy link
Contributor

RFC link for folks interested: fsharp/fslang-design#317

@dsyme
Copy link
Contributor Author

dsyme commented Nov 6, 2018

@cartermp @TIHan I done a couple of major things on this

  • The F# metadata blob is now binary compatible. The nullness information is stored in a secondary resource blob and integrated into the TAST during unpickle. THis means the FSHarp.Core produced with nullness information can be consumed by older compilers.

  • Added hacky /langversion and /checknulls and /test:AssumeNullOnImport flags

    • /langversion:4.5 feature is totally off.
    • /langversion:5.0 (the default) nullness inference happens just no warnings are emitted. If you use string? syntax you get a warning
    • /langversion:5.0 /checknulls nullness inference happens and warnings are emitted.
    • /langversion:5.0 /checknulls /test:AssumeNullOnImport nullness inference happens, warnings are emitted and we interpret all reference types in .NET assemblies as nullable (rather than oblivious)

The compiler changes are now architected to be "neutral" , i.e. all nullness information is set to Oblvious everywhere for /langversion < 5.0. This makes me much more confident that we aren't messing with the existing behaviour and performance of the compiler. Also we can assess the impact of the language feature independently of other issues, and before even turning it on (there are memory and performance costs associated with the language feature even when it is not on)

@cartermp
Copy link
Contributor

cartermp commented Nov 6, 2018

@dsyme How difficult would it be to split that out into a separate PR? Or is it more practical to just merge this into master?

@dsyme
Copy link
Contributor Author

dsyme commented Nov 6, 2018

I have also added

  • implementation of 'T when 'T : not null constraint. Needs to be correlated with .NET metadata

@dsyme
Copy link
Contributor Author

dsyme commented Nov 6, 2018

@dsyme How difficult would it be to split that out into a separate PR? Or is it more practical to just merge this into master?

Those things are integral to this PR, though the "langversion" switch could be carved out.

We should merge this into master soon after confirming the overall performance degradation for the feature, to avoid massive conflicts (I'm working through the conflicts above now)

@cartermp
Copy link
Contributor

cartermp commented Nov 6, 2018

Okay. We don't have a date for this, but once we lock down on compiler/core library changes for VS 16.0, we can "lock" and merge this into master.

@dsyme
Copy link
Contributor Author

dsyme commented Nov 13, 2018

OK, good news!

  • I have successfully enabled nullness checking on FSharp.Compiler.Private.dll using the prototype

This is a very large codebase so it's good to see it checks

Some notes about this work so far and its limitations are in fsharp/fslang-design#339

@dsyme dsyme changed the title [WIP] Nullness checking early prototype [CompilerPerf] Nullness checking prototype Nov 16, 2018
dsyme and others added 25 commits April 16, 2019 00:13
@brettfo
Copy link
Member

brettfo commented May 21, 2019

Closing due to branch rename nullness -> feature/nullness.

@brettfo brettfo closed this May 21, 2019
@brettfo brettfo deleted the nullness branch May 21, 2019 22:25
@dsyme dsyme mentioned this pull request May 22, 2019
15 tasks
@dsyme dsyme mentioned this pull request May 3, 2023
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants