You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[http-client-csharp] Restore constructors for inherited optional properties - #12453
The constructor-restoration lookup previously considered only properties declared on the current model. When AIProjectDataset.DataUri became optional, this prevented restoring the published FileDataset(Uri dataUri) and FolderDataset(Uri dataUri) constructors.
Include inherited, assignable wire-backed properties, respecting member hiding and inaccessible setters.
Retain the current parameterless constructor and its initialization, then assign inherited properties through their setters instead of accessing private base-class backing fields.
Retain ordinary required-reference null validation. Nullable-reference annotation compatibility is outside this change because the generator does not currently support nullable reference types.
Reject nullable value-type assignments to nonnullable properties or local backing fields while preserving safe nullable-value assignments.
Preserve nested CLR value types when matching restored parameters, selecting chaining constructors, and checking backing-field assignments. A published List<int?> parameter must not silently become List<int> or vice versa.
Check mapped external properties against their actual CLR setter and type. Resolve closed generic arguments and ignore enum serialization markers when comparing assignment types, without weakening setter access or nested value-nullability checks.
Add dataset output baselines and positive/negative regressions for member hiding, accessibility, value-type signature/assignment safety, and mapped external properties.
Validation
All 6,075 generator/Input/ClientModel unit tests passed.
Six new cases cover nested nullable-value signature changes in both directions, incompatible local backing fields, and compatible/incompatible chained parameters. Unsafe cases reproduced the bug before the fixes; positive cases retain the published collection parameter type.
Earlier regressions cover compatible closed-generic and extensible-enum constructors, unsafe value assignments, and getter-only/missing external CLR properties, with public/protected/inherited setter cases.
Generator solution build, C# whitespace formatting, emitter build, API extraction, and Cop checks passed.
Inheritance test libraries regenerated successfully with no generated-output changes.
Earlier emitter tests: 411 passed, 2 skipped.
Full library regeneration was previously blocked by the unchanged response/status-code-range emitter scenario with TypeError: Cannot read properties of undefined (reading 'getFeatureLifecycleDetails') before C# generator invocation. npm run lint is unavailable in this checkout because eslint is not installed; the C# Cop checks passed.
This PR restores published C# model constructors when inherited properties become optional, addressing the dataset constructor compatibility regression.
Changes:
Extends constructor restoration to inherited, assignable wire-backed properties.
Chains restored overloads through existing constructors and assigns inherited properties through setters.
Adds compatibility baselines and regression tests for inherited properties, null validation, and nonassignable members.
When a required url | null property becomes optional, this adds AssertNotNull even though its published constructor accepted null. Normal generated contracts use #nullable disable and omit ? on reference types (Writers/CodeWriter.cs:679,979), so the annotation check cannot recover the null acceptance established by ParameterProvider.cs:310–313. Preserve explicit spec nullability or historical validation for these contracts, and add a regression using generated nullable-disabled source.
Addressed the nullable-disabled constructor issue from the latest review in 002a147. Restoration now also checks explicit TypeSpec nullability for reference-type properties, so a required url | null property becoming optional does not introduce a null check just because the published C# contract used #nullable disable.
Added a nullable-disabled contract fixture and generated-output baseline. The new test failed before the fix; both the annotated and nullable-disabled overloads now preserve null acceptance, while FileDataset and FolderDataset still validate their nonnullable URI inputs.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
An optional nullable property can cause a published constructor’s explicitly nonnullable Uri parameter to lose its null validation, requiring nullable-annotation preservation and regression coverage.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because compatibility matching ignores nullability and can generate a nullable parameter assignment to a non-nullable property that fails compilation.
I don't see a reference-type compilation failure in this change. Generated files use #nullable disable (CodeWriter.cs:979), and reference types do not get a nullable suffix (CodeWriter.cs:679). The NullableDataset baseline therefore emits Uri dataUri and DataUri = dataUri, not a Uri? declaration that would produce a nullable-reference assignment warning. Retaining the historical null acceptance is intentional.
I have not changed the implementation based on this general concern. A concrete failing value-type or custom-code example would help distinguish a real conversion error from nullable-reference metadata that is not emitted here.
Removed the nullable-reference compatibility additions in f284cd0 as requested: Roslyn annotation metadata, the explicit-spec-nullability fallback, and the three associated fixture/baseline cases. The generator does not currently support nullable reference types, so these are outside the scope of this fix. Inherited constructor restoration, member-hiding/accessibility safeguards, and ordinary required-reference null validation remain intact.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because constructor restoration can assign a nullable value-type parameter to a nonnullable property, causing generated C# to fail compilation.
Addressed the nullable value-type concern in review 5724315623 with cd977d4. A published constructor such as Counter(int? count) can initialize an int property using count.GetValueOrDefault(). Restoration was replacing that conversion with a direct Count = count assignment, which is invalid C#.
The generator now skips restoration when a nullable value-type parameter cannot be assigned to the actual destination property or local backing field. Added regression cases for local and inherited properties, a nonnullable local backing field, safe nullable assignments, and an inherited private field that must be ignored in favor of the setter.
This is limited to nullable value types; nullable-reference annotation handling remains removed as requested.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed to evaluate a critical compilation risk from generated constructors assigning to getter-only properties on mapped external types.
Addressed the mapped external getter-only property concern in review 5724472173 with 52bd634. I reproduced restoration of MessageException(string message) against an externally mapped System.Exception: the input property metadata looked settable, but Exception.Message is getter-only, so the restored assignment was invalid. A nonexistent external property was also incorrectly accepted.
Restored constructors now verify the actual CLR property and setter using the existing mapped-member lookup. Getter-only, missing, inaccessible, static, init-only, indexed, or incompatible properties are not assigned. Public, protected, and inherited setters remain supported. Added seven positive/negative regression cases, including nullable value-type assignment to a mapped nonnullable property.
Nullable-reference compatibility remains outside the change, as requested.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because nested generic nullability differences can prevent a rebuilt constructor overload from preserving its published signature.
Addressed the nested generic nullability concern from review 5724824484 in 4670a1d. I reproduced published List<int?> parameters being rebuilt as List<int> (and the reverse), plus an incompatible local backing-field assignment. A related test also exposed the same signature change for parameters forwarded to the current constructor.
Restoration now compares CLR types recursively when matching extra parameters and selecting/forwarding chained parameters, and checks nested argument types against the actual assignment destination. Incompatible overloads are skipped rather than silently changing the published signature or emitting invalid assignments. Compatible nullable-value collections and the earlier closed-generic/enum cases remain supported. Global parameter equality is unchanged.
Added six positive/negative regression cases; the unsafe cases failed before their fixes. All 6,075 unit tests passed, the generator built without warnings, and inheritance regeneration produced no output changes. Emitter build, API extraction, formatting, and Cop checks also passed.
This is only nullable value-type CLR compatibility. Nullable-reference annotation handling remains removed as requested.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
No active review issues remain for the constructor-compatibility changes.
No open issues 🎉
Review effort: Balanced
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the constructor compatibility regression reported in Azure/azure-sdk-for-net#64402.
The constructor-restoration lookup previously considered only properties declared on the current model. When
AIProjectDataset.DataUribecame optional, this prevented restoring the publishedFileDataset(Uri dataUri)andFolderDataset(Uri dataUri)constructors.List<int?>parameter must not silently becomeList<int>or vice versa.Validation
Full library regeneration was previously blocked by the unchanged
response/status-code-rangeemitter scenario withTypeError: Cannot read properties of undefined (reading 'getFeatureLifecycleDetails')before C# generator invocation.npm run lintis unavailable in this checkout becauseeslintis not installed; the C# Cop checks passed.