Skip to content

Fix C# compat discriminator constructor parameters#11051

Open
ArcturusZhang wants to merge 2 commits into
microsoft:mainfrom
ArcturusZhang:fix/csharp-discriminator-api-compat
Open

Fix C# compat discriminator constructor parameters#11051
ArcturusZhang wants to merge 2 commits into
microsoft:mainfrom
ArcturusZhang:fix/csharp-discriminator-api-compat

Conversation

@ArcturusZhang

Copy link
Copy Markdown
Member

Fixes #10996

Summary

  • omit required discriminator properties from public initialization constructors when ApiCompat preserves a discriminator base model as concrete
  • keep the internal full constructor shape unchanged for serialization/back-compat paths
  • add a regression test for a concrete last-contract discriminator base

Validation

  • dotnet test packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Microsoft.TypeSpec.Generator.Tests.csproj --filter FullyQualifiedName~BackCompat_ConcreteDiscriminatorBaseDoesNotExposeDiscriminatorConstructorParameter --no-restore
  • dotnet test packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Microsoft.TypeSpec.Generator.Tests.csproj --filter "FullyQualifiedNameBackCompat_AbstractTypeConstructorAccessibility|FullyQualifiedNameBackCompat_NonAbstractTypeIsRespected|FullyQualifiedName~BackCompat_ConcreteDiscriminatorBaseDoesNotExposeDiscriminatorConstructorParameter" --no-restore
  • npm ci
  • npm run build
  • npm run test:generator
  • npm run format
  • npm run lint
  • npm run cop
  • pwsh ./eng/scripts/Generate.ps1
  • npm run test:emitter

…tors

Co-authored-by: Copilot <223556219+Copilot@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 Jun 23, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jun 23, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@11051

commit: 20925ae

@github-actions

Copy link
Copy Markdown
Contributor

No changes needing a change description found.

@JoshLove-msft

Copy link
Copy Markdown
Contributor

Review

Summary

The fix is well-scoped — ShouldOmitDiscriminatorParameterFromPublicConstructor only triggers for concrete back-compat discriminator bases (guarded by the IsAbstract logic), correctly excludes multi-level intermediate discriminators via the DiscriminatorValue is null check, and preserves the internal FullConstructor for serialization. Build is clean and the discriminator + back-compat test suites pass.

🔴 Blocking issue: derived models no longer compile

The fix removes the discriminator parameter from the base public constructor but does not update the base(...) call generated in derived models. Generating both the base and a derived model in this PR''s exact scenario produces:

public partial class BaseModel {
    public BaseModel(string baseProp) { ... }                                     // 1 param
    internal BaseModel(KindEnum kind, string baseProp, IDictionary<...> raw) {...} // 3 params
}

public partial class DerivedModel : BaseModel {
    public DerivedModel(string baseProp, int derivedProp)
        : base(KindEnum.One, baseProp)   // ❌ 2 args — matches NEITHER base ctor
}

base(KindEnum.One, baseProp) (2 args) matches neither the 1-param public nor the 3-param internal base constructor → compile error (CS1729).

This is exactly the issue''s real-world case: MetricAlertCriteria / MultiMetricCriteria have derived types, so the generated SDK would fail to compile. The new regression test doesn''t catch this because it inspects BaseModel in isolation and never builds/inspects a derived model''s base(...) call.

Recommendations

  1. When the base public ctor drops the discriminator, the derived model''s initializer must call the matching base constructor (e.g. base(baseProp)) and set the discriminator value separately. The derived base(...) construction at ModelProvider.cs:982–1012 / GetExpressionForCtor needs to account for the omitted base discriminator parameter.
  2. Add a regression test that generates a derived model of a concrete back-compat discriminator base and asserts (ideally compiles) that the emitted base(...) call matches an existing base constructor.

The intent and scoping are right, but as written it trades a public-API issue for a compilation break whenever the concrete discriminator base has derived types — the common case.

--generated by Copilot

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 exposes discriminator parameter on concrete model due to ApiCompatVersion

3 participants