-
Notifications
You must be signed in to change notification settings - Fork 317
[http-client-csharp] Implement support for external type references (framework types only) #8770
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
Co-authored-by: JoshLove-msft <[email protected]>
Co-authored-by: JoshLove-msft <[email protected]>
|
|
||
| // If the type cannot be resolved as a framework type, create a generic CSharpType | ||
| // using the identity as the fully qualified name | ||
| return new CSharpType(externalType.Identity, isValueType: false, isNullable: false, null, null, null, true); |
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 there are build failures related to this line.
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.
Fixed in commit 74e3f90. The issue was an incorrect call to the CSharpType constructor. I updated the CreateExternalType method to properly parse the fully qualified name and use the internal constructor with all required parameters.
Co-authored-by: JoshLove-msft <[email protected]>
| return new CSharpType(externalType.Identity, isValueType: false, isNullable: false, null, null, null, true); | ||
| // If the type cannot be resolved as a framework type, parse the fully qualified name | ||
| // and create a CSharpType using the internal constructor | ||
| var lastDotIndex = externalType.Identity.LastIndexOf('.'); |
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 this is not going to result in buildable libraries. We should emit a diagnostic saying that an unsupported external type was used and just return null.
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.
Fixed in commit c8a6e4b. Updated CreateExternalType to return null for external types that cannot be resolved as framework types. Also added a diagnostic warning in the TypeScript emitter for non-framework external types, preventing generation of non-buildable code.
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 The diagnostic can be emitted directly from the generator code. We should not add this to the emitter code. We can use the CodeModelGenerator.Instance.Emitter property.
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.
Fixed in commit 3e8bf94. Moved the diagnostic emission from the TypeScript emitter to the C# generator code. The diagnostic is now emitted using CodeModelGenerator.Instance.Emitter.ReportDiagnostic in the CreateExternalType method, and the diagnostic code has been added to DiagnosticCodes.cs.
Co-authored-by: JoshLove-msft <[email protected]>
Co-authored-by: JoshLove-msft <[email protected]>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs
Outdated
Show resolved
Hide resolved
…ator/src/TypeFactory.cs
|
No changes needing a change description found. |
| public const string MissingItemsProperty = "missing-items-property"; | ||
| public const string NoMatchingItemsProperty = "no-matching-items-property"; | ||
| public const string UnsupportedSerialization = "unsupported-serialization"; | ||
| public const string UnsupportedExternalType = "unsupported-external-type"; |
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 this can be removed.
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.
Removed in commit 8e63fa5. The constant was not being used since the diagnostic code is passed as a string literal.
| /// </summary> | ||
| /// <param name="externalType">The <see cref="InputExternalType"/> to convert.</param> | ||
| /// <returns>A <see cref="CSharpType"/> representing the external type, or null if the type cannot be resolved.</returns> | ||
| protected virtual CSharpType? CreateExternalType(InputExternalType externalType) |
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 let's add a few tests that validate the new functionality:
- simulate a model that is decorated with alternateType. The model should be used as a property in a different model.
- simulate a property that is decorated with alternateType;
These should go in ModelProviderTests.
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.
Added in commit fc47d2e. Three tests added to ModelProviderTests.cs:
- TestExternalType_ModelDecoratedWithAlternateType - validates model decorated with @alternateType using System.Uri as property in another model
- TestExternalType_PropertyDecoratedWithAlternateType - validates property decorated with @alternateType using System.Net.IPAddress
- TestExternalType_UnsupportedExternalType - validates unsupported external types emit diagnostics and are skipped
All tests pass successfully.
Co-authored-by: JoshLove-msft <[email protected]>
...ient-csharp/generator/Microsoft.TypeSpec.Generator.Input/src/InputTypes/InputExternalType.cs
Show resolved
Hide resolved
Co-authored-by: JoshLove-msft <[email protected]>
...p/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: JoshLove-msft <[email protected]>
Co-authored-by: JoshLove-msft <[email protected]>
External Types Implementation for http-client-csharp
This PR implements support for external type references in the C# http-client-csharp emitter, enabling the use of the
@alternateTypedecorator withExternalTypeobjects as defined in Azure/typespec-azure#3233.Note: Only .NET framework types are supported as external type references. Non-framework external types will emit a warning diagnostic and be skipped during code generation to ensure buildable libraries.
Changes Made
InputExternalTypeinterface to TypeScript input-type.tsInputTypeunion to includeInputExternalTypeInputExternalTypeC# class to the generatorInputExternalTypeConverterfor JSON deserializationInputTypeConverterto handle "external" kindTypeFactory.CreateCSharpTypeCoreto handleInputExternalTypeCreateExternalTypemethod to create CSharpType from external referencesImplementation Details
Emitter (TypeScript):
InputExternalTypeinterface withidentity,package, andminVersionpropertiesfromSdkTypeto detect when an SDK type has anexternalproperty and convert it toInputExternalTypefromSdkExternalTypehelper function that converts external type information to InputExternalTypeGenerator (C#):
InputExternalTypeclass in Microsoft.TypeSpec.Generator.InputInputExternalTypeConverterfor JSON deserialization of InputExternalType from the TypeScript emitter outputInputTypeConverterto handle "external" kind and route to InputExternalTypeConverterTypeFactory.CreateCSharpTypeCoreswitch statement to handleInputExternalTypeCreateExternalTypemethod that:CreateFrameworkTypeCodeModelGenerator.Instance.Emitter.ReportDiagnosticfor types that cannot be resolvedUsage Example
Framework types like
System.Uri,System.BinaryData,System.Text.Json.JsonElementwill be successfully resolved and referenced in the generated code. Non-framework external types will emit a warning during code generation and be skipped, ensuring that all generated libraries are buildable.Testing
TypeScript Tests (in
type-converter.test.ts):C# Generator Tests (in
ModelProviderTests.cs):ExternalTypeModelUsedAsProperty: Tests a model decorated with@alternateTypethat referencesSystem.Uri, used as a property in another modelExternalTypePropertyIsResolved: Tests a property decorated with@alternateTypereferencingSystem.Net.IPAddressUnsupportedExternalTypeEmitsDiagnostic: Tests that unsupported external types (non-framework) emit diagnostics and are properly skippedDiagnostics
The warning diagnostic
unsupported-external-typeis emitted from the generator code (not the emitter) when:Fixes #8187
Original prompt
Fixes #8187
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.