Skip to content

fix(cbor): accept readonly input data in encoders#7148

Open
LeSingh1 wants to merge 2 commits into
denoland:mainfrom
LeSingh1:cbor-accept-readonly-input
Open

fix(cbor): accept readonly input data in encoders#7148
LeSingh1 wants to merge 2 commits into
denoland:mainfrom
LeSingh1:cbor-accept-readonly-input

Conversation

@LeSingh1
Copy link
Copy Markdown

@LeSingh1 LeSingh1 commented May 18, 2026

Closes #5831 for the @std/cbor module. Same shape as the @std/msgpack fix in #5832.

The encoder functions and the encoder streams never mutate their inputs, but the existing CborType requires CborType[], Map<CborType, CborType>, and a mutable { [k: string]: CborType } index signature. That means you can't pass an as const literal, a ReadonlyMap, or a readonly T[] to encodeCbor without a cast, even though the runtime behavior is identical. Repro from the issue:

import { encodeCbor } from "jsr:@std/cbor";

const data = {
  a: 1,
  b: { c: 2n },
  d: [3, { e: 4 }],
} as const;

encodeCbor(data); // TS2345 before this PR

I added a separate input-only type CborInputType instead of changing CborType in place, because CborType is also the decoder output (decodeCbor returns CborType) and changing it in place would be a breaking change for consumers that mutate decoded values. CborInputType mirrors CborType but with readonly CborInputType[], ReadonlyMap<CborInputType, CborInputType>, and { readonly [k: string]: CborInputType }. Encoders accept CborInputType; decoders still return CborType. CborStreamInput is input-only, so I widened it in place rather than adding a second type.

CborTag<T>'s constraint widens from T extends CborType | CborStreamInput | CborStreamOutput to also accept CborInputType so new CborTag(1, readonlyValue) type-checks. This widens the constraint, doesn't narrow it, so existing instantiations stay valid.

In _common_encode.ts, x instanceof Map doesn't narrow ReadonlyMap on its own (TS treats them as separate types), so there's a small isReadonlyMap predicate. The #encodeArray and #encodeObject stream helpers widen to accept readonly variants.

Existing callers passing mutable CborType keep working because mutable subtypes assign to readonly supertypes.

Regression tests in encode_cbor_test.ts cover deeply-readonly as const literals (exact repro from #5831), readonly tuples, ReadonlyMap input, readonly index signatures, and a CborTag with readonly content. encode_cbor_sequence_test.ts covers a readonly array of values.

Ran locally: deno check cbor/, deno lint cbor/, deno fmt --check cbor/ all clean. deno test --allow-all cbor/ shows 89 passed, 0 failed.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions Bot added the cbor label May 18, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.61%. Comparing base (95a1e2e) to head (8df2d62).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7148   +/-   ##
=======================================
  Coverage   94.61%   94.61%           
=======================================
  Files         634      634           
  Lines       51843    51850    +7     
  Branches     9346     9347    +1     
=======================================
+ Hits        49050    49057    +7     
  Misses       2218     2218           
  Partials      575      575           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Closes denoland#5831 (for the `@std/cbor` module).

`encodeCbor` / `encodeCborSequence` / `CborSequenceEncoderStream` /
`CborArrayEncoderStream` never mutate their inputs, but the
`CborType` parameter required mutable arrays, a mutable `Map`, and a
mutable index signature. That meant `as const` literals, `ReadonlyMap`
values, and `readonly T[]` arrays could not be passed without a cast,
even though the runtime behaviour is identical.

Add a new input-only type `CborInputType` that mirrors `CborType` but
substitutes:

- `CborType[]`               -> `readonly CborInputType[]`
- `Map<CborType, CborType>`  -> `ReadonlyMap<CborInputType, CborInputType>`
- `{ [k: string]: CborType }`  -> `{ readonly [k: string]: CborInputType }`

`CborType` (which is also the decoder output type) stays mutable, so
this is fully backwards-compatible for existing callers and for the
decode side. `CborStreamInput` is input-only and is widened in place.

In the encoders, the existing `x instanceof Map` narrowing does not
narrow `ReadonlyMap` on its own, so a tiny `isReadonlyMap` type
predicate is added in `_common_encode.ts` and the relevant
`#encodeArray` / `#encodeObject` stream helpers are widened to
accept the readonly variants.

`CborTag<T>`'s constraint is widened from
`T extends CborType | CborStreamInput | CborStreamOutput` to also
permit `CborInputType`.

Mirrors the `@std/msgpack` change from denoland#5832.

Regression tests added:

- `encodeCbor()` accepts deeply-readonly `as const` literals.
- `encodeCbor()` accepts readonly tuple literals.
- `encodeCbor()` accepts `ReadonlyMap` input.
- `encodeCbor()` accepts `{ readonly [k: string]: ... }` input.
- `encodeCbor()` accepts a `CborTag` whose content is readonly.
- `encodeCborSequence()` accepts a `readonly` array of values.

All 89 CBOR module tests pass (24 pre-existing + 65 from related files;
6 new). `deno check cbor/` clean. `deno lint cbor/` clean.
`deno fmt --check cbor/` clean.
@LeSingh1 LeSingh1 force-pushed the cbor-accept-readonly-input branch from 8e60a59 to 80d0944 Compare May 18, 2026 23:06
@BlackAsLight
Copy link
Copy Markdown
Contributor

For the most part the pull request looks good.

I'm personally not a fan of the name CborInputType in this instance. The CborType is no longer listed in the encodeCbor function so the description on its type saying its accepted for said function feels misleading even if TypeScript doesn't complain. I think the name ReadonlyCborType would be more fitting for what its offering, and I do wonder if a signature of encodeCbor(value: CborType | ReadonlyCborType): Uint8Array would be possible instead.

My only other concern is the comments in the tests you added. I feel like their content is pointless and that their meaning would be lost to time.

…ments

Per @BlackAsLight on denoland#7148:

- Rename CborInputType -> ReadonlyCborType. Encoder signature now
  reads encodeCbor(value: CborType | ReadonlyCborType): Uint8Array
  (and the equivalent in encodeCborSequence), which keeps CborType
  prominent in the public API and clearly documents what additional
  shapes the encoder accepts.
- Drop the descriptive comments on the new tests. The test names
  already describe what they cover, and the comments would lose
  context over time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accept readonly array inputs for all functions/methods that don't need to mutate their data

3 participants