Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 10, 2025

Summary

This PR fixes the issue where TCGC treats response types with no body as nullable, making it impossible to distinguish between:

  1. A response with a nullable body ({@body body: Type | null})
  2. Multiple responses where some have bodies and some don't (Type | NoContentResponse)

Changes

  • Add optional property to SdkMethodResponse interface in interfaces.ts
  • Update getSdkMethodResponse function in methods.ts to set optional correctly:
    • true when there is a response without body
    • false when all responses have body
    • undefined when there is no response type
  • Remove the nullable wrapping logic for responses without bodies
  • Update existing tests to expect optional property instead of nullable type
  • Add new test to verify the behavior distinguishes between Type | null and responses without body
  • Update comment for nullable response type handling in paging/LRO logic
  • Run full test suite - all 1016 tests pass
  • Format code with pnpm format
  • Add changeset with migration instructions

Breaking Change

⚠️ This is a breaking change. Code generators consuming TCGC output must be updated to check the new optional property when handling responses that may not have a body.

Migration Guide

Before:

// Both cases returned nullable type
if (method.response.type?.kind === "nullable") {
  // Could be either:
  // 1. Actual nullable body: {@body body: Widget | null}
  // 2. Optional response: Widget | NoContentResponse
}

After:

// Check optional property to distinguish the cases
if (method.response.optional === true) {
  // Response may not have a body (e.g., Widget | NoContentResponse)
  // type is the actual response type (not wrapped in nullable)
} else if (method.response.type?.kind === "nullable") {
  // Actual nullable body type (e.g., {@body body: Widget | null})
  // optional will be false in this case
}
// optional is undefined when there's no response type at all

Property Values

The optional property on SdkMethodResponse has three possible values:

  • true: At least one HTTP response has no body (e.g., Widget | NoContentResponse)
  • false: All HTTP responses have a body (e.g., {@body body: Widget | null})
  • undefined: No response type exists (e.g., void or NoContentResponse only)

Examples

Case 1: Nullable body type

op withNullableBody(): {@body body: Widget | null};

Result: type.kind === "nullable", optional === false

Case 2: Optional response (no body in some cases)

op withOptionalResponse(): Widget | NoContentResponse;

Result: type.kind === "model", optional === true

Case 3: No response type

op deleteOperation(): NoContentResponse;

Result: type === undefined, optional === undefined

Test Results

✅ All 1016 tests pass (2 skipped)
✅ No regressions detected
✅ New test validates the distinction between nullable body and optional response

Fixes #3302

Original prompt

This section details on the original issue you should resolve

<issue_title>[TCGC] The response type nullable generated by tcgc is not as expected</issue_title>
<issue_description>typespec link: https://github.com/Azure/azure-rest-api-specs/blob/1520fd083bdd17bc5bf5edc13f6d7c35e0845b73/specification/hardwaresecuritymodules/resource-manager/Microsoft.HardwareSecurityModules/HardwareSecurityModules/CloudHsmCluster.tsp#L173

Playground

Image

/cc @MaryGao

TCGC currently treats response with no body as nullable type. This behavior is not accurate since it could not distinguish with body with Type | null. The solution is to break current behavior and add a new optional property into the SdkMethodResponse, which is set to true when the response contains HTTP response without body.</issue_description>

Comments on the Issue (you are @copilot in this section)

@tadelesh Several points: 1. We believe TCGC should change `nullable` to `optional`, but it is a breaking change, we should consider more carefully. 2. It seems to be a migration case. Deprioritize for now to see if there are more cases.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Fix nullable response type generated by tcgc [TCGC] Add optional property to SdkMethodResponse to distinguish from nullable body types Nov 10, 2025
Copilot AI requested a review from tadelesh November 10, 2025 04:00
Copilot finished work on behalf of tadelesh November 10, 2025 04:00
Copy link
Member

@tadelesh tadelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot run pnpm format to format code and pnpm change add to add changeset. In the changeset, you need to mention how to do migration for the breaking change.

Copy link
Contributor Author

Copilot AI commented Nov 10, 2025

@copilot run pnpm format to format code and pnpm change add to add changeset. In the changeset, you need to mention how to do migration for the breaking change.

Done. The code was already properly formatted. Added changeset with migration instructions in commit ba4067b.

Copilot AI requested a review from tadelesh November 10, 2025 05:43
Copilot finished work on behalf of tadelesh November 10, 2025 05:43
Copilot AI requested a review from tadelesh November 10, 2025 05:50
Copilot finished work on behalf of tadelesh November 10, 2025 05:50
@tadelesh tadelesh marked this pull request as ready for review November 10, 2025 05:57
@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 10, 2025

Open in StackBlitz

npm i https://pkg.pr.new/Azure/typespec-azure/@azure-tools/typespec-client-generator-core@3493

commit: 2344aa7

@markcowl markcowl added int:azure-specs Run integration tests against azure-rest-api-specs breaking-change A change that might cause specs or code to break labels Nov 10, 2025
Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be merged until after the November release (this week).

…ponse without body exists, false when all responses have body, undefined when no response type

Co-authored-by: tadelesh <[email protected]>
Copilot finished work on behalf of tadelesh November 11, 2025 06:05
@tadelesh tadelesh dismissed markcowl’s stale review November 28, 2025 07:04

TypeSpec has released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change A change that might cause specs or code to break int:azure-specs Run integration tests against azure-rest-api-specs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TCGC] The response type nullable generated by tcgc is not as expected

4 participants