Skip to content

chore(pass-style): make CopyArray compatible with ReadonlyArray - #3061

Open
michaelfig wants to merge 1 commit into
masterfrom
mfig/readonly-array-is-passable
Open

chore(pass-style): make CopyArray compatible with ReadonlyArray#3061
michaelfig wants to merge 1 commit into
masterfrom
mfig/readonly-array-is-passable

Conversation

@michaelfig

@michaelfig michaelfig commented Jan 28, 2026

Copy link
Copy Markdown
Member

Closes: #3060

Description

This PR updates the CopyArray type definition from Array to ReadonlyArray to better reflect that CopyArrays are hardened and immutable at runtime. This allows readonly arrays and tuples to be accepted as Passable, resolving issue #3060.

Security Considerations

Types now reflect that @endo/pass-style's Passable type doesn't require CopyArrays to be mutable.

Scaling Considerations

n/a

Documentation Considerations

If you typed a mutable array as a CopyArray, Typescript will show errors if you try to mutate it directly, or call methods that would mutate it. You can achieve the original behavior by typing your array as Passable[].

Testing Considerations

Type tests have been added.

Compatibility Considerations

Makes for better compatibility.

Upgrade Considerations

No runtime change, typing only.

@michaelfig michaelfig self-assigned this Jan 28, 2026
@michaelfig michaelfig added the bug Something isn't working label Jan 28, 2026
@michaelfig
michaelfig force-pushed the mfig/readonly-array-is-passable branch from 31f81e8 to e6a4c16 Compare January 28, 2026 17:52
michaelfig added a commit to Agoric/agoric-sdk that referenced this pull request Jan 28, 2026
@michaelfig
michaelfig force-pushed the mfig/readonly-array-is-passable branch from e6a4c16 to 97a0744 Compare January 28, 2026 19:04
@michaelfig
michaelfig requested a review from Copilot January 28, 2026 19:06

Copilot AI 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.

Pull request overview

This PR updates the CopyArray<T> type definition from Array<T> to ReadonlyArray<T> to better reflect that CopyArrays are hardened and immutable at runtime. This allows readonly arrays and tuples to be accepted as Passable, resolving issue #3060.

Changes:

  • Changed CopyArray<T> type from Array<T> to ReadonlyArray<T> in type definitions
  • Added type tests to verify readonly arrays and tuples are accepted as Passable
  • Updated NEWS.md to document the type change

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
packages/pass-style/src/types.d.ts Changes CopyArray<T> from Array<T> to ReadonlyArray<T> to reflect immutability
packages/pass-style/src/types.test-d.ts Adds type tests for readonly arrays, const tuples, and various array forms
packages/pass-style/NEWS.md Documents the type change in the release notes

The change is semantically correct - CopyArrays are hardened at runtime and should not be mutated. The type system should reflect this. However, there is a downstream impact on the patterns package that uses incorrect type annotations for mutable accumulators.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@michaelfig

Copy link
Copy Markdown
Member Author

@copilot, can you specify a few examples of exactly where it is in "the patterns package that uses incorrect type annotations for mutable accumulators"?

Copilot AI commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

@michaelfig I've opened a new pull request, #3062, to work on those changes. Once the pull request is ready, I'll request review from you.

Comment thread packages/pass-style/src/types.d.ts Outdated
Comment on lines +208 to +213
export type CopyArray<T extends Passable = any> = Array<T>;

/**
* A Passable and Readonly sequence of Passable values.
*/
export type CopyReadonlyArray<T extends Passable = any> = ReadonlyArray<T>;

@gibson042 gibson042 Jan 29, 2026

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.

Why introduce a new type? What breaks if CopyArray is instead updated to use ReadonlyArray rather than Array?

@michaelfig michaelfig Jan 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for asking!

The breaking change is that there are some uses in the wild of CopyArray as the type of a mutable array, and then the code proceeds to mutate the array. If CopyArray = ReadonlyArray, the code fails to typecheck.

At this stage in the game, I'm very reluctant somewhat willing to introduce breaking changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@copilot, for this endojs/endo repository, in *.js files containing JSDoc TypeScript types and *.ts TypeScript files, can you please identify all arrays that are declared to be of type CopyArray that are later mutated?

Use shell commands with intermediary files whenever possible to save your work and avoid consuming too much of your agent's context space.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Besides the highly enthusiastic affirmations of my AI sycophant, I also see that a mutable CopyArray type is useful to enforce that all its elements are Passable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Having said all that, the friendly bots scoured Agoric/agoric-sdk and endojs/endo, and only @endo/patterns/src/patterns/patternMatchers.js uses a mutable CopyArray in its internals. What do you see as the best way forward?

@gibson042 gibson042 Jan 29, 2026

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.

#3065 will fix the typing that makes it look like patternMatchers.js is attempting CopyArray mutation, and any mutability associated with Passable is itself a bug whose footprint I'd rather reduce than expand. So my preference is to land #3065 and then update this PR to tighten CopyArray without introducing a new type.

michaelfig added a commit to Agoric/agoric-sdk that referenced this pull request Jan 29, 2026
michaelfig added a commit to Agoric/agoric-sdk that referenced this pull request Jan 29, 2026
michaelfig added a commit to Agoric/agoric-sdk that referenced this pull request Jan 29, 2026

Copilot AI commented Jan 29, 2026

Copy link
Copy Markdown
Contributor

@michaelfig I've opened a new pull request, #3063, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/pass-style/src/types.test-d.ts Outdated
Comment on lines +48 to +59
arr.slice(0, 1);
arr.shift();
expectPassable(arr);

// Not so for CopyReadonlyArray.
const roArr: CopyReadonlyArray<number> = [];
// @ts-expect-error not mutable
roArr.push(1);
roArr.slice(0, 1);
// @ts-expect-error not mutable
roArr.shift();
expectPassable(roArr);

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

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

Result from call to slice ignored.

Suggested change
arr.slice(0, 1);
arr.shift();
expectPassable(arr);
// Not so for CopyReadonlyArray.
const roArr: CopyReadonlyArray<number> = [];
// @ts-expect-error not mutable
roArr.push(1);
roArr.slice(0, 1);
// @ts-expect-error not mutable
roArr.shift();
expectPassable(roArr);
const slicedArr = arr.slice(0, 1);
arr.shift();
expectPassable(arr);
expectPassable(slicedArr);
// Not so for CopyReadonlyArray.
const roArr: CopyReadonlyArray<number> = [];
// @ts-expect-error not mutable
roArr.push(1);
const slicedRoArr = roArr.slice(0, 1);
// @ts-expect-error not mutable
roArr.shift();
expectPassable(roArr);
expectPassable(slicedRoArr);

Copilot uses AI. Check for mistakes.
Comment thread packages/pass-style/src/types.test-d.ts Outdated
@michaelfig
michaelfig force-pushed the mfig/readonly-array-is-passable branch from e5684ea to 8d8a604 Compare January 30, 2026 10:22
@kriskowal

kriskowal commented Apr 28, 2026

Copy link
Copy Markdown
Member

@gibson042 @turadg Can we confirm this is superseded by #3172 and close, or follow-up?

Related #3063

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arrays typed as readonly aren't Passable

5 participants