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

Null reference on equality with nested record #18286

Open
Szer opened this issue Jan 31, 2025 · 3 comments
Open

Null reference on equality with nested record #18286

Szer opened this issue Jan 31, 2025 · 3 comments
Assignees
Labels
Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@Szer
Copy link
Contributor

Szer commented Jan 31, 2025

Two snippets below fails with NRE

  1. Equality
type Bar = { b: obj }
type Foo = { f: Bar | null }
let a = { f = null }
let b = { f = null }
a = b

Error

System.NullReferenceException: Object reference not set to an instance of an object.
   at FSI_0045.Foo.Equals(Foo obj, IEqualityComparer comp)
   at <StartupCode$FSI_0048>.$FSI_0048.main@()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Stopped due to error
  1. Hash code
type Bar = { b: obj }
type Foo = { f: Bar | null }
let a = { f = null }
a.GetHashCode()

Error

System.NullReferenceException: Object reference not set to an instance of an object.
   at FSI_0017.Foo.GetHashCode(IEqualityComparer comp)
   at FSI_0017.Foo.GetHashCode()
   at <StartupCode$FSI_0019>.$FSI_0019.main@()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Stopped due to error

Known workarounds

change Bar to be NON record

type Bar = class end
type Foo = { f: Bar | null }
let a = { f = null }
let b = { f = null }
a = b // works now

Related information

  • Windows 10
  • Microsoft (R) F# Interactive version 12.9.101.0 for F# 9.0
dotnet --list-sdks
5.0.408 [C:\Program Files\dotnet\sdk]
6.0.203 [C:\Program Files\dotnet\sdk]
6.0.321 [C:\Program Files\dotnet\sdk]
6.0.428 [C:\Program Files\dotnet\sdk]
7.0.120 [C:\Program Files\dotnet\sdk]
7.0.203 [C:\Program Files\dotnet\sdk]
7.0.317 [C:\Program Files\dotnet\sdk]
7.0.410 [C:\Program Files\dotnet\sdk]
8.0.112 [C:\Program Files\dotnet\sdk]
8.0.308 [C:\Program Files\dotnet\sdk]
8.0.405 [C:\Program Files\dotnet\sdk]
9.0.102 [C:\Program Files\dotnet\sdk]
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 31, 2025

@T-Gro equality codegen is not accounting for the nullability now it seems, before we would've had when trying to assign null directly, you would've had

    error FS0043: The type 'Bar' does not have 'null' as a proper value
    error FS0043: The type 'Bar' does not have 'null' as a proper value

Although you would've hit NRE when using unchecked default value, this makes it easier to achieve.

Since now we allow | null for records, nullability will be more explicit and used. Should we generate a defensive nullcheck? I think any performance hit will be negligible.

@Szer
Copy link
Contributor Author

Szer commented Jan 31, 2025

This code also fails with NRE

type Bar = { b: obj }
type Foo = { f: Bar | null }
let a = { f = null }
a.GetHashCode()
System.NullReferenceException: Object reference not set to an instance of an object.
   at FSI_0017.Foo.GetHashCode(IEqualityComparer comp)
   at FSI_0017.Foo.GetHashCode()
   at <StartupCode$FSI_0019>.$FSI_0019.main@()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Stopped due to error

Added that to the first post

@T-Gro T-Gro self-assigned this Jan 31, 2025
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 31, 2025

If someone wants to fix it, it's a really easy one, for equality, there needs to be a tester added for each field, before .Equals(that) call is generated, around here (around when we mapping over the fields), and made a part of expression:

let expr = mkEqualsTestConjuncts g m (List.map mkTest fields)
let expr =
if tycon.IsStructOrEnumTycon then
expr
else
mkBindThatNullEquals g m thise thataddre expr
let thatv, expr = mkThatVarBind g m ty thataddrv expr
thisv, thatv, expr

Hashing will be somewhere around there as well.

@0101 0101 added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend and removed Needs-Triage labels Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Status: New
Development

No branches or pull requests

4 participants