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.

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

Generic Function with Indexed Type ignores readonly #18374

Closed
Nathan-Fenner opened this issue Sep 11, 2017 · 6 comments
Closed

Generic Function with Indexed Type ignores readonly #18374

Nathan-Fenner opened this issue Sep 11, 2017 · 6 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Nathan-Fenner
Copy link
Contributor

Nathan-Fenner commented Sep 11, 2017

TypeScript Version: 2.4.2

Code

function mapAssign<T, K extends keyof T>(obj: T, k: K, f: (v: T[K]) => T[K]) {
    obj[k] = f(obj[k]);
}

type NoChange = {
    readonly value: number
}
const obj: NoChange = {value: 5};

mapAssign(obj, "value", x => x+1);

Expected behavior:
Since NoChange.value is readonly, it should not be modifiable.

Actual behavior:
The code compiles and runs, modifying obj.value.

This might just be a design limitation that can't be worked around.

If a stricter check was desirable, I could imagine it being something like this:

  • if K extends keyof T and k: K and x: T, then x[k] = e would fail, but T = {readonly [k]: any} would be okay
  • if K extends !readonly keyof T and k: K and x: T then x[k] = e would succeed, but T = {readonly [k]: any} would fail

So the above program would become

function mapAssign<T, K extends !readonly keyof T>(obj: T, k: K, f: (v: T[K]) => T[K]) {
    obj[k] = f(obj[k]); // okay, since K extends !readonly keyof T
}
mapAssign(obj, "value", x => x+1); // error: Const.value is readonly

An alternative change (which I think is much less ergonomic) would be the following:

  • if K extends keyof T and k: K and x: T then x[k] = e would succeed, but T = {readonly [k]: any} would fail
  • if K extends readonly keyof T and k: K and x: T then x[k] = e would fail but T = {readonly [k]: any} would succeed.

So the program would be

function mapAssign<T, K extends keyof T>(obj: T, k: K, f: (v: T[K]) => T[K]) {
    obj[k] = f(obj[k]);
}
mapAssign(obj, "value", x => x+1); // error: Const.value is readonly
@olegdunkan
Copy link

Most significant change is that, in order to ensure backwards compatibility, the readonly modifier doesn't affect subtype and assignability type relationships of the containing type (but of course it affects assignments to individual properties).
#6532 (comment)

You can do so:

function mapAssign<T, K extends keyof T>(obj: Readonly<T>, k: K, f: (v: T[K]) => T[K]) {
    obj[k] = f(obj[k]); //error
}

type NoChange = {
    value: number
}
const obj: NoChange = {value: 5};

mapAssign(obj, "value", x => x+1);

@Nathan-Fenner
Copy link
Contributor Author

Note that the solutions I proposed does not affect subtyping or assignability of containing types, only the keys of those types. (Unfortunately, it's in a somewhat backwards incompatible manner).

The sample you gave does not resolve the problem. mapAssign is a perfectly reasonable function whose implementation should not produce an error. It is only incorrect to use it on readonly keys.

@olegdunkan
Copy link

My example doesn't solve your problem but mentions that we can use Readonly in places we want to restrict write access

@Nathan-Fenner
Copy link
Contributor Author

The purpose of this issue is to illustrate a case where readonly fails to restrict write access.

function modify<T, K extends keyof T>(k: K, x: T, f: (v: T[K]) => T[K]) {
    x[k] = f(x[k]);
}
type Point = {
    x: number,
    y: number,
}
const myConstant: Readonly<Point> = { x: 4, y: 7 };
modify("x", myConstant, x => x + 1);
console.log(myConstant) // => 5

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Sep 12, 2017
@RyanCavanaugh
Copy link
Member

Same comment above about backward compatibility applies

@mhegazy
Copy link
Contributor

mhegazy commented Oct 2, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Oct 2, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants