[Analyzer] RequestDelegate return value detection

[JamesNK]

Background and Motivation

RequestDelegate can be used with methods/lambdas that return a value that is then ignored.

Delegate signature:

aspnetcore/src/Http/Http.Abstractions/src/RequestDelegate.cs

Lines 6 to 11 in 86c7e01

	/// <summary>
	/// A function that can process an HTTP request.
	/// </summary>
	/// <param name="context">The <see cref="HttpContext"/> for the request.</param>
	/// <returns>A task that represents the completion of request processing.</returns>
	public delegate Task RequestDelegate(HttpContext context);

Because Task is the base type of Task<T>, generic variance means it's possible to return Task<T> from a method that's used with RequestDelegate. The caller of the delegate never sees the value and it is ignored.

Example: #39956

Proposed API

An analyzer that detects using a method or lambda that returns Task<T> with RequestDelegate and warns the user.

Usage Examples

Task<string> HelloWorld(HttpContext c) => Task.FromResult("Hello " + c.Request.RouteValues["name"]);

endpoints.MapGet("/", HelloWorld); // <- detect bad method

endpoints.MapGet("/", (HttpContext c) => Task.FromResult("Hello " + c.Request.RouteValues["name"])); // <- detect bad lambda

Note that an async lambda can't get in this situation and doesn't need to be checked. The compiler detects a return value and prevents the lambda being converted to RequestDelegate:

RequestDelegate d = new RequestDelegate(async Task<string> (HttpContext context) =>
{
    await Task.Yield();
    return "hello world"; // <- compiler error
});

Alternative Designs

Risks

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[davidfowl]

What does a "bad method" mean? We fixed this in .NET 7.

[JamesNK]

A RequestDelegate method that returns Task<T>.

[davidfowl]

That works now. This is why the issue is closed.

[JamesNK]

The change was reverted later - #42519

[davidfowl]

So is this about trimming or something else? The change wasn't reverted, just the trimming attributes.

[halter73]

API Review Notes:

• This especially important since this isn't caught at runtime and is likely misusage.

Category: Usage
Severity: Warning