-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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.
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
isArray() preserve mutability and element type #48228
base: main
Are you sure you want to change the base?
Conversation
The TypeScript team hasn't accepted the linked issue #33700. If you can get it accepted, this PR will have a better chance of being reviewed. |
1 similar comment
The TypeScript team hasn't accepted the linked issue #33700. If you can get it accepted, this PR will have a better chance of being reviewed. |
@DanielRosenwasser let's discuss this next week - seems promising |
Not sure if I get this right, but the immutable case only seems to work because both // https://github.com/microsoft/TypeScript/issues/17002
declare const immutable: number | readonly number[];
if (Array.isArray(immutable)) {
const notMutable: number[] = immutable; // ❌ No error: type of immutable is `any[]`
} else {
const narrowed: number = immutable; // ❌ Error: `readonly number[]` is not removed from false branch.
} |
tests/baselines/reference/destructuringParameterDeclaration4.errors.txt
Outdated
Show resolved
Hide resolved
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: ramda
Package: node
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Reopening #42316, with
any
andunknown
backward compatibility: #42316 (comment)There are a couple of related issues here:
Array.isArray()
narrowsreadonly T[]
-> mutableany[]
readonly T[]
ArrayLike<T>
andIterable<T>
#17002 concerns issues 1 and 2. 1 and 3 are pretty similar --- #33700 calls out issue 3, the still-wider type
Iterable<T>
vs.readonly T[]
.#42316 fixed #17002 (1 and 2) but not #33700 (issue 3), by changing the return type from
arg is any[]
->arg is readonly unknown[]
. That preserves mutability and therefore removes it from the false branch. It narrows:T[]
->T[] & readonly unknown[]
(which is mutable because theT[]
component is mutable)readonly T[]
->readonly T[] & readonly unknown[]
(immutable)It also preserves the element type of
readonly T[]
.#42316 was closed because it narrowed
any
andunknown
->readonly unknown[]
vs.any[]
(which is the current behavior), breaking code that relies onunknown
narrowing to mutable, or narrowing to elements of typeany
:The current behavior is exceedingly permissive in those cases. The change added errors that weren't there before, but aren't necessarily correct or incorrect, because
any
andunknown
can be anything. The fix wasn't worth the breakage: #42316 (comment)🩹 Proposed solution
This PR addresses backward compatibility, and also fixes #33700 (issue 3), by instead adding overloads for:
This fixes 1, 2, and 3, for
ArrayLike<T>
,Iterable<T>
, and their narrower types, includingreadonly T[]
, while preserving the current behavior for everything else, includingany
andunknown
.Fixes #17002
Fixes #33700
Fixes #41808
⏯️ Playground link
Workbench repro.
🧑💻 Code
🙁 Actual behavior
🙂 Expected behavior
With this PR: