Skip to content

Fix label names when internalSignature has optional params - #1615

Merged
ssalbdivad merged 2 commits into
arktypeio:mainfrom
aswinsvijay:fix-optional-param-labels
Sep 17, 2026
Merged

ssalbdivad merged 2 commits into
arktypeio:mainfrom
aswinsvijay:fix-optional-param-labels

Conversation

@aswinsvijay

@aswinsvijay aswinsvijay commented May 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1614

  • Code is up-to-date with the main branch
  • You've successfully run pnpm prChecks locally
  • There are new or updated unit tests validating the change

Unsure how this can be tested since tuple labels are not part of the type system.

@pullfrog

pullfrog Bot commented May 1, 2026 •

Copy link
Copy Markdown
Contributor

Reviewed PR #1615 — approved. The one-line fix correctly wraps Parameters<internalSignature> with Required<> to prevent optional implementation parameters from breaking label inference in applyElementLabels.

Task list (5/5 completed)

Pullfrog  | View workflow run | via Pullfrog | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed — no issues found.

The fix is correct: Required<Parameters<internalSignature>> ensures the labels tuple passed to applyElementLabels is always fully required, which is the right semantics since parameter optionality in the external signature is determined by distill.In<paramsT> (the first arg), not the implementation's parameter declarations. The recursive [unknown, ...infer labelsTail] destructuring in applyElementLabels can break when the labels tuple has optional elements — this cleanly prevents that.

Pullfrog  | View workflow run | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed — no issues found.

The fix is correct: Required<Parameters<internalSignature>> ensures the labels tuple passed to applyElementLabels is always fully required, which is the right semantics since parameter optionality in the external signature is determined by distill.In<paramsT> (the first arg), not the implementation's parameter declarations. The recursive [unknown, ...infer labelsTail] destructuring in applyElementLabels can break when the labels tuple has optional elements — this cleanly prevents that.

Pullfrog  | View workflow run | 𝕏

@aswinsvijay aswinsvijay changed the title fix label names when internalSignature has optional params Fix label names when internalSignature has optional params May 1, 2026
@ssalbdivad
ssalbdivad force-pushed the fix-optional-param-labels branch from 0e6e185 to c411b27 Compare September 17, 2026 19:25
…parameter

The fix had no test because tuple labels look untestable - they are cosmetic
and carry no runtime representation. They are still part of the inferred type,
so `attest(...).type.toString` pins them exactly.

The case is narrower than the issue suggests. A contextually inferred parameter
already labels correctly, both alone and alongside others:

    type.fn("number?")(n => n)                     // (n?: number | undefined)
    type.fn("string", "number?")((a, b) => {})     // (a: string, b?: number | undefined)

It is annotating the optional parameter that breaks it, since only then does
`Parameters<internalSignature>` carry an optional element - which
`applyElementLabels` cannot match against `labels extends [unknown, ...infer
labelsTail]`, so it drops every label rather than just that one:

    type.fn("number?")((n?: number) => n)          // (args_0?: number | undefined)

The test uses that last form, with the parameter optional on both sides. The
issue's own repro instead pairs a required `"number"` with an optional `b?`,
which reproduces the bug but asks for something incoherent: the schema requires
the argument, so an implementation marking it optional describes a call that
cannot happen.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AvpxQ2tfRbpXYxMH3NbXou

@ssalbdivad ssalbdivad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified the fix and added the regression test. Labels are cosmetic but still part of the inferred type, so attest(...).type.toString pins them.

@ssalbdivad

Copy link
Copy Markdown
Member

Thanks @aswinsvijay — merging.

I added the regression test and rebased onto main. Labels looked untestable because they're cosmetic, but they're still part of the inferred type, so attest(...).type.toString pins them exactly.

One note for the future: the declared type's optionality should match the implementation's. fn('string', 'number') says the second argument is required, so an implementation taking (a, b?) describes a call that can't happen — prefer fn('string', 'number?') and let the declared type drive the signature. The test uses that form, since a contextually inferred parameter already labels correctly; it's annotating the optional one that triggers the bug.

@ssalbdivad
ssalbdivad merged commit b746a01 into arktypeio:main Sep 17, 2026
6 checks passed
@github-project-automation github-project-automation Bot moved this from To do to Done (merged or closed) in arktypeio Sep 17, 2026
@ssalbdivad ssalbdivad mentioned this pull request Sep 24, 2026
4 tasks done
ssalbdivad added a commit that referenced this pull request Sep 24, 2026
Bumps all publishable packages and documents the changes merged since 2.2.4:

- preserve escaped backslashes in regex literals (#1634, @spokodev)
- fix recursive discriminated unions referenced from a Record (#1641, @xianjianlf2)
- prevent a crash validating unions of object arrays (#1638, @xianjianlf2)
- merge index-derived props with a declared key of the same name (#1659)
- preserve parameter labels when a type.fn implementation annotates an
  optional param (#1615, @aswinsvijay)

#1634 changes behavior for regex literals that contain `\\`: `"/\\\\d/"`
previously compiled to the digit class `\d` and now matches a literal
backslash followed by "d", matching the equivalent RegExp instance. The
changelog calls this out with a migration note.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done (merged or closed)

Development

Successfully merging this pull request may close these issues.

fn does not infer parameter labels correctly if internalSignature has optional parameters

2 participants