Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix Attribute implementations of Equals and GetHashCode #6240

Merged
merged 1 commit into from
Oct 20, 2016
Merged

Fix Attribute implementations of Equals and GetHashCode #6240

merged 1 commit into from
Oct 20, 2016

Conversation

leppie
Copy link

@leppie leppie commented Jul 12, 2016

Properly deals with derived types.

Fix #6232

Please advise where to provide tests.

Note: This fix actually seems to match the same behavior as CoreRT.

Notes:
Type.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) will NOT return any non-visible fields from base classes. NonPublic only applies to declared fields in the type it is called on. FlattenHierarchy does not help here as it applies only to static members, but it's description is actually appropriate here. I suggest a documentation update.

This came as a surprise to me too, even after 15 years of .NET.

PS: This might be the oldest bug in existence. It is present in .NET 2.0 and the rotor sources. Unfortunately I cannot verify it again anything earlier than .NET 2.

@jkotas
Copy link
Member

jkotas commented Jul 18, 2016

This fix actually seems to match the same behavior as CoreRT.

@atsushikan Do we want to take this change, to reduce differences between CoreCLR and CoreRT?

@ghost
Copy link

ghost commented Jul 18, 2016

Yep, this looks a good thing to fix.

@leppie
Copy link
Author

leppie commented Jul 18, 2016

@jkotas @atsushikan Will any of the same reflection access restrictions apply as with CoreRT?

@danmoseley
Copy link
Member

@dotnet-bot test this please
@jkotas any CR feedback for @leppie ?

@jkotas
Copy link
Member

jkotas commented Oct 13, 2016

@leppie Could you please exclude the test from this PR, and add it as xunit test to corefx instead? We prefer to have tests for framework APIs like this one in corefx. Thank you!

The change looks good to me otherwise.

@leppie
Copy link
Author

leppie commented Oct 13, 2016

@jkotas Will do, but probably only during the weekend

@leppie
Copy link
Author

leppie commented Oct 20, 2016

@jkotas Rebased and removed the test. Will get on the test next.

@jkotas jkotas merged commit 9463e5d into dotnet:master Oct 20, 2016
@leppie
Copy link
Author

leppie commented Oct 23, 2016

@jkotas Now that the fix is in, can I build a test for it yet on corefx? If so, had a look, most natural existing place is System.Runtime, correct? (I do feel these should live in mscorlib tests)

@leppie leppie deleted the fix-6232 branch October 23, 2016 18:37
@jkotas
Copy link
Member

jkotas commented Oct 23, 2016

most natural existing place is System.Runtime, correct?

Correct. It may fit well into existing http://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System/AttributeTests.cs

ghost pushed a commit to dotnet/corert that referenced this pull request May 8, 2017
This ports dotnet/coreclr#6240 over to Project N.

The TL;DR version of the change is "private fields in base classes are no
longer disregarded by Equals()/GetHashCode()".

In fact, changing ExposeToDerivedClasses() to return true always is sufficient
to effect the desired change. The rest of the delta is just the result of
constant-folding and dead-coding from that point.

[tfs-changeset: 1657635]
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants