Export diagnostic-collecting API for downstream emitter composition#9416
Export diagnostic-collecting API for downstream emitter composition#9416
Conversation
Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
…odel parameter Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
ArcturusZhang
left a comment
There was a problem hiding this comment.
@copilot we need to remove the method reportDiagnostic in Logger.
for those places that were previously calling this function, we need to change that method to use the same pattern to return [Result, readonly Diagnostic[]] instead.
I understand you want to remove |
…tern - Removed reportDiagnostic, warn, and error methods from Logger class - Added __diagnostics array to CSharpEmitterContext for collecting diagnostics - Updated all code to use createDiagnostic and push to sdkContext.__diagnostics - Updated createModel to return diagnostics from __diagnostics instead of logger - Updated all tests to report returned diagnostics to program for assertions - Simplified Logger to only handle tracing, removed diagnostic collection mode Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
|
@copilot I saw you added a diagnostic field in the context, but this is not quite following the best practice. at the beginning, and use its apis to wrap normal results without diagnostics, or add diagnostics into the result. |
- Replaced __diagnostics array with DiagnosticCollector from TypeSpec - Updated createModel to use diagnostics.wrap() for returning tuple - Functions now access diagnostics via sdkContext.__diagnostics.add() - Updated tests to set up diagnostic collector when needed - Follows the pattern from TCGC: createDiagnosticCollector at top level Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
Refactored to use |
Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
|
@copilot I think you misunderstood the pattern. If a method would be called here and it returns a diagnostic and something else, and we need this "something", we should do it in this way: This pattern is kind of contagious, if one method is returning the diagnostics, its caller must also return the diagnostics in its signature. |
ArcturusZhang
left a comment
There was a problem hiding this comment.
@copilot please resolve my comments, and fix the build issues in test cases, there are quire a few.
- Removed .tspd/docs/downstream-emitters.md file per reviewer feedback - Renamed $emitCodeModel to emitCodeModel (removed $ prefix) - Updated all references in emitter.ts, index.ts, tests, and README - Removed link to deleted documentation from README - All tests pass (1,172 total tests) Co-authored-by: ArcturusZhang <10554446+ArcturusZhang@users.noreply.github.com>
Merge origin/main into copilot/export-api-for-downstream-emitter. - Resolved merge conflicts in operation-converter.ts and type-converter.ts by combining the diagnostic pattern (from PR) with new sdkProperty parameter (from main) - Updated new test files to destructure createModel() tuple return - Made getMethodParameterSegments() return diagnostic tuple - Made getCollectionHeaderPrefix() return diagnostic tuple instead of calling removed logger.reportDiagnostic() - Fixed test to check returned diagnostics instead of program.diagnostics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
commit: |
|
No changes needing a change description found. |
packages/http-client-csharp/emitter/test/Unit/client-initialization.test.ts
Outdated
Show resolved
Hide resolved
Remove unused imports from code-model-writer.ts, logger.ts, sdk-context.ts, emitter.test.ts, and namespace-converter.test.ts. Remove dead 'program' field from Logger class (only user was the removed reportDiagnostic method). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests that don't validate diagnostics no longer destructure or report modelDiagnostics. Tests that DO validate diagnostics now check the returned modelDiagnostics directly instead of program.diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Run 'npm run regen-docs' to remove stale 'update-code-model' option from generated emitter reference documentation - Run 'pnpm format:dir' to fix formatting across all emitter files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add blank lines between markdown sections to match the tspd doc output from CI pipeline. Removes trailing blank line from decorators.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
You can try these changes here
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the C# HTTP client emitter to follow TypeSpec’s tuple-based diagnostic collection pattern and exports a new emitCodeModel entry point so downstream emitters can build/modify the CodeModel while collecting diagnostics without side effects.
Changes:
- Introduces
emitCodeModel(context, updateCodeModel?)and refactors$onEmitto call it and report returned diagnostics. - Updates model/converter pipelines to return
[Result, readonly Diagnostic[]]and usecreateDiagnosticCollector()+diagnostics.pipe()/wrap(). - Removes the
update-code-modelemitter option and removes diagnostic reporting fromLogger(trace-only).
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/content/docs/docs/emitters/clients/http-client-csharp/reference/emitter.md | Removes update-code-model option documentation. |
| packages/http-client-csharp/readme.md | Removes update-code-model option documentation. |
| packages/http-client-csharp/emitter/test/Unit/usage.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/type-converter.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/string-format.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/scalar.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/property-type.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/operation-paging.test.ts | Verifies diagnostics returned from createModel() rather than via program.diagnostics. |
| packages/http-client-csharp/emitter/test/Unit/operation-converter.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/namespace-converter.test.ts | Updates tests for tuple-returning fromSdkNamespaces(). |
| packages/http-client-csharp/emitter/test/Unit/model-type.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/method-parameter-segments.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/input-parameter.test.ts | Updates tests for tuple-returning createModel() and diagnostics capture. |
| packages/http-client-csharp/emitter/test/Unit/encode.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/emitter.test.ts | Adds emitCodeModel tests and updates _validateDotNetSdk to return diagnostics. |
| packages/http-client-csharp/emitter/test/Unit/decorator-list.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/constant-type.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/client-model-builder.test.ts | Adds tests asserting createModel() returns [CodeModel, diagnostics]. |
| packages/http-client-csharp/emitter/test/Unit/client-initialization.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/client-converter.test.ts | Updates tests for tuple-returning createModel(). |
| packages/http-client-csharp/emitter/test/Unit/auth.test.ts | Updates tests to assert diagnostics returned from createModel(). |
| packages/http-client-csharp/emitter/src/options.ts | Removes update-code-model option/type/schema/default. |
| packages/http-client-csharp/emitter/src/lib/typespec-server.ts | Converts resolveServers() to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/lib/type-converter.ts | Converts type conversion to tuple-returning diagnostics pattern and replaces logger diagnostics. |
| packages/http-client-csharp/emitter/src/lib/service-authentication.ts | Converts auth processing to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/lib/operation-converter.ts | Converts operation conversion to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/lib/namespace-converter.ts | Converts namespace conversion to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/lib/logger.ts | Removes diagnostic reporting from logger; trace-only. |
| packages/http-client-csharp/emitter/src/lib/example-converter.ts | Converts example conversion to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/lib/client-model-builder.ts | Updates createModel() to return [CodeModel, diagnostics] and pipes diagnostics through model build. |
| packages/http-client-csharp/emitter/src/lib/client-converter.ts | Converts client conversion to tuple-returning diagnostics pattern. |
| packages/http-client-csharp/emitter/src/index.ts | Exports new emitCodeModel function. |
| packages/http-client-csharp/emitter/src/emitter.ts | Adds emitCodeModel(), refactors $onEmit, and converts .NET SDK validation to tuple diagnostics. |
Comments suppressed due to low confidence (2)
packages/http-client-csharp/emitter/src/lib/operation-converter.ts:1076
diagnosticsis created but never used in the twofromSdkType(...)return paths, which makes the collector dead code and can triggernoUnusedLocalsbuild failures. Either remove the local collector entirely, or use it consistently (e.g.,diagnostics.wrap(diagnostics.pipe(fromSdkType(...)))) so this function always merges diagnostics through its own collector.
): [InputType | undefined, readonly Diagnostic[]] {
const diagnostics = createDiagnosticCollector();
if (!type) {
return diagnostics.wrap(undefined);
}
// handle anonymous union enum response types by defaulting to the enum value type in the case of
if (type.kind === "enum" && type.isUnionAsEnum && type.isGeneratedName) {
return fromSdkType(sdkContext, type.valueType);
}
return fromSdkType(sdkContext, type);
}
website/src/content/docs/docs/emitters/clients/http-client-csharp/reference/emitter.md:1
- The
update-code-modeloption docs were removed, but the docs don’t appear to add the replacement guidance for downstream customization via the new exportedemitCodeModel(context, updateCodeModel?)API. Please add a short section describing how downstream emitters should customize theCodeModelnow (and, if applicable, note the breaking change/removal ofupdate-code-model).
| updateCodeModel?: (model: CodeModel, context: CSharpEmitterContext) => CodeModel, | ||
| ): Promise<[void, readonly Diagnostic[]]> { | ||
| const diagnostics = createDiagnosticCollector(); | ||
| const program: Program = context.program; |
There was a problem hiding this comment.
Program is referenced as a type here, but it isn't included in the updated @typespec/compiler import list in this diff. This will fail to compile unless Program is imported (or the explicit type annotation is removed).
| const program: Program = context.program; | |
| const program = context.program; |
| format: { message: message }, | ||
| target: NoTarget, | ||
| }); | ||
| this.tracer.trace("warning", message); |
There was a problem hiding this comment.
warn() now bypasses the existing log-level gating implemented in trace(...) by writing directly to the tracer. To preserve the logger's level filtering behavior, route warnings through the trace(...) method (or apply the same level check here).
| this.tracer.trace("warning", message); | |
| if ( | |
| this.level === LoggerLevel.INFO || | |
| this.level === LoggerLevel.DEBUG || | |
| this.level === LoggerLevel.VERBOSE | |
| ) { | |
| this.tracer.trace("warning", message); | |
| } |
| * import { createModel } from "@typespec/http-client-csharp"; | ||
| * | ||
| * const sdkContext = createCSharpEmitterContext(context, logger); |
There was a problem hiding this comment.
The JSDoc example references createCSharpEmitterContext(context, logger), but the surrounding code/tests in this PR use createCSharpSdkContext(...) and the exported API surface may differ. Update the example to use the actual public API names/flow so consumers can copy/paste it successfully.
| * import { createModel } from "@typespec/http-client-csharp"; | |
| * | |
| * const sdkContext = createCSharpEmitterContext(context, logger); | |
| * import { createModel, createCSharpSdkContext } from "@typespec/http-client-csharp"; | |
| * | |
| * const sdkContext = await createCSharpSdkContext(context, logger); |
| * @beta | ||
| */ | ||
| export function createModel(sdkContext: CSharpEmitterContext): CodeModel { | ||
| export function createModel(sdkContext: CSharpEmitterContext): [CodeModel, readonly Diagnostic[]] { |
There was a problem hiding this comment.
createModel() is exported from the package entrypoint and its return type changed from CodeModel to a tuple, which is a breaking change for any external callers. If backward compatibility is desired, consider providing a transitional wrapper (e.g., keep createModel() returning CodeModel and add a new createModelWithDiagnostics()), or ensure the package versioning/release notes explicitly treat this as a breaking API change.
The emitter was directly reporting diagnostics to the program, preventing downstream emitters from collecting and handling diagnostics according to TypeSpec best practices (
[Result, readonly Diagnostic[]]pattern).Changes
createModel()— Now returns[CodeModel, readonly Diagnostic[]]usingcreateDiagnosticCollector()anddiagnostics.wrap()emitCodeModel()— New exported function wrapping emission logic, returns[void, readonly Diagnostic[]]. Accepts optionalupdateCodeModelcallback parameter for downstream emitters to customize the code model.reportDiagnostic()method; Logger now only handles tracing_validateDotNetSdk()— Now returns[boolean, readonly Diagnostic[]]with its own diagnostic collector$onEmit()— Refactored to callemitCodeModel()and report collected diagnosticscreateDiagnosticCollector(). Functions do NOT acceptDiagnosticCollectoras a parameter. Callers usediagnostics.pipe()to unwrap tuple-returning functions.Over 40 functions were updated across 9 source files to properly handle diagnostic collection:
type-converter.ts,service-authentication.ts,client-converter.ts,operation-converter.ts,namespace-converter.ts,emitter.ts,example-converter.ts,typespec-server.ts, andclient-model-builder.ts.Migration Guide for Downstream SDKs
Before (old pattern — accessing internals)
After (new pattern — using
emitCodeModel)Key points
emitCodeModel(context, updateCodeModel?)— it handles SDK context creation, model building, validation, and code generation.emitCodeModelreturns[void, readonly Diagnostic[]]. Callcontext.program.reportDiagnostics(diagnostics)to surface them.updateCodeModelcallback to modify theCodeModelbefore it is serialized and handed to the C# generator.createModel[CodeModel, readonly Diagnostic[]]if you need the model without running the generator.fromSdkType,fromSdkServiceMethod,fromSdkClients, etc.) now return[Result, readonly Diagnostic[]]tuples. Usediagnostics.pipe()when calling them.Logger.reportDiagnostic()has been removed. Diagnostics now flow through return values, not side effects.Diagnostic collection pattern
This PR follows the same pattern used in TCGC and the TypeSpec compiler: