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

Open
kerams opened this issue Sep 13, 2024 · 15 comments
Open

Rolling out nullness annotations #3887

kerams opened this issue Sep 13, 2024 · 15 comments

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?

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

No branches or pull requests

4 participants