Inline array Equals unexpected behavior

[Ilchert]

Description

Default Equals implementation compares only the first element of inline array.

Reproduction Steps

var a1 = new A();
a1[0]=1;
a1[1]=2;

var a2 = new A();
a2[0] = 1;
a2[1] = 1;


Console.WriteLine(a1.Equals(a2));

[InlineArray(2)]
struct A
{
   public int? P;
}

This code prints true sharplab

Expected behavior

Should compare all values in inline array and print false in this case.

Actual behavior

Compares only the first element.

Regression?

No

Known Workarounds

Override Equals

Configuration

.net 8, Win 10

Other information

No response

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Default Equals implementation compares only the first element of inline array.

Reproduction Steps

var a1 = new A();
a1[0]=1;
a1[1]=2;

var a2 = new A();
a2[0] = 1;
a2[1] = 1;


Console.WriteLine(a1.Equals(a2));

[InlineArray(2)]
struct A
{
   public int? P;
}

This code prints true sharplab

Expected behavior

Should compare all values in inline array and print false in this case.

Actual behavior

Compares only the first element.

Regression?

No

Known Workarounds

Override Equals

Configuration

.net 8, Win 10

Other information

No response

Author:	Ilchert
Assignees:	-
Labels:	area-System.Runtime, untriaged
Milestone:	-

[tannergooding]

CC. @VSadov, @jkotas

This is likely also a consideration for GetHashCode. The workaround here is to use SequenceEqual over the span that C# will let you implicitly create.

[VSadov]

Where does Default Equals implementation come from? The type system only provides the size and layout.

I think ideally there should either be no default implementation or it should consider all elements.

[tannergooding]

System.ValueType has a default behavior that is done by the runtime where effectively calls Equals on each field. I expect it wasn't updated to consider InlineArray at all, potentially by design.

[jkotas]

System.ValueType.Equals for CoreCLR is here:

runtime/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs

Lines 25 to 67 in d44bc24

	public override unsafe bool Equals([NotNullWhen(true)] object? obj)
	{
	if (null == obj)
	{
	return false;
	}
	if (GetType() != obj.GetType())
	{
	return false;
	}
	// if there are no GC references in this object we can avoid reflection
	// and do a fast memcmp
	if (CanCompareBits(this))
	{
	return SpanHelpers.SequenceEqual(
	ref RuntimeHelpers.GetRawData(this),
	ref RuntimeHelpers.GetRawData(obj),
	RuntimeHelpers.GetMethodTable(this)->GetNumInstanceFieldBytes());
	}
	FieldInfo[] thisFields = GetType().GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
	for (int i = 0; i < thisFields.Length; i++)
	{
	object? thisResult = thisFields[i].GetValue(this);
	object? thatResult = thisFields[i].GetValue(obj);
	if (thisResult == null)
	{
	if (thatResult != null)
	return false;
	}
	else
	if (!thisResult.Equals(thatResult))
	{
	return false;
	}
	}
	return true;
	}

.

Our docs recommend overriding it for correctness and performance reason: https://learn.microsoft.com/en-us/dotnet/api/system.valuetype.equals

Should we throw from the default implementation for InlineArrays? (It would be a breaking change.)

[VSadov]

I think we should make Equals work. The CanCompareBits part could be efficient and field-wise compare fallback is hopelessly slow anyways.

By looking at the implementation it seems like the effort is to provide a behavior that recursively compares field's bits , which is a reasonably correct behavior, no matter how slow it is - likely the assumption is that incomplete/throwing implementation would be worse.

Also should check GetHashcode
The comment says "We look for the first non-static field and get its hashcode." I think that would be acceptable. Just need to be sure it indeed does that for inlined arrays.

[AaronRobinsonMSFT]

Should we throw from the default implementation for InlineArrays? (It would be a breaking change.)

Yes, I think we should.

[agocke]

Why would the InlineArray behavior be different than structs with manual size? That is, if you write

[StructLayout(LayoutKind.Sequential, Size = 40)]
public struct Buffer
{
   public int Field;
}

It looks like the auto-generated equality for that struct does a bitwise-compare. Shouldn't we do the same for InlineArray?

[AaronRobinsonMSFT]

It looks like the auto-generated equality for that struct does a bitwise-compare. Shouldn't we do the same for InlineArray?

I don't see an auto-generated equality. I see this falling back to the ValueType.Equals() implementation that will do, as you mention, a bit-wise check. However, if one changes the field to object, fields will be examined.

How are you viewing the relationship between the Size and InlineArray? One alters the size of the ValueType - not fields, while the other impliciltly introduces new, well-defined, fields.

[jkotas]

It looks like the auto-generated equality for that struct does a bitwise-compare.

It does not. Run the following:

using System.Runtime.InteropServices;

unsafe
{
    Buffer b1 = new Buffer();
    *((int*)&b1 + 1) = 2;
    Buffer b2 = new Buffer();
    *((int*)&b2 + 1) = 3;

    Console.WriteLine(b1.Equals(b2));
}

[StructLayout(LayoutKind.Sequential, Size = 40)]
public struct Buffer
{
   public int Field;
}

It prints true. It compares the Field only. It does not compare the remaining 36 bytes. There is no obvious right way to compare the remaining 36 bytes. We do not know what's actually stored in there bytes.