Skip to content

Fix LastContractPropertiesMap overriding collection property type when element type changes#10319

Draft
Copilot wants to merge 4 commits intomainfrom
copilot/fix-lastcontractpropertiesmap-collection-type
Draft

Fix LastContractPropertiesMap overriding collection property type when element type changes#10319
Copilot wants to merge 4 commits intomainfrom
copilot/fix-lastcontractpropertiesmap-collection-type

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 9, 2026

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 preserve IReadOnlyList<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 an AreNamesEqual-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 use AreNamesEqual rather than SequenceEqual/Equals because the argument types may come from different sources (TypeProvider vs compiled assembly) but represent the same logical type. We compare all Arguments (not just ElementType) to cover both list element types and dictionary key/value types.
if (LastContractPropertiesMap.TryGetValue(outputProperty.Name,
        out CSharpType? lastContractPropertyType) &&
    !outputProperty.Type.Equals(lastContractPropertyType) &&
    outputProperty.Type.Arguments.Count == lastContractPropertyType.Arguments.Count &&
    outputProperty.Type.Arguments.Zip(lastContractPropertyType.Arguments).All(
        pair => pair.First.AreNamesEqual(pair.Second)))
  • ModelProviderTests.cs: Added BackCompat_CollectionPropertyTypeNotOverriddenWhenElementTypeChanges — last contract has IList<IDictionary<string, BinaryData>>, new model produces IList<NewElementModel>, asserts the new element type is preserved.
  • Test data: MockInputModel.cs simulating last contract with the old element types.

…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>
@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Apr 9, 2026
Copilot AI changed the title [WIP] Fix lastContractPropertiesMap collection property type override Fix LastContractPropertiesMap overriding collection property type when element type changes Apr 9, 2026
Copilot AI requested a review from live1206 April 9, 2026 02:12
out CSharpType? lastContractPropertyType) &&
!outputProperty.Type.Equals(lastContractPropertyType))
!outputProperty.Type.Equals(lastContractPropertyType) &&
outputProperty.Type.Arguments.SequenceEqual(lastContractPropertyType.Arguments))
Copy link
Copy Markdown
Contributor

@JoshLove-msft JoshLove-msft Apr 9, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot could you help to add comment explaining this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@live1206
Copy link
Copy Markdown
Contributor

live1206 commented Apr 9, 2026

@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>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot could you help to check the CI failures?

The CI workflows show action_required (pending approval) with no jobs executed yet. However, I found an actual test failure locally: BackCompat_ReadOnlyCollectionEnumPropertiesAreRetained was failing because Arguments.SequenceEqual uses CSharpType.Equals which does full structural comparison — the ElementEnum from the current TypeProvider and from the compiled last contract assembly have different internal _type fields, so they don't match structurally even though they represent the same logical type.

Fixed in e5061a8 by replacing SequenceEqual with AreNamesEqual-based argument comparison, which correctly matches types across different sources by their fully-qualified names. All 29 BackCompat tests pass now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C# Generator: LastContractPropertiesMap overrides collection property type even when element type changes

3 participants