Inline array Equals unexpected behavior

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

@VSadov

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.

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.

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.

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.)

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.

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

Yes, I think we should.

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?

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.

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.

Oops. Yes, I had a bug in my test program.

If ValueType.Equals compares fields, and [InlineArray(N)] is a conceptual N fields in sequential order, then it seems wrong to only compare the first element (i.e., the sole field). It also seems wrong to throw when a proper, correct method exists: Span<T>.SequenceEquals. The default JIT-based implementation could just forward to that. Forcing users to write it themselves is the same logic as saying ValueType.Equals shouldn't exist: "just have the user override Object.Equals themselves!"

@colejohnson66

@colejohnson66 It is a fair opinion, but not an obviously "correct" one.

Forcing users to write it themselves is the same logic as saying ValueType.Equals shouldn't exist: "just have the user override Object.Equals themselves!"

Your last statement, above, is at least part of the reasoning. Providing a default for an equality value check has been contentious for a long time and one that we would likely not make again.

Aside from the opinions regarding default equality, there are other considerations too. Specifically, common uses of the InlineArrayAttribute feature, inherent complexity in what many consider the correct default implementation, and finally performance of the default implementation.

A common use case for InlineArrayAttribute is as a scratch or temporary buffer that may be partially set. This means that simply doing a naive bitwise check can be incorrect. It is reasonable to say the default of comparing the entire buffer is correct, but this leads into the second consideration, complexity. If the InlineArrayAttribute is a set of object instances, we can't perform a simple bitwise check, but instead must iterate over each of them, check for null, and then call Equals(). All this can be done with Span<object> so this isn't a technical issue, but rather one of complexity. Finally, let's consider the performance angle. The default implementation uses reflection and so is, by default, slow and not AOT friendly. None of this push back is technical, but rather a debate about what is correct, intuitive, and performant, and if it is worth investing in a default implementation. In the opinion of people on the runtime team, it isn't.

User defined ValueTypes that expect to have their Equals() or GetHashCode() called, should be overridden by the user.

I would like to remember, that according to docs GetHashCode should not throw exceptions.

The GetHashCode method should not throw exceptions.

@stephentoub

I would like to remember, that according to docs GetHashCode should not throw exceptions.

The GetHashCode method should not throw exceptions.

Yes, that is the guidance here. The issue we face is what is the correct result of this and the intent of that statement. My reading, which may be wrong, is that is intented to provide guidance to users overridding the default, not necessarily a restriction on a default implementation provided by the runtime.

We can look to @stephentoub or @jkotas for clarification on this point. The concern I have would be the following expectation on the function:

A hash function is used to quickly generate a number (hash code) that corresponds to the value of an object.

The intended value of the object is unlikely to be simply the first field. This would imply it is clearer to fail (throw an exception) and inform the user the default "result" is suspect rather than let them consider the result useful.

This is not the first GetHashCode/Equals that throws NotSupportedException. Span/ReadOnlySpan is an example of prior art: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs#L181-L201.

The intended value of the object is unlikely to be simply the first field.

Right, the poor hashcode distribution of the default implementation is one of the common problems that people run into.

But you can't use spans as dictionary key or box it unlike inline array and it does not violate object contract.

But you can't use spans as dictionary key or box it unlike inline array and it does not violate object contract.

I think mentioning Dictionary<,> is only reinforcing the opinion that users should be overriding these methods and properly defining them so they are what is expected.

Looks like we should be extremely careful when using InlineArray and read the manual before using :)

Looks like we should be extremely careful when using InlineArray and read the manual before using :)

Yep. Here is the updated doc PR if you'd like to see how we are documenting this - dotnet/dotnet-api-docs#10036

Navigation Menu

Repository navigation

Inline array Equals unexpected behavior #96943

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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;
	}

Inline array Equals unexpected behavior #96943

Description

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

Activity

tannergooding commented on Jan 13, 2024

VSadov commented on Jan 23, 2024

tannergooding commented on Jan 23, 2024

jkotas commented on Jan 23, 2024

VSadov commented on Jan 23, 2024

AaronRobinsonMSFT commented on Jun 14, 2024

agocke commented on Jun 17, 2024

AaronRobinsonMSFT commented on Jun 17, 2024

jkotas commented on Jun 17, 2024

agocke commented on Jun 17, 2024

colejohnson66 commented on Jun 18, 2024

AaronRobinsonMSFT commented on Jun 18, 2024

Ilchert commented on Jun 19, 2024

AaronRobinsonMSFT commented on Jun 19, 2024

jkotas commented on Jun 19, 2024

Ilchert commented on Jun 19, 2024

AaronRobinsonMSFT commented on Jun 20, 2024

Ilchert commented on Jun 20, 2024

AaronRobinsonMSFT commented on Jun 20, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions