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

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jan 26, 2018

Fixes #15645.
Fixes #15644.

@MichalStrehovsky
Copy link
Member Author

@atsushikan Could you have a look please?

Cc @sergiy-k

RuntimeMethodHandleInternal classRtMethodHandle = GetTypeHandleInternal().GetInterfaceMethodImplementation(ifaceRtTypeHandle, ifaceRtMethodHandle);

if (slot == -1) continue;
if (classRtMethodHandle.IsNullHandle()) continue;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is preexisting but the "continue" should be on its own line.

@ghost
Copy link

ghost commented Jan 26, 2018

Other than that, LGTM

(RuntimeMethodHandle.GetAttributes(methodHandle) & MethodAttributes.RTSpecialName) == 0 ||
RuntimeMethodHandle.GetName(methodHandle).Equals(".cctor"));

if ((methodAttributes & MethodAttributes.RTSpecialName) != 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this check up to align with how this is done for the else (not interface) case below.

@MichalStrehovsky
Copy link
Member Author

@atsushikan I had to also add what I had to do to fix reflection invoke because I didn't realize this hits that code path with the assert (I worked on several things in parallel in my local branch and Invoke didn't have tests ready yet...)

@MichalStrehovsky MichalStrehovsky changed the title Fix Type.GetInterfaceMap to work with default interface methods Fix reflection to work with default interface methods Jan 29, 2018
RuntimeMethodHandle.GetName(methodHandle).Equals(".ctor") ||
RuntimeMethodHandle.GetName(methodHandle).Equals(".cctor") ||
RuntimeMethodHandle.GetName(methodHandle).StartsWith("IL_STUB", StringComparison.Ordinal));
(RuntimeMethodHandle.GetAttributes(methodHandle) & MethodAttributes.RTSpecialName) == 0 ||
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could now use the cached value for methodAttributes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I would also have to update the "not an interface" case below to do the same to stay consistent. I'm not convinced it's worth the noise in git blame.

@ghost
Copy link

ghost commented Jan 29, 2018

LGTM. Is there anything that needs to happen on the Invoke path on Project N?

@MichalStrehovsky
Copy link
Member Author

LGTM. Is there anything that needs to happen on the Invoke path on Project N?

I'll open that particular can of worms later.

Btw: do you know of any other parts of the CLR reflection stack that would need updating?

@MichalStrehovsky MichalStrehovsky merged commit 0be8947 into dotnet:master Jan 29, 2018
@MichalStrehovsky MichalStrehovsky deleted the getInterfaceMap branch January 29, 2018 16:37
@ghost
Copy link

ghost commented Jan 29, 2018

Reflection doesn't even expose method .overrides today so these things are going to be largely invisible to the apis that walk up base class chains to generate lists of methods. (Assuming interface defaults work the same way as other .overrides- i.e. you have to invoke them through the interface-typed this, not a class-based this)

So I imagine any update, if any, will come in the form of requests to add or change the behavior of apis - not so much bug fixes. Or at least documentation that anticipates user confusion (why does GetMethod() on a class find inherited members but not GetMethod on an interface.)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant