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

Fix up handling for non-generic Task and ValueTask #61085

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/OpenApi/gen/XmlCommentGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public sealed partial class XmlCommentGenerator
if (DocumentationCommentId.GetFirstSymbolForDeclarationId(name, compilation) is ISymbol symbol &&
// Only include symbols that are declared in the application assembly or are
// accessible from the application assembly.
(SymbolEqualityComparer.Default.Equals(symbol.ContainingAssembly, input.Compilation.Assembly) || symbol.IsAccessibleType()) &&
(SymbolEqualityComparer.Default.Equals(symbol.ContainingAssembly, compilation.Assembly) || symbol.IsAccessibleType()) &&
// Skip static classes that are just containers for members with annotations
// since they cannot be instantiated.
symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsStatic: true })
Expand All @@ -104,7 +104,7 @@ public sealed partial class XmlCommentGenerator
{
var memberKey = symbol switch
{
IMethodSymbol methodSymbol => MemberKey.FromMethodSymbol(methodSymbol, input.Compilation),
IMethodSymbol methodSymbol => MemberKey.FromMethodSymbol(methodSymbol),
IPropertySymbol propertySymbol => MemberKey.FromPropertySymbol(propertySymbol),
INamedTypeSymbol typeSymbol => MemberKey.FromTypeSymbol(typeSymbol),
_ => null
Expand Down
14 changes: 4 additions & 10 deletions src/OpenApi/gen/XmlComments/MemberKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ internal sealed record MemberKey(
typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces,
genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters);

public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compilation)
public static MemberKey FromMethodSymbol(IMethodSymbol method)
{
string returnType;
if (method.ReturnsVoid)
Expand All @@ -32,16 +32,10 @@ public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compi
{
// Handle Task/ValueTask for async methods
var actualReturnType = method.ReturnType;
if (method.IsAsync && actualReturnType is INamedTypeSymbol namedType)
if (method.IsAsync
&& actualReturnType is INamedTypeSymbol { TypeArguments.Length: 1 } namedType)
{
if (namedType.TypeArguments.Length > 0)
{
actualReturnType = namedType.TypeArguments[0];
}
else
{
actualReturnType = compilation.GetSpecialType(SpecialType.System_Void);
}
actualReturnType = namedType.ConstructedFrom;
Copy link
Member

Choose a reason for hiding this comment

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

Was trying to understand what exactly this did, and why we couldn't just get rid of this check since ReplaceGenericArguments below should handle Task<T>.
Looks like ConstructedFrom would return Task<int> if you had a type like Example : Task<int>. Is that the desired effect? We should probably add a test like that.

We also have tests for Task and Task<T>

_cache.Add(new MemberKey(typeof(global::ExampleClass), MemberType.Method, "AddAsync", typeof(global::System.Threading.Tasks.Task<>), [typeof(global::System.Int32), typeof(global::System.Int32)]), new XmlComment(@"This method is an example of a method that
returns an awaitable item.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ExampleClass), MemberType.Method, "DoNothingAsync", typeof(global::System.Threading.Tasks.Task), []), new XmlComment(@"This method is an example of a method that

So not too sure what was broken.

}

returnType = actualReturnType.TypeKind == TypeKind.TypeParameter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public async Task SupportsXmlCommentsOnOperationsFromMinimalApis()
{
var source = """
using System;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.AspNetCore.Http.HttpResults;
Expand All @@ -32,6 +33,14 @@ public async Task SupportsXmlCommentsOnOperationsFromMinimalApis()
app.MapGet("/5", RouteHandlerExtensionMethods.Get5);
app.MapPost("/6", RouteHandlerExtensionMethods.Post6);
app.MapPut("/7", RouteHandlerExtensionMethods.Put7);
app.MapGet("/8", RouteHandlerExtensionMethods.Get8);
app.MapGet("/9", RouteHandlerExtensionMethods.Get9);
app.MapGet("/10", RouteHandlerExtensionMethods.Get10);
app.MapGet("/11", RouteHandlerExtensionMethods.Get11);
app.MapGet("/12", RouteHandlerExtensionMethods.Get12);
app.MapGet("/13", RouteHandlerExtensionMethods.Get13);
app.MapGet("/14", RouteHandlerExtensionMethods.Get14);
app.MapGet("/15", RouteHandlerExtensionMethods.Get15);

app.Run();

Expand Down Expand Up @@ -114,13 +123,85 @@ public static IResult Put7(int? id, string uuid)
{
return TypedResults.NoContent();
}

/// <summary>
/// A summary of Get8.
/// </summary>
public static async Task Get8()
{
await Task.Delay(1000);
return;
}

/// <summary>
/// A summary of Get9.
/// </summary>
public static async ValueTask Get9()
{
await Task.Delay(1000);
return;
}

/// <summary>
/// A summary of Get10.
/// </summary>
public static Task Get10()
{
return Task.CompletedTask;
}

/// <summary>
/// A summary of Get11.
/// </summary>
public static ValueTask Get11()
{
return ValueTask.CompletedTask;
}

/// <summary>
/// A summary of Get12.
/// </summary>
public static Task<string> Get12()
{
return Task.FromResult("Hello, World!");
}

/// <summary>
/// A summary of Get13.
/// </summary>
public static ValueTask<string> Get13()
{
return new ValueTask<string>("Hello, World!");
}

/// <summary>
/// A summary of Get14.
/// </summary>
public static async Task<Holder<string>> Get14()
{
await Task.Delay(1000);
return new Holder<string> { Value = "Hello, World!" };
}

/// <summary>
/// A summary of Get15.
/// </summary>
public static Task<Holder<string>> Get15()
{
return Task.FromResult(new Holder<string> { Value = "Hello, World!" });
}
}

public class User
{
public string Username { get; set; } = string.Empty;
public string Email { get; set; } = string.Empty;
}

public class Holder<T>
{
public T Value { get; set; } = default!;
}
""";
var generator = new XmlCommentGenerator();
await SnapshotTestHelper.Verify(source, generator, out var compilation);
Expand Down Expand Up @@ -159,6 +240,30 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
var idParam = path7.Parameters.First(p => p.Name == "id");
Assert.True(idParam.Deprecated);
Assert.Equal("Legacy ID parameter - use uuid instead.", idParam.Description);

var path8 = document.Paths["/8"].Operations[OperationType.Get];
Assert.Equal("A summary of Get8.", path8.Summary);

var path9 = document.Paths["/9"].Operations[OperationType.Get];
Assert.Equal("A summary of Get9.", path9.Summary);

var path10 = document.Paths["/10"].Operations[OperationType.Get];
Assert.Equal("A summary of Get10.", path10.Summary);

var path11 = document.Paths["/11"].Operations[OperationType.Get];
Assert.Equal("A summary of Get11.", path11.Summary);

var path12 = document.Paths["/12"].Operations[OperationType.Get];
Assert.Equal("A summary of Get12.", path12.Summary);

var path13 = document.Paths["/13"].Operations[OperationType.Get];
Assert.Equal("A summary of Get13.", path13.Summary);

var path14 = document.Paths["/14"].Operations[OperationType.Get];
Assert.Equal("A summary of Get14.", path14.Summary);

var path15 = document.Paths["/15"].Operations[OperationType.Get];
Assert.Equal("A summary of Get15.", path15.Summary);
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,14 @@ private static Dictionary<MemberKey, XmlComment> GenerateCacheEntries()
""email"": ""[email protected]""
}", null, null, false, null, [new XmlParameterComment(@"user", @"The user information.", @"{""username"": ""johndoe"", ""email"": ""[email protected]""}", false)], [new XmlResponseComment(@"201", @"Successfully created the user.", @""), new XmlResponseComment(@"400", @"If the user data is invalid.", @"")]));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Put7", typeof(global::Microsoft.AspNetCore.Http.IResult), [typeof(global::System.Int32?), typeof(global::System.String)]), new XmlComment(@"Updates an existing record.", null, null, null, null, false, null, [new XmlParameterComment(@"id", @"Legacy ID parameter - use uuid instead.", null, true), new XmlParameterComment(@"uuid", @"Unique identifier for the record.", null, false)], [new XmlResponseComment(@"204", @"Update successful.", @""), new XmlResponseComment(@"404", @"Legacy response - will be removed.", @"")]));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get8", typeof(global::System.Threading.Tasks.Task), []), new XmlComment(@"A summary of Get8.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get9", typeof(global::System.Threading.Tasks.ValueTask), []), new XmlComment(@"A summary of Get9.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get10", typeof(global::System.Threading.Tasks.Task), []), new XmlComment(@"A summary of Get10.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get11", typeof(global::System.Threading.Tasks.ValueTask), []), new XmlComment(@"A summary of Get11.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get12", typeof(global::System.Threading.Tasks.Task<>), []), new XmlComment(@"A summary of Get12.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get13", typeof(global::System.Threading.Tasks.ValueTask<>), []), new XmlComment(@"A summary of Get13.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get14", typeof(global::System.Threading.Tasks.Task<>), []), new XmlComment(@"A summary of Get14.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::RouteHandlerExtensionMethods), MemberType.Method, "Get15", typeof(global::System.Threading.Tasks.Task<>), []), new XmlComment(@"A summary of Get15.", null, null, null, null, false, null, null, null));

return _cache;
}
Expand Down
Loading