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 - cannot assign null to property #17733

Closed
sayurin opened this issue Sep 14, 2024 · 6 comments · Fixed by #17845
Closed

Nullness issue - cannot assign null to property #17733

sayurin opened this issue Sep 14, 2024 · 6 comments · Fixed by #17845
Assignees
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@sayurin
Copy link

sayurin commented Sep 14, 2024

Please provide a succinct description of the issue.

Repro steps

Enable nullable.

type T () =
    let mutable v : T | null = null
    member val P : T | null = null with get, set
    member this.M() =
        v <- null
        this.P <- null

let mutable v can assign null as expected.
But member val P cannot assign null, it causes FS3261.

Expected behavior

compile it successfully.

Actual behavior

It causes FS3261 warning.

Program.fs(6,19): warning FS3261: Nullness warning: The types 'T' and 'T | null' do not have compatible nullability.

Known workarounds

Use Uncheked.defaultof<T | null>.

Related information

Provide any related information (optional):

  • Windows 11
  • .NET 9 RC1
  • Visual Studio Professional Preview 17.12.0 Preview 2.0
@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

This might be related to usage of null in unconstrained generic context.
No matter what, this does need a better diagnostics.

Putting that aside, such code can only work if T is constrained to be a reference type.
Because assignment to null, if T gets instantiated by a value type, would lead to invalid IL code (you cannot assign null to a value type like int or byte).

@charlesroddie
Copy link
Contributor

@T-Gro here T is a specific reference type, not generic.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 15, 2024

@T-Gro this is what we codegen:

[NullableContext(1)]
public class T
{
    [Nullable(2)]
    internal T v;

    [Nullable(2)]
    internal T P@;

    [Nullable(2)]
    public T P
    {
        [CompilerGenerated]
        [DebuggerNonUserCode]
        [return: Nullable(2)]
        get
        {
            return P@;
        }
        [CompilerGenerated]
        [DebuggerNonUserCode]
        [param: Nullable(2)]
        set
        {
            P@ = value;
        }
    }

    public T()
    {
        v = null;
        P@ = null;
    }

    public void M()
    {
        v = null;
        P = null;
    }
}

It seems that we generate context correctly, but do not infer nullness? We infer it for the field, but not for property (is it due to the fact we transorm it to set_P(value) call?

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

Looking at the codegen, I think we even infer it correctly - the attributes are correct, which means C# consumers will see it just fine even.

I suspect this might have to do with how and when we process "member val".

@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

As a workaround, passing in a "fancified null" makes it work:

            let mutable v : T | null = null
            static let nullz : T | null = null
            member val P : (T | null) = v with get, set
            member this.M() =
                v <- null
                this.P <- nullz

@T-Gro T-Gro self-assigned this Sep 15, 2024
@sayurin
Copy link
Author

sayurin commented Sep 15, 2024

In addition, member val S : string | null can assign null.

type T () =
    let mutable v : T | null = null
    member val P : T | null = null with get, set
    member val S : string | null = null with get, set
    member this.M() =
        v <- null       // OK
        this.P <- null  // FS3261
        this.S <- null  // OK

@abonie abonie added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Area-Nullness Issues related to handling of Nullable Reference Types and removed Needs-Triage labels Sep 24, 2024
@T-Gro T-Gro modified the milestones: Backlog, October-2024 Oct 2, 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 Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants