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

Nullness : downcasting should understand nullness information #17961

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Nov 6, 2024

Addresses #17532

This revisits what downcasts and typetests do in the context of nullability analysis.
In the end, both unboxing and typetests are a runtime instruction, which is not aware of any nullability in the form | null. It is aware of internal nullability of NullTrueValue F# types, like option and unit.

The following is ensured:

  • Downcasting from a value which can carry null into a WithoutNull one gives a new warning
  • Downcasting from a non-nullable value into nullable gives (existing) warning about type erasure
  • Typetesting againts nullable versions of types gives warning about type erasure (the "isinst" instruction will always return false for null in the source, so typetesting for nullable versions does not make sense - except for type where null is a true value, where the FSharp.Core function understands this and special-cases types like unit and option)

The UnboxGeneric function in F# Core, in its current version, is doing a runtime test against null and throws if the runtime-known information is not a type accepting null. However, this is runtime and reflection, not aware of nullable analysis.
For that reason, downcasting into | null versions of types, including nullable generic type parameters, now avoid this UnboxGeneric call and immediately go to the unbox.any instruction (which turns null in the input into null in the output, instead of throwing).

@T-Gro T-Gro linked an issue Nov 6, 2024 that may be closed by this pull request
7 tasks
Copy link
Contributor

github-actions bot commented Nov 6, 2024

❗ Release notes required

@T-Gro,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md No release notes found or release notes format is not correct

@T-Gro T-Gro closed this Nov 6, 2024
@T-Gro T-Gro deleted the 17532-nullness-issue---downcasting-should-preserve-nullness-information branch November 6, 2024 14:39
@@ -1118,7 +1118,7 @@ Expected:
Actual:
{actual}"""
let updateBaseline () =
snd (Int32.TryParse(Environment.GetEnvironmentVariable("TEST_UPDATE_BSL"))) <> 0
true//snd (Int32.TryParse(Environment.GetEnvironmentVariable("TEST_UPDATE_BSL"))) <> 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget about this :)

Sign up for free to 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.

Nullness issue - Downcasting should preserve nullness information
2 participants