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

RDG does not support generic types from outer scope #47338

Open
halter73 opened this issue Mar 20, 2023 · 6 comments
Open

RDG does not support generic types from outer scope #47338

halter73 opened this issue Mar 20, 2023 · 6 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc bug This issue describes a behavior which is not expected - a bug. feature-rdg

Comments

@halter73
Copy link
Member

halter73 commented Mar 20, 2023

Given the code:

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());
}

RDG will generate code that fails to compile because TUser is undefined. As a short-term mitigation, we can skip generating MapPost and similar methods when we see an open generic parameter in the delegate signature. Long term, we can look at flowing the generic parameters through the Map methods we generate which can then hopefully be inferred.

GeneratedRouteBuilderExtensions.g.cs(73,85): error CS0246: The type or namespace name 'TUser' could not be found (are you missing a using directive or an assembly reference?)
GeneratedRouteBuilderExtensions.g.cs(88,33): error CS0246: The type or namespace name 'TUser' could not be found (are you missing a using directive or an assembly reference?)
        internal static global::Microsoft.AspNetCore.Builder.RouteHandlerBuilder MapPost(
            this global::Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints,
            [global::System.Diagnostics.CodeAnalysis.StringSyntax("Route")] string pattern,
            global::System.Action<global::Microsoft.AspNetCore.Identity.UserManager<TUser>> handler, // <---- Error!
            [global::System.Runtime.CompilerServices.CallerFilePath] string filePath = "",
            [global::System.Runtime.CompilerServices.CallerLineNumber]int lineNumber = 0)
        {
// ...
                            handler(ic.GetArgument<global::Microsoft.AspNetCore.Identity.UserManager<TUser>>(0)!);
// ... 
                        var userManager_local = httpContext.RequestServices.GetRequiredService<Microsoft.AspNetCore.Identity.UserManager<TUser>>();
// ...
                    var handler = (Func<TUser>)del;
// ...
                    var jsonTypeInfo =  (JsonTypeInfo<TUser>)serializerOptions.GetTypeInfo(typeof(TUser));
@halter73 halter73 added bug This issue describes a behavior which is not expected - a bug. area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-rdg labels Mar 20, 2023
@captainsafia captainsafia added this to the .NET 8 Planning milestone Mar 29, 2023
@ghost
Copy link

ghost commented Mar 29, 2023

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@halter73
Copy link
Member Author

FWIW, I worked around this in MapIdentityApi<TUser> by injecting IServiceProvider instead of UserManager<TUser> the and calling GetRequiredService<UserManager<TUser>>() inside the body of the route handler.

@captainsafia
Copy link
Member

I investigated what it would look like to add support for this but ultimately believe that it should be done after #48289 and dotnet/roslyn#68218 are resolved.

A current limitation in the interceptors feature is that it does not support intercepting methods with generic type arguments. This gives us three options with how to handle this moving forward:

  • Implement support for endpoints that capture type parameters using overload resolution semantics and use interceptors for other endpoints. This is the least ideal option, because although we end up supporting this scenario, we do so at the cost of a straightforward generator implementation and a less robust interceptors API.
  • Add support for intercepting generic methods in the interceptors feature and leverage that in the implementation. This is the ideal scenarios as it means that our source generator can rely on one strategy for intercepting method calls (using the interceptors feature) and the interceptors feature is made more robust.
  • Do not support this feature (for now maybe).

My preference is the 2nd, 3rd, and 1st options in that order. I'd like to move us away from a world where we ship a generator that has two different interception strategies that we need to support for the lifetime of 8.0.

@eerhardt eerhardt modified the milestones: 8.0-preview5, 8.0-preview6 May 24, 2023
@adityamandaleeka adityamandaleeka modified the milestone: 8.0-preview6 May 30, 2023
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
@halter73
Copy link
Member Author

halter73 commented Jun 6, 2023

Even if dotnet/roslyn#68218 is resolved, I do not think it will address our issue. I explain why in https://github.com/dotnet/roslyn/pull/68218/files#r1220428975, but it mostly boils down to that proposal being about intercepting methods that are already generic rather than introducing generic parameters. And even if we could change the generic arity, we'd then also need to change the handler parameter type from Delegate to something like Action<UserManager<TUser>> for type inference to work. That part in particular feels like it might be a big ask.

In any case, it doesn't look like any sort of generic interceptor support is planned for .NET 8. Is the plan for preview6 to have RDG skip over delegates with generic types in their signature and emit a warning?

@captainsafia captainsafia removed the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jun 6, 2023
@captainsafia
Copy link
Member

Moving this to the backlog. For preview7, we fixed this so that static codegen does not happen for these codepaths. We'll need to fix the underlying issues in the interceptors feature for .NET 9.

@captainsafia captainsafia modified the milestones: 8.0-preview6, Backlog Jul 19, 2023
@ghost
Copy link

ghost commented Jul 19, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc bug This issue describes a behavior which is not expected - a bug. feature-rdg
Projects
None yet
Development

No branches or pull requests

7 participants
@halter73 @adityamandaleeka @captainsafia @eerhardt @amcasey @wtgodbe and others