Roslyn: IInvocationOperation and IObjectCreationOperation should expose the underlying IMethodReferenceOperation

[mavasani]

See discussion here: https://github.com/dotnet/roslyn/pull/26188#discussion_r182178995

[CyrusNajmabadi]

Thanks! Can we rope in the compatability council? My recollection is that we have fixed semantic model calls in the past when they returned incorrect results. this could certainly break anyone who took a dependency on the old values. But, in practice, just having good behavior rather than preserving bugs won out and was healthier for the ecosystem as a whole. Considering how new IOperation is, i'm hoping we can do the same for it here.

[AlekseyTs]

I think IInvocationOperation should implement IMemberReferenceOperation instead, see https://github.com/dotnet/roslyn/issues/23084

[CyrusNajmabadi]

That's def an option! Though i think it will lead to a lot of surprise from users. Namely that they can register for method references, and then not actually hear about method references (i.e. exactly the issue that bit this feature :D)**. That said, it would be a viable option that avoids the compat issue. So that's a huge pro going for it.

** However, anyone who cares about methods for a feature will almost certainly try things out with an actual invocation. So it's seems nearly impossible that they would miss this. if we go this route we should probably document IMethodReference and IPropertyReference and say that you may have to also register for IInvocationExpression to hear about Methods/properties if they're used in invocation scenarios.

[AlekseyTs]

Documentation clearly states that IMethodReferenceOperation "Represents a reference to a method other than as the target of an invocation." So, I don't see why one would be surprised.

[CyrusNajmabadi]

So, I don't see why one would be surprised.

Because humans are humans :) Note that IPropertyRef does not say the same. Should i assume that me.Prop(1, 2, 3) in VB has an IInvocationOp and an IPropertyReferenceOp? :) (i honestly don't know. either IOp tree wouldn't surprise me).

[AlekseyTs]

Because humans are humans :)

Well, if that is the reason, then no matter what we do some humans will be surprised.

Should i assume that me.Prop(1, 2, 3) in VB has an IInvocationOp and an IPropertyReferenceOp? :)

You shouldn't assume anything, you should find an answer to the question. :-)

[sharwell]

@mavasani Am I correct in my interpretation that the current proposal would not be a breaking change for analyzers based on the 2.6 API?

[mavasani]

I think we have few proposals here:

1. Replace IInvocationOperation.Instance with IInvocationOperation.MemberReference, which is obviously a breaking change for analyzers on 2.6 API, hence likely least preferable.
2. Mark IInvocationOperation.Instance as deprecated and add a new property IMemberReferenceOperation MemberReference { get; } to IInvocationOperation. Current analyzers should work, but analyzers written on newer API can easily be moved to new API. Analyzers that register for OperationKind.MethodReference will now get callbacks for method references in invocation and non-invocation context. This could be a semantic break for analyzers, but might be for the general goodness/intuitiveness of analyzer authors.
3. Aleksey's proposal that IInvocationOperation just implement IMemberReferenceOperation. No API or semantic breaking change for analyzers, but analyzer authors have to register both OperationKind.MethodReference and OperationKind.Invocation to get all IMemberReferenceOperation that reference or invoke a method.

I think our general view is 3. is probably our best option, especially given analyzer authors will quickly figure out that registering just for OperationKind.MethodReference will not help them for invocation contexts.

[CyrusNajmabadi]

👍 on option '3' for me as well.

[mavasani]

Just a note: I think even IObjectCreationOperation should implement IMethodReferenceOperation. I will update the title accordingly.