Fix LastContractPropertiesMap overriding collection property type when element type changes#10319
Fix LastContractPropertiesMap overriding collection property type when element type changes#10319
Conversation
…perty type when element type changes Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/6f54fa17-1102-4fb6-8ce6-812c730e37c3 Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
Show resolved
Hide resolved
| out CSharpType? lastContractPropertyType) && | ||
| !outputProperty.Type.Equals(lastContractPropertyType)) | ||
| !outputProperty.Type.Equals(lastContractPropertyType) && | ||
| outputProperty.Type.Arguments.SequenceEqual(lastContractPropertyType.Arguments)) |
There was a problem hiding this comment.
I think it would be more straightforward to use ElementType, though I suppose that wouldn't account for dictionary keys. If we leave it like this we should probably add a comment.
There was a problem hiding this comment.
@copilot could you help to add comment explaining this?
There was a problem hiding this comment.
Added a comment explaining the Arguments.SequenceEqual usage — it covers both list element types and dictionary key/value types, whereas ElementType alone wouldn't account for dictionary keys. See 1ee9866.
|
@copilot could you help to check the CI failures? |
…tionary coverage Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/f0f7bda5-5e09-47cc-83fe-700c60a15190 Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
… vs compiled assembly types Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/fa5a9394-f6b4-4432-aa3f-c3ffd8551f7c Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
The CI workflows show Fixed in |
The backcompat logic in
ModelProvider.BuildProperties()unconditionally overrides a collection property's type to match the last contract when types differ. This was intended to preserveIReadOnlyList<T>→IList<T>wrapper changes, but it also overrides the type when the element type itself has legitimately changed (e.g., via@typeChangedFrom), discarding the intentional type change.Fix
ModelProvider.cs: Added anAreNamesEqual-based argument comparison guard so the override only fires when the element/value types match between old and new — i.e., only the collection wrapper changed. We useAreNamesEqualrather thanSequenceEqual/Equalsbecause the argument types may come from different sources (TypeProvider vs compiled assembly) but represent the same logical type. We compare allArguments(not justElementType) to cover both list element types and dictionary key/value types.ModelProviderTests.cs: AddedBackCompat_CollectionPropertyTypeNotOverriddenWhenElementTypeChanges— last contract hasIList<IDictionary<string, BinaryData>>, new model producesIList<NewElementModel>, asserts the new element type is preserved.MockInputModel.cssimulating last contract with the old element types.