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 - unclear how to deal with 'T | null interop #17734

Closed
1 of 7 tasks
Lanayx opened this issue Sep 15, 2024 · 26 comments
Closed
1 of 7 tasks

Nullness issue - unclear how to deal with 'T | null interop #17734

Lanayx opened this issue Sep 15, 2024 · 26 comments
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Bug Needs-Triage
Milestone

Comments

@Lanayx
Copy link
Contributor

Lanayx commented Sep 15, 2024

Issue description

Some C# public API's currently expose 'T | null in signature, while F# adds additional constraints per RFC. With F# it should be possible to:

  1. Consume such types
  2. Expose such types (without additional constraints)

It seems that neither of those points is posssible with .NET 9 RC1. Real application is Oxpecker library where I'm unable to express deserialize method signature to match C# underlying call.

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

No response

Reproducible code snippet and actual behavior

open System.Text.Json
let x = JsonSerializer.Deserialize<int>("")  // x has type int | null

image

Possible workarounds

I couldn't find any workarounds

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

Lanayx commented Sep 15, 2024

Adding C# code for reference:

var x = ctx.Request.ReadFromJsonAsync<int>().Result; // x is of type int
var y = ctx.Request.ReadFromJsonAsync<string>().Result; // y is of type string?

Declaration:

public static ValueTask<TValue?> ReadFromJsonAsync<TValue>(
    this HttpRequest request, 
    CancellationToken cancellationToken = default(CancellationToken))
 in class Microsoft.AspNetCore.Http.HttpRequestJsonExtensions

@vzarytovskii
Copy link
Member

Expose such types (without additional constraints

| null is only about nullable reference types, if you declare it as a type, it is inferred as reference. For nullable value types System.Nullable should be used, so adding a constraint is expected.

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

This is a consequence of the C# code misleading about the return type - because the returned value is not nullable in the System.Nullable sense (as it would be in C# in a non-generic context), the nullness simply isn't there at all if instantiated by value type.

Without knowing the implementation, one could assume that the C# code returns null for reference types and default for value types - without any indication to the caller that a default value was chosen. It will not crash with NRE, but it might be another source of issues if the absence of information is not communicated via the type system.

Intention-revealing workaround would be to wrap this method in a dedicated generic call restricted to value types, and dealing with absence of information in value types explicitly.

I do understand that C# does allow (T | null) in generic code without it having the right meaning for value types ;; a compromise taken to allow coexistence of NRTs and value types. I do not believe F# should repeat exactly the same, at least not without an approved suggestion.

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

My suggestion for now would be to expose two methods:

Deserialise for reference types returning T | null.
Deserialise for value types returning Nullable of T (or other container for absence of information).

A bigger ask would be the unification of NRTs and NVTs in the type system already. However, since they have different representations at runtime (none for NRTs, System.Nullable for NVTs) this would still not work across interop boundary. But assuming an approved suggestion and RFC, I think an implementation for F#-inlined generic code could make this work (ignoring the efforts for now).

@vzarytovskii
Copy link
Member

I do not believe F# should repeat exactly the same

I second that

@T-Gro T-Gro removed their assignment Sep 15, 2024
@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 15, 2024

A bigger ask would be the unification of NRTs and NVTs in the type system already.

I have discussed it with Don numerous times, and it was decided that we don't want to do it. We don't want to encourage people to use nullable reference types let alone unify nullables under one syntactic structure. It will make it harder to understand what is going on and will be hiding concrete types behind the new construct. In future this can be partially (on the declaration and params side) remediated when we introduce anonymous unions.

That said, I suggest creating a new separate language suggestion w.r.t constraint-by-default, S.Nullable<_> and their interop/unification with nullable references feature.

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 15, 2024

A bigger ask would be the unification of NRTs and NVTs in the type system already.

I have given it a thought and I think this is not required, the ability to speficify or in costraints should be sufficient:

'T when ('T: not null and 'T: not struct) or 'T: struct

As far as I remember while this can't be done in user code, it still can be used in FSharp.Core, so defining

type NRT<'T  when ('T: not null and 'T: not struct) or 'T: struct> = 'T

might work, i.e. it will allow exposing the same type as C# exposes now

That said, I suggest creating a language suggestion w.r.t constraint-by-default, S.Nullable<_> and their interop with nullable references feature.

This still doesn't resolve the consuming part right now, e.g. once you use this API you'll get int | null type derived while it seems that this should be impossible. So this interop case should be handled anyway.

My suggestion for now would be to expose two methods:
Deserialise for reference types returning T | null.
Deserialise for value types returning Nullable of T (or other container for absence of information).

This is very unpleasant from consumer's view and also doesn't work
image

To be clear - I don't know what is the implementation underneath (when null is returned for ref types), I just want to preserve the same types that existing API returns

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

Syntactically, I do get the ask for "or" in constraints.

However, this does not have an IL equivalent for codegen.
And even in the case of inlining, it would mean typechecking the body of a function with "or" type parameter constraints via the common denominator (so comparison with null would not be possible, assignment to null, binding with " | null" etc.).

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

What could be done is removal of the nullness if a generic type allow " | null" is instantiated with a value type.

However, that means also hiding a flaw in the API (the API basically admits that missing information is possible, you just do not know how it is represented at runtime and likely cannot even detect it), whereas current behaviour gives a warning - not with the appropriate wording blaming a generic construct, but a warning at least.

I do not see how F# would support authoring such code, since F# even explicitly makes sure that " | null" in authored generic code automatically enforces a reference type constraint.

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 15, 2024

What could be done is removal of the nullness if a generic type allow " | null" is instantiated with a value type.

I think this has to be done anyway, since there should be a way for user to consume C# Serializer API anyway as it's currently not clear how to deal with int | null value (pattern match doesn't work on it since it requires reference type)

I do not see how F# would support authoring such code, since F# even explicitly makes sure that " | null" in authored generic code automatically enforces a reference type constraint.

Can't we relax enforcing one constraint? I mean keep 'T when 'T: not null, but don't enforce 'T when 'T: not struct. This could be known as "compatible with C#" option.

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

The added nullness should be subsumed, if you annotate the binding to be without null
let x : int = ... // json parsing code

With the constraints it gets tricky - as soon as it is not enforced, it could lead to invalid IL when the generic type/method is instantiated. (imagine the generic code having a let mutable x : 'T | null = null - it is not valid to assign null to a value type)
Which means that typechecking rules for mutable null assignment, pattern matching against null or calling into other generic functions would have to adjusted to understand this special mode coming from C# as well.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 16, 2024

it's currently not clear how to deal with int | null

int | null is not a thing, System.Nullable<int> is. How did these APIs work before?

Think of | null as case of literal type (like in TS), so when you say let x: string | null, the only things which can be bound to x are null (which is "reference") and string (also a reference), null as a value type makes no sense, it's invalid in the context of value types (hence ValueType | null is invalid).

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 16, 2024

int | null is not a thing, System.Nullable is. How did these APIs work before?

Before F# 9 it derived int, now int | null is derived (I attached screenshots from Visual Studio at the top), so users will need to know how to handle it.

The added nullness should be subsumed, if you annotate the binding to be without null
let x : int = ... // json parsing code

This works, thanks, probably should be documented

With the constraints it gets tricky - as soon as it is not enforced, it could lead to invalid IL when the generic type/method is instantiated. (imagine the generic code having a let mutable x : 'T | null = null - it is not valid to assign null to a value type)
Which means that typechecking rules for mutable null assignment, pattern matching against null or calling into other generic functions would have to adjusted to understand this special mode coming from C# as well.

I agree, that's why I suggested to keep not null constraint (which would mean either NRT or value type). I've checked C# and it doesn't allow assigning null to T? (errors with CS0403).

It is still unclear how to handle 'T | null returned from C# in generic function.

@abonie
Copy link
Member

abonie commented Sep 24, 2024

This was already decided during designing of this feature, so if that's to be change it has to be a fully-fledged language suggestion.

@abonie abonie closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2024
@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 25, 2024

@abonie sorry, what exactly was decided during design of this feature? How do I write the JsonSerializer.Deserialize wrapper without warnings? Is the "already decided" solution to disable warnings for such code with #nowarn 3261?

let tryDeserialize<'T>(value: string) =
    try
        JsonSerializer.Deserialize<'T>(value) |> Some
    with ex ->
        None

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 25, 2024

@abonie sorry, what exactly was decided during design of this feature? How do I write the JsonSerializer.Deserialize wrapper without warnings? Is the "already decided" solution to disable warnings for such code with #nowarn 3261?

let tryDeserialize<'T>(value: string) =

    try

        JsonSerializer.Deserialize<'T>(value) |> Some

    with ex ->

        None

Constraint inference was part of design decision, suggestion is needed to omit it.

@vzarytovskii
Copy link
Member

Representing both nullable reference type and value type in IL is tricky, and what C# does is it just pretends that null is not there from the flow analysis standpoint. In F# | null is possible only with reference types, hence the constraint is for correctness.

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 25, 2024

Constraint inference was part of design decision, suggestion is needed to omit it.

With this issue I don't suggest removing constraints or do any other particular suggestion, it is to point out that currently it is impossible to normally use popular .NET libraries (which is undesired behavior), and this ticket shouldn't be closed until the issue is resolved. I agree that the concrete suggestion "how to fix it" might go into suggestion repo, but it's not a reason to close the unresolved issue.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 25, 2024

It is currently not actionable for us here. It needs a specific suggestion of what the behaviour currently and what it should be. Suggestions repo is an ideal place for something like it, as most of the discusssions happen there.

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 25, 2024

Ok sounds like "currently not a bug", but before closing the issue users like me should get an answer - what to do in current situation. Is the currently suggested solution to disable nullness warnings?

@vzarytovskii
Copy link
Member

Ok sounds like "currently not a bug", but before closing the issue users like me should get an answer - what to do in current situation. Is the currently suggested solution to disable nullness warnings?

I would say so, or use optional types where possible

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Sep 25, 2024

I do think there is an issue here with the way F# consumes the metadata for generic 'T | null, although it only really seems to affect the displayed (pseudo-)type, not the actual (?) (pseudo-)type.

// Tooling shows x: int | null. This could never actually be null when 'T is a value type.
let x = Unchecked.defaultof<HttpContext>.Request.ReadFromJsonAsync<int>().Result
// Tooling shows y: int | null. No warning is given.
let y = x + 2
// Tooling shows z: int. No warning is given.
let z = 2 + x

Contrast that against

let x: string | null = null
// Warning FS3261. y is inferred to be string | null (even though it will never actually be null due to how + works in F#).
let y = x + ""
// No warning. z is inferred to be string.
let z = "" + x

At the very least, it's confusing.

I can see the logic behind the C# approach: T? indicates that the value could be null — if T is instantiated to a reference type or System.Nullable.

int a = M<int>();
int? b = M<int?>();
int? c = M<System.Nullable<int>>();
string d = M<string>();  // Warning.
string? e = M<string?>();

T? M<T>() => default;

I think it would make sense for F# to do the same thing here — or at least for the tooling not to show impossible (and inaccurate) types like int | null. That part definitely seems like a bug to me.

image

@T-Gro
Copy link
Member

T-Gro commented Sep 26, 2024

I will check that.
It should definitely be possible to instantiate such a C#-defined generict type/method with a value type, and work with it without getting warnings.
The display follows the original signature coming from IL, but at this point the type application can remove the constraint.

What remains impossible is authoring null-allowing generic code supporting both reference and value types (authoring does include wrappers). Which does not prevent using them with concrete non-generic instances.

@T-Gro
Copy link
Member

T-Gro commented Sep 26, 2024

(if a suggestion is created - a strong point should be raised against modelling the absence of information with default for F#-created generic code that includes value types. Doing that blocks that user from determining if the information was really missing, or has a regular value which happens to be equal to default.

If the solution would be to convert such null-allowing generic code into option/voption, the modelling problem disappears)

@Lanayx
Copy link
Contributor Author

Lanayx commented Sep 26, 2024

What remains impossible is authoring null-allowing generic code supporting both reference and value types (authoring does include wrappers). Which does not prevent using them with concrete non-generic instances.

I would add that authoring null-disallowing generic code is also impossible. Any generic code that consumes C# library with generic T? parameter is impossible without disabling warnings. In the following case I'd be ready to expose null-disallowing generic type (with current constraints) by adding additional null check in function body, but this is not possible, since all null-checking functions are constrained.

// possible solution which is currently impossible
let tryDeserialize<'T>(value: string) =
    try
        match JsonSerializer.Deserialize<'T>(value) with
        | Null -> None
        | NonNull x -> Some x
    with ex ->
        None

@T-Gro
Copy link
Member

T-Gro commented Sep 27, 2024

That is correct for the new nullness checking functions.
Some null checking can be done with boxing, either explicit via box or implicit via obj.ReferenceEquals(value,null) - but that is obviously not meaningful to do in the case of value types.

The proposal could include some relaxed conditions + optimizations for built-in functions specifically for consumption of null-allowing unconstrained generic code.
E.g. I could imagine a version similar to "Option.ofObj", which would be a no-op in case of value types (the emited IL could be a typetest on the generic T argument, which JIT would optimize away at time of generic instantiation).

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 Bug Needs-Triage
Projects
Archived in project
Development

No branches or pull requests

5 participants