diff --git a/CHANGES.md b/CHANGES.md index 98dc8ef21..d78fe4022 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -358,6 +358,11 @@ To be released. multiple source contexts with the same `id`. Duplicate ids now throw a `TypeError` instead of producing order-dependent results. [[#495], [#746]] + - Fixed `runWith()` and `runWithSync()` discarding the original parse error + when a source context's disposal also throws. The disposal error now + wraps both failures in a `SuppressedError` (following TC39 conventions) + instead of silently replacing the parse error. [[#246], [#771]] + - Fixed `optional()` and `withDefault()` crashing when the parser's state is an annotation-injected object instead of `undefined`. The state discrimination in `modifiers.ts` now uses `Array.isArray(state)` to @@ -1182,6 +1187,7 @@ To be released. [#241]: https://github.com/dahlia/optique/issues/241 [#242]: https://github.com/dahlia/optique/issues/242 [#245]: https://github.com/dahlia/optique/issues/245 +[#246]: https://github.com/dahlia/optique/issues/246 [#247]: https://github.com/dahlia/optique/issues/247 [#248]: https://github.com/dahlia/optique/issues/248 [#249]: https://github.com/dahlia/optique/issues/249 @@ -1451,6 +1457,7 @@ To be released. [#765]: https://github.com/dahlia/optique/pull/765 [#768]: https://github.com/dahlia/optique/issues/768 [#769]: https://github.com/dahlia/optique/pull/769 +[#771]: https://github.com/dahlia/optique/pull/771 ### @optique/config diff --git a/packages/core/src/facade.test.ts b/packages/core/src/facade.test.ts index 31ffbd95a..ce732a5f8 100644 --- a/packages/core/src/facade.test.ts +++ b/packages/core/src/facade.test.ts @@ -6300,6 +6300,131 @@ describe("runWith", () => { }); assert.equal(disposed, 1); }); + + it("should throw SuppressedError when parse fails and disposal also fails", async () => { + let disposed = false; + const context: SourceContext = { + id: Symbol.for("@test/dispose-shadow"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed = true; + throw new Error("dispose failed."); + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + try { + await runWith(parser, "test", [context], { + args: ["not-a-number"], + }); + assert.fail("Expected an error to be thrown"); + } catch (error) { + assert.ok(error instanceof Error); + assert.equal(error.name, "SuppressedError"); + const se = error as Error & { suppressed: unknown; error: unknown }; + assert.ok( + se.suppressed instanceof Error, + "suppressed should be the parse error", + ); + assert.ok( + se.error instanceof Error, + "error should be the disposal error", + ); + assert.equal( + (se.error as Error).message, + "dispose failed.", + ); + } + + assert.ok(disposed); + }); + + it("should throw SuppressedError with AggregateError when parse fails and multiple disposals fail", async () => { + const disposed: string[] = []; + + const context1: SourceContext = { + id: Symbol.for("@test/dispose-shadow-multi-1"), + getAnnotations() { + return {}; + }, + [Symbol.asyncDispose]() { + disposed.push("context1"); + throw new Error("dispose 1 failed."); + }, + }; + + const context2: SourceContext = { + id: Symbol.for("@test/dispose-shadow-multi-2"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed.push("context2"); + throw new Error("dispose 2 failed."); + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + try { + await runWith(parser, "test", [context1, context2], { + args: ["not-a-number"], + }); + assert.fail("Expected an error to be thrown"); + } catch (error) { + assert.ok(error instanceof Error); + assert.equal(error.name, "SuppressedError"); + const se = error as Error & { suppressed: unknown; error: unknown }; + assert.ok( + se.suppressed instanceof Error, + "suppressed should be the parse error", + ); + assert.ok( + se.error instanceof AggregateError, + "error should be AggregateError from multiple disposal failures", + ); + assert.equal((se.error as AggregateError).errors.length, 2); + } + + assert.deepEqual(disposed, ["context1", "context2"]); + }); + + it("should preserve parse error when disposal succeeds", async () => { + let disposed = false; + const context: SourceContext = { + id: Symbol.for("@test/dispose-no-shadow"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed = true; + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + let errorCaught: unknown; + try { + await runWith(parser, "test", [context], { + args: ["not-a-number"], + }); + } catch (error) { + errorCaught = error; + } + + assert.ok(disposed); + assert.notEqual((errorCaught as Error).name, "SuppressedError"); + assert.ok(errorCaught instanceof Error); + }); }); describe("options passthrough", () => { @@ -6997,6 +7122,131 @@ describe("runWithSync", () => { }); assert.equal(disposed, 1); }); + + it("should throw SuppressedError when sync parse fails and disposal also fails", () => { + let disposed = false; + const context: SourceContext = { + id: Symbol.for("@test/sync-dispose-shadow"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed = true; + throw new Error("sync dispose failed."); + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + try { + runWithSync(parser, "test", [context], { + args: ["not-a-number"], + }); + assert.fail("Expected an error to be thrown"); + } catch (error) { + assert.ok(error instanceof Error); + assert.equal(error.name, "SuppressedError"); + const se = error as Error & { suppressed: unknown; error: unknown }; + assert.ok( + se.suppressed instanceof Error, + "suppressed should be the parse error", + ); + assert.ok( + se.error instanceof Error, + "error should be the disposal error", + ); + assert.equal( + (se.error as Error).message, + "sync dispose failed.", + ); + } + + assert.ok(disposed); + }); + + it("should throw SuppressedError with AggregateError when sync parse fails and multiple disposals fail", () => { + const disposed: string[] = []; + + const context1: SourceContext = { + id: Symbol.for("@test/sync-dispose-shadow-multi-1"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed.push("context1"); + throw new Error("sync dispose 1 failed."); + }, + }; + + const context2: SourceContext = { + id: Symbol.for("@test/sync-dispose-shadow-multi-2"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed.push("context2"); + throw new Error("sync dispose 2 failed."); + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + try { + runWithSync(parser, "test", [context1, context2], { + args: ["not-a-number"], + }); + assert.fail("Expected an error to be thrown"); + } catch (error) { + assert.ok(error instanceof Error); + assert.equal(error.name, "SuppressedError"); + const se = error as Error & { suppressed: unknown; error: unknown }; + assert.ok( + se.suppressed instanceof Error, + "suppressed should be the parse error", + ); + assert.ok( + se.error instanceof AggregateError, + "error should be AggregateError from multiple disposal failures", + ); + assert.equal((se.error as AggregateError).errors.length, 2); + } + + assert.deepEqual(disposed, ["context1", "context2"]); + }); + + it("should preserve sync parse error when disposal succeeds", () => { + let disposed = false; + const context: SourceContext = { + id: Symbol.for("@test/sync-dispose-no-shadow"), + getAnnotations() { + return {}; + }, + [Symbol.dispose]() { + disposed = true; + }, + }; + + const parser = object({ + port: argument(integer()), + }); + + let errorCaught: unknown; + try { + runWithSync(parser, "test", [context], { + args: ["not-a-number"], + }); + } catch (error) { + errorCaught = error; + } + + assert.ok(disposed); + assert.notEqual((errorCaught as Error).name, "SuppressedError"); + assert.ok(errorCaught instanceof Error); + }); }); describe("priority handling (sync)", () => { diff --git a/packages/core/src/facade.ts b/packages/core/src/facade.ts index f3b50755a..272d439d0 100644 --- a/packages/core/src/facade.ts +++ b/packages/core/src/facade.ts @@ -66,6 +66,28 @@ import type { ParserValuePlaceholder, SourceContext } from "./context.ts"; export type { ParserValuePlaceholder, SourceContext }; +type SuppressedErrorConstructor = new ( + error: unknown, + suppressed: unknown, + message?: string, +) => Error & { readonly error: unknown; readonly suppressed: unknown }; + +// Polyfill for runtimes that lack SuppressedError (Node < 24): +const SuppressedErrorCtor: SuppressedErrorConstructor = + typeof SuppressedError === "function" ? SuppressedError : (() => { + class SuppressedErrorPolyfill extends Error { + readonly error: unknown; + readonly suppressed: unknown; + constructor(error: unknown, suppressed: unknown, message?: string) { + super(message); + this.name = "SuppressedError"; + this.error = error; + this.suppressed = suppressed; + } + } + return SuppressedErrorPolyfill; + })(); + function finalizeParsedForContext( context: SourceContext, parsed: unknown, @@ -2978,6 +3000,163 @@ export type ContextOptionsParam< ? { readonly contextOptions?: ExtractRequiredOptions } : { readonly contextOptions: ExtractRequiredOptions }; +/** + * Body of {@link runWith}, extracted so that the caller can handle + * disposal outside a `finally` block (avoiding `no-unsafe-finally` lint). + */ +async function runWithBody< + TParser extends Parser, + THelp = void, + TError = never, +>( + parser: TParser, + programName: string, + contexts: readonly SourceContext[], + args: readonly string[], + options: RunWithOptions, +): Promise> { + validateContextIds(contexts); + + // Early exit: skip context processing for help/version/completion + if (needsEarlyExit(args, options)) { + if (parser.$mode === "async") { + return runParser(parser, programName, args, options) as Promise< + InferValue + >; + } + return Promise.resolve( + runParser(parser, programName, args, options) as InferValue, + ); + } + + // Phase 1: Collect initial annotations + const ctxOptions = options.contextOptions; + const { + annotations: phase1Annotations, + annotationsList: phase1AnnotationsList, + hasDynamic: needsTwoPhase, + } = await collectPhase1Annotations(contexts, ctxOptions); + + if (!needsTwoPhase) { + // All static contexts - single pass is sufficient + // Inject annotations into the parser's initial state + const augmentedParser = injectAnnotationsIntoParser( + parser, + phase1Annotations, + ); + + if (parser.$mode === "async") { + return runParser( + augmentedParser, + programName, + args, + options, + ) as Promise>; + } + return Promise.resolve( + runParser(augmentedParser, programName, args, options) as InferValue< + TParser + >, + ); + } + + // Two-phase parsing for dynamic contexts + // First pass: parse with Phase 1 annotations to get initial result + const augmentedParser1 = injectAnnotationsIntoParser( + parser, + phase1Annotations, + ); + + let firstPassResult: unknown; + let firstPassDeferred: true | undefined; + let firstPassDeferredKeys: DeferredMap | undefined; + let firstPassFailed = false; + try { + if (parser.$mode === "async") { + firstPassResult = await parseAsync(augmentedParser1, args); + } else { + firstPassResult = parseSync( + augmentedParser1 as Parser<"sync", unknown, unknown>, + args, + ); + } + + // Extract value and deferred metadata from result + if ( + typeof firstPassResult === "object" && firstPassResult !== null && + "success" in firstPassResult + ) { + const result = firstPassResult as + & Result + & { deferred?: true; deferredKeys?: DeferredMap }; + if (result.success) { + firstPassResult = result.value; + firstPassDeferred = result.deferred; + firstPassDeferredKeys = result.deferredKeys; + } else { + firstPassFailed = true; + } + } + } catch { + firstPassFailed = true; + } + + // First pass failed - run through runParser for proper error handling. + // This is done outside the try-catch to prevent the catch block from + // re-invoking runParser when it throws (which caused double error output). + if (firstPassFailed) { + const augmentedParser = injectAnnotationsIntoParser( + parser, + phase1Annotations, + ); + if (parser.$mode === "async") { + return runParser( + augmentedParser, + programName, + args, + options, + ) as Promise< + InferValue + >; + } + return Promise.resolve( + runParser(augmentedParser, programName, args, options) as InferValue< + TParser + >, + ); + } + + // Phase 2: Collect annotations with parsed result + const { annotationsList: phase2AnnotationsList } = await collectAnnotations( + contexts, + firstPassResult, + ctxOptions, + firstPassDeferred, + firstPassDeferredKeys, + ); + + // Final parse with merged annotations + const finalAnnotations = mergeTwoPhaseAnnotations( + phase1AnnotationsList, + phase2AnnotationsList, + ); + const augmentedParser2 = injectAnnotationsIntoParser( + parser, + finalAnnotations, + ); + + if (parser.$mode === "async") { + return runParser(augmentedParser2, programName, args, options) as Promise< + InferValue + >; + } + return Promise.resolve( + runParser(augmentedParser2, programName, args, options) as InferValue< + TParser + >, + ); +} + /** * Runs a parser with multiple source contexts. * @@ -3006,6 +3185,9 @@ export type ContextOptionsParam< * @returns Promise that resolves to the parsed result. * @throws {TypeError} If two or more contexts share the same * {@link SourceContext.id}. + * @throws {SuppressedError} If the runner throws and a context's disposal + * also throws. The original error is available via `.suppressed` and the + * disposal error via `.error`. * @since 0.10.0 * * @example @@ -3055,150 +3237,122 @@ export async function runWith< ); } + let result!: InferValue; + let primaryError: unknown; + let hasPrimaryError = false; try { - validateContextIds(contexts); - - // Early exit: skip context processing for help/version/completion - if (needsEarlyExit(args, options)) { - if (parser.$mode === "async") { - return runParser(parser, programName, args, options) as Promise< - InferValue - >; - } - return Promise.resolve( - runParser(parser, programName, args, options) as InferValue, + result = await runWithBody(parser, programName, contexts, args, options); + } catch (error) { + hasPrimaryError = true; + primaryError = error; + } + + // Disposal runs unconditionally (success and error paths both reach here) + try { + await disposeContexts(contexts); + } catch (disposeError) { + if (hasPrimaryError) { + throw new SuppressedErrorCtor( + disposeError, + primaryError, + "An error was suppressed during context disposal.", ); } + throw disposeError; + } - // Phase 1: Collect initial annotations - const ctxOptions = options?.contextOptions; - const { - annotations: phase1Annotations, - annotationsList: phase1AnnotationsList, - hasDynamic: needsTwoPhase, - } = await collectPhase1Annotations(contexts, ctxOptions); - - if (!needsTwoPhase) { - // All static contexts - single pass is sufficient - // Inject annotations into the parser's initial state - const augmentedParser = injectAnnotationsIntoParser( - parser, - phase1Annotations, - ); + if (hasPrimaryError) { + throw primaryError; + } + return result; +} - if (parser.$mode === "async") { - return runParser( - augmentedParser, - programName, - args, - options, - ) as Promise>; - } - return Promise.resolve( - runParser(augmentedParser, programName, args, options) as InferValue< - TParser - >, - ); - } +/** + * Body of {@link runWithSync}, extracted so that the caller can handle + * disposal outside a `finally` block (avoiding `no-unsafe-finally` lint). + */ +function runWithSyncBody< + TParser extends Parser<"sync", unknown, unknown>, + THelp = void, + TError = never, +>( + parser: TParser, + programName: string, + contexts: readonly SourceContext[], + args: readonly string[], + options: RunWithOptions, +): InferValue { + validateContextIds(contexts); + + // Early exit: skip context processing for help/version/completion + if (needsEarlyExit(args, options)) { + return runParser(parser, programName, args, options); + } - // Two-phase parsing for dynamic contexts - // First pass: parse with Phase 1 annotations to get initial result - const augmentedParser1 = injectAnnotationsIntoParser( + // Phase 1: Collect initial annotations + const ctxOptions = options.contextOptions; + const { + annotations: phase1Annotations, + annotationsList: phase1AnnotationsList, + hasDynamic: needsTwoPhase, + } = collectPhase1AnnotationsSync(contexts, ctxOptions); + + if (!needsTwoPhase) { + // All static contexts - single pass is sufficient + const augmentedParser = injectAnnotationsIntoParser( parser, phase1Annotations, ); + return runParser(augmentedParser, programName, args, options); + } - let firstPassResult: unknown; - let firstPassDeferred: true | undefined; - let firstPassDeferredKeys: DeferredMap | undefined; - let firstPassFailed = false; - try { - if (parser.$mode === "async") { - firstPassResult = await parseAsync(augmentedParser1, args); - } else { - firstPassResult = parseSync( - augmentedParser1 as Parser<"sync", unknown, unknown>, - args, - ); - } - - // Extract value and deferred metadata from result - if ( - typeof firstPassResult === "object" && firstPassResult !== null && - "success" in firstPassResult - ) { - const result = firstPassResult as - & Result - & { deferred?: true; deferredKeys?: DeferredMap }; - if (result.success) { - firstPassResult = result.value; - firstPassDeferred = result.deferred; - firstPassDeferredKeys = result.deferredKeys; - } else { - firstPassFailed = true; - } - } - } catch { - firstPassFailed = true; - } + // Two-phase parsing for dynamic contexts + // First pass: parse with Phase 1 annotations + const augmentedParser1 = injectAnnotationsIntoParser( + parser, + phase1Annotations, + ); - // First pass failed - run through runParser for proper error handling. - // This is done outside the try-catch to prevent the catch block from - // re-invoking runParser when it throws (which caused double error output). - if (firstPassFailed) { - const augmentedParser = injectAnnotationsIntoParser( - parser, - phase1Annotations, - ); - if (parser.$mode === "async") { - return runParser( - augmentedParser, - programName, - args, - options, - ) as Promise< - InferValue - >; - } - return Promise.resolve( - runParser(augmentedParser, programName, args, options) as InferValue< - TParser - >, - ); + let firstPassResult: unknown; + let firstPassDeferred: true | undefined; + let firstPassDeferredKeys: DeferredMap | undefined; + try { + const result = parseSync(augmentedParser1, args) as + & { success: boolean; value?: unknown; error?: unknown } + & { deferred?: true; deferredKeys?: DeferredMap }; + if (result.success) { + firstPassResult = result.value; + firstPassDeferred = result.deferred; + firstPassDeferredKeys = result.deferredKeys; + } else { + // First pass failed - run through runParser for proper error handling + return runParser(augmentedParser1, programName, args, options); } + } catch { + // First pass threw - run through runParser for proper error handling + return runParser(augmentedParser1, programName, args, options); + } + + // Phase 2: Collect annotations with parsed result + const { annotationsList: phase2AnnotationsList } = collectAnnotationsSync( + contexts, + firstPassResult, + ctxOptions, + firstPassDeferred, + firstPassDeferredKeys, + ); - // Phase 2: Collect annotations with parsed result - const { annotationsList: phase2AnnotationsList } = await collectAnnotations( - contexts, - firstPassResult, - ctxOptions, - firstPassDeferred, - firstPassDeferredKeys, - ); - - // Final parse with merged annotations - const finalAnnotations = mergeTwoPhaseAnnotations( - phase1AnnotationsList, - phase2AnnotationsList, - ); - const augmentedParser2 = injectAnnotationsIntoParser( - parser, - finalAnnotations, - ); + // Final parse with merged annotations + const finalAnnotations = mergeTwoPhaseAnnotations( + phase1AnnotationsList, + phase2AnnotationsList, + ); + const augmentedParser2 = injectAnnotationsIntoParser( + parser, + finalAnnotations, + ); - if (parser.$mode === "async") { - return runParser(augmentedParser2, programName, args, options) as Promise< - InferValue - >; - } - return Promise.resolve( - runParser(augmentedParser2, programName, args, options) as InferValue< - TParser - >, - ); - } finally { - await disposeContexts(contexts); - } + return runParser(augmentedParser2, programName, args, options); } /** @@ -3221,6 +3375,9 @@ export async function runWith< * {@link SourceContext.id}. * @throws {Error} If any context returns a Promise or if a context's * `[Symbol.asyncDispose]` returns a Promise. + * @throws {SuppressedError} If the runner throws and a context's disposal + * also throws. The original error is available via `.suppressed` and the + * disposal error via `.error`. * @since 0.10.0 */ export function runWithSync< @@ -3250,81 +3407,34 @@ export function runWithSync< return runParser(parser, programName, args, options); } + let result!: InferValue; + let primaryError: unknown; + let hasPrimaryError = false; try { - validateContextIds(contexts); - - // Early exit: skip context processing for help/version/completion - if (needsEarlyExit(args, options)) { - return runParser(parser, programName, args, options); - } + result = runWithSyncBody(parser, programName, contexts, args, options); + } catch (error) { + hasPrimaryError = true; + primaryError = error; + } - // Phase 1: Collect initial annotations - const ctxOptions = options?.contextOptions; - const { - annotations: phase1Annotations, - annotationsList: phase1AnnotationsList, - hasDynamic: needsTwoPhase, - } = collectPhase1AnnotationsSync(contexts, ctxOptions); - - if (!needsTwoPhase) { - // All static contexts - single pass is sufficient - const augmentedParser = injectAnnotationsIntoParser( - parser, - phase1Annotations, + // Disposal runs unconditionally (success and error paths both reach here) + try { + disposeContextsSync(contexts); + } catch (disposeError) { + if (hasPrimaryError) { + throw new SuppressedErrorCtor( + disposeError, + primaryError, + "An error was suppressed during context disposal.", ); - return runParser(augmentedParser, programName, args, options); } + throw disposeError; + } - // Two-phase parsing for dynamic contexts - // First pass: parse with Phase 1 annotations - const augmentedParser1 = injectAnnotationsIntoParser( - parser, - phase1Annotations, - ); - - let firstPassResult: unknown; - let firstPassDeferred: true | undefined; - let firstPassDeferredKeys: DeferredMap | undefined; - try { - const result = parseSync(augmentedParser1, args) as - & { success: boolean; value?: unknown; error?: unknown } - & { deferred?: true; deferredKeys?: DeferredMap }; - if (result.success) { - firstPassResult = result.value; - firstPassDeferred = result.deferred; - firstPassDeferredKeys = result.deferredKeys; - } else { - // First pass failed - run through runParser for proper error handling - return runParser(augmentedParser1, programName, args, options); - } - } catch { - // First pass threw - run through runParser for proper error handling - return runParser(augmentedParser1, programName, args, options); - } - - // Phase 2: Collect annotations with parsed result - const { annotationsList: phase2AnnotationsList } = collectAnnotationsSync( - contexts, - firstPassResult, - ctxOptions, - firstPassDeferred, - firstPassDeferredKeys, - ); - - // Final parse with merged annotations - const finalAnnotations = mergeTwoPhaseAnnotations( - phase1AnnotationsList, - phase2AnnotationsList, - ); - const augmentedParser2 = injectAnnotationsIntoParser( - parser, - finalAnnotations, - ); - - return runParser(augmentedParser2, programName, args, options); - } finally { - disposeContextsSync(contexts); + if (hasPrimaryError) { + throw primaryError; } + return result; } /** @@ -3343,6 +3453,9 @@ export function runWithSync< * @returns Promise that resolves to the parsed result. * @throws {TypeError} If two or more contexts share the same * {@link SourceContext.id}. + * @throws {SuppressedError} If the runner throws and a context's disposal + * also throws. The original error is available via `.suppressed` and the + * disposal error via `.error`. * @since 0.10.0 */ export function runWithAsync<