Expand back-compat property type preservation to all public model properties#10413
Expand back-compat property type preservation to all public model properties#10413
Conversation
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/33d000a2-d811-4f6c-a72b-359b57cee1a2 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
JoshLove-msft
requested review from
JoshLove-msft,
ShivangiReja,
christothes,
joseharriaga,
jsquire and
m-nash
as code owners
commit: |
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/e014a173-0713-44e4-a0b5-4ce8f896d407 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| - The generator compares property types against the `LastContractView` | ||
| - For read-write lists and dictionaries, if the previous type was different, the previous type is retained | ||
| - The generator compares property types against the `LastContractView`. | ||
| - If the previous type is logically compatible with the new type (same top-level name, with matching generic argument names), the previous type is retained. This covers read-write collection wrapper changes (e.g. `IList<T>` ↔ `IReadOnlyList<T>`) as well as nullability changes on scalars, enums, and models. |
There was a problem hiding this comment.
@copilot this is no longer the case - sync from main
There was a problem hiding this comment.
Could you clarify what you'd like changed here? Line 168 describes the unified rule introduced by this PR (collection wrapper changes + nullability), and main hasn't moved since I last synced it (ab5caa4 is still tip for this file). If you'd prefer I drop the "Implementation Details" bullet list entirely — keeping just the two scenario sections above — happy to do that.
There was a problem hiding this comment.
You're right, sorry — I missed 38a8c7a (the revert of #10319). Merged main in 6275b90 and dropped the AreNamesEqual generic-argument check from the collection branch so it matches main's behavior (override on any inequality). Updated the doc accordingly. Non-collection properties still use the conservative top-level-name-match rule for the new nullability case.
…n back-compat) Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/1cb571b4-94fe-450a-bda0-73a4c98e8f9f Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
|
@copilot sync from main and resolve all merge issues |
…ompat-support # Conflicts: # packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Synced from main in 7c64485. Resolved the conflict in |
| // argument names) to match. This ensures we only override when the types are | ||
| // logically the same (e.g. differ only in nullability) and never when the | ||
| // underlying type has genuinely changed (e.g. string to int). | ||
| if (outputProperty.Type.AreNamesEqual(candidate)) |
There was a problem hiding this comment.
@copilot why wouldn't we just always enforce the existing type in the back compat scenario? Users can work around this with custom code if they need to.
There was a problem hiding this comment.
Good point — done in 86c40d0. TryGetLastContractPropertyTypeOverride now always returns the last contract's type whenever it differs from the new spec, with no collection-vs-scalar or AreNamesEqual branching. Updated the previously-negative test (BackCompat_ScalarPropertyTypeOverriddenWhenTypeNameDiffers) to assert the override now applies even when the top-level name changes (string → int is preserved as string), and rewrote the docs to describe the unconditional behavior. All 1317 generator tests pass.
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/111ae35d-3fde-4326-95a5-11485d35034c Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
…ompat-support Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/ceb10cde-230f-420c-9ac3-076d097c12b6 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…onditional preservation Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/aa2dc7c2-4537-41bc-823e-a6c608afa956 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| /// underlying type, etc.). Users can override this behavior with custom code if they | ||
| /// want the new spec's type instead. | ||
| /// </summary> | ||
| protected internal override IReadOnlyList<PropertyProvider> BuildPropertiesForBackCompatibility(IEnumerable<PropertyProvider> originalProperties) |
There was a problem hiding this comment.
@copilot this needs to run after all visitors have run.
There was a problem hiding this comment.
Moved the call from BuildProperties into ProcessTypeForBackCompatibility in 0f006de — it now runs alongside the existing methods/constructors back-compat passes, after all visitors have completed (per CSharpGen.cs line 79–82). Updated the affected ModelProviderTests to trigger ProcessTypeForBackCompatibility explicitly. All 1421 generator tests + 1311 ClientModel tests pass.
There was a problem hiding this comment.
@copilot Not sure if that is the issue, but seeing a regression where the known azure type property serialization is not applied for nullable AzureLocation.
https://github.com/Azure/azure-sdk-for-net/blob/main/eng/packages/http-client-csharp/generator/Azure.Generator/src/Primitives/KnownAzureTypes.cs
There was a problem hiding this comment.
That's very likely caused by the move. Before, back-compat ran inside BuildProperties and the Azure visitor ran after it, re-stamping outputProperty.Type with the AzureLocation TypeProvider's CSharpType (which carries the Implementation/known-Azure-type metadata that MRW serializer codegen keys off of). Now back-compat runs after the visitor and wholesale replaces outputProperty.Type with lastContractPropertyType from LastContractPropertiesMap — that one is built by reflecting over the previous binary, so it's a "raw" framework CSharpType without the TypeProvider association. The serializer then doesn't recognize it as a known Azure type.
A targeted fix: when lastContractPropertyType and outputProperty.Type have the same underlying name/namespace and only differ in nullability (or collection wrapping), keep the visitor's outputProperty.Type and just adjust nullability/wrapping on it (preserving the Implementation). When they differ in the underlying type (the string→int scenario), fall back to wholesale replacement as today.
Want me to go that route, or do you have a different shape in mind? If you can point me at a small failing test (or the exact property/type pairing), I can verify locally before pushing.
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/4980a671-b6b7-46c0-a820-196ce0d078fa Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
BuildPropertiesForBackCompatibilityvirtual hook onTypeProviderModelProviderback-compat property logic into the new hookBuildPropertiesForBackCompatibilityfromProcessTypeForBackCompatibilityso it runs after all visitorsProcessTypeForBackCompatibilityto trigger property back-compatbackward-compatibility.mdModel Properties section