Restore removed extensible enum members via back compat - #11502
Restore removed extensible enum members via back compat#11502jorgerangel-msft with Copilot wants to merge 17 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
commit: |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
jorgerangel-msft
left a comment
There was a problem hiding this comment.
@copilot I'd like to see some unit tests where custom code is used in combination with there being a last contract. In addition, we have some spell check failures. We should address those by simply changing / removing the word(s) causing the issues
…ests Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Both addressed in 0ae951a:
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the C# HTTP client generator’s back-compat behavior for extensible (string-backed) enums by restoring members that were removed from the current spec but still exist in the last shipped contract, recovering their original wire values from <Member>Value const fields (including from compiled assembly metadata).
Changes:
- Expand constant-value recovery in
NamedTypeSymbolProviderso extensible-enum<Member>Valuefields can be read from the last contract. - Add extensible-enum-specific back-compat logic to re-add removed members (while honoring custom code and ApiCompat baselines).
- Update back-compat processing, visitor access, tests/fixtures, and documentation to support and validate the new behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/NamedTypeSymbolProviders/NamedTypeSymbolProviderTests.cs | Adds tests validating const-field initializer recovery and non-const behavior. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueReadded/MockInputEnum.cs | Adds last-contract fixture for re-adding an extensible enum member. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueReadded.cs | Adds expected generated output for the “re-added extensible enum member” scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenProvidedByCustomCode(Last)/MockInputEnum.cs | Adds last-contract fixture for the “custom code provides removed member” scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenProvidedByCustomCode(Custom)/MockInputEnum.cs | Adds custom-code fixture to ensure generator doesn’t collide with custom-provided members. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenProvidedByCustomCode.cs | Adds expected generated output for the “custom code prevents re-add” scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenBaselineAccepts/MockInputEnum.cs | Adds last-contract fixture for baseline-accepted removals. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenBaselineAccepts.xml | Adds XML baseline suppression fixture for accepted member removals. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenBaselineAccepts.txt | Adds text baseline fixture for accepted member removals. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumRemovedValueNotReaddedWhenBaselineAccepts.cs | Adds expected generated output ensuring accepted removals are not re-added. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumCustomCodeMemberPreservedWhileOtherMemberRestored(Last)/MockInputEnum.cs | Adds last-contract fixture for mixed custom-code + restored-member scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_ExtensibleEnumCustomCodeMemberPreservedWhileOtherMemberRestored(Custom)/MockInputEnum.cs | Adds custom-code fixture to ensure custom-owned member is not regenerated. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs | Adds end-to-end tests covering re-add, baseline-accepted removal, and custom-code interactions. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/OutputLibraryVisitorTests.cs | Updates test visitor override access modifier to match LibraryVisitor change. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs | Rebuilds extensible-enum fields/properties from updated enum values and re-visits only newly restored members. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/NamedTypeSymbolProvider.cs | Broadens const-field initializer recovery beyond enum types. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ExtensibleEnumProvider.cs | Implements extensible-enum back-compat member restoration and wire-value recovery from last-contract const fields. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/EnumProvider.cs | Exposes custom member name set to derived providers for back-compat filtering. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/LibraryVisitor.cs | Widens VisitProperty accessibility to allow back-compat-added properties to be visited. |
| packages/http-client-csharp/generator/docs/backward-compatibility.md | Documents the new extensible-enum member restoration behavior and scenario. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/LibraryVisitor.cs:303
- Changing
VisitPropertytoprotected internalrequires updating all overrides to match (overrides cannot reduce accessibility). At leastMicrosoft.TypeSpec.Generator.ClientModel.StubLibrary/src/StubLibraryVisitor.csstill declaresprotected override VisitProperty, which will fail to compile after this change.
/// <summary>
/// Visits a <see cref="PropertyProvider"/> and returns a possibly modified version of it.
/// </summary>
/// <param name="property">The original <see cref="PropertyProvider"/>.</param>
/// <returns>Null if it should be removed otherwise the modified version of the <see cref="PropertyProvider"/>.</returns>
protected internal virtual PropertyProvider? VisitProperty(PropertyProvider property)
{
return property;
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/NamedTypeSymbolProvider.cs:256
GetFieldInitializerstill requiresfieldSymbol.ConstantValue != null, soconst string Foo = null;(whereHasConstantValueis true) won’t have its initializer recovered. This blocks recovering null consts and contradicts the goal of recovering anyconstvalue.
private static LiteralExpression? GetFieldInitializer(IFieldSymbol fieldSymbol)
{
return fieldSymbol.HasConstantValue &&
fieldSymbol.ConstantValue != null &&
LiteralExpression.TryCreate(fieldSymbol.ConstantValue, out var initializer)
? initializer
: null;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ExtensibleEnumProvider.cs:286
BuildEnumValuesForBackCompatibilityiterates allLastContractView.Properties. Since the workspace now usesMetadataImportOptions.AllandNamedTypeSymbolProviderdoesn’t filter by accessibility, this can include non-public or non-enum-member properties and may lead to attempting to resurrect members that were never part of the public API surface.
var lastContractProperties = LastContractView?.Properties;
if (lastContractProperties == null || lastContractProperties.Count == 0)
{
return null;
}
jorgerangel-msft
left a comment
There was a problem hiding this comment.
@copilot there are unexpected changes in this regen pr from our changes https://github.com/Azure/azure-sdk-for-net/pull/61689/changes#diff-c784106df58578cd614bde7699d50b31bce5da3ca7455874c64f33e92c4fde02. It seems we are applying back compat from some non public types. We need to ensure we only scope the param name back compat to public accessible members
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Addressed in 8ba57e4. Parameter-name restoration from the last contract is now gated to public API surfaces, so internal protocol/create-request paths and convenience methods made internal by non-public parameter types keep their current generated names. Added regression tests for both cases. |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Extensible enums that dropped members between versions produced a source-breaking change for consumers. Unlike fixed string enums (whose wire value can't be recovered), extensible enum members are backed by a private
const <Member>Valuefield whose value is recoverable from the last contract — including compiled-assembly metadata — so removed members can be re-added, mirroring the existing integer fixed-enum behavior.Changes
NamedTypeSymbolProvider.GetFieldInitializer— recover the constant value of anyconstfield (previously only enum members), so extensible-enum<Member>Valuewire values load from the last contract.ExtensibleEnumProvider.BuildEnumValuesForBackCompatibility(new override) — re-add members present in the last contract but absent from the current spec, restoring the wire value from the const field. Skips members already present, supplied by custom code, or whose removal is accepted in the ApiCompat baseline; restored members are appended after current spec members.TypeProvider.ProcessTypeForBackCompatibility— for extensible enums, rebuild fields (preserving the_valuebacking field) and properties from the updated members, reusing already-visited property instances and running only restored properties through the visitors.LibraryVisitor.VisitProperty— widenedprotected→protected internal(consistent withVisitField/VisitConstructor) so restored properties can be visited; overrides updated.backward-compatibility.md.Behavior
Given a last contract that still declares a member the current spec removed:
the member is restored (property + const field); serialization is unaffected since it round-trips through
_value. Enums with no removed members are unchanged.