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

ValueTuple.IStructuralComparable/Equatable throws NRE instead of ArgumentNullException #19275

Closed
Tracked by #93172 ...
hughbe opened this issue Nov 7, 2016 · 5 comments
Closed
Tracked by #93172 ...

Comments

@hughbe
Copy link
Contributor

hughbe commented Nov 7, 2016

The following tests demonstrates this.

[Fact]
public void IStructuralEquatable_Equals_NullEqualityComparer_ThrowsNullReferenceException()
{
    IStructuralEquatable structuralEquatable = ValueTuple.Create(1);
    Assert.Throws<NullReferenceException>(() => structuralEquatable.Equals(Tuple.Create(1), null));
}

[Fact]
public void IStructuralEquatable_GetHashCode_NullEqualityComparer_ThrowsNullReferenceException()
{
    IStructuralEquatable structuralEquatable = ValueTuple.Create(1);
    Assert.Throws<NullReferenceException>(() => structuralEquatable.GetHashCode(null));
}

[Fact]
public void IStructuralComparable_CompareTo_NullEqualityComparer_ThrowsNullReferenceException()
{
    IStructuralComparable structuralComparable = ValueTuple.Create(1);
    Assert.Throws<NullReferenceException>(() => structuralComparable.CompareTo(Tuple.Create(1), null));
}

Spun off from #19265

/cc @karelz (who can help triage this :) )

@hughbe hughbe changed the title ValueTuple.IStructuralComparable/Equatable throws NRE ValueTuple.IStructuralComparable/Equatable throws NRE instead of ArgumentNullException Nov 7, 2016
@karelz
Copy link
Member

karelz commented Nov 7, 2016

@terrajobst @weshaggard @stephentoub do we have room to tweak ValueTuple to follow the right patterns? I think we don't have the huge compatibility burden yet.

@weshaggard
Copy link
Member

@jcouv would be able to talk to this

@jcouv
Copy link
Member

jcouv commented Nov 7, 2016

@karelz Let me check with F# folks. I tried to keep ValueTuple as close as possible to Tuple for their purpose.

@jcouv
Copy link
Member

jcouv commented Nov 7, 2016

Talked with @KevinRansom and he confirmed that matching the logic to System.Tuple should take precedence over any conventions (although reasonable), so that we facilitate users switching between Tuple and ValueTuple without having to remember to update their exception-checking code.

So we recommend resolving this issue as "by design".

@hughbe
Copy link
Contributor Author

hughbe commented Nov 7, 2016

Awesome - I'll close this when I send in some tests documenting the behaviour

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants