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 issue - Downcasting should preserve nullness information #17532

Closed
1 of 7 tasks
kerams opened this issue Aug 14, 2024 · 6 comments
Closed
1 of 7 tasks

Nullness issue - Downcasting should preserve nullness information #17532

kerams opened this issue Aug 14, 2024 · 6 comments
Assignees
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Feature Improvement
Milestone

Comments

@kerams
Copy link
Contributor

kerams commented Aug 14, 2024

Issue description

Downcasting is an operation that returns null when input is null. However, when the explicit coercion type is not nullable, the downcasting result currently won't be nullable either. Either a nullable type should be required (:?> string | null) or the downcasting operator should automatically adjust nullness based on the input type.

Choose one or more from the following categories of impact

  • Unexpected nullness warning (false positive in nullness checking, code uses --checknulls and langversion:preview).
  • Missing nullness warning in a case which can produce nulls (false negative, code uses --checknulls and langversion:preview).
  • Breaking change related to older null constructs in code not using the checknulls switch.
  • Breaking change related to generic code and explicit type constraints (null, not null).
  • Type inference issue (i.e. code worked without type annotations before, and applying the --checknulls enforces type annotations).
  • C#/F# interop issue related to nullness metadata.
  • Other (none of the categories above apply).

Operating System

Windows (Default)

What .NET runtime/SDK kind are you seeing the issue on

.NET SDK (.NET Core, .NET 5+)

.NET Runtime/SDK version

9.0.0-preview.7.24405.7

Reproducible code snippet and actual behavior

((null: obj | null) :?> string).Length

No warning, throws NRE.

Possible workarounds

No response

@kerams kerams added Bug Needs-Triage Area-Nullness Issues related to handling of Nullable Reference Types labels Aug 14, 2024
@github-actions github-actions bot added this to the Backlog milestone Aug 14, 2024
@kerams
Copy link
Contributor Author

kerams commented Aug 14, 2024

@T-Gro
Copy link
Member

T-Gro commented Aug 14, 2024

I think there are two perspectives:

  1. Admit that with downcasting, the code is inherently dangerous at runtime. i.e. allow downcasting e.g. a nullable obj into a DU
  2. Try to maintain nullness (similar to [NotNullIfNotNull] in C# flow analysis), even though this might complicate downcasting-heavy code.

This is further complicated by the fslib Unboxing functions doing a runtime lookup, where nullness information is no longer known.

A solution which would insist on downcasting a nullable source into a nullable target ( :?> string | null) would only work on target types which can carry nullness, which is not equal to all types that can be downcasted to (tuples, anon records).

@T-Gro
Copy link
Member

T-Gro commented Aug 14, 2024

A safe but not all-scenarios-covering change would be:

  • If source is nullable
  • Target can be enhanced to be nullable
  • Insist on the downcasting target to be decorated | null, report a nullness warning if it isn't

Would that meet the expectations here?

@charlesroddie
Copy link
Contributor

((null: obj | null) :?> string).Length

This shouldn't throw an NRE but should throw an exception during the :?> string cast.

f(x: string | null) = x :?> string should succeed if x is a string, and fail if x is null (since, post NRTs, null is not a string).

No separate warning should be given since now new danger is introduced: :?> is known to throw if the LHS is not of the type on the RHS.

If there is a warning it should apply to all uses of :?> ("don't use this").

@kerams
Copy link
Contributor Author

kerams commented Oct 2, 2024

but should throw an exception during the :?> string cast

Feel free to take up that breaking change (which is what a different exception constitutes, I think) with the runtime team (unless you're suggesting F# should start injecting extra null checks before the unbox.any instruction).

@T-Gro
Copy link
Member

T-Gro commented Nov 25, 2024

Fixed by #17965

@T-Gro T-Gro closed this as completed Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Feature Improvement
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants