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.

Sign up for GitHub

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

Jump to bottom

Harden handling for generic type arguments in source generator #61145

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Harden handling for generic type arguments in source generator #61145

wants to merge 7 commits into from
+1,693 −1,024

Conversation

captainsafia
Copy link
Member

Closes #61035.

@captainsafia captainsafia requested a review from a team as a code owner March 24, 2025 22:16
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 24, 2025
@captainsafia captainsafia mentioned this pull request Mar 26, 2025
Closed
@captainsafia
Copy link
Member Author

@BrennanConroy @martincostello I talked with both of you online about the notion of switching to a string-based key and what the key would be. Turns out that MetadataName isn't full-proof since it's only defined/exposed for Types so we still have to figure out a mapping for MethodInfo-based values.

I dusted off an old idea around reusing the DocumentationIds that exist in the XML doc comment to support this. We know emit a helper method that emits a documentation ID from PropertyInfo, Type, or MethodInfo. I used Copilot to iterate on this API and it seems to provide coverage for the test cases we have here.

Worst case scenario: the failure mode here is a lot nicer since we won't emit any compiler errors for invalid keys. In the event that happens, the implementation will no-op and fail to apply a doc comment but it shouldn't impact users builds.

martincostello
martincostello reviewed Mar 27, 2025
View reviewed changes
src/OpenApi/gen/XmlCommentGenerator.Emitter.cs Outdated
if (!omitGenericArity)
{
int arity = genericDef.GetGenericArguments().Length;
defName += "`" + arity;
Copy link
Member

Choose a reason for hiding this comment

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

I would err on the side of caution here and format the arity explicitly with CultureInfo.InvariantCulture so that users on systems configured for non-English won't potentially get incorrect number formatting (e.g. scenario of 1000 becoming 1,000 or 1.000). Might be some other occurrences of this elsewhere too.

martincostello
martincostello approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

:shipit:

This was referenced Mar 29, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martincostello martincostello martincostello approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[OpenAPI] error CS0308: The non-generic type 'TypeName' cannot be used with type arguments
3 participants
@captainsafia @martincostello @BrennanConroy