-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix OpenAPI XML code gen #60977
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 OpenAPI XML code gen #60977
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,9 @@ internal static string GenerateXmlCommentSupportSource(string commentsFromXmlFil | |
// </auto-generated> | ||
//------------------------------------------------------------------------------ | ||
#nullable enable | ||
// Suppress warnings about obsolete types and members | ||
// in generated code | ||
#pragma warning disable CS0612, CS0618 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suppress obsoletion warnings in generated code. If someone happens to be using an obsolete type in their API, we don't want to resurface this warning in generated code where we call |
||
|
||
namespace System.Runtime.CompilerServices | ||
{ | ||
|
@@ -487,7 +490,7 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment) | |
else | ||
{ | ||
codeWriter.Write("["); | ||
for (int i = 0; i < comment.Examples.Count; i++) | ||
for (var i = 0; i < comment.Examples.Count; i++) | ||
{ | ||
var example = comment.Examples[i]; | ||
codeWriter.Write(FormatStringForCode(example)); | ||
|
@@ -506,13 +509,18 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment) | |
else | ||
{ | ||
codeWriter.Write("["); | ||
for (int i = 0; i < comment.Parameters.Count; i++) | ||
for (var i = 0; i < comment.Parameters.Count; i++) | ||
{ | ||
var parameter = comment.Parameters[i]; | ||
var exampleLiteral = string.IsNullOrEmpty(parameter.Example) | ||
? "null" | ||
: FormatStringForCode(parameter.Example!); | ||
codeWriter.Write($"new XmlParameterComment(@\"{parameter.Name}\", @\"{parameter.Description}\", {exampleLiteral}, {(parameter.Deprecated == true ? "true" : "false")})"); | ||
codeWriter.Write("new XmlParameterComment("); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below we format strings in the emitted code so that we can properly escape quoted strings inside XML descriptions/summaries/etc. |
||
codeWriter.Write(FormatStringForCode(parameter.Name) + ", "); | ||
codeWriter.Write(FormatStringForCode(parameter.Description) + ", "); | ||
codeWriter.Write(exampleLiteral + ", "); | ||
codeWriter.Write(parameter.Deprecated == true ? "true" : "false"); | ||
codeWriter.Write(")"); | ||
if (i < comment.Parameters.Count - 1) | ||
{ | ||
codeWriter.Write(", "); | ||
|
@@ -528,10 +536,13 @@ internal static string EmitSourceGeneratedXmlComment(XmlComment comment) | |
else | ||
{ | ||
codeWriter.Write("["); | ||
for (int i = 0; i < comment.Responses.Count; i++) | ||
for (var i = 0; i < comment.Responses.Count; i++) | ||
{ | ||
var response = comment.Responses[i]; | ||
codeWriter.Write($"new XmlResponseComment(@\"{response.Code}\", @\"{response.Description}\", {(response.Example is null ? "null" : FormatStringForCode(response.Example))})"); | ||
codeWriter.Write("new XmlResponseComment("); | ||
codeWriter.Write(FormatStringForCode(response.Code) + ", "); | ||
codeWriter.Write(FormatStringForCode(response.Description) + ", "); | ||
codeWriter.Write(response.Example is null ? "null)" : FormatStringForCode(response.Example) + ")"); | ||
if (i < comment.Responses.Count - 1) | ||
{ | ||
codeWriter.Write(", "); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compi | |
|
||
returnType = actualReturnType.TypeKind == TypeKind.TypeParameter | ||
? "typeof(object)" | ||
: $"typeof({actualReturnType.ToDisplayString(_typeKeyFormat)})"; | ||
: $"typeof({ReplaceGenericArguments(actualReturnType.ToDisplayString(_typeKeyFormat))})"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to make sure that all generics are emitted as open generics for method declarations. We already do this elsewhere in the |
||
} | ||
|
||
// Handle extension methods by skipping the 'this' parameter | ||
|
@@ -62,10 +62,10 @@ public static MemberKey FromMethodSymbol(IMethodSymbol method, Compilation compi | |
// For params arrays, use the array type | ||
if (p.IsParams && p.Type is IArrayTypeSymbol arrayType) | ||
{ | ||
return $"typeof({arrayType.ToDisplayString(_typeKeyFormat)})"; | ||
return $"typeof({ReplaceGenericArguments(arrayType.ToDisplayString(_typeKeyFormat))})"; | ||
} | ||
|
||
return $"typeof({p.Type.ToDisplayString(_typeKeyFormat)})"; | ||
return $"typeof({ReplaceGenericArguments(p.Type.ToDisplayString(_typeKeyFormat))})"; | ||
}) | ||
.ToArray(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helps us avoid generating XML comments for non-public types and internal properties in non-public types.
Note: this does mean that DTOs have to be defined as public types if they are defined in the same assembly as the API (see where this is called in the
AssemnlyTypeSymbolVisitor
above). We can consider changing this behavior to allow XML comments on internal types in the application assembly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit overaggressive. I'm guessing we'll change this behavior a bit in the future.
Any reason to disallow the type entirely just because it has internal types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just hit this error in preview 2:
This relates to a
private
nested class inpublic
class that has some///
comments on theinternal const string
members in it.Would this change fix that scenario too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more scenario for you - seems like it's trying to document things to do with the Regex source generator (example):
If not catered for by this change, I can open a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on whether the type in question is defined in or outside the assembly.
I believe these bits have landed in the nightly preview3 SDK if you want to take another bump to try it out.
Alternatively, I'd recommend filing an issue so we can at least capture this scenario in a test case even if it is already fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try out a nightly tomorrow and see if you think it should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed these two are catered for as of
10.0.100-preview.3.25169.19
, but confirmed #61019 is still an issue and found two new ones 😅. Issues shortly...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they're probably the same one issue: #61035
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, I'd recommend filing an issue so we can at least capture this scenario in a test case even if it is already fixed.
Found a different case where a private type causes a compiler error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#61037