fix(http-server-csharp): avoid duplicate nullable suffixes - #11900
fix(http-server-csharp): avoid duplicate nullable suffixes#11900sophia-ramsey wants to merge 1 commit into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, includes coverage in both component and end-to-end tests, and only leaves a minor test robustness nit.
Pull request overview
This PR fixes a C# server emitter bug where optional parameters whose TypeSpec type is already a nullable value-type union (e.g. int32 | null) could be emitted with a duplicated nullable suffix (int??) in generated business-logic interfaces and mock implementations.
Changes:
- Added a helper (
getNullableValueTypeUnionInnerType) to detect nullable value-type unions and extract the non-null inner type for special handling. - Updated interface and mock parameter type rendering to avoid double-appending
?for optional nullable value-type unions. - Added targeted tests (component-level and end-to-end) to ensure
int??does not appear in generated signatures.
File summaries
| File | Description |
|---|---|
| packages/http-server-csharp/test/nullable-parameters.test.ts | New end-to-end test validating optional nullable value parameters render with a single ? in both interfaces and mocks. |
| packages/http-server-csharp/src/components/type-expression/type-expression.tsx | Introduces helper for nullable value-type unions and refactors nullable-union rendering to use it. |
| packages/http-server-csharp/src/components/scaffolding/mock-implementations.tsx | Avoids T?? in mock method signatures by stripping nullable unions to their inner value type when also optional. |
| packages/http-server-csharp/src/components/interfaces/interfaces.tsx | Applies the same inner-type logic for optional parameters when building interface method parameters. |
| packages/http-server-csharp/src/components/interfaces/interfaces.test.tsx | Adds a component test asserting the interface signature uses only a single nullable suffix. |
| .chronus/changes/sramsey-csharp-duplicate-nullable-suffixes-2026-09-08.md | Adds a fix changelog entry for @typespec/http-server-csharp. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expect(interfaceContent).toContain("TestAsync(int? value, Choice? choice)"); | ||
| expect(mockContent).toContain("TestAsync(int? value, Choice? choice)"); | ||
| expect(interfaceContent).not.toMatch(/\w+\?\?\s+\w+/); | ||
| expect(mockContent).not.toMatch(/\w+\?\?\s+\w+/); |
| import { beforeEach, expect, it } from "vitest"; | ||
| import { ApiTester, compileAndDiagnose, getStandardService } from "./test-host.js"; | ||
|
|
||
| let tester: Awaited<ReturnType<typeof ApiTester.createInstance>>; |
There was a problem hiding this comment.
should use the actual type I think TesterInstance, alsomight be able to delete that all together see other comment
| }); | ||
|
|
||
| it("emits one nullable suffix for optional nullable value parameters", async () => { | ||
| const [result] = await compileAndDiagnose( |
There was a problem hiding this comment.
Any reason to use this compileAndDiagnose? Is htere not a component you can use to keep the test small?
If not I think just using the EmitterTester.compile should give you a map of all the output files making it at least a little simpler
compileAndDiagnose is a leftover of the old single test file that I want to try to not repeat
| )?.[1]; | ||
|
|
||
| expect(interfaceContent).toContain("TestAsync(int? value, Choice? choice)"); | ||
| expect(mockContent).toContain("TestAsync(int? value, Choice? choice)"); |
There was a problem hiding this comment.
Could we apply the same normalization to controller action parameters too? This spec also emits NullableParametersController.cs, even with "emit-mocks": "mocks-only". ControllerAction still passes a nullable TypeExpression together with optional: true, producing int?? value and Choice?? choice.
This is preexisting, but it means the same reproducer still generates C# that cannot compile after the interface/mock fix. Including the controller path would make the fix complete.
| export function getNullableValueTypeUnionInnerType($: Typekit, type: Type): Type | undefined { | ||
| if (type.kind !== "Union" || isUnionEnum(type)) return undefined; | ||
| const innerType = getNullableUnionInnerType(type); | ||
| return innerType !== undefined && isValueType($, innerType) ? innerType : undefined; |
There was a problem hiding this comment.
This only unwraps one union level, so a nested nullable union still escapes the normalization:
union MaybeInt { int32, null }
interface Example {
test(value?: MaybeInt | null): void;
}Here innerType is the MaybeInt union, which isValueType rejects because it is not a union-enum. However, TypeExpression recursively renders it as int?, and optional-parameter rendering appends another ?, leaving TestAsync(int?? value) in interfaces and mocks.
Could we handle nested nullable unions as well and add regression coverage? This is an existing case of the same bug that the new helper does not yet cover.
This pull request fixes an issue where optional nullable value parameters in generated C# interfaces and mocks would emit duplicate nullable suffixes (e.g.,
int??). The changes ensure that only a single nullable suffix is emitted for such parameters. The update includes logic changes, new helper functions, and additional tests to verify correct behavior.Bug Fix: Prevent Duplicate Nullable Suffixes
int32 | null) are rendered with only one nullable suffix (int?), preventing cases likeint??. [1] [2] [3] [4] [5]Helper Function
getNullableValueTypeUnionInnerTypeto detect and extract the correct inner type for nullable value types in unions, used to avoid double nullable suffixes. [1] [2] [3]Testing
These changes ensure that the generated C# code is correct and idiomatic when handling optional nullable value parameters.