Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

A second attempt at readonly array support persisting through isArray#42316

Closed
orta wants to merge 1 commit into
microsoft:mainmicrosoft/TypeScript:mainfrom
orta:readonly_arr_2orta/TypeScript:readonly_arr_2Copy head branch name to clipboard
Closed

A second attempt at readonly array support persisting through isArray#42316
orta wants to merge 1 commit into
microsoft:mainmicrosoft/TypeScript:mainfrom
orta:readonly_arr_2orta/TypeScript:readonly_arr_2Copy head branch name to clipboard

Conversation

@orta

@orta orta commented Jan 13, 2021

Copy link
Copy Markdown
Contributor

A different attempt at #39258 which persists readonly through isArray found during some investigations for #42231 without the complications of breaking generics etc.

@orta orta self-assigned this Jan 13, 2021
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 13, 2021
@orta

orta commented Jan 13, 2021

Copy link
Copy Markdown
Contributor Author

@typescript-bot pack this
@typescript-bot pack run dt
@typescript-bot pack user test this

@typescript-bot

typescript-bot commented Jan 13, 2021

Copy link
Copy Markdown
Contributor

Heya @orta, I've started to run the tarball bundle task on this PR at 289b0c8. You can monitor the build here.

@orta orta force-pushed the readonly_arr_2 branch 2 times, most recently from 74341a0 to f6b438b Compare January 13, 2021 13:03
v // Validator & Partial<OnChanges> via subtype reduction
if (v.onChanges) {
~~~~~~~~~
!!! error TS2339: Property 'onChanges' does not exist on type 'C | (Validator & Partial<OnChanges>)'.

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.

this looks like a regression. I reported this case back then, because the effect of instaceof affected code outside of the if statement

Comment thread lib/lib.es5.d.ts
<T>(arrayLength: number): T[];
<T>(...items: T[]): T[];
isArray(arg: any): arg is any[];
isArray(arg: any): arg is readonly unknown[];

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.

This is a safer type, but it can't help but break anybody who was previously able to mutate an array or use its contents after checking it with isArray. I think it's too breaky for the benefit.

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.

@sandersn You mean anybody who was previously able to mutate a mutable array after checking it with isArray? I tested this change locally and confirmed it doesn't break that: Mutable arrays remain mutable. The type predicate narrows the type to the intersection of typeof arg & readonly unknown[], which is only immutable if arg was already immutable.

Would you consider reopening this PR?

⏯️ Playground link

Workbench repro.

🧑‍💻 Code

declare const mutable: string[];
declare const immutable: readonly string[];

// Mutable arrays remain so with and without
// https://github.com/microsoft/TypeScript/pull/42316
if (Array.isArray(mutable)) {
  const should: string[] = mutable; // ✔️ A mutable array should remain so
}

// This assignment should error but doesn't, not without
// https://github.com/microsoft/TypeScript/pull/42316
if (Array.isArray(immutable)) {
  const shouldNot: string[] = immutable; // ❌ An immutable array should remain so
}

🙁 Actual behavior

$ tsc input.ts

No errors: You're free to mutate an immutable array.

🙂 Expected behavior

With this PR:

$ node built/local/tsc.js input.ts 
input.ts:13:9 - error TS4104: The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.

13   const shouldNot: string[] = immutable; // ❌ An immutable array should remain so
           ~~~~~~~~~


Found 1 error in input.ts:13

Mutating an immutable array is an error.

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.

What I mean is that if somebody writes this today, it's not an error:

declare const u: unknown;
if (Array.isArray(u)) {
    u[1] = 12
    u[1].length
}

unknown narrowing is an important scenario for Array.isArray, and this change would add errors that were not there before and are not necessarily correct.

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.

@sandersn I should've realized what you meant, thanks for your patience.

Instead of replacing the existing signature, would adding overloads for ArrayLike<T> and Iterable<T> work, without affecting the current behavior of any and unknown? I've opened #48228 with that modification to this PR.

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

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.