Skip to content

[RFC FS-1060] Nullness checking - #15181

Merged
vzarytovskii merged 208 commits into
mainfrom
feature/nullness
Jul 17, 2024
Merged

[RFC FS-1060] Nullness checking#15181
vzarytovskii merged 208 commits into
mainfrom
feature/nullness

Conversation

@dsyme

@dsymedsyme commented May 3, 2023

Copy link
Copy Markdown
Contributor

Continuation of #5790 and #6804

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

See tests\adhoc\nullness for testing and samples including baselines of outputs from

  • existing compiler
  • updated compiler
  • updated compiler with /langversion:preview
  • updated compiler with /langversion:preview /checknulls

TODO:

  • revise RFC and resolve all unresolved issues (@dsyme)
  • implement syntax string | null and : not null (@T-Gro )
  • implement import of .NET metadata (@T-Gro )
  • implement emit of .NET metadata (@T-Gro )
  • Import+Export TODOs (to test and adjust if needed)
  • Nullness used in inheritance (e.g. inherit System.Collections.Generic.List<string | null>)
  • Soundness of import of CLI EventHandlers (possibly overly defensive)
  • Nullness and import of TypeProvider-generated types + In general the overall TP scenario whet it comes to nullness support - do in separate PR
  • Object overrides can change nullness (e.g. boolean.ToString() does or StringBuilder.ToString()), make it work to avoid false alarms
  • Type.GetType(..) |> Option.ofObj produces a false alarm, fix that
  • printfn "%s" someNullableString produces a false alarm, check the printing pattern annotations
  • (+) operator for strings gives false alarms on nullable strings, should allow nullables since String.Concat also does
  • Pattern matching flow analysis on match x with | null -> Support this as a scenario
  • Allow incoming arrays to be null (this causes false alarms as of now, it's a bug)
  • resolve all // TODO NULLNESS (@dsyme)
  • use it in the codebase and get it green
  • there's a case in tests\adhoc\nullness we're getting The types 'System.String (...)' and 'System.String (...)' do not have compatible nullability. which is wrong - either the nullability aren't shown for some reason or the types should be considered compatible

Testing:

  • add proper testing, moving tests across from tests\adhoc\nullness
  • test and 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)

To be moved to RFC and resolved, then tested here:

  • should nullness be supported on ref tuples and anon tuples
  • work out what to do about compat of Option.ofObj and others
  • the "type equiv" relation is currently directional only giving a nullness warning if "actual" has a null and "expected" is non-null. This includes a dubious change of direction in SolveFunType thing - for the contravariance of inputs. This has caused a few outputs to change in tests. This is both unsound in the general case (nested cases of equivalence should demand true equivalence) and is dubious for functions because the "MatchingOnly" case of type equivalence is also directional.
  • assess nullness checking in conditionals, e..g if x then null else "" and if x then "" else null

@dsyme
dsyme requested a review from a team as a code ownerMay 3, 2023 13:22
@dsyme
dsymeforce-pushed the feature/nullness branch from 125d7db to 605cde1CompareMay 6, 2023 00:06
@dsyme
dsymeforce-pushed the feature/nullness branch from f2f698d to 2e8ab38CompareMay 24, 2023 14:20
Comment threadsrc/Compiler/pars.fsy
@T-Gro

T-Gro commented Jul 12, 2024

Copy link
Copy Markdown
Member

A few points (so far):

Good for spotting this, thumbs up. I created new issues to cover for them.
Especially the diagnostics is better to be covered with a separate issue so that all options for various texting can be designed and presented there.

#17410
#17409

Created to cover these, can be again taken independently

@T-Gro

Copy link
Copy Markdown
Member

A non-binding question: do you see any opportunities here for further feature optimization, performance-wise?

If you do, it might be interesting to submit some benchmark along the way. This was often done when doing big features (but obviously it doesn't have to apply everywhere).

It is a compile-time feature without runtime impact.
One could in theory have a look at the added library features, but they are all pretty much basic checks against null and a branching instruction.

For the compilation times, the e2e build tests under different configurations remained the same as far as I can see (comparing this branch, which does not apply checknulls, and its sibling PR which does).

@psfinaki

Copy link
Copy Markdown
Contributor

It is a compile-time feature without runtime impact.
One could in theory have a look at the added library features, but they are all pretty much basic checks against null and a branching instruction.

No that makes, it was more about optimization potential in the implementation details. I am totally fine with not adding benchmarks here.

@psfinakipsfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few more notes.

Comment threadsrc/Compiler/Checking/CheckDeclarations.fs
Comment threadsrc/Compiler/Checking/CheckPatterns.fs
Comment threadsrc/Compiler/Checking/ConstraintSolver.fs
Comment threadsrc/Compiler/Driver/CompilerDiagnostics.fs
Comment threadsrc/FSharp.Core/prim-types-prelude.fs
Comment threadsrc/Compiler/Checking/CheckPatterns.fs
@vzarytovskii

Copy link
Copy Markdown
Member

I intend to merge it.
@psfinaki could you please make an issue (or multiple) with your findings.
The faster we get it to upstream, the sooner we can start inserting again and the sooner we will be able to test it ourselves. It might even make it to p7

@psfinakipsfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job @dsyme and @T-Gro.

I played a lot of with the feature, IMO this definitely passes all the smoke tests, as per other things - see comments.

What would be amazing is to actually update the RFC because it has some TBDs and some inconsistencies, both internal ones (e.g. withNull function) and w.r.t. implementation (e.g. obj | null support). Splitting the helper RFCs will be also benefecial.

Let's :shipit:

@psfinaki

Copy link
Copy Markdown
Contributor

@vzarytovskii it's all green now, I fixed the trimming checks. As for the issues, I'd prefer force-merging it with unresolved comments so that @T-Gro can later make sense of them.

@vzarytovskii
vzarytovskii merged commit b73be15 into mainJul 17, 2024
@dlidstrom

Copy link
Copy Markdown

It’s fantastic how you are bringing F# forward! 🎉

@nojafnojaf mentioned this pull request Sep 13, 2024
6 tasks
bartelink pushed a commit to bartelink/fsharp that referenced this pull request Aug 12, 2026
The function-domain unification order change in PR dotnet#15181 (nullness) caused
recursive inline SRTP resolution to be truncated by one currying level: an
inference variable still carrying an unsolved member constraint could be
absorbed by the required domain, merging away the pending recursive trait
resolution. This regressed patterns such as FSharpPlus `memoizeN`.
SolveFunTypeEqn now keeps the SRTP-constrained inference variable as the
unification representative for that specific case, and only when not
MatchingOnly (mirroring SolveTypeEqualsType).
Adds two ComponentTests (the memoizeN repro across currying depths, and a
MatchingOnly overload-resolution guard) and repoints the FSharpPlus
regression matrix at a minimal global.json-bump branch that exercises the
fix end-to-end.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4cfb1c86-0d7e-45e6-9529-3c5f301c0912
Sign up for freeto 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.

13 participants

@dsyme@edgarfgp@vzarytovskii@kerams@T-Gro@isaacabraham@ken-okabe@Lanayx@psfinaki@dlidstrom@xperiandri@KevinRansom@dotnet-bot