Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/lib.es5.d.ts
View file
Open in desktop
Expand Up @@ -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.

readonly prototype: any[];
}

Expand Down
15 changes: 10 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Expand Up @@ -22999,11 +22999,16 @@ namespace ts {
}
}

// If the candidate type is a subtype of the target type, narrow to the candidate type,
// if the target type is a subtype of the candidate type, narrow to the target type,
// otherwise, narrow to an intersection of the two types.
return isTypeSubtypeOf(candidate, type) ? candidate : isTypeSubtypeOf(type, candidate) ? type : getIntersectionType([type, candidate]);
}
// If the candidate type is a subtype of the target type, narrow to the candidate type.
// Otherwise, if the target type is assignable to the candidate type, keep the target type.
// Otherwise, if the candidate type is assignable to the target type, narrow to the candidate
// type. Otherwise, the types are completely unrelated, so narrow to an intersection of the
// two types.
return isTypeSubtypeOf(candidate, type) ? candidate :
isTypeAssignableTo(type, candidate) ? type :
isTypeAssignableTo(candidate, type) ? candidate :
getIntersectionType([type, candidate]);
}

function narrowTypeByCallExpression(type: Type, callExpression: CallExpression, assumeTrue: boolean): Type {
if (hasMatchingArgument(callExpression, reference)) {
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/instanceOfAssignability.types
View file
Open in desktop
Expand Up @@ -70,8 +70,8 @@ function fn2(x: Base) {
// 1.5: y: Base
// Want: y: Derived1
let y = x;
>y : Base & Derived1
>x : Base & Derived1
>y : Derived1
>x : Derived1
}
}

Expand Down Expand Up @@ -104,8 +104,8 @@ function fn4(x: Base|Derived2) {
// 1.5: y: {}
// Want: Derived1
let y = x;
>y : (Base | Derived2) & Derived1
>x : (Base | Derived2) & Derived1
>y : Derived1
>x : Derived1
}
}

Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/typeGuardIntersectionTypes.symbols
View file
Open in desktop
Expand Up @@ -176,37 +176,37 @@ function identifyBeast(beast: Beast) {
>beast : Symbol(beast, Decl(typeGuardIntersectionTypes.ts, 64, 23))

if (beast.legs === 4) {
>beast.legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast.legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast : Symbol(beast, Decl(typeGuardIntersectionTypes.ts, 64, 23))
>legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))

log(`pegasus - 4 legs, wings`);
>log : Symbol(log, Decl(typeGuardIntersectionTypes.ts, 48, 1))
}
else if (beast.legs === 2) {
>beast.legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast.legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast : Symbol(beast, Decl(typeGuardIntersectionTypes.ts, 64, 23))
>legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))

log(`bird - 2 legs, wings`);
>log : Symbol(log, Decl(typeGuardIntersectionTypes.ts, 48, 1))
}
else {
log(`unknown - ${beast.legs} legs, wings`);
>log : Symbol(log, Decl(typeGuardIntersectionTypes.ts, 48, 1))
>beast.legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast.legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast : Symbol(beast, Decl(typeGuardIntersectionTypes.ts, 64, 23))
>legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
}
}

// All non-winged beasts with legs
else {
log(`manbearpig - ${beast.legs} legs, no wings`);
>log : Symbol(log, Decl(typeGuardIntersectionTypes.ts, 48, 1))
>beast.legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast.legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
>beast : Symbol(beast, Decl(typeGuardIntersectionTypes.ts, 64, 23))
>legs : Symbol(legs, Decl(typeGuardIntersectionTypes.ts, 55, 38), Decl(typeGuardIntersectionTypes.ts, 56, 21))
>legs : Symbol(Legged.legs, Decl(typeGuardIntersectionTypes.ts, 56, 21))
}
}

Expand Down
10 changes: 5 additions & 5 deletions tests/baselines/reference/typeGuardIntersectionTypes.types
View file
Open in desktop
Expand Up @@ -166,12 +166,12 @@ function identifyBeast(beast: Beast) {
if (hasWings(beast)) {
>hasWings(beast) : boolean
>hasWings : (x: Beast) => x is Winged
>beast : Beast & Legged
>beast : Legged

if (beast.legs === 4) {
>beast.legs === 4 : boolean
>beast.legs : number
>beast : Beast & Legged & Winged
>beast : Legged & Winged
>legs : number
>4 : 4

Expand All @@ -183,7 +183,7 @@ function identifyBeast(beast: Beast) {
else if (beast.legs === 2) {
>beast.legs === 2 : boolean
>beast.legs : number
>beast : Beast & Legged & Winged
>beast : Legged & Winged
>legs : number
>2 : 2

Expand All @@ -198,7 +198,7 @@ function identifyBeast(beast: Beast) {
>log : (s: string) => void
>`unknown - ${beast.legs} legs, wings` : `unknown - ${number} legs, wings`
>beast.legs : number
>beast : Beast & Legged & Winged
>beast : Legged & Winged
>legs : number
}
}
Expand All @@ -210,7 +210,7 @@ function identifyBeast(beast: Beast) {
>log : (s: string) => void
>`manbearpig - ${beast.legs} legs, no wings` : `manbearpig - ${number} legs, no wings`
>beast.legs : number
>beast : Beast & Legged
>beast : Legged
>legs : number
}
}
Expand Down
12 changes: 11 additions & 1 deletion tests/baselines/reference/typeGuardsWithInstanceOf.errors.txt
View file
Open in desktop
@@ -1,8 +1,12 @@
tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts(7,20): error TS2339: Property 'global' does not exist on type 'never'.
The intersection 'I & RegExp' was reduced to 'never' because property 'global' has conflicting types in some constituents.
tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts(31,11): error TS2339: Property 'onChanges' does not exist on type 'C | (Validator & Partial<OnChanges>)'.
Property 'onChanges' does not exist on type 'C'.
tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts(32,11): error TS2339: Property 'onChanges' does not exist on type 'C | (Validator & Partial<OnChanges>)'.
Property 'onChanges' does not exist on type 'C'.


==== tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts (1 errors) ====
==== tests/cases/conformance/expressions/typeGuards/typeGuardsWithInstanceOf.ts (3 errors) ====
interface I { global: string; }
var result!: I;
var result2!: I;
Expand Down Expand Up @@ -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

!!! error TS2339: Property 'onChanges' does not exist on type 'C'.
v.onChanges({});
~~~~~~~~~
!!! error TS2339: Property 'onChanges' does not exist on type 'C | (Validator & Partial<OnChanges>)'.
!!! error TS2339: Property 'onChanges' does not exist on type 'C'.
}
}

Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/typeGuardsWithInstanceOf.symbols
View file
Open in desktop
Expand Up @@ -71,14 +71,10 @@ function foo() {
>v : Symbol(v, Decl(typeGuardsWithInstanceOf.ts, 25, 7))

if (v.onChanges) {
>v.onChanges : Symbol(onChanges, Decl(typeGuardsWithInstanceOf.ts, 11, 21))
>v : Symbol(v, Decl(typeGuardsWithInstanceOf.ts, 25, 7))
>onChanges : Symbol(onChanges, Decl(typeGuardsWithInstanceOf.ts, 11, 21))

v.onChanges({});
>v.onChanges : Symbol(onChanges, Decl(typeGuardsWithInstanceOf.ts, 11, 21))
>v : Symbol(v, Decl(typeGuardsWithInstanceOf.ts, 25, 7))
>onChanges : Symbol(onChanges, Decl(typeGuardsWithInstanceOf.ts, 11, 21))
}
}

Expand Down
18 changes: 9 additions & 9 deletions tests/baselines/reference/typeGuardsWithInstanceOf.types
View file
Open in desktop
Expand Up @@ -65,21 +65,21 @@ function foo() {
>C : typeof C

v // Validator & Partial<OnChanges> & C
>v : Validator & Partial<OnChanges> & C
>v : C
}
v // Validator & Partial<OnChanges> via subtype reduction
>v : Validator & Partial<OnChanges>
>v : C | (Validator & Partial<OnChanges>)

if (v.onChanges) {
>v.onChanges : ((changes: Record<string, unknown>) => void) | undefined
>v : Validator & Partial<OnChanges>
>onChanges : ((changes: Record<string, unknown>) => void) | undefined
>v.onChanges : any
>v : C | (Validator & Partial<OnChanges>)
>onChanges : any

v.onChanges({});
>v.onChanges({}) : void
>v.onChanges : (changes: Record<string, unknown>) => void
>v : Validator & Partial<OnChanges>
>onChanges : (changes: Record<string, unknown>) => void
>v.onChanges({}) : any
>v.onChanges : any
>v : C | (Validator & Partial<OnChanges>)
>onChanges : any
>{} : {}
}
}
Expand Down