-
Notifications
You must be signed in to change notification settings - Fork 5k
[Azure Mgmt Generator] Investigate :C format specifier for cref generation with generic type error reproduction #51869
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
base: main
Are you sure you want to change the base?
Conversation
…tags and class keyword Co-authored-by: ArcturusZhang <[email protected]>
@@ -16,7 +16,7 @@ internal static class ConstructorProviderHelpers | |||
public static ConstructorProvider BuildMockingConstructor(TypeProvider enclosingType) | |||
{ | |||
return new ConstructorProvider( | |||
new ConstructorSignature(enclosingType.Type, $"Initializes a new instance of {enclosingType.Name} for mocking.", MethodSignatureModifiers.Protected, []), | |||
new ConstructorSignature(enclosingType.Type, $"Initializes a new instance of the <see cref=\"{enclosingType.Name}\"/> class for mocking.", MethodSignatureModifiers.Protected, []), |
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.
please use the format specifier :C
to generate crefs.
We could use :C
when the argument is a System.Type or CSharpType.
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 attempted to implement the :C
format specifier as requested, but encountered an issue with generic types. When using {enclosingType.Type:C}
, the generator crashes for generic types like MgmtTypeSpecArmOperation<T>
with the error "TypeProvider doesn't support generics" at CSharpType.GetGenericTypeDefinition()
.
Could you clarify how to handle generic types with the :C
format specifier? Should we:
- Use a conditional approach to detect generic types and fall back to manual formatting?
- Is there a different way to use
:C
with generic types? - Or should we use
:C
only for non-generic types?
I've reverted to the manual approach for now to maintain functionality while waiting for guidance.
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.
@copilot could you do it any way so that I could see the error message and find if there is a workaround of it?
If there is a bug on this API, then we could first put aside the topic of other ctors
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've implemented the :C
format specifier as requested (commit e1d17fb). The error with generic types is now reproduced:
TypeProvider doesn't support generics.
at Microsoft.TypeSpec.Generator.Primitives.CSharpType.GetGenericTypeDefinition()
at Microsoft.TypeSpec.Generator.CodeWriter.AppendTypeForCRef(CSharpType type)
The issue occurs when the :C
formatter calls AppendTypeForCRef
which then calls GetGenericTypeDefinition()
on generic types like MgmtTypeSpecArmOperation<T>
. This method is not supported for generics.
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.
@copilot please resolve the comments.
Please also double confirm the diff in the issue - I think other ctor also might need to fix their xml doc
I've confirmed that there are indeed other constructor patterns that need to be reviewed:
The RestOperations constructors seem to follow an inconsistent pattern. Should these also be updated to include |
Co-authored-by: ArcturusZhang <[email protected]>
This PR investigates using the
:C
format specifier for generating<see cref="..."/>
tags in XML documentation for mocking constructors, as requested in code review feedback. However, the implementation reveals a fundamental issue with generic types.The Problem:
When using
{enclosingType.Type:C}
instead of manually constructing<see cref="{enclosingType.Name}"/>
, the generator crashes for generic types with the error:Current Implementation:
Error Details:
The
:C
format specifier callsAppendTypeForCRef
which attempts to callGetGenericTypeDefinition()
on generic types likeMgmtTypeSpecArmOperation<T>
. This method is not supported for generics in the current TypeProvider implementation.Next Steps:
This implementation exposes the error as requested in code review to help determine if there's a workaround for the generic type limitation or if a conditional approach is needed.
Related to #51868.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.