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

[JS/TS] add tests for nullness supports #4070

Merged
merged 5 commits into from
Mar 8, 2025
Merged

[JS/TS] add tests for nullness supports #4070

merged 5 commits into from
Mar 8, 2025

Conversation

MangelMaxime
Copy link
Member

@MangelMaxime MangelMaxime commented Mar 5, 2025

Fix #3887
Fix #3991

@kerams

It seems like Fable already supports "nullness" and its new API thanks to @ncave adding the new NonNull, Null, APIs.

I tried to create some tests suites to check if "nullness" is working but I think that right now this is more testing if T | null and related APIs works with Fable.

I am not sure if we want to enable on the test project <Nullable>enable</Nullable>, this generates several new warnings and I am not sure if this adds value for this specific project.

Example of places with warnings:

image

image

image

If we thinks that enabling nullable make sense, I can give a try to fix them.

I am not familiar with nullness API, @kerams do you see a common case that is not covered and you would like to be covered? I tried to look at F# repository for tests regarding but it seems like most of them are related to testing compiler warning.

[JS/TS] Add support for `Unchecked.nonNull`
@kerams
Copy link
Contributor

kerams commented Mar 6, 2025

Off the top of my head, adding nullness to input parameters of some Fable.Core functions. More often than not it will be changing obj -> objnull.

  • JsInterop.isNullOrUndefined
  • JsInterop.emitJsExpr (you can pass null or () if you don't refer to the parameters)
  • JsInterop.emitJsStatement
  • JsInterop.jsTypeOf
  • JS.Constructors.Array.isArray
  • etc.

There's a LOT more, but the functions can be easily adjusted on demand later. The only downside is the signature in Fable.Core has to be hidden behind #if FABLE_COMPILER_5 || NUGET_BUILD.

@kerams
Copy link
Contributor

kerams commented Mar 6, 2025

I am not sure if we want to enable on the test project enable, this generates several new warnings and I am not sure if this adds value for this specific project.

The problem is, as far as I can see F# does not respect #nullable enable, so without <Nullable>enable</Nullable> you are not checking whether Fable actually shows warnings and enforces them in combination with TreatWarningsAsErrors. If you just test it manually for now, I'll believe you:)

@MangelMaxime
Copy link
Member Author

Off the top of my head, adding nullness to input parameters of some Fable.Core functions. More often than not it will be changing obj -> objnull.

* `JsInterop.isNullOrUndefined`

* `JsInterop.emitJsExpr` (you can pass `null` or `()` if you don't refer to the parameters)

* `JsInterop.emitJsStatement`

* `JsInterop.jsTypeOf`

* `JS.Constructors.Array.isArray`

* etc.

There's a LOT more, but the functions can be easily adjusted on demand later. The only downside is the signature in Fable.Core has to be hidden behind #if FABLE_COMPILER_5 || NUGET_BUILD.

Here, I think you are not speaking about nullness support directly but more shaping Fable.Core API to takes advantages of nullness.

Fable.Core is a pure binding, meaning that it is served as a .dll only not fable folder with the sources files. This means that the compiler directives will have no effect.

We will need to check if nullness information are backward compatible in general. But if we move to use | null instead of Option in the API design this will probably be a major bump because of breaking changes.

The problem is, as far as I can see F# does not respect #nullable enable, so without <Nullable>enable</Nullable> you are not checking whether Fable actually shows warnings and enforces them in combination with TreatWarningsAsErrors. If you just test it manually for now, I'll believe you:)

You are right neither the warning or error is reported by Fable right now. I will take a look why we don't get the diagnostics (perhaps we are not ignoring it or not passing the configuration to FCS).

@kerams
Copy link
Contributor

kerams commented Mar 6, 2025

We will need to check if nullness information are backward compatible in general

It is, so if Fable.Core is not distributed in source form, we don't need directives.

@MangelMaxime
Copy link
Member Author

Nullness warnings are now reported.

image

Supporting TreatWarningsAsErrors is a whole other task.

This is because Fable recreate a "single project" using the user project and dependencies. So when enabling, TreatWarningsAsErrors the compilation could fails because of a warning coming from a dependencies project.

@MangelMaxime
Copy link
Member Author

It seems like when we enable nullness F# generates errors in certain situation when a constraint don't have the not null constraint.

Below is the output of using Fable with <Nullable> enabled on the main project.

image

I suspect this is because Fable compile the sources files of the packages where in a normal situation F# just consume the dll and if it don't have the nullable metadata then it is fine with it.

This seems to show that in order to support nullness in Fable ecosystem, we will need to update all the libraries to use compatible nullness code.

Hopefully, if a code use | null syntax it will still be able to be processed by F# even if nullable is not enabled otherwise libraries will probably need to add #if NULLABLE to offer both APIs...

@kerams
Copy link
Contributor

kerams commented Mar 7, 2025

Damn, that's a good point - we're compiling every library in a null-aware context.

Hopefully, if a code use | null syntax it will still be able to be processed by F#

Only with Fable 5 :/ Sadly, I don't think there's a way around this, and we'll just need to go update these projects.

@MangelMaxime
Copy link
Member Author

Hopefully, if a code use | null syntax it will still be able to be processed by F#

Only with Fable 5 :/ Sadly, I don't think there's a way around this, and we'll just need to go update these projects.

Oh because otherwise, it will generate a syntax error right?

Unless, the library author use #if !NULLABLE or #if FABLE_COMPILER_4 to support non null able syntax.

This is unfortunate that this is causing issues in both direction...

@MangelMaxime
Copy link
Member Author

If we wanted to avoid this problem, we would probably have to explore the idea of compiling the libraries and the main project in 2 separates projects.

Like mentioned here

I think we could have what is is needed to do that now, because Fable has the ability to use pre-compiled projects even if this is something I am not familiar with at all... And I am not sure how much work it would be to rework Fable to compile 2 projects instead of one.

Note: I believe I found a solution to support TreatWarningsAsErrors at Fable level without too much code change. Need to test it more

@MangelMaxime MangelMaxime merged commit 048f27e into main Mar 8, 2025
19 checks passed
@MangelMaxime MangelMaxime deleted the nullness branch March 8, 2025 14:11
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.

Support Unchecked.nonNull for F# 9 Rolling out nullness annotations
2 participants