Repository navigation
[http-client-csharp] Support accessibility customization through partial constructors - #12528
Jorge Rangel (jorgerangel-msft) with Copilot wants to merge 12 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>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
commit: |
There was a problem hiding this comment.
🟡 Changes recommended
Custom constructor parameter names are not preserved exactly, so distinct identifiers such as first_name and firstName can collapse into duplicate names and generate uncompilable C#.
3 open issues
What changed in this PR
Extends the C# client generator’s partial-member customization to C# 14 instance constructors, allowing authors to change accessibility while retaining generated implementations.
Changes:
- Recognizes and merges partial constructor declarations, preserving generated bodies, initializers, and metadata.
- Clones parameters and scopes name bindings to isolate customizations; preserves partial accessibility during compatibility processing.
- Upgrades Roslyn Workspaces to 5.0 and adds parsing, matching, and output tests.
Paths below are relative to packages/http-client-csharp/generator/.
| File | Description |
|---|---|
Packages.Data.props |
Upgrades Roslyn for C# 14 constructor parsing. |
Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs |
Tests constructor merging, accessibility, and matching. |
Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomPartialConstructorsPreserveImplementation/CustomPartialConstructorType.cs |
Supplies partial declarations with custom names and defaults. |
Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomPartialConstructorsPreserveImplementation(True).cs |
Baselines expression-bodied constructor output. |
Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomPartialConstructorsPreserveImplementation(False).cs |
Baselines block-bodied constructor output. |
Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomPartialConstructorsOnlyMergeMatchingDeclarations/CustomPartialConstructorType.cs |
Supplies unmatched, replacement, and suppression cases. |
Microsoft.TypeSpec.Generator/test/Providers/NamedTypeSymbolProviders/TestData/NamedTypeSymbolProviderTests/ValidatePartialConstructorDeclarations/WithPartial.cs |
Supplies constructor declaration and implementation variants. |
Microsoft.TypeSpec.Generator/test/Providers/NamedTypeSymbolProviders/NamedTypeSymbolProviderTests.cs |
Tests detection of bodyless partial constructors. |
Microsoft.TypeSpec.Generator/src/Writers/CodeWriter.cs |
Scopes parameter bindings and registers parameter declarations. |
Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs |
Centrally merges partial constructors and preserves accessibility. |
Microsoft.TypeSpec.Generator/src/Providers/NamedTypeSymbolProvider.cs |
Recognizes partial constructor declarations. |
Microsoft.TypeSpec.Generator/src/Providers/ConstructorProvider.cs |
Tracks original parameters for scoped bindings. |
Review effort: Balanced
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Jorge Rangel (jorgerangel-msft)
left a comment
There was a problem hiding this comment.
Copilot there's test and regen failures. For each commit, we should rerun tests and rerun Generate.ps1
…customization Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because constructor matching can conflate distinct parameter passing modes, while accessibility changes can discard existing constructor bodies and initializers.
No open issues 🎉
Review effort: Balanced
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Partial constructor generation can fail compilation due to mismatched explicit accessibility, while compatibility restoration can emit duplicate constructors or lose collection initialization.
2 open issues
Review effort: Balanced
|
No changes needing a change description found. |
Roslyn 5.9 supports partial constructors in its default C# language version, so remove the explicit Preview parsing added for Roslyn 4.14. Restore the nullable assertion required by the simplification workaround. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Constructor restoration incorrectly replaces generated parameterless constructors customized to internal or private, discarding their initialization logic and leaving optional collection properties null.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
JoshLove01
left a comment
There was a problem hiding this comment.
Reviewed the generator changes (partial ctor detection, merge in FilterCustomizedConstructors, CodeWriter default omission, back-compat exclusions, Roslyn 5.9 bump plus the simplifier workaround). The core merge is clean, and the test matrix (expression body, shared params, back-compat restore, non-matching types) is good. Two inline findings concern places outside the merge that still see the partial ctor as "custom".
Other notes:
- PR description is stale. It still says the merge adopts "custom accessibility and parameter names" and "clone[s] shared parameters and scope name bindings". After b987178, only accessibility is merged and the generated parameters are reused unchanged. Please update the description so reviewers and the changelog aren't misled.
- Roslyn 4.8 → 5.9 is a significant dependency change bundled into a feature PR. Downstream generators (Azure, mgmt) consume
Microsoft.TypeSpec.Generatorand will pick up Roslyn 5.9 transitively. Your own numbers show about 18% slower end-to-end generation (29.2s → 34.4s), from JIT/startup cost. Theextension→@extensionescaping (now a contextual keyword) will also churn generated output in every library with a parameter namedextension, though it is not source-breaking since named arguments still bind. Consider splitting the bump plus simplifier workaround into its own PR so it can be evaluated and reverted independently. - Parameter-name mismatches between the defining declaration and the generated implementation trigger CS8826 in the customer's project. That is a warning, but it becomes an error under
TreatWarningsAsErrors. That is probably acceptable, but worth documenting for users of this customization.
--generated by Copilot
| MethodSignatureBase.SignatureComparer.Equals(partial.Signature, constructor.Signature)); | ||
| if (matchingPartial != null) | ||
| { | ||
| constructors.Add(CreatePartialConstructorFromCustomSignature(matchingPartial.Signature, constructor)); |
There was a problem hiding this comment.
Returning a new ConstructorProvider instance here breaks reference identity with ModelProvider.FullConstructor. ModelFactoryProvider.GetBinaryDataParamAndFullCtorForFactoryMethod (ModelFactoryProvider.cs:847) checks !modelProvider.Constructors.Contains(fullConstructor) to detect a suppressed/replaced full ctor. So when a user only changes the full ctor's accessibility via a partial declaration, the factory now goes down the custom-constructor path. It then searches CanonicalView.Constructors for an Internal ctor with at least as many params. If the partial made it public, that search can pick a different handwritten internal ctor, if one exists, as the factory target.
Suggest either keeping the factory check signature-based, or treating the merged ctor as the full ctor (e.g., Constructors.Any(c => c == fullConstructor || SignatureComparer.Equals(c.Signature, fullConstructor.Signature) && c.BodyStatements == fullConstructor.BodyStatements ...)). A model-factory test with a partial-customized full constructor would also help.
--generated by Copilot
| var modifiers = GetAccessModifier(constructorSymbol.DeclaredAccessibility); | ||
| if (constructorSymbol.MethodKind == MethodKind.Constructor | ||
| && constructorSymbol.PartialImplementationPart is null | ||
| && IsPartialDeclaration(constructorSymbol)) |
There was a problem hiding this comment.
With this flag, the bodyless defining declaration still stays in CustomCodeView.Constructors. Every consumer outside FilterCustomizedConstructors/ModelProvider back-compat still treats it as a handwritten constructor:
ClientSettingsProvider(lines 117, 185) andConfigurationSchemaGenerator(line 122) read public custom constructors to add settings properties, schema entries, orUnsupportedClientSettingsParameterdiagnostics. A client that usespublic partialto expose an internal generated ctor (e.g., one taking anAuthenticationPolicy/pipeline) would get spurious settings/schema entries or warnings.CanonicalTypeProvider.BuildConstructorsemits both the merged ctor and the partial declaration.
Consider having these consumers skip Partial custom constructors, as ModelProvider now does, or excluding matched partial declarations from the custom view.
--generated by Copilot


Generated methods support accessibility changes through bodyless partial declarations without replacing their implementation. Extend that customization to C# 14 instance constructors.
global::qualifications when it removes a cast that gives a conditional expression its type (dotnet/roslyn#86393). This affects Azure settings constructors, e.g.useKey ? new global::Azure.Core.AzureKeyCredentialPolicy(...) : new global::Azure.Core.Pipeline.BearerTokenAuthenticationPolicy(...); the issue reports 11 changed files across 8 Azure SDK clients.null/defaultcasts). Output matches Roslyn 4.8.disable-roslyn-reduceskips both passes. A TODO tracks removing this once Roslyn ships a fix.Example customization:
The matching generated implementation becomes
public partial, retaining its generated API-version fallback and body without repeating the optional default.Performance impact
Measured on a shared Windows development machine with .NET 10, using Sample-TypeSpec (239 generated files). Timings vary between runs, so treat them as indicative.
Workaround cost: Roslyn simplification only, warm process, 18 samples across 3 alternating runs
End-to-end generation: real generator on Sample-TypeSpec, fresh process per run, 6 alternating pairs
Previewparse option. A separate comparison of C# 12, C# 14, andPreviewparse options found no consistent timing difference and identical output.Spector scenarios: three Spector projects selected at random (seed
12528), full client generation withScmCodeModelGenerator, this PR's current head, fresh process per run, 6 alternating pairs eachhttp/payload/pageablehttp/versioning/removed/v1http/parameters/spreadValidation
special-wordswere regenerated again with no diffs.