Fix array comparison in params

[YegorStepanov]

fixes #791

Look at the third commit to see what the PR changes.

[adamsitnik]

It's almost ready. PTAL at my comments and again thank you @YegorStepanov !

[adamsitnik]

what do we gain by having duplicate params (new byte[] { 0 })? one day we might move https://github.com/dotnet/performance/blob/main/src/harness/BenchmarkDotNet.Extensions/UniqueArgumentsValidator.cs into BDN and this would be considered as a bug

Suggested change

	[Params(new byte[] { 0 }, new byte[] { 1 }, new byte[] { 0 })]
	[Params(new byte[] { 0 }, new byte[] { 1 })]

[YegorStepanov]

You want to remove the purpose of these tests and PR :)

These tests check that arrays can be grouped if they have the same primitive values (but they are actually different arrays).

We can disallow duplicate arguments for [Params] because Attribute supports only the built-in types. (IMO it should do Roslyn Analyzer only, not a validator)

But how can we disable it for [ParamSource]?

If the values are not primitive, it fallbacks to .ToString, which means that most of user classes are equal:

x.ToString().CompareTo(y.String()) == 0 //always if .ToString is not overloaded

UniqueArgumentsValidator

If I remember correctly, you can't distinguish between [Params] and [ParamSource] values, so UniqueArgumentsValidator falls for:

[ArgumentSource("GetValues")]
[Benchmark]
public void Benchmark(MyType instance) { }

public IEnumerable<MyType> GetValues()
{
    // fails because both ToString() are equal:
    yield return new MyType { Value = 10 };
    yield return new MyType { Value = 100 };
}

public class MyType { public int Value; }

[YegorStepanov]

If the type implements IComparable then we shouldn't jump to ToString() (because it can be overloaded).
So the code will look like:

if(IComparableComparer.TryCompareTo(x, y, var userCompareTo))
    return userCompareTo;

But it works for primitives/arrays.

Are you satisfied with the current implementation, or should I remake it?

[adamsitnik]

same as above

Suggested change

yield return new int[][] { new[] { 0 }, new[] { 0 }, };

[YegorStepanov]

answered above

[adamsitnik]

I was surprised to see that it works, then I've checked and found that we are relying on the .ToString comparison:

BenchmarkDotNet/src/BenchmarkDotNet/Parameters/ParameterComparer.cs

Line 49 in 58d4bae

: string.CompareOrdinal(x?.ToString(), y?.ToString());

It would be more future proof to perform an explicit check:

int compareTo = x[i]?.Value is Array xArray && y[i]?.Value is Array yArray
   ? CompareArrays(xArra, yArray)
   : PrimitiveComparer.CompareTo(x[i]?.Value, y[i]?.Value);

[YegorStepanov]

The code works like this:

1) if type is primitive:
1.1) if (x > y or x < y) : return 1 or -1
2) if type is array:
1.1) if (x > y or x < y) : return 1 or -1
3) compare by string and return

So, if the x=0 and y=0, it exits on the last line:

return string.CompareOrdinal(0.ToString(), 0.ToString()); 

Future

Also, we need to add support for user IComparable types here, so the code will look like:

int primitiveCompareTo = PrimitiveComparer...;
if (primitiveCompareTo != 0)
   return primitiveCompareTo;

int arrayCompareTo = ArrayComparer...;
if (arrayCompareTo != 0)
   return arrayCompareTo;

int userCompareTo = IComparableComparer...;
if (userCompareTo != 0)
   return userCompareTo;

return string.CompareOrdinal(x?.ToString(), y?.ToString()); 

Stuns but looks elegant

[kapilepatel]

Hi, is this still active/needed ?
Can I work on it?

[timcassell]

@kapilepatel Yes, feel free to work on this.

[timcassell]

Done by #2887