DispatchProxy should emit interface properties and events metadata #9220

Closed
yfakariya opened this Issue Jun 7, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@yfakariya
Contributor

yfakariya commented Jun 7, 2016

Currently, DispatchProxyGenerator only emits accessor methods of the interface type, does not emit any properties and/or events. Thus, getting properties and/or events via reflection fails as following:

interface IFoo { string Bar { get; set; } };

IFoo proxy = DispatchProxy.Create<IFoo, SomeProxy>();
PropertyInfo[] props = proxy.GetType().GetProperties();

In this point, it is natural to think that props has 1 property (string Bar), but it is actually an empty array even though var bar = proxy.Bar and proxy.Bar = "something" can be compiled.

It looks unnatural. In addition, it limits effectiveness of DispatchProxy. For example, when we generate a wrapper which implements INotifyPropertyChanged around underlying model object using DispatchProxy, we cannot do that because the data binding engine fails to retrieve PropertyInfo and then fails to bind object. Other AOP scenario may also be restricted by this issue.

So, DispatchProxyGenerator should also emit properties and events of specified interface type as well as their accessor methods.

@yfakariya

This comment has been minimized.

Show comment
Hide comment
@yfakariya

yfakariya Jun 18, 2016

Contributor

@AtsushiKan Do you have any questions? I have a patch with unit tests for this, but I'm waiting for further discussion about it :)

Contributor

yfakariya commented Jun 18, 2016

@AtsushiKan Do you have any questions? I have a patch with unit tests for this, but I'm waiting for further discussion about it :)

yfakariya added a commit to yfakariya/corefx that referenced this issue Jun 18, 2016

Make DispatchProxy declares PropertyInfos and EventInfos of the inter…
…face.

This commit is implementation of issue #9220.
This adds code to declare properties and events to generating proxy type as well.
Because declaring properties and events requires accessor methods,
this commit changes AddMethodImpl to return created MethodBuilder.
@AtsushiKan

This comment has been minimized.

Show comment
Hide comment
@AtsushiKan

AtsushiKan Jun 20, 2016

Member

I’ll probably have lots of questions once I get to it (starting with questions like “What’s DispatchProxy…”) but given that it’s Milestoned for “Future” and not “1.1.0”, I wasn’t anticipating getting to it anytime soon.

If you feel it needs to be in 1.1.0, please change the Milestone.

Member

AtsushiKan commented Jun 20, 2016

I’ll probably have lots of questions once I get to it (starting with questions like “What’s DispatchProxy…”) but given that it’s Milestoned for “Future” and not “1.1.0”, I wasn’t anticipating getting to it anytime soon.

If you feel it needs to be in 1.1.0, please change the Milestone.

@AtsushiKan

This comment has been minimized.

Show comment
Hide comment
@AtsushiKan

AtsushiKan Jun 20, 2016

Member

It sounds like you have a fix ready to PR – if so, don’t wait on me. Issues that sounds “Reflection-y” just tend to get auto-assigned to me.

Member

AtsushiKan commented Jun 20, 2016

It sounds like you have a fix ready to PR – if so, don’t wait on me. Issues that sounds “Reflection-y” just tend to get auto-assigned to me.

yfakariya added a commit to yfakariya/corefx that referenced this issue Jun 20, 2016

Make DispatchProxy declares PropertyInfos and EventInfos of the inter…
…face.

This commit is implementation of issue #9220.
This adds code to declare properties and events to generating proxy type as well.
Because declaring properties and events requires accessor methods,
this commit changes AddMethodImpl to return created MethodBuilder.
@yfakariya

This comment has been minimized.

Show comment
Hide comment
@yfakariya

yfakariya Jun 20, 2016

Contributor

Thank you for reply! Yes, I had a fix as you thought, so I've just sent the PR.

Contributor

yfakariya commented Jun 20, 2016

Thank you for reply! Yes, I had a fix as you thought, so I've just sent the PR.

@karelz karelz added enhancement and removed suggestion labels Sep 18, 2016

@yfakariya

This comment has been minimized.

Show comment
Hide comment
@yfakariya

yfakariya Oct 3, 2016

Contributor

Should I close this issue? I'm realizing that this issue was solved but a work to split the test project is remaining.

Contributor

yfakariya commented Oct 3, 2016

Should I close this issue? I'm realizing that this issue was solved but a work to split the test project is remaining.

@AtsushiKan

This comment has been minimized.

Show comment
Hide comment
@AtsushiKan

AtsushiKan Oct 3, 2016

Member

Go ahead and close. The test work can be tracked separately if and when it becomes important.

Member

AtsushiKan commented Oct 3, 2016

Go ahead and close. The test work can be tracked separately if and when it becomes important.

@yfakariya

This comment has been minimized.

Show comment
Hide comment
@yfakariya

yfakariya Oct 3, 2016

Contributor

I got it.

Contributor

yfakariya commented Oct 3, 2016

I got it.

@yfakariya yfakariya closed this Oct 3, 2016

@karelz karelz modified the milestones: Future, 1.2.0 Dec 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment