Switch to explicit implementation for redeclared events #618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test being added here fails PEVerify with "Event has a duplicate".
The failing test from the previous commit causes the following metadata
to be emitted:
.property instance class [mscorlib]System.Action`1<bool> Property()
{
.get instance class [mscorlib]System.Action`1<bool>
Castle.Proxies.IDerivedProxy::get_Property()
}
.property instance class [mscorlib]System.Action Property()
{
.get instance class [mscorlib]System.Action
Castle.Proxies.IDerivedProxy::get_Property()
}
Obviously we need to switch to explicit implementation for one of these
events in order to avoid a name collision. We do that by changing the
"collision detection" such that it does not take into account events'
return types; only their name matters.
Since events and properties have very similar metadata, the same issue we just solved for events might be present for properties; let's test this suspicion.
This might be a PEVerify bug. Given that the C# compiler would force us to implement either the base or derived property explicitly (in a class implementing `IDerived`), let's do the same in DynamicProxy. Use the same approach as for events.
stakx
changed the title
|
@jonorossi, this is fairly trivial and shouldn't hold any surprises, so feel free to review or not review as time permits. I may go ahead and merge this in about 2 weeks' time. |
Thanks! I wanted to make the PR easily reviewable in isolation, since it's been a while and it can be difficult to remember (or having to look up) all relevant details of past discussions. It's been a good Git exercise; I might try to do this more often in the future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #590. I've added most of the information gathered in that issue to the commit messages.