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

Rolling out nullness annotations #3887

Closed
kerams opened this issue Sep 13, 2024 · 32 comments · Fixed by #4070
Closed

Rolling out nullness annotations #3887

kerams opened this issue Sep 13, 2024 · 32 comments · Fixed by #4070
Assignees

Comments

@kerams
Copy link
Contributor

kerams commented Sep 13, 2024

.NET 9 is around the corner, and with it support for consuming and emitting nullness metadata in F# projects. JS APIs will often return null/undefined, and while Fable bindings often capture this information by using options, sometimes we give no indication that nulls are a possibility. I think F# 9 will be really useful here, so I'd like to start a discussion about the adoption plan for these annotations.

From the technical point of view, binding projects need to be compiled using .NET 9 SDK, <LangVersion>preview</LangVersion> (for now) and with the --checknulls+ flag (also for now - I think this will be aligned with C#'s <Nullable>enable</Nullable>). TFMs don't matter, so netstandard2.0 assemblies will contain nullness metadata too.

Finally, every API will need to be manually reviewed and annotated as required. In the case of existing bindings that use options with reference types, there's the question of whether we want to preserve options or replace them with null annotations, just so that a single, unified approach is used.

@MangelMaxime
Copy link
Member

Not only the binding needs to be updated to support nullness supports but we will also need to update the FCS fork that Fable use so we can get access to the language feature.

Are the configuration you mentioned to be set on the binding project or do the user project also needs to set it? What I mean is what happens if we decide to use nullness for the binding but the user project doesn't active the language feature on its project?

Will he still have the error generated?

@kerams
Copy link
Contributor Author

kerams commented Sep 13, 2024

we will also need to update the FCS fork that Fable use so we can get access to the language feature

True, but that should be easy enough. Also, you'll get warnings in the IDE regardless.

Are the configuration you mentioned to be set on the binding project or do the user project also needs to set it?

Consumers of Fable bindings also need to enable nullness checking in their projects, meaning we don't need to publish a separate set of packages with nullness metadata - it's business as usual for users that do not opt into the feature, even with assemblies enriched with nullness annotations.

Ah, I just realized Fable packages include source files, this makes things much more complicated.

@MangelMaxime
Copy link
Member

I am tracking the nullness supports in Glutinum project here. It ask questions regarding how things should be represented.

It could also be a good time for checking how Glutinum handle Dom APIs. I always had the vision to be able to generate such APIs because they are kind of big and being able to generate them means we should be able to applied optimisation to them over time.

The original version of Fable.Browser, was actually generated and over time we stopped doing that because the project was splitted in different package. It was to simply the maintenance I believe but over time we discovered that it was not so much the case.

@MangelMaxime
Copy link
Member

Consumers of Fable bindings also need to enable nullness checking in their projects, meaning we don't need to publish a separate set of packages with nullness metadata - it's business as usual for users that do not opt into the feature, even with assemblies enriched with nullness annotations.

So if I read correctly, if we decide to use NullNess instead of Option and the user don't active the feature in their project. Then there is a regression because they will not be forced to check for the Null case like it is for Option?

@kerams
Copy link
Contributor Author

kerams commented Sep 13, 2024

If you switch an existing option annotation to | null, you'll get a compile-time breaking change, so no silent regressions.

@MangelMaxime
Copy link
Member

Ah yes right, this is one of the reason we use F# after all 😅 (sometimes you forget some of the simplest things).

@kerams
Copy link
Contributor Author

kerams commented Sep 13, 2024

Ah, I just realized Fable packages include source files, this makes things much more complicated.

Since we have to distribute source files, I think the only option is to use conditional compilation for annotations. Something like

#if NET9_0_OR_GREATER
abstract getElementById: elementId: string -> HTMLElement | null
#else
abstract getElementById: elementId: string -> HTMLElement
#endif

but then we'll also need to add net9.0 TFMs so that nullness warnings show up in the IDE, and users also need to retarget their Fable projects to net9.0 so that nullness is checked during compilation (once the Fable compiler is updated).

@MangelMaxime
Copy link
Member

MangelMaxime commented Sep 14, 2024

If we need to duplicate the API surface to support nullness then I will probably vote against it, we need to either commit to it or keep using Option for the time being.

Duplicating the API surface means depending on your configuration you have a different API which his confusing
and documentation become harder to write.

I wonder why for this new feature people need to use target net9.0?

I mean in previous addition to F# people where able to keep using netstandard2.1 for example without issues.

After thinking, I think I understand why you want to do that. This is to have support for compiler directives to branch out the API with or without nullness.

If I remember how TFM works, in order to use net9.0 in your project you need to use .NET 9.0 SDK. And this is problematic because it means forcing people to upgrade to .NET 9.0 which is not a LTS version. I think people tend to avoid upgrading in general and favor LTS.

That NET 9.0 upgrade is not so much a problem for Fable because we don't really respect/use that information. But it is a problem for Server code I think.


I suppose the fact that F# distribute source files, is what prevents this part of the RFC to works:

Older F# components consuming F# assemblies that do have nullability

F# components are no different than non-F# components when it comes to being consumed by an older F# codebase. The nullability attribute is simply ignored by older F# compilers.

RFC Source


Depending what can or cannot be done, I think it is also possible to use a custom type to avoid duplicating the code:

type Nullable<'T> =
    #if NET9_0_OR_GREATER
    'T | null
    #else
    'T option
    #endif

It still provides 2 different API depending on the configuration but at least doesn't duplicate code in the binding/library code.

@MangelMaxime MangelMaxime transferred this issue from fable-compiler/fable-browser Sep 14, 2024
@MangelMaxime
Copy link
Member

I decided to move this issue into Fable repository because there is more to decide that the aesthetic of the API.

@ncave As you probably know more about FCS + Fable internals, what is your vision on how nullness could be added to Fable?

@MangelMaxime
Copy link
Member

but then we'll also need to add net9.0 TFMs so that nullness warnings show up in the IDE, and users also need to retarget their Fable projects to net9.0 so that nullness is checked during compilation (once the Fable compiler is updated).

I wonder why for this new feature people need to use target net9.0?

I mean in previous addition to F# people where able to keep using netstandard2.1 for example without issues.

After thinking, I think I understand why you want to do that. This is to have support for compiler directives to branch out the API with or without nullness.

To avoid the need for targeting net9.0, we could also decide to add a custom compiler directive in Fable like FABLE_NULLNESS. And we could make it active by default or not starting a certain version of Fable.

@kerams
Copy link
Contributor Author

kerams commented Sep 14, 2024

If I remember how TFM works, in order to use net9.0 in your project you need to use .NET 9.0 SDK

You need .NET 9 to use nullness in the first place, so I don't see why requiring net9.0 to make use of nullness-aware Fable bindings is a problem.

Duplicating the API surface means depending on your configuration you have a different API

Technically speaking, I'd say it's the same API surface with extra metadata. In any case, that's just how C# and now F# work. With different values of <Nullable>enable</Nullable> you get a "different" API surface for methods across the BCL, and if you also use warnings as errors, code that no longer compiles. In my opinion this is just par for the course.

I suppose the fact that F# distribute source files, is what prevents this part of the RFC to works

Precisely.

type Nullable<'T> =

This will give you truly different API surfaces. In Fable JS there isn't going to be any difference in the transpiled output as far as I am aware (options only exist at compile time), but with projects compiled with .NET, you get incompatible IL and run-time differences. Most importantly, you will also need different method bodies.

@MangelMaxime
Copy link
Member

Duplicating the API surface means depending on your configuration you have a different API

Technically speaking, I'd say it's the same API surface with extra metadata. In any case, that's just how C# and now F# work. With different values of <Nullable>enable</Nullable> you get a "different" API surface for methods across the BCL, and if you also use warnings as errors, code that no longer compiles. In my opinion this is just par for the course.

Perhaps, but it still means that as a maintainer you need to double the amount of code you maintains. And personally, I don't think I would do it, having bindings with thousands of lines is already not fun to maintain 😅 (so if I need to double the amount ...)

Especially, because using Option has worked good enough for such scenarios.

type Nullable<'T> =

This will give you truly different API surfaces. In Fable JS there isn't going to be any difference in the transpiled output as far as I am aware (options only exist at compile time), but with projects compiled with .NET, you get incompatible IL and run-time differences. Most importantly, you will also need different method bodies.

I see, my vision was primarily focused on Fable because the original discussion was about bindings. And from Fable POV, option or | null will probably be the same. So Nullable was only aimed to be used for interop not libraries.

Then perhaps the way to mimic what you are describing is:

type Nullable<'T> =
    #if NET9_0_OR_GREATER
    'T | null
    #else
    'T
    #endif

For me, it is also difficult to take a decision on such things without being able to play with the code. Because, there are a lot of things that I don't see without having a compiler result / code output to check.

@T-Gro
Copy link

T-Gro commented Sep 15, 2024

As an independent observer, I would not recommend replacing existing options with T | null. The option type has stronger support both in the Fsharp.Core standard library as well as in the surrounding ecosystem, it is a more idiomatic container to use.

I would consider adding the Nullable annotation around values that have not been optional until now, but were practically shown to be null at runtime in various cases.
To enable conditional compilation, the <Nullable>project property is adding an automatic --define:NULLABLE. Which could then be used to decide between 'T | null and 'T

@ncave
Copy link
Collaborator

ncave commented Sep 15, 2024

@MangelMaxime Currently Fable transpiles F# options to undefined (when None), I wonder if that would be a better default for Nullable<T> as well. But I don't maintain a lot of JS bindings, nor do I have strong opinions about it, so whatever works best, I just agree with others that we should try to keep using options where we can. Perhaps reasoning about an actual JS binding (or a fragment of) as an example can help clarify what works well.

On a side note, the best explanation of nullable types I've ever read was, oddly enough, in Dart documentation.

@MangelMaxime
Copy link
Member

Thanks both of you for your input.

Personally, my current position is:

  1. We add support for nullness in Fable, because we want feature parity regarding F# specs/language.

    I think that the initial implementation should probably behave like option. If there is a value output that value otherwise generate

    • null for Python, Rust, Dart, etc.
    • undefined for JavaScript? Or do we want null, allowing Fable to have None to represents undefined and | null for null.

    I think undefined is less confusing. If a user want to distinguish between T, null, undefined they are doing something really special and could use a specialised type perhaps?

  2. We keep recommending option in bindings because up to now it worked fine. Thanks to that, it give time for every one to experiment with the new nullness support.

    For example, F# community need to figure out its usage in normal F# code regarding the existing option type. @T-Gro seems to say that Option has a stronger support so perhaps people will limit nullness for a safer usage of BCL/C# code.

    From Fable point of view, if nullness is deemed a better candidate/experimentation for binding (both for .NET and others runtimes). It will give us the time, to check what impact it has.

    I am saying a better candidate here, because if the changes are just a "rename" then I think keeping option is better because it will give a single way of representing | null or | undefined from TypeScript, etc. With the benefit of not forking the ecosystem. We need to consider that now days, existing libraries for Fable evolve slowly compare to before. Look at the Fable.React 9.0.0 + Feliz 2 delays for upgrading the libraries (some of them required a fork..)

    This is both because:

    • In general once a library is produced using F# then it tends to just works
    • And, because we have a fewer number of maintainers
  3. We also need to think how does nullness works in regard to libraries that works both for .NET + Fable runtimes.

    • Does it works out of the box?
    • Do we need to create a type Nullable and have some Fable internal/replacement magic happening/
    type Nullable<'T> =
        #if NULLABLE
        'T | null
        #else
        'T
        #endif

    Will this code mimic the behaviour on .NET compared to string | null (vs Nullable<string>). How does it behave for Fable, and what does it output?

@MangelMaxime
Copy link
Member

I am starting to look at how to implement Nullable in Fable.

I think we could map Nullable to check against only null in JavaScript. My idea being, once F# supports anonymous DUs which is planned and is waiting for the C# implementation of it.

We could make some Fable magic to map | undefined to check against undefined.

This would gives us the ability to support string | null | undefined just like in TypeScript.

If we map | null to check for both null and undefined like I originally proposed above we will loose the possibility of doing something like string | null | undefined without introducing a subtle breaking chance.

@joprice
Copy link
Contributor

joprice commented Dec 17, 2024

For comparison, Scalajs did a similar solution https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/index.html#UndefOr[+A]=A@scala.annotation.unchecked.uncheckedVariance|Unit. Scala 3 has union types, but Scalajs supports scala 2 as well, so has it's own union type that uses implicits, similar to U2 https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/$bar.html

@ncave
Copy link
Collaborator

ncave commented Jan 7, 2025

We need a way to model certain JS APIs (e.g. Map.get or Array.find) that return T | undefined.
Here are the existing types we have:

export type Nullable<T> = T | null | undefined;
export type Option<T> = T | Some<T> | undefined;

IMO it makes most sense to use the new nullable types as T | undefined, instead of one of the existing types. But I could be wrong, so feel free to disagree.

@MangelMaxime
Copy link
Member

Here are some examples code with their compilation equivalent trying to explains my point of view:

Mapping nullness to T | null in JS/TS

type NullableInt = int | null;
// TS => type NullableInt = number | null

// Defined in Fable.Core
type undefined = obj; // perhaps obj is not the correct representation but this is for example

type NullableOrUndefinedInt = int | null | undefined;
// TS => type NullableOrUndefinedInt = number | null | undefined

Mapping nullness to T | undefined in JS/TS

type NullableInt = int | null;
// TS => type NullableInt = number | undefined
// Here we have an issue because we can't express the `null` constraint in TS/JS from the F# code

// Unless we defined some custom types like
type jsUndefined = obj
type jsNull = obj

type NullableInt = int | jsNull;
// TS => type NullableInt = number | null

type NullableOrUndefinedInt = int | jsNull | jsUndefined;
// TS => type NullableOrUndefinedInt = number | null | undefined

For this reason, I think mapping Nullness to null offer a more straightforward mapping to JS/TS and allows us to support undefined transparently in the future.

@kerams
Copy link
Contributor Author

kerams commented Jan 8, 2025

You're all talking about TS type annotations, right? When targetting JS, I'd expect string | null to cover both null and undefined, because I consider the distinction between the two a language implementation detail that I don't care about as an F# developer outside of low-level interop scenarios (unless there's some API in the Fable.Browser packages where distinguishing between a returned null and undefined is of actual significance).

@MangelMaxime
Copy link
Member

You're all talking about TS type annotations, right?

We are talking about which behavior it should have when targeting JS/TS.

The distinction can be/is needed for interop with JavaScript libraries, in JavaScript null and undefined are not exactly equivalent.

  • undefined, means a variable has been declared but not assigned
  • null, means a variable has been declared and assigned the value null

@kerams
Copy link
Contributor Author

kerams commented Jan 8, 2025

OK, but if you're saying that we need to be able to tell the difference in F# too and an API accepts or can return undefined, we cannot use nullable reference types, since string | null | undefinedType would require fsharp/fslang-suggestions#538

@MangelMaxime
Copy link
Member

Yes, I know that to do we need string | null | undefinedType fsharp/fslang-suggestions#538.

But IHMO we need to plan for when it will be available, anonymous DUs is planned by the F# team but they are waiting for C# to implement it first for compatibility. So "we know" it is coming.

However, if we originally map nullness to null | undefined we will not be able to later say that actually nullness map to null only as it would introduce a breaking changes that can't be easily spotted, and could break libraries that has been published since then.

@kerams
Copy link
Contributor Author

kerams commented Jan 8, 2025

It's coming who knows when. Regardless, we can close the issue then?

@MangelMaxime
Copy link
Member

It's coming who knows when.

I know ...

Regardless, we can close the issue then?

I don't think we support nullness yet in Fable at least no work was done in that direction beside updating the FCS. We need to check what actually happens when nullness is used with Fable currently.

@T-Gro
Copy link

T-Gro commented Jan 9, 2025

Yes, I know that to do we need string | null | undefinedType fsharp/fslang-suggestions#538.

The waiting is mainly for possible changes in the runtime representation, which of course are not relevant for Fable.
Since jsNull and jsUndefined are only tags without data, I don't think much is to be gained to be honest (anonymous type-tagged unions are useful when you have typed-data and want to omit the tag, kind of an opposite scenario).

For the low level scenarios that need to see a difference (i.e., should not be the default because of ergonomy), is there a reason why something like:
type JsLowLevel<'T> = Data of 'T | Undefined | Null
would not work?

Reason is, besides not having to wait for anonymous unions, this would also free Fable from .NET-imposed limitations on the F#9 | null feature, mainly around generic code and ability to work with value types.

@MangelMaxime
Copy link
Member

Reason is, besides not having to wait for anonymous unions, this would also free Fable from .NET-imposed limitations on the F#9 | null feature, mainly around generic code and ability to work with value types.

I don't know well | null, so your comment made me read again the documentation of F# nullable.

I didn't find information about "limitations [...] mainly around generic code" but I found this section:

CleanShot 2025-01-09 at 09 52 27@2x

Which seems to defeat my idea of using | null to represent null in JavaScript, because we can't assign null in F# to nullness. I hope my sentence is clear.

Because of that it seems like we need to consider nullness supports as a F# feature, and not so much as a JS/TS equivalent to null value (for interop).

I am also not sure what you mean by:

and ability to work with value types

For the low level scenarios that need to see a difference (i.e., should not be the default because of ergonomy), is there a reason why something like:
type JsLowLevel<'T> = Data of 'T | Undefined | Null
would not work?

Here you are using a real DUs right? I suppose we could indeed make it work, I am not sure how much magic from Fable we would need to do it.

@ncave
Copy link
Collaborator

ncave commented Jan 9, 2025

@MangelMaxime Do you have a binding example of why can't we just transpile the F# nullable types (T | null) to the existing wider Fable TS type Nullable<T>?

export type Nullable<T> = T | null | undefined;

@MangelMaxime
Copy link
Member

I don't have one under the hand not, and the more the discussion goes and the more compiling to the existing seems to be ok for me.

My initial reason was to make a distinction in the future and have almost a 1 to 1 match with TypeScript syntax which is always good for bindings but this is not possible.

@MangelMaxime
Copy link
Member

I think the next step now is to give it a try to compile to:

export type Nullable<T> = T | null | undefined;

add some tests to check how it plays and adapt if needed.

@T-Gro
Copy link

T-Gro commented Jan 9, 2025

Here you are using a real DUs right? I suppose we could indeed make it work, I am not sure how much magic from Fable we would need to do it.

A real DU, yes.

I think you got it correctly:
The F# 9 T | null construct enforces constraints on T in typechecking: The underlying T must be non nullable, and must be a reference type. Meaning you cannot do int | null in F# 9. And if you use | null in a generic wrapper, the type argument constraint is propagated to that wrapper as well.

@ncave
Copy link
Collaborator

ncave commented Mar 8, 2025

@MangelMaxime

Then there is the question of how to properly represent the Nullable Reference Types in Fable.AST.
The best I could come up with (without making large changes to the Fable.AST) was to wrap them in a Nullable as a marker (not enabled yet).

Once we come up with some tests for <Nullable>enable</Nullable>, we can start working on proper support.

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 a pull request may close this issue.

5 participants