Add public API to determine if a call is being intercepted

[RikkiGibson]

Background and Motivation

As we work to stabilize the interceptors feature, we have public API additions which are needed in order to deliver a fully coherent and usable feature experience.

Need for interceptor semantic info in IDE

See also https://github.com/dotnet/csharplang/blob/interceptors-2024-01/proposals/interceptors-issues-2024-01.md#need-for-interceptor-semantic-info-in-ide

Currently, the Roslyn public APIs provide no indication of whether a method call is being intercepted. After shipping the interceptors experimental feature, we found that some analyzers had a need to know when a call is being intercepted, in order to produce correct diagnostics.

routes.MapGet("/products/", () => { ... }); // warning: MapGet is not NativeAOT-compatible

static class Extensions
{
    // Original method signature looks like:
    [RequiresUnreferencedCode]
    public static IRouteEndpointBuilder MapGet(this IRouteEndpointBuilder builder, string route, Delegate handler) => ...;

    // Interceptor signature looks like (i.e. lacks '[RequiresUnreferencedCode]'):
    [InterceptsLocation(/* routes.MapGet(...) */)]
    public static IRouteEndpointBuilder Interceptor(this IRouteEndpointBuilder builder, string route, Delegate handler) => ...;
}

In the above sample, the ILLinker analyzer will warn about use of a method marked with [RequiresUnreferencedCode]. If it knew that the call was intercepted, and could inspect the interceptor method instead, then it would be able to give more accurate diagnostics (i.e., it would know that it does not need to warn on the above call.)

In .NET 8, we worked around the spurious warning in the above sample by having the generator author ship a diagnostic suppressor. We think this is problematic as a practice because it requires the generator author to understand the semantics of the warning deeply enough to be able to claim "actually, this doesn't apply to me". If, for example, the interceptor method also had [RequiresUnreferencedCode], we wouldn't want the warning to be suppressed, and it seems like a layering violation for the source generator to need to know these kinds of in-depth details about another component.

Proposed API

 namespace Microsoft.CodeAnalysis;

 public static class CSharpExtensions
 {
+    /// <summary>If the call represented by <paramref name="node"/> is referenced in an InterceptsLocationAttribute, returns the original definition symbol which is decorated with that attribute. Otherwise, returns null.</summary>
+    public static IMethodSymbol? GetInterceptorMethod(this SemanticModel model, InvocationExpressionSyntax node, CancellationToken cancellationToken = default);
 }

This API shape is intended to reflect the fact that the first time an analyzer asks about interceptors, a certain amount of fixed-overhead work will need to be done and cached on the compilation. Specifically, we will need to search declarations in the compilation for any attributes which might be [InterceptsLocation] (taking aliases into account), and bind them. We expect a quite small subset of attributes on methods to be bound in this process.

Usage Examples

public sealed override void Initialize(AnalysisContext context)
{
    context.RegisterOperationAction(handleInvocation, OperationKind.Invocation);

    void handleInvocation(OperationAnalysisContext context)
    {
        var invocationOperation = (IInvocationOperation)context.Operation;
        if (invocationOperation.Syntax is InvocationExpressionSyntax invocationSyntax
            && context.SemanticModel.GetInterceptorMethod(invocationSyntax, context.CancellationToken) is IMethodSymbol interceptor)
        {
            AnalyzeInterceptor(interceptor);
        }
    }
}

Alternative Designs

We rejected a design which added a member to IInvocationOperation to get the interceptor--either a method which takes CancellationToken or a property accessor.

Risks

This API adds complexity to analyzers: they need to decide if they care about intercepted calls or not. In most cases, we believe analyzers do not need to care about intercepted calls. Looking at the original call should be good enough.

The essential case where an analyzer may need to look at the interceptor of a call is when legal signature differences between the interceptor and original methods might influence the analyzer's behavior. In the ILLinker analyzer case, its behavior differs based on presence or absence of the [RequiresUnreferencedCode] attribute. This makes interceptors of interest to that analyzer.

[CyrusNajmabadi]

See: #72070

It will allow you to be fast about finding attributes.

Also, since the invocation expression is being passed in, you can use that to prefilter potential attributes syntactically to those that reference the file/line/col of that invocation expression. No need to bind if they couldn't possibly match.

[CyrusNajmabadi]

With this approach, we would probably want to do the work eagerly for consistency with other properties on the interface.

That's sounds bad, unless you can guarantee that this will be fast/cheap. We don't want getting operations to fall off a perf cliff.

[CyrusNajmabadi]

GetInterceptorMethod

i'm not a huge fan of this being typed to InvocationExpression. What happens when we expand interceptors to constructors, operators, indexers, etc.

I do like tying this to IOperation only, but behind a cancellable call.

[CyrusNajmabadi]

How do we intend to mark this experimental btw? An attribute on the api?

[RikkiGibson]

i'm not a huge fan of this being typed to InvocationExpression. What happens when we expand interceptors to constructors, operators, indexers, etc.

This is a fair concern. I outlined my view on this question in the linked csharplang proposal. Pasting here:

The above API shape is specific to InvocationExpressionSyntax. The reason for this is that we are trying to make correct usage easy. If we took a more general syntax type such as ExpressionSyntax or SyntaxNode, then correct usage would become more ambiguous. Which node is the generator (or analyzer) supposed to pass in? Should it be the NameSyntax whose location is used to denote the location of the intercepted call? If so, what is the proper way to dig through an InvocationExpressionSyntax to obtain that name syntax? What is the failure mode supposed to be if the syntax passed as an argument doesn't support interceptors?

We expect that if support for intercepting more member kinds is permitted, then additional API for those member kinds should be introduced. The commitment we are making when we support intercepting a particular member kind is similar in significance to the commitment we make when introducing a public API. It doesn't serve generator (or analyzer) authors to try and optimize for fewest number of public APIs in this case.

[RikkiGibson]

How do we intend to mark this experimental btw? An attribute on the api?

Yes, if we want to ship this as experimental, we can definitely just put [Experimental] on the method. We are planning to do the same for "nullable disabled semantic model".

[jaredpar]

Don't we also need an API for generators that maps a InvocationExpressionSyntax into the opaque identifier we use in the [InterceptsLocation] attribute?

[CyrusNajmabadi]

InvocationExpression seems like the wrong type.

Consider:

var v = this.MethodToIntercept;

When this is intercepted I won't have an invite invocation to pass in to her the info on.

The interceptor is on a token span-start. That's what should be passed in.

[CyrusNajmabadi]

InvocationExpression seems like the wrong type.

Consider:

var v = this.MethodToIntercept;

When this is intercepted I won't have an invite invocation to pass in to her the info on.

The interceptor is on a token span-start. That's what should be passed in.

[CyrusNajmabadi]

This is a fair concern. I outlined my view on this question in the linked csharplang proposal. Pasting here:

I personally don't like this. It doesn't work that way for GetSymbolInfo/GetTypeInfo/GetConstantValue.

This feels like over indexing on a spike. The spike is to demonstrate viability. But the API should be designed against the general concept.

Put another way, imagine we added constants to the language, but initially in a prototype where it was just integer literals. Having the API only allow that would just box us in when doing the natural and expected expansion later.

[CyrusNajmabadi]

The more I think about this, the more this should be an iop API. I don't see how things like intercepting an implicit operator would work here. Iop properly represents the semantic tree, and is the modern way to get all the info we just to try to have to jam in with specialized helpers on semanticmodel

[RikkiGibson]

I personally would be fine with the following shape:

 namespace Microsoft.CodeAnalysis.Operations;

 public interface IInvocationOperation : IOperation
 {
     IMethodSymbol TargetMethod { get; }

+    /// <summary>Gets the method which is intercepting this call, if any.</summary>
+    IMethodSymbol? GetInterceptorMethod(CancellationToken token = default);
 }

the above indicating that calling it may cause work to be done, which can be canceled with a token if desired.

[RikkiGibson]

Per offline feedback, I have edited issue title and description to include the proposal for the full set of public APIs we want for the interceptors feature.

[RikkiGibson]

Tagging @captainsafia @agocke for visibility

[RikkiGibson]

Broken out second part of the API request into #72133

[333fred]

API Review

• Do we need to point at the entire IInvocationOperation of the return?
  • Differences in parameter or return?
  • Currently there are none allowed, but would that change in the future, and should we try to plan for that?
  • This would effectively be exposing a new lowered form of the tree, and we don't do that at all today.
• Do we want to make GetInterceptorMethod an extension method in the Microsoft.CodeAnalysis.CSharp namespace?
• Why would we want to just put it on IOperation?
  • Implementing this as a method on SemanticModel
  • Does an API shape that puts the method on IOperation lock us into having lazy state on IInvocationOperation?
• Could we invert the API and provide "here's the list of locations that are being intercepted"?
  • Might be the wrong approach for analyzer-based approaches
• We only have one specific customer scenario for this at the moment, a narrowly-focused API seems appropriate
• What symbol should be returned here?
  • Should it have type substitution performed, for example?
    • Type substitution is effectively a form of lowering
  • We should adjust the documentation to make it clear that you're getting the definition of the method, not a substituted version
• What if we had the method on Compilation, rather than SemanticModel?
  • The input SemanticModel is somewhat irrelevant: the actual intercepting method is likely to be in a different tree
  • Compilation doesn't have any public APIs for getting semantic information from a specific syntax element.

Conclusion: We approved the SemanticModel-based GetInterceptorMethod API that takes an InvocationExpressionSyntax. We need to be careful to be very clear in the documentation for the method that it returns the original definition of the intercepting method, not the fully-substituted version.