Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

orta
Copy link
Contributor

@orta orta commented Jan 13, 2021

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
Copy link
Contributor Author

orta commented Jan 13, 2021

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 13, 2021

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
@@ -37,7 +41,13 @@ tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts(7,20)
}
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
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

@@ -1408,7 +1408,7 @@ interface ArrayConstructor {
(arrayLength?: number): any[];
<T>(arrayLength: number): T[];
<T>(...items: T[]): T[];
isArray(arg: any): arg is any[];
isArray(arg: any): arg is readonly unknown[];
Copy link
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
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
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
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.

# for free to join this conversation on GitHub. Already have an account? # to comment
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