EventInfo.EventHandlerType.GetMethod("Invoke") could be made trim safe

[MichalStrehovsky]

EventHandlerType is going to be a delegate and we keep the Invoke method on all delegates as a rule. We could analyze this and avoid a warning.

Alternatively, we could introduce a new API on EventInfo to get the MethodInfo for the delegate Invoke method. EventInfo already has such proxy shortcut property such as EventInfo.IsMulticast

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

Or another new API that would always work without warnings: Type.GetDelegateInvokeMethod. Getting the Invoke method on a delegate is a pretty common pattern so it might be nice to have a macro for foo.GetMethod("Invoke"). The macro would not have any dataflow requirements on the annotation of the Type since it would only return non-null on actual delegates.

[agocke]

@sbomer thoughts?

[sbomer]

To make EventInfo.EventHandlerType.GetMethod("Invoke") or EventInfo.EventHandlerType.GetDelegateInvokeMethod() work in ILLink, I think the main missing piece is tracking the EventInfo (along with the event type) that comes out of a GetEvent call. We don't currently track EventInfos. Once we have the event type, GetMethod("Invoke") would likely just work, and a new GetDelegateInvokeMethod API would just avoid keeping extra methods.

This doesn't feel high-priority to me unless customers are hitting it. @MichalStrehovsky do you remember where you encountered this?

[MichalStrehovsky]

I think the main missing piece is tracking the EventInfo

I think we can get away without modeling the EventInfo. If we are processing Type_GetMethod intrinsic on a MethodReturnValue that comes from EventInfo.get_EventHandlerType, we can just let it slide without any warnings. We know this must be a delegate type. We keep all Invoke methods on delegates. It's not important what event this delegate comes from.

This doesn't feel high-priority to me unless customers are hitting it. @MichalStrehovsky do you remember where you encountered this?

It was a customer scenario - see the suppression referencing this issue in microsoft/vs-streamjsonrpc#1207.

[sbomer]

I went ahead and tried this approach, and realized that we can't guarantee that EventHandlerType is actually a delegate without tracking something else, because some custom EventInfo could override EventHandlerType. I think this is OK because it's an abuse of the API and I believe we already have similar holes for other reflection intrinsics (for example someone could override GetMethodImpl to do something other than the rules we follow for the intrinsic).

[MichalStrehovsky]

I think this is OK because it's an abuse of the API and I believe we already have similar holes for other reflection intrinsics (for example someone could override GetMethodImpl to do something other than the rules we follow for the intrinsic).

Agreed - EventHandlerType being a delegate is a documented contract. I don't think we need to worry about situations when someone doesn't implement the contract. If anyone ever reports an issue with that, we'll just point them to the docs and ask them to fix their bug.