Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add design for interceptors with nonzero arity #68218

Closed

Conversation

RikkiGibson
Copy link
Contributor

ASP.NET has a scenario where permitting type parameters on an interceptor would be helpful: dotnet/aspnetcore#47338

cc @captainsafia

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 16, 2023
@RikkiGibson RikkiGibson mentioned this pull request May 16, 2023
84 tasks
@jcouv jcouv self-assigned this May 17, 2023
@RikkiGibson
Copy link
Contributor Author

From offline discussion, we'd like to support these scenarios eventually (e.g. .NET 9+), but we're comfortable with the change proposed here slipping from .NET 8.

@captainsafia
Copy link
Member

From offline discussion, we'd like to support these scenarios eventually (e.g. .NET 9+), but we're comfortable with the change proposed here slipping from .NET 8.

Addendum that we should support mapping generic type paramarters from containing scopes, perhaps using the same rules as defined in the extensions spec as outlined here.

}
```

If an interceptable method signature uses type parameters from the containing type (in parameters and returns, including `this`), it won't be possible to declare an interceptor for it in another type. This is because there's no position where the original containing type's type parameters can be substituted in to the interceptor. We could revisit this limitation if needed, perhaps by reviewing the arity matching requirements proposed for *implicit and explicit extensions* as a starting point.
Copy link
Member

Choose a reason for hiding this comment

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

We need to resolve this issue to fix dotnet/aspnetcore#47338. The methods we're trying to intercept in ASP.NET Core were never generic to begin with. We want the interceptor method to capture the generic parameter from an outer scope that was never passed to the original intercepted method, so the arity will be different.

Take the examples I originally posted in the ASP.NET Core issue:

public static void TestGenericParam<TUser>(IEndpointRouteBuilder app) where TUser : class
{
    app.MapPost("/test-param", ([FromServices] UserManager<TUser> userManager) => { });
}

public static void TestGenericResponse<TUser>(IEndpointRouteBuilder app) where TUser : new()
{
    app.MapPost("/test-return", () => new TUser());
}

The original MapPost has a generic arity of zero. The goal was to be able to define a MapPost<TUser>(IEndpointRouteBuilder app, string pattern, Delegate handler) so we could then use TUser in the generated code to resolve the arguments and serialize the response correctly.

Of course, for TUser to be inferred at these call sites, MapPost<TUser> would need to take an Action<UserManager<TUser>> or Func<TUser> respectively, rather than a Delegate like it does in the current PR to make ASP.NET Core use this feature at dotnet/aspnetcore#48555.

Looking at the signature matching part of this document, it appears arguments have to match the original method arguments exactly rather than take a more specific type. It'd be nice if we could relax that if we could statically prove that the call site will always use a more specific type that the interceptor accepts.

Here's an example that demonstrates the need for the interceptor to change the parameter type from Delegate to Func<T2> for type inference to work.

class C
{
    [Interceptable]
    public static void InterceptableMethod(Delegate handler) => throw null!;
}

static class Program
{
    public static void M<T1>(T1 t)
    {
        C.InterceptableMethod(() => t); // intercepts with 'Interceptor<T1>(Func<T1>)'
    }
}

static class D
{
    [InterceptsLocation("Program.cs", 13, 11)]
    public static void Interceptor<T2>(Func<T2> handler) => throw null!;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is good information and it's a tricky scenario. Is it safe to assume that we can't know the full set of possible type arguments to TestGenericResponse<TUser> in advance? (letting us possibly test and branch on the type of the return value.)

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, it's not safe to assume we can discover the full set of type arguments at compile time. I discovered this issue working on MapIdentityApi<TUser>() where the TUser is provided by the consumer of the library.

docs/features/interceptors.md Show resolved Hide resolved
@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Jun 7, 2023

It was also suggested that we should lift the requirement that a static method be an extension method in order to intercept a call. Then such interceptors can be declared in generic types, and the implementation can simply behave as though the static methods are extensions, whenever we intercept a call which has a receiver expression.

The type arguments in the original call are then substituted in the corresponding positions when intercepting.

C<int>.M1<string>();

class C<TC>
{
    public static void M1<TM1>()
    {

    }
}

class D<TD>
{
    // interception allowed because method arity and
    // all containing type arities match the original method
    [InterceptsLocation("Program.cs", 1, 8)]
    public static void M2<TM2>()
    {
    }
}

(note that I don't think this solves the scenario outlined in #68218 (comment).)

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Jun 9, 2023

We met today to discuss generics support in interceptors. Here are our conclusions:

  1. We want to deliver a critical level of generics support in .NET 8. Namely, in addition to permitting interceptors with arity 0, we will also permit interceptors whose arity equals the net arity of the original method (sum of arity of containing types plus arity of original method). The type arguments from the original method and containing types are passed along to the interceptor in source order, i.e. starting from the outermost generic type, and inwards to the original method.
  2. Also in .NET 8, we think it should be possible to intercept any instance method call using a static method, as if that static method were a classic extension method. Essentially, we will adjust the signature compatibility checks such that extension method scenarios which compile today will continue to compile if the 'this' modifier is removed from the interceptor method. We think requiring declaring an extension method makes it easier to unintentionally "pollute" IntelliSense and attaches unnecessary "baggage" to the process of declaring an interceptor.
  3. After .NET 8, we'd like to look more closely at supporting the scenario @halter73 outlined in the above discussion, where the interceptor signature accepts a more specific type than the original method, and adds type parameters which weren't present in the original method.

My plan is to get the interceptors feature ready to merge to main, and afterwards, I will retarget this PR to main and adjust the doc changes to reflect the above conclusions.

@RikkiGibson
Copy link
Contributor Author

Open issues remaining from this PR will be rolled into future PRs

@RikkiGibson RikkiGibson closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants