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

Support readonly type operator to allow correct definition of Object.freeze #10725

Closed
spiffytech opened this issue Sep 6, 2016 · 46 comments
Closed
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@spiffytech
Copy link

TypeScript Version: 2.0@RC

Code

// Current definition of `Object.freeze`:
// freeze<T>(o: T): T;

const o = Object.freeze({a: 1});
o.a = 2;  // Should throw type error
console.log(o);  // {a: 1}

Expected behavior:
Frozen object has readonly properties
Actual behavior:
Type system allows me to write to o's properties

@mhegazy
Copy link
Contributor

mhegazy commented Sep 6, 2016

There is no readonly type operator; i.e. freeze<T>(o: T): readonly T;. so there is no way to do this at the time being.

We have discussed adding a readonly type operator that would recursively mark all properties as readonly.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 6, 2016

updating the title.

@mhegazy mhegazy changed the title Object.freeze declaration should mark fields readonly Support readonly type operator to allow correct definition of Object.freeze Sep 6, 2016
@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Sep 6, 2016
@deregtd
Copy link

deregtd commented Sep 14, 2016

In our huge Typescript app, we're using a data storage mechanism that keeps complicated class-based types inside data stores. The rest of the app consumes these objects from the stores, but there's always a risk of developers trying to modify the objects that come out of the stores, which ruins delta checking across the app. We could use object.freeze(), but that plays havoc with JIT.

Instead, we maintain duplicate deep readonly versions of all of our model objects. The stores distribute readonly versions, and if anyone actually needs to update them, they first clone the readonly object back into an impl object, modify it, and then shove it back in the store. The store takes it in and stores it as a readonly version.

The general data access pattern works fantastically. There's no javascript overhead from object.freeze(), and our engineers are unable to do terrible things with our data models (outside of casting, of course...) The big issue is just that we have a ton of these deep readonly models that we're manually maintaining, and also having to have comments to maintain which methods are readonly-accessible. We're used to just using "const" in C++ to solve this problem, both on the accessors (from stores) and for the readonly-safe methods in a class, so this is definitely a step backward, and has introduced some engineer errors by not keeping the two versions in sync.

At least we have readonly at all, it was a huge boon to us to get that. :) I'm just hoping for a true deep "const"-style readonly someday.

@asfernandes
Copy link

Should not this be closed after #12114 and Readonly<T>?

@deregtd
Copy link

deregtd commented Feb 5, 2017

It depends if there's any hope of a const-style. We haven't bothered using ReadOnly<> because, to be frank, it's worthless to us. We need deep readonly, so we have just created basically deep readonly types as copies of normal types. I don't really understand how shallow readonly helps, unless all of your interfaces are just a list of primitives, without any deep classes on them.

@asfernandes
Copy link

@deregtd I agree with you on worthless of shallow Readonly. But Object.freeze is shallow too and is already changed per this issue.

@deregtd
Copy link

deregtd commented Feb 5, 2017

Fair enough. Should I just open a new one asking for true const?

@asfernandes
Copy link

Isn't what #11535 (referenced in this issue) is already asking for?

@deregtd
Copy link

deregtd commented Feb 5, 2017

Kinda. He's asking to mark interfaces as full readonly. I want to have a basic set of interfaces, and then be able to mark certain usages of them as const, and have it be deep C++-style const, without having to make a complete second copy of all of them. All the attached bugs seem to basically be solved by the partial types business.

@asfernandes
Copy link

asfernandes commented Feb 5, 2017

C++-style const does not do what you say when using references and pointers (and that's how js/ts works), for example:

class Parent
{
public:
    /*const*/ Child* child;
};

const Parent* parent = ...;
parent->child->iCanMutateThis = true;

I think the idea should not be about have a const making a variable readonly, but a form of deeply apply something like Readonly<>. See my #13214 and the referenced issue #12424. Unfortunately, seems #12424 is more about theory than something practical.

@deregtd
Copy link

deregtd commented Feb 5, 2017

I understand the reference issue, but there's still a pretty easy way to do a better version of it in TS.

class SubBlah {
c: string;
}

class Blah {
a: string;
b: SubBlah;

const getA(): string {
return this.a;
}

const getC(): string {
return this.b.c;
}

setC(val: string) {
this.b.c = val;
}
}

function DoStuffConst(d: const Blah) {
const a = d.getA(); //Works
const b = d.getC(); //Works
d.setC('a'); //Doesn't compile
}

function DoStuff(d: Blah) {
const a = d.getA(); //Works
const b = d.getC(); //Works
d.setC('a'); //Works now
}

@gcanti
Copy link

gcanti commented Mar 29, 2017

@deregtd what about

type DeepReadonly<T> = { readonly [K in keyof T]: DeepReadonly<T[K]> }

type T = DeepReadonly<{
  a: {
    b: number
  }
}>

const x: T = { a: { b: 1 } }

x.a.b = 2 // error Cannot assign to 'b' because it is a constant or a read-only property.

@deregtd
Copy link

deregtd commented Mar 29, 2017

Interesting. Will have to play with that concept. That still doesn't get us there (can't have immutable methods and mutable methods, so anything with methods is hosed), but it might help some of our simpler method-less models that we currently have copies of (RO and RW). Thanks!

@asfernandes
Copy link

@gcanti this does not work correctly for embedded arrays.

@gcanti
Copy link

gcanti commented Mar 30, 2017

@asfernandes neither for tuples. Without mapped conditional types is pretty hard to write conditional logic. This is the best result I got so far (with some bad hacks)

declare global {
  interface Array<T> {
    _type: T
    _kind: 'Array'
  }
  interface Object {
    _kind: 'Object'
  }
}

export type Readonlyable = Array<any> | Object

export type DeepReadonlyObject<T> = {
  readonly [K in keyof T]: DeepReadonlyObject<T[K]>
}

export type OneLevelReadonly<T extends Readonlyable> = {
  Array: ReadonlyArray<Readonly<T['_type']>>,
  Object: { readonly [K in keyof T]: DeepReadonlyObject<T[K]> }
}[T['_kind']]

export type DeepReadonly<T extends { [key: string]: Readonlyable }> = { readonly [K in keyof T]: OneLevelReadonly<T[K]> }

type T = DeepReadonly<{
  a: {
    b: {
      c: number
      // bar: Foo // this leads to Element implicitly has an 'any' type
      // baz: Array<number> // this leads to Element implicitly has an 'any' type
    }
  },
  d: Array<number>,
  e: Array<{ foo: number }>,
  f: Array<{ foo: number, bar: { bax: number } }>, // this doesn't work, bax is not readonly
  g: Array<{ foo: number, bar: Array<number> }> // this doesn't work, bar is not readonly
  // the previuos 2 cases must be handled with another type definition like in h
  h: Array<Foo>,
  i: Foo
}>

type Foo = DeepReadonly<{ foo: number, bar: Array<number> }>

declare var x: T

x.a.b.c = 2 // ok, error
x.d[0] = 2 // ok, error
x.e[0].foo = 2 // ok, error
x.f[0].foo = 2 // ok, error
x.f[0].bar.bax = 2 // NO error
x.g[0].foo = 2 // ok, error
x.g[0].bar[0] = 2 // NO error
x.i.bar = 2 // ok, error

@MichaelTontchev
Copy link

Would like to add my voice to the request for a deep, recursive readonly. Would be super useful and make immutability very good in TypeScript.

@lukescott
Copy link

lukescott commented Jun 15, 2017

I can currently use this:

export type Freeze<T> = {
	readonly [P in keyof T]: T[P]
}

Which allows me to set each property as readonly:

type Foo = Freeze<{
  one: string
  two: string
}>
var foo: Foo = {one:"one", two:"two"}
foo.one = "ONE" // error

The issue I have with this is type Foo isn't named, and I lose other benefits of using an interface over a type def.

I would like to be able to do something like this:

readonly interface Foo {
  one: string
  two: string
}

It could be a deep readonly, but I would be ok with it being shallow. Ultimately I'm trying to avoid doing this:

interface Foo {
  readonly one: string
  readonly two: string
}

If someone adds another property, it may not be clear that Foo is supposed to be treated as an immutable type.

@skishore
Copy link

skishore commented Aug 3, 2017

Very nice, @gcanti! I think I was able to extend your example to be closer to working!

declare global {
  interface Array<T> {
    _type: T
    _kind: 'Array'
  }
  interface Object {
    _kind: 'Object'
  }
}

export type Readonlyable = Array<any> | Object

export type OneLevelReadonly<T extends Readonlyable> = {
  Array: ReadonlyArray<DeepReadonly<T['_type']>>,
  // Here's the main difference between my version and yours -
  // in your version, you iterated over the keys of the object here,
  // but I delegate that back to DeepReadonly. That's what
  // makes array values of objects work.
  Object: DeepReadonly<T>,
}[T['_kind']]

export type DeepReadonly<T extends { [key: string]: Readonlyable }> = { readonly [K in keyof T]: OneLevelReadonly<T[K]> }

type T = DeepReadonly<{
  a: {
    b: {
      c: number,
      bar: Foo,
      baz: Array<number>,
    }
    foo: Foo,
  },
  d: Array<number>,
  e: Array<{ foo: number }>,
  f: Array<{ foo: number, bar: { bax: number } }>,
  g: Array<{ foo: number, bar: Array<number> }>,
  h: Array<Foo>,
  i: Foo
}>

type Foo = { foo: number, bar: Array<number> };

declare var x: T
// All have type number!
const a = x.a.b.c;
const b = x.a.b.bar.bar[0];
const c = x.a.b.baz[0];
const d = x.a.foo.foo;
const e = x.a.foo.bar[0];

x.a.b.c = 2 // ok, error
x.d[0] = 2 // ok, error
x.e[0].foo = 2 // ok, error
x.f[0].foo = 2 // ok, error
x.f[0].bar.bax = 2 // ok, error
x.g[0].foo = 2 // ok, error
x.g[0].bar[0] = 2 // ok, error
x.i.bar = 2 // ok, error

Note that things will still break if I make Foo a DeepReadonly object. However, with this method, we can at least use DeepReadonly as a replacement for C++ style "const" on functions, since that use case only ever depends on one level of DeepReadonly being available. That's the main way I wanted to use it, so I'm very pleased. Nice trick with the "_type" / "_kind" way to go from Array -> T!

@skishore
Copy link

skishore commented Sep 13, 2017

One more go: I think I've ironed out a lot of problems. It now works for primitives, for ES6 Map and Set, and for some pretty complex nested structures. Best of all, all you need to do is export the "Const" type and use it, and type T can be assigned to type Const without issue.

I started with some of the ideas from @gcanti and also used the "TupleHasIndex" trick from @tycho01's typical repo. This code only works for me under Typescript 2.5.2 with target ES6.

// An attempt at implementing const-correctness in Typescript.

declare global {
  interface Array<T> {_kind: 'array', _t1: T, _t2: never}
  interface Map<K,V> {_kind: 'map', _t1: K, _t2: V}
  interface Object {_kind: 'object', _t1: never, _t2: never}
  interface Set<T> {_kind: 'set', _t1: T}
};

export type Const<T extends Array<any> | Map<any,any> | Object | Set<any>> = {
  array: ConstArray<T & {length: number}>,
  map: ReadonlyMap<Readonly<T['_t1']>,Readonly<T['_t2']>>;
  object: ConstObject<T>,
  set: ReadonlySet<Readonly<T['_t1']>>,
}[T['_kind']];

type ConstArray<T extends ArrayLike<any>> = {
  0: ReadonlyArray<Const<T[0]>>,
  1: {
    0: Readonly<[Const<T[0]>]>,
    1: {
      0: Readonly<[Const<T[0]>,Const<T[1]>]>,
      1: {
        0: Readonly<[Const<T[0]>,Const<T[1]>,Const<T[2]>]>,
        1: Readonly<[Const<T[0]>,Const<T[1]>,Const<T[2]>,Const<T[3]>]>,
      }[TupleHasIndex<T,'3'>]
    }[TupleHasIndex<T,'2'>]
  }[TupleHasIndex<T,'1'>]
}[TupleHasIndex<T,'0'>];

type ConstObject<T extends {[key: string]: any}> =
  {readonly [K in keyof T]: Const<T[K]>};

type TupleHasIndex<A, I extends string> =
  ({[K in keyof A]: '1'} & {[key: string]: '0'})[I];

// Example usages. Note that the only interface you need is Const.
// You can check that things are working correctly by using the Typescript server's
// GetType call on these values. You'll see that they reduce to simple types with
// "readonly" and Readonly[Array|Map|Set] where appropriate.
declare const a: Const<number>;
declare const b: Const<string>;
declare const c: Const<boolean>;
declare const d: Const<Map<number,{a: boolean, b: string}>>;
declare const e: Const<Set<string>>;
declare const x: Const<number[]>;
declare const y: Const<[number, string, boolean, {a: number, b: string}]>;
declare const z: Const<{a: number[], b: {c: string, d: string[]}[]}>;

type MyType = [number[], {a: string, b: boolean}];
const value: MyType = [[1, 2, 3], {a: 'test', b: true}];
const f: Const<MyType> = value;

I'll wrap this up in a library and make it available.

@KiaraGrouwstra
Copy link
Contributor

@skishore: interesting, that looks like something that'd be nice for typelevel-ts / typical as well :D, though I hope we could get to an iterative solution not limited to x layers.
Coincidentally, there's been a few attempts at #12424 as well recently.

I think one of the tough parts right now is still how to deal with objects with string/number index types. That is to say, { a: 123 } may work, but it's currently tough to handle both that and { a: 123; [k: string]: number } in the same algorithm... that one requires obtaining its index type (done by accessing o[string]), but that just errors when done on an object without it.

I'd imagined that #6606 would finally allow making a check to confirm whether there is such an index, but in my current PR for that (#17961) found that this isn't actually the case. At this point I have no idea how we might go about tackling that part... which would definitely be more unfortunate if it would recursively cause information loss throughout the object structure.

@skishore
Copy link

skishore commented Sep 13, 2017

There's no limitation for the approach above on layers - the only numeric limit is tuple length, which is a pretty standard thing in Typescript definitions that I've seen.

I also haven't seen any problems with mixed index types as you suggest - I just tried one out. Admittedly, there were a few bugs in that initial code listing. I'm ironing those out now as I apply const-correctness to one of my projects.

There is a very serious issue I've found, though. Typescript seems to drop readonly object fields at will when calling a function. This problem is unrelated to the Const type, as you can see here:

type Record = {a: number, b: string};
const test = (record: Record): void => { record.a = 2; };
const record: Readonly<Record> = {a: 1, b: 'test'}; /* fields are readonly */
test(record); /* No compile error! */

So...what's the point of readonly if you can accidentally cast it away by calling a function?

@AlexGalays
Copy link

@niieani

That's some serious type manipulation haha

Does it cover all known test cases so far?

@KiaraGrouwstra
Copy link
Contributor

@AlexGalays: the following we can't preserve yet, limitations not of his type but of our set of operators we can use today:

  • tuples
  • string indices
  • symbols

Note that at #12424 they're similarly trying to lift a key based operator to become 'deep', in their case partial/? rather than readonly.

@AlexGalays
Copy link

@tycho01 Strange, string indices (or Records) appear to work just fine: They're read-only and the value type you read follows the index declaration.

@KiaraGrouwstra
Copy link
Contributor

@AlexGalays: the tough part is ideally we'd wanna preserve explicitly defined indices throughout operations, but I think we don't have a way to check for them.
What we can do is try to access them, but if we do while they weren't there, the type just errors, which we cannot currently 'catch'.

@pelotom
Copy link

pelotom commented Dec 24, 2017

I just want to submit that ideally a solution here could also solve another problem which is one of my biggest gripes with TypeScript: unnecessary type widening. Since readonly fields should not be widened, it seems like literals passed to Object.freeze should not have their types widened:

const o = Object.freeze({ x: 3, y: 'hello' });
// o: { x: number; y: string }
// but would ideally be
// o: { readonly x: 3; readonly y: 'hello' }

In the linked issue I proposed a readonly term operator for this purpose, but it would actually be preferable if it were possible to express a type for Object.freeze that could accomplish this, since then I could give the same type to a function which did nothing but return the original object, eliminating the runtime overhead if all I want is the static check. Thus being able to type Object.freeze such that it inferred the type { readonly x: 3; readonly y: 'hello' } in the above example would be a strictly more powerful feature and would kill two birds with one stone.

@KiaraGrouwstra
Copy link
Contributor

I think the DeepReadOnly implementation in #21316 should mean this issue is now resolved.
All that seems left is to have it added to lib.d.ts and have the Object.freeze definition adjusted accordingly, though technically they seem outside of what was asked here.

@ikokostya
Copy link
Contributor

ikokostya commented Feb 11, 2018

One note: DeepReadOnly implementation in #21316 removes all function properties from provided object, but Object.freeze doesn't.

@alecmev
Copy link

alecmev commented Apr 15, 2018

Not sure why function props are singled out in #21316. Perhaps to showcase the usage of NonFunctionPropertyNames<T>? Regardless, unless I'm missing something, this ought to do the trick:

type DeepReadonly<T> =
  T extends any[] ? DeepReadonlyArray<T[number]> :
  T extends object ? DeepReadonlyObject<T> :
  T;

interface DeepReadonlyArray<T> extends ReadonlyArray<DeepReadonly<T>> {}

type DeepReadonlyObject<T> = T &
  { readonly [P in keyof T]: DeepReadonly<T[P]> };

Playground.

Note the union in DeepReadonlyObject<T>, it takes care of both regular functions and hybrid interfaces-functions, like this one:

interface A {
  (): string;
  prop: number;
}

Just in case, I don't know TS that well, this may have unexpected side effects.

swpease added a commit to swpease/codenames that referenced this issue Jul 1, 2018
It was a bit concerning having to deal with worrying about mutations, so I decided to just write out the stuff two-ish times. My concerns were along the lines of, e.g. microsoft/TypeScript#10725 , and as a side note, it's a bit annoying that I have to worry about mutating prevState in this.setState.
@karlhorky
Copy link
Contributor

This can be closed now that the freeze method uses the Readonly and ReadonlyArray generic utility types, I take it?

TypeScript/src/lib/es5.d.ts

Lines 195 to 211 in b36c8a0

/**
* Prevents the modification of existing property attributes and values, and prevents the addition of new properties.
* @param o Object on which to lock the attributes.
*/
freeze<T>(a: T[]): ReadonlyArray<T>;
/**
* Prevents the modification of existing property attributes and values, and prevents the addition of new properties.
* @param o Object on which to lock the attributes.
*/
freeze<T extends Function>(f: T): T;
/**
* Prevents the modification of existing property attributes and values, and prevents the addition of new properties.
* @param o Object on which to lock the attributes.
*/
freeze<T>(o: T): Readonly<T>;

@deregtd
Copy link

deregtd commented Jul 4, 2019

Maybe for the original post, yes, but then I will have to break my follow up post into a separate one for deep read only since that is only one level deep. :)

@karlhorky
Copy link
Contributor

Yep, should probably close the original issue and open a followup asking for a built-in Utility Type for DeepReadonly.

Since the implementation above and in #21316 (comment) is written by ahejlsberg, maybe it's not too far away from a the quality for inclusion?

cc @spiffytech

@KiaraGrouwstra
Copy link
Contributor

@karlhorky

maybe it's not too far away from the quality for inclusion?

well type recursion is not deemed a supported use of the language so far :/

@lazytype
Copy link

I propose the issue be closed now. Read-only array types have been around for a year, which means the following works as expected. @DanielRosenwasser @RyanCavanaugh

type DeepReadonly<T> = { readonly [K in keyof T]: DeepReadonly<T[K]> };

type Foo = { a: { b: { c: [ { x: 'y' } ] } } };
const foo: Foo = { a: { b: { c: [ { x: 'y' } ] } } };

type ReadonlyFoo = DeepReadonly<Foo>;

const readonlyFoo: ReadonlyFoo = foo;

// All type errors
readonlyFoo.a = readonlyFoo.a;
readonlyFoo.a.b = readonlyFoo.a.b;
readonlyFoo.a.b.c = readonlyFoo.a.b.c;
readonlyFoo.a.b.c[0] = readonlyFoo.a.b.c[0];
readonlyFoo.a.b.c[0].x = readonlyFoo.a.b.c[0].x;

@MartinJohns
Copy link
Contributor

I'd keep it open, but adjust the issue instead. Given the mention of Object.freeze it's clear that he (and I as well) seek to represent immutability in the type-system, which currently is not supported.

@RyanCavanaugh
Copy link
Member

I agree this is effectively solved for the use cases in OP and related posts. New issues welcomed for further scenarios.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jul 5, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests