microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.89k stars 12.47k forks source link

[lib] readonly modifier is missing in a lot of places #47823

Open Conaclos opened 2 years ago

Conaclos commented 2 years ago

lib Update Request

I have code that does not type-check because of missing readonly modifiers.

For instance the following code does not type-check:

declare const vals: readonly number[]
Math.max.apply(null, vals)
                     ^^^^
                     Argument of type 'readonly number[]' is not
                     assignable to parameter of type 'number[]'

Playground link

This is caused by the following signatures:

interface Math {
    max(...values: number[]): number
}

interface CallableFunction extends Function {
    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R
}

These signatures should be replaced by:

interface Math {
    max(...values: readonly number[]): number
}

interface CallableFunction extends Function {
    apply<T, A extends readonly any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R
}

Playground link

Basically every array rest parameter should be readonly, and there is other places in the library where readonly should be added. I already made these changes on a fork (see the last commit of the fork). I'm not sure how to proceed. there is a lot of changes to perform. Maybe the changes should be spitted in several ones?

Note that a lot of changes could be avoided if TypeScript implicitly handled array rest paramter as readonly (regardless the presence or the absence of the readonly modifier). I do not see any value to have mutable array rest parameter. However, this adds a special case in the type-checker, and I am personally not a big fan of that.

Jamesernator commented 2 years ago

However, this adds a special case in the type-checker, and I am personally not a big fan of that.

I wouldn't neccessarily call this a "special case". Rest parameters is a syntactical construct, plenty of other syntatical constructs change the types of things.

In fact this could even be done so that the internal type of the variable is unchanged, but the function rest parameter is wrapped with Readonly. i.e.:

function f(...args: number[]): void {
    args.push(10); // fine
}

f([1,2,3] as ReadonlyArray<number>); // fine as f actually has type (...readonly number[]) => void
DanielRosenwasser commented 2 years ago

I don't think there's a need for any rest parameters to be rewritten (such as in Math.max). At most, you need to rewrite any captured argument list types A as readonly [...A], right? So we could switch the type of args

interface CallableFunction extends Function {
-    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R
+    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: readonly [...A]): R
}
fatcerberus commented 2 years ago

This is already allowed:

declare const vals: readonly number[]
Math.max(...vals);

Which makes sense because spreading an array always makes a copy. So the problem is specific to func.apply().

Conaclos commented 2 years ago

I don't think there's a need for any rest parameters to be rewritten (such as in Math.max). At most, you need to rewrite any captured argument list types A as readonly [...A], right? So we could switch the type of args

interface CallableFunction extends Function {
-    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R
+    apply<T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: readonly [...A]): R
}

This is definitively a better proposal.

Rudxain commented 14 hours ago

Has this issue been resolved?