Fix overload resolution issue for Delegate and RequestDelegate #60593
Conversation
ghost
added
the
area-networking
dotnet-policy-service
bot
added
the
community-contribution
| @@ -74,7 +75,7 @@ public static IEndpointConventionBuilder MapFallback( | |||
| ArgumentNullException.ThrowIfNull(pattern); | |||
| ArgumentNullException.ThrowIfNull(requestDelegate); | |||
|
|
|||
| var conventionBuilder = endpoints.Map(pattern, requestDelegate); | |||
| var conventionBuilder = endpoints.Map(RoutePatternFactory.Parse(pattern), requestDelegate); | |||
There was a problem hiding this comment.
This appears to be related to #44970.
Is passing a RoutePattern instead of a string for the first argument the only way to force the compiler to select the RequestDelegate over the Delegate overload now? That doesn't feel very discoverable.
I see why you needed to do this. There are no RequiresUnreferencedCode or RequiresDynamicCode attributes currently on this method, and it'd be breaking to add them now. Unfortunately, for the exact same reason, it's a breaking change to prefer the Delegate overloads in cases where we were using RequestDelegate before.
It's true it's not binary breaking and rebuilding would present you with new IL2026 and IL3050 warnings, but I don't think it's worth it to even take a source break despite the warnings. The RequestDelegate overloads are older and simpler. And I think the analyzer introduced by #44393 catches the worst issues caused when the RequestDelegate overload gets selected for a Task<T>-returning endpoint.
Not everyone needs or wants the extra OpenAPI support added by the Delegate overloads. Those that do can easily opt in by casting the route handler to Delegate if need be. And everyone else that has been using the RequestDelegate overload since 3.0 just fine without need for the extra features supported by the Delegate overload can easily continue to do so without fighting IL2026 and IL3050 warnings.
There was a problem hiding this comment.
"Is passing a RoutePattern instead of a string for the first argument the only way to force the compiler to select the RequestDelegate over the Delegate overload now? That doesn't feel very discoverable."
Yes, I think so, as far as I know. I'm not entirely sure how the overload resolution works in every scenario.
According to VS intellisence, before the fix, the following examples used the RequestDelegate:
builder.MapGet("/withoutPR1", async context => await Task.FromResult("RequestDelegate overload is used"));
builder.MapGet("/withoutPR2", async (HttpContext context) => await Task.FromResult("RequestDelegate overload is used"));
After the PR, only the version that explicitly includes HttpContext uses the new Delegate overload:
builder.MapGet("/withPR1", async context => await Task.FromResult("RequestDelegate overload is used"));
builder.MapGet("/withPR2", async (HttpContext context) => await Task.FromResult("Delegate overload is used"));
"I think the analyzer introduced by #44393 catches the worst issues caused when the RequestDelegate overload gets selected for a Task-returning endpoint."
Yes, I think so too. It does seem to catch most cases, but for some reason, it doesn’t flag the ones above.
"It's true it's not binary breaking and rebuilding would present you with new IL2026 and IL3050 warnings,
but I don't think it's worth it to even take a source break despite the warnings."
I agree. This change only makes the experience better for new projects using minimal APIs.
As you mentioned, there are breaking changes for older code.
I’m totally fine with dropping this PR if it’s not worth the trade-off—I mainly wanted to explore whether the benefits outweighed the drawbacks.
Attempt to resolve the issue where the RequestDelegate overload is incorrectly preferred over the Delegate overload by using the new OverloadResolutionPriority attribute.
bjornen77
force-pushed
the
fix/overload-resolution-priority
branch
from
bf2f9d1 to
011305b
Compare
|
Closing this PR as it is not worth the breaking changes. |
Test to try resolve the issue where the RequestDelegate overload is
chosen by the compiler instead of the Delegate overload by using the
OverloadResolutionPriority attribute.
However, maybe this change will cause other unwanted side effects?