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 - Option<string> is marked as NotNull using NullabilityInfoContext #17517

Closed
1 of 7 tasks
pchalamet opened this issue Aug 10, 2024 · 8 comments · Fixed by #17528
Closed
1 of 7 tasks

Nullness issue - Option<string> is marked as NotNull using NullabilityInfoContext #17517

pchalamet opened this issue Aug 10, 2024 · 8 comments · Fixed by #17528
Assignees
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Bug Needs-Triage
Milestone

Comments

@pchalamet
Copy link

pchalamet commented Aug 10, 2024

Issue description

Looks like Option<string> in marked as NotNull when using NullabilityInfoContext. I expect it to be reported as Nullable since Option<string> for null has a null representation (UseNullAsTrueValue).

With provided snippet, I expect StringOption to be reported as Nullable, not NotNull.

StringOrNull = <null>
StringOption = None <-- this
ObjNull = <null>
ObjOrNull = <null>
StringOrNull => Nullable / Nullable
StringOption => NotNull / NotNull <-- this
ObjNull => Nullable / Nullable
ObjOrNull => Nullable / Nullable

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

macOS

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

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

.NET Runtime/SDK version

9.0.100-preview.6.24328.19

Reproducible code snippet and actual behavior

open System

open System
open System.Reflection

type MyClass(StringOrNull: string | null, StringOption: string option, ObjNull: objnull, ObjOrNull: obj | null) =
    member val StringOrNull = StringOrNull
    member val StringOption = StringOption
    member val ObjNull = ObjNull
    member val ObjOrNull = ObjOrNull

let classType = typeof<MyClass>
let ctor = classType.GetConstructors() |> Seq.exactlyOne
let paramInfos = ctor.GetParameters()

let inst = ctor.Invoke([| null; null; null; null |]) :?> MyClass
printfn "StringOrNull = %A" inst.StringOrNull
printfn "StringOption = %A" inst.StringOption
printfn "ObjNull = %A" inst.ObjNull
printfn "ObjOrNull = %A" inst.ObjOrNull

let nrtContext = NullabilityInfoContext()
for paramInfo in paramInfos do
    let nrtInfo = nrtContext.Create(paramInfo)
    let readState = nrtInfo.ReadState
    let writeState = nrtInfo.WriteState
    printfn $"{paramInfo.Name} => {readState} / {writeState}"

Possible workarounds

No response

@pchalamet pchalamet added Area-Nullness Issues related to handling of Nullable Reference Types Bug Needs-Triage labels Aug 10, 2024
@github-actions github-actions bot added this to the Backlog milestone Aug 10, 2024
@T-Gro
Copy link
Member

T-Gro commented Aug 11, 2024

Thanks for reporting this.
This affects in general all types that UseNullAsTrueValue.

It is safe to consider them not-null in F# understanding, but for C# consumers, they must be reported as nullable via IL metadata.
Thanks for spotting this.

@edgarfgp
Copy link
Contributor

@T-Gro T-Gro modified the milestones: Backlog, August-2024 Aug 12, 2024
@T-Gro
Copy link
Member

T-Gro commented Aug 12, 2024

At least for C# consumers (or NullabilityInfoContext for serializers, schema generators etc.) it must indicate that runtime value of option is potentially null.

The usage within F# (or rather, what can be improved without breaking much of existing code) is harder, I agree.

@edgarfgp
Copy link
Contributor

@T-Gro Would it be possible to consider UseNullAsTrueValue attribute deprecation ?

@pchalamet
Copy link
Author

pchalamet commented Aug 12, 2024

At least for C# consumers (or NullabilityInfoContext for serializers, schema generators etc.) it must indicate that runtime value of option is potentially null.

Yes I've seen the problem with my own deserializer. I don't want to deal specifically building a given type (here option<>.None - if null is ok for a type instance, there is no problem. And null here is a perfectly valid value).

The usage within F# (or rather, what can be improved without breaking much of existing code) is harder, I agree.

There are so many attributes on f# types I would be surprised something can't be sorted out.

Between UseNullAsTrueValue and AllowNullLiteral there is a semantic distinction. Outside F# (and I include deserializers), probably we do not care. It's nullable and it's ok - it's expected to work correctly as before for UseNullAsTrueValue.

Within F#, it's not matchable with null (despite being null, for eg Option.None for RT). And this shall be ok too - and for me this shall rely on AllowNullLiteral only).

Maybe I do not understand the whole story tho 🙂

@T-Gro
Copy link
Member

T-Gro commented Aug 12, 2024

Unlikely.
Code with C#/F#/.. interop relying on IL layout of option for sure exists, and option is very central to F#.

@T-Gro
Copy link
Member

T-Gro commented Aug 12, 2024

At least for C# consumers (or NullabilityInfoContext for serializers, schema generators etc.) it must indicate that runtime value of option is potentially null.

Yes I've seen the problem with my own deserializer. I don't want to deal specifically building a given type (here option<>.None - if null is ok for a type instance, there is no problem. And null here is a perfectly valid value).

The usage within F# (or rather, what can be improved without breaking much of existing code) is harder, I agree.

There are so many attributes on f# types I would be surprised something can't be sorted out.

Between UseNullAsTrueValue and AllowNullLiteral there is a semantic distinction. Outside F# (and I include deserializers), probably we do not care. It's nullable and it's ok - it's expected to work correctly as before for UseNullAsTrueValue.

Within F#, it's not matchable with null (despite being null, for eg Option.None for RT). And this shall be ok too - and for me this shall rely on AllowNullLiteral only).

Maybe I do not understand the whole story tho 🙂

I think you are right here.
The schism exists - F# compiler tries very hard to pretend it is not null and will not be null for F# users (matching, assignment, generic constraints,..).
But for C# users, it is clearly a nullable type.

@pchalamet
Copy link
Author

And NullableInfoContext is about the value site (param, field, event, property) - not about the type itself as a reference can always be nullable (CLR is not aware of non-null). I guess this can be annotated as nullable without compromising F# type system and obviously without changing type layout.

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

Successfully merging a pull request may close this issue.

3 participants