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

Nullable reference types in F# #44047

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Nullable reference types in F# #44047

wants to merge 12 commits into from

Conversation

@dotnetrepoman dotnetrepoman bot added this to the December 2024 milestone Dec 20, 2024
@psfinaki
Copy link
Member Author

@T-Gro PTAL - I didn't want to copypaste your whole blog post to one place so tried to distribute the information among the various related docs... If you think I over- or under-copied something, let me know, will update this accordingly - the null-related information shouldn't be overwhelming but should be exhaustive.

@T-Gro
Copy link
Member

T-Gro commented Dec 30, 2024

A checklist of things I expect to see in the docs (will tick the ones I identified while reviewing):

  • Syntax change for type definition
  • Ways of handling null (pattern match, active patterns, one or two selected library functions)
  • Recommended usage (interop, vanilla F# prefers option for indicating absence of value) == guidelines
  • Generic type constraints (null and not null)
  • Nullable Project property
  • CLI fsc.exe and fsi settings
  • Conditional DEFINE added by project property

@BillWagner BillWagner modified the milestones: December 2024, January 2025 Jan 6, 2025
@psfinaki psfinaki marked this pull request as ready for review January 7, 2025 16:01
@psfinaki
Copy link
Member Author

psfinaki commented Jan 7, 2025

@T-Gro thanks for the checklist, all valid points, I think I have addressed the missing parts.

let checkNonNull argName (arg: obj) =
match arg with
// nullness warning: the type 'obj' does not support 'null'
| null -> nullArg argName
Copy link
Member

Choose a reason for hiding this comment

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

The if-else branches show identical code here.
Motivating example could e.g. be with a conditional not null constraint, or perharps for exposing a part of an public API.
Even the MaybeNull alias/wrapper could be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the idea was that the compiler directive makes the compiler produce a warning. We can have something more useful here - I added an example but not sure it counts really.

else ()
```

To address the warnings, you could explicitly specify `null` as a possible argument value:
Copy link
Member

Choose a reason for hiding this comment

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

This needs rework based on how the first example is redone.
Right now, it shows the wrong incentive.

If e.g. the first example shows a warning for (string|null).Length, that this example should show how to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed and reworked the text, take a look if it's more consistent and helpful now.

Here is an example of the valid usage of the syntax:

```fsharp
let processStream (stream: System.IO.StreamReader) =
Copy link
Member

Choose a reason for hiding this comment

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

The example has some value, but it is unrelated to the text:
The text speaks about API boundaries, but here the processLine is private to the function, so not exposed to the outside world...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, fair point. I updated the example but still don't like it too much... it might help to add something from the real world, like implementing some tiny .NET interface with nulls - would you have something from the top of your head?

| null -> ...
```

Note that the extra null related capabilities were added to the language for the interoperability purposes. Using `| null` within F# code is not considered idiomatic for denoting missing information - for that purpose, use options (as described above). Read more about null-related [conventions](../../style-guide/conventions.md#nulls-and-default-values) in the style guide.
Copy link
Member

Choose a reason for hiding this comment

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

This should be a lot more targeted.

  1. Make sure this is speaking about type modelling (|null can be used internally, in pattern matching, for function arguments,..), nothing else
  2. Make sure this is speaking about signalling a possible absence of value, and is not enforced by interop API boundaries

Copy link
Member Author

Choose a reason for hiding this comment

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

I reformulated the sentence a bit - don't want to elaborate too much since this is what conventions page is for...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc updates for F# 9 features
3 participants