tc39 / proposal-collection-methods

https://tc39.github.io/proposal-collection-methods/
Other
172 stars 8 forks source link

Addition for freezing collections #22

Closed bradennapier closed 5 years ago

bradennapier commented 6 years ago

As mentioned in https://github.com/tc39/proposal-set-methods/issues/30 it would be nice to have a way of "freezing" (or some other name if "freeze" doesn't match perfectly) collections seems ideal.

bakkot commented 6 years ago

Continuing discussion from that issue:

i implement with

I'd be very wary of that particular solution. If TC39 adds another method which mutates the underlying set, this loses its frozenness guarantee. Better to explicitly list out the methods which can be supported.

however it seems like something that does make sense on the language level since we have the ability to freeze other data types

We have the ability to freeze objects. But that has to be done at a language level, because the syntax for modifying objects is built in and (before proxies) not deniable. For things like sets, it's doable in userland.

bradennapier commented 6 years ago

I'd be very wary of that particular solution. If TC39 adds another method which mutates the underlying set, this loses its frozenness guarantee. Better to explicitly list out the methods which can be supported.

Good point! Thanks


I think it'd still be beneficial. After all most if not all of these collection methods are also implementable in the user land ;-). I think your example of why the proxy isn't the best solution points to why a freeze makes sense for collections. In that case we also wouldnt end up supporting new methods added that don't mutate (like when all these methods are added ;-))

ljharb commented 6 years ago

Also repeating my comment from there:

I think it would be a bad idea to have two different concepts of “frozen” in the language - one that prevents property changes, and another that makes internal slots immutable.

I like the idea of making sets/maps freezeable, to be clear - I’m just not sure how doing it would fit into the language.

Note as well, that any convention for making internal slots immutable should also apply to regexes, promises, and dates, to name a few - which means it should probably be a separate proposal.

cc @erights

bradennapier commented 6 years ago

Yeah I don't think there's really any ideal way of implementing this type of functionality without major drawbacks that are not invisible to the user for the most part. Only solution is to change it to an object then freeze it right now if I want to be able to return a Map or Set with its useful functionality such as .has() and .get() (as well as flexible key types) but I don't want to worry that a single function call in the callers code will break my own.

I could clone it on every return but that is also not ideal by any means.

Also as pointed out in the linked topic, there are other situations that using a Proxy and/or a custom class would not really do the job and/or would break if and when new things are added.

hax commented 5 years ago

Agree to @ljharb , two different concepts of “frozen” in the language is bad.

As my observation, in most cases, programmers don't really need "frozen" version, but only a weaken readonly view which disallow any mutation methods, TypeScript ReadOnly<T> satisfy this but only in compile-time, currently programmers use wrapper or Proxy to ensure runtime readonly.

bradennapier commented 5 years ago

A proxy would not work though as new methods that mutate could easily be introduced and would need to be accounted for. Otherwise the other method means new methods just wouldn’t be supported even if they only read.

It seems to make sense to support this within the collections directly.

Fine with me if there was a way to just make the write methods throw an error when the collection is set to read only. Just sucks to have to copy the whole collection any time it may be exposed to an unknown user from lib or similar.

hax commented 5 years ago

@bradennapier

A proxy would not work though as new methods that mutate could easily be introduced and would need to be accounted for. Otherwise the other method means new methods just wouldn’t be supported even if they only read.

Yes, we use whitelist of immutable methods, so new readonly methods would not be supported. In practice it's not a big deal, if we add new code which use new methods, we could also update the whitelist. 😊

Anyway, I would be happy if we can have X.prototype.readonly which returns a readonly view of a collection.

Here is a experimental implementation of X.prototype.readonly: https://gist.github.com/hax/fa6a5c6bd62364b64c4829be68493fd0

bradennapier commented 5 years ago

Awesome yeah essentially what I did https://github.com/tc39/proposal-set-methods/issues/30 here but I was going opposite way which is not as safe obviously.

Thanks!

bakkot commented 5 years ago

I do personally like readonly better as a name than frozen.

But I'm not entirely sure it's a good idea to have built-in support for readonly views of collection which remain mutable. I feel like such views are often quite confusing: you don't get the guarantees one usually expects out of immutable data structures.

It's obviously doable in userland if you really need it, and there are appropriate use cases if you know what you're doing - for example when creating such a view and immediately dropping all references to the underlying mutable container - I just don't know that it's a concept we want to bless.

bradennapier commented 5 years ago

I can’t tell you how many times I run into situations where I want to use collections in libs but don’t because I hate worrying about passing it to a user then having them mess with it. Cloning every time also is not ideal for obvious reasons.

In that case passing a readonly version would be nice but can see how having both results in situations that make things confusing and error-prone.

Definitely want to figure out an ideal solution, however. Userland complexity for this just seems like too much and with how simple a “.clear()” makes it to cause issues to the less informed, it’d be a good addition.

A .lock() perhaps. Or even .readOnly(). Or .immutable() but that comes with a whole new set of semantics to consider and wouldn’t exactly match here 😇

The overall dev experience of collections is just too strong I’d hate to have to defer to arrays and objects when collections make more sense. I’ve grow to love my Map and Set :)

bakkot commented 5 years ago

Here's a summary of the issue as I see it:

  1. Passing someone a collection shouldn't require passing them the ability to modify it. (Currently it does.)
  2. Passing someone a mutable collection shouldn't require passing them the ability to lock it. (A .lock prototype method would.)
  3. Immutable structures shouldn't change. (Readonly views would allow this.)
  4. Cloning is expensive.
  5. If you have a handle to a mutable collection, it's surprising if it becomes immutable without you touching it. (A .lock prototype method would allow this.)

I don't mean to imply the above are all equally severe, just that they're all considerations. That leaves no particularly good options among the obvious choices. We could pick one - my preference would be for the status quo or readonly views - or explore more subtle options.

As it happens, there's some prior art in Proxy.revokable (in JS) and AbortController (in HTML). Proxy.revokable seems more apt; I just mention both for completeness. Both allow you to create a thing which you can modify in a particular way (revoking a proxy or aborting a fetch respectively) and also pass that thing around without passing the ability to perform that modification, by keeping the capability to perform the modification on a separate object (the revoke method or AbortController).

So, following the example of Proxy.revokable, one possible option: a LockableMap type (better names would be bikeshedded) with an API similar to:

let { map, lock } = Map.lockable(iterable);
map.set(a, b); // works
lock();
map.set(a, b); // throws

which allows you to transition from mutable to immutable and solves all of the problems above except 5 at the cost of a somewhat more awkward API. It also requires some care in that it relies on the person producing the class to lock it at the appropriate time.

The bifrucan library for Java has a kind of similar idea, though it relies on being able to trust consumers not to turn your immutable thing back into a mutable one.

I realize this is a pretty awkward design; just thought I'd throw it out there. Thoughts?


A partial polyfill, assuming private fields:

// this class would not be exposed
class LockableMap {
  #map;
  #locked = false;
  constructor(iterable) {
    this.#map = new Map(iterable);
    return { map: this, lock: () => { this.#locked = true; } };
  }
  set(k, v) {
    if (this.#locked) {
      throw new TypeError('collection is locked');
    }
    this.#map.set(k, v);
  }
  get(k) {
    return this.#map.get(k);
  }
  // etc
}

function lockableMap(iterable) {
  return new LockableMap(iterable);
}

(Open question: would the methods on native LockableMap objects have distinct identity from the Map.prototype methods, or would the Map.prototype methods be modified to work on LockableMap objects?)

erights commented 5 years ago

E collections http://www.erights.org/elang/collect/tables.html http://www.erights.org/javadoc/org/erights/e/elib/tables/ESet.html all use the following pattern:

There is an abstract supertype (ESet, EMap, EList) that defines only the query methods, not the mutation methods.

There are three concrete subclasses (ConstSet, FlexSet, ReadOnlySet) (ConstMap, FlexMap, ReadOnlyMap) (ConstList, FlexList, ReadOnlyList), where the Flex define mutation methods. The Const and ReadOnly do not define any API beyond that defined by the common supertype. Rather, they provide a behavioral contract over that supertype: Const guarantees that the contents of the collection won't change. This isn't transitive. A Const collection can still contain mutable objects. A ReadOnly collection cannot provide its clients any ability to change the collection, but they provide a read-only view of a collection that might be mutated by other means.

All these collections provide the following methods:

I could imagine that we could do something similar for many existing JavaScript collections: Sets, Maps, and even typed arrays. But I doubt we could succeed at retrofitting such a thing to regular arrays.

It is interesting to contemplate applying this pattern to the weak collections but makes my head hurt. If it could work smoothly that would probably be good, but I don't yet know if that makes sense.

bakkot commented 5 years ago

It is interesting to contemplate applying this pattern to the weak collections but makes my head hurt. If it could work smoothly that would probably be good, but I don't yet know if that makes sense.

Because these patterns can be implemented in userland in terms of a (currently impossible) .clone method for weak collections, which is a problem I have thought about, I'm reasonably confident it makes sense and is not dangerous.

ljharb commented 5 years ago

Do you think we could change the prototype chain of existing collections so that the mutable ones were subclasses of the readonly ones? If so that would be very exciting.

erights commented 5 years ago

@bakkot .diverge() seems to subsume .clone() and is a better API. I would expect a .clone() of a Const to return in a copy of the Const which would be mostly pointless.

@ljharb It would be a worthy experiment but I doubt it. OTOH, we could still get the effect by separate classes that follow these supertype relationships structurally without actually using inheritance / subclassing.

bradennapier commented 5 years ago

I really like where this is going.

I may be misunderstanding @erights but I'd personally prefer any situation that we forego cloning during this process since we can also do return new Map(map) and forego worrying about the original map being mutated in a way that will cause side effects. The .clone() really is just new Map(map) or new Set(set) and .diverge() would basically be doing the clone with a lockable collection if we went that route but with lockable I believe we could forego cloning.

While the LockableMap has a bit of an odd API - I agree that since Proxy has the revoke using similar it is attractive, leaning on a precedent is always easier for people to adopt. It also has the benefit of being a fairly simple implementation. Regardless, utilizing a static function to provide an alternative model is fairly easy to grok even if a different API for it is considered IMO. It seems it would provide all the capabilities mentioned by @erights which all do seem appealing to have as well by using a combination of features implemented already.

Also no question this would be at surface level only, not on objects within the collection itself. I don't think JS is in a place that we can consider deep immutability. Libraries like immutable.js (or perhaps my lil lib https://github.com/odo-network/immuta which has naive support for deep immutable collections using a proxy-on-read concept). Although some sort of native performant immutability would make me very happy if it was done at some point :).


Next question is around if this makes sense as part of this proposal or it ends up being its own as mentioned previously by @ljharb - personally I think it makes sense to be apart of this one.

Note as well, that any convention for making internal slots immutable should also apply to regexes, promises, and dates, to name a few - which means it should probably be a separate proposal.

hax commented 5 years ago

@bakkot

I do personally like readonly better as a name than frozen. But I'm not entirely sure it's a good idea to have built-in support for readonly views of collection which remain mutable. I feel like such views are often quite confusing: you don't get the guarantees one usually expects out of immutable data structures.

Actually I tried to trap isExtensible() to return false for readonly view, but I found Proxy do not allow this. So in reflection layer, frozen/sealed objects will never be confused with readonly/unmodifiable views though it's always hard to say how programmers understand the terms.

It's obviously doable in userland if you really need it,

Writing the readonly views case by case in userland is very inconvenient if you do not use Proxy, and even use Proxy, it's hard to be correct (I just found my previous experimental implementation had a issue that I forgot to trap other mutable operations like set, setPrototypeOf, defineProperty, etc. which allow the user modify the original target via the readonlyview), and even they are correct, it's not easy to decide and align the semantics of all userland implementations. For example, should readonlySet instanceof Set be true? (my implementation currently returns true though as LSP it should not)

I just don't know that it's a concept we want to bless.

Both Java and C# bless it.

bradennapier commented 5 years ago

Definitley not against it but would be in favor of a LockableMap regardless. I really like it and it would also potentially allow for things like Flow to make assumptions for these types of collections which is appealing to me personally (similar to how they behave when Object.freeze() is utilized).

For example, if a collection is locked, a refine using .has(value) can safely be maintained even if untyped functions are called later in the function body.

ATM collections are pretty painful when it comes to things like TypeScript and Flow although not sure it makes sense to consider these details much here.


That being said, there are definitely cases for libs where we would want to pass a readOnly view of a collection but still be able to mutate it internally.

I would also consider the different semantics of collections for iterations though which may be relevant

let i = 0;
const set = new Set([i++, i++, i++])
set.forEach(value => { console.log(value); set.add(i++) })

Since this causes an endless loop - a readOnly view received from a lib could be dangerous as should the libs functions be called within a loop it could easily produce an endless loop. This is obviously an issue today as well and not the case for standard objects. For obvious reasons a Lockable collection resolves this concern as well.

erights commented 5 years ago

One disadvantage of the LockableMap approach is that the locked collection still has all the mutation methods in its API, though they now all throw. Java collections made this mistake as well, ironically when they introduced the highly typed collections API.

I prefer API designs where I can generally use the things that I can see. IOW, when restrict visibility when restricting possible action.

hax commented 5 years ago

@erights

There are three concrete subclasses (ConstSet, FlexSet, ReadOnlySet)

Greate APIs!

Java also have fixed-size collections, which you can get/set but can't add/remove, though I don't know whether fixed-size collections have many use cases.

@ljharb

so that the mutable ones were subclasses of the readonly ones?

It's not easy for average js programmers understand why readOnlySet instanceof Set is false and set instanceof ReadOnlySet is true. I would prefer readOnlySetView which is not subclass/superclass of Set so programmers would not expect instanceof to be true in either direction.

bakkot commented 5 years ago

@erights I agree, though of course Proxy.revokable has the same problem. One solution is to have lock (or equivalent) return a new collection which is immutable while invalidating the old one. But this trades one API awkwardness for another.

erights commented 5 years ago

Hi @bradennapier , I don't understand the example. It already has an infinite loop without using readOnly views. LockableMap would only help if the collection is locked. If it is locked before the loop is started, how is it different from

let i = 0;
const set = new Set([i++, i++, i++]).readOnly();
set.forEach(value => { console.log(value); set.add(i++) });

As with LockableMap, this fails when calling set.add. Unlike LockableMap, a type checker might detect that the call to set.add is erroneous, catching the error earlier.

hax commented 5 years ago

I agree that since Proxy has the revoke using similar it is attractive, leaning on a precedent is always easier for people to adopt.

As my experience, 90% js programmers never use Proxy directly, 90% js programmers even never know Proxy.revocable API. On the other side, I expect collection.snapshot()/readOnly()/diverge() would be used frequently in daily programming. So I don't think LockableMap is a good API for most js programmers.

erights commented 5 years ago

@hax I agree. Set should not be a subtype of ReadOnlySet, since Set does not obey the behavioral contract of ReadOnlySet. Rather, the common supertype should be one that defines only the query methods. ReadOnlySet and Set would both be subtypes whose behavioral contract is consistent with, but more specific than, the behavioral contract of the common supertype.

Everything I say above re ReadOnlySet also applies to ConstSet. As I write this, I notice something I never noticed in the E days: ConstSet also obeys the behavioral contract of ReadOnlySet, and so could consistently be a subtype of ReadOnlySet. Then, c.readOnly() could be typed as returning a ReadOnlyX. This gives us the pleasing:

for all collections x.

bradennapier commented 5 years ago

@erights yes i was indicating that in the case of a lockable map vs the previously mentioned readOnly view which can still be mutated using the original collection it can cause the issue.

aka:

// lib file
const set = new Set();
export function getSet() { return set.readonly };

export function doSomething() { set.add(someValue) };

now since set is mutable but the user has a read only view - they may think it safer to do something like

// user file
import { getSet, doSomething } from 'lib';

getSet().forEach(value => { doSomething() });

which would cause a confusing issue due to the disconnect between a library author and a consumer. Obviously it's about knowing not to loop collections you don't control in such a manner (or cloning them first) - but it's also detail that I personally think will be lost on many and a potential pain point since its not behavior seen in standard objects.

Obviously the situation is more obvious in this case but I can see many situations this could happen where some function called multiple frames after the iteration is called synchronously without considering that it may potentially be within the iterator.

So was just indicating that in the same case a locked collection wouldn't have such issues and would provide a lot easier to understand overall experience. Of course the downside being that once it is locked, it is locked and can't be mutated at all anymore.

Just considering various situations.

hax commented 5 years ago

@bradennapier

Export both readonly view and the ability of mutating original collection is just weird. Even without readOnly() api, you can still write such bad libs now. So I think it's the duty of the lib authors to use language facilities correctly and provide reasonable APIs for clients. I don't see locked collection have any difference with readonly collections on this requirement.

bradennapier commented 5 years ago

@hax I agree completely. In practice such responsibility doesn't seem to be the case too often with JS libs. At least in my experience. I personally review the source of any lib I would add to any project extensively but I can say with certainty many do not.

Just bringing up considerations. I'd imagine the actual situations that such a problem would occur would be a lot less obvious.

Either way for my personal worries all these concepts work in the end so I am happy regardless ;-).

hax commented 5 years ago

Actually LockableCollection API is BAD and introduce unnecessary burden to programmers in most cases.

const set = apiReturnsLockableSet()
set.add(1) // would it throw?
... // some other code which may trigger lock() indirectly
set.add(2) // would it throw?
await void 0 // lock() can be triggered any time
set.add(3) // would it throw?

Actually lock() can be called any time! So you always need to read the documentation of apiReturnsLockableSet() very carefully and pray the real behavior follows documentation.

bradennapier commented 5 years ago

Is this not also the case today with Revocable Proxies or even Object.freeze()? I do agree its not a perfect API, just the precedence is appealing. I'm definitely a fan of staying with convention wherever possible to reduce cognitive overhead of the language as a whole.

I mean I would imagine the lockable sets would be used to pass a locked set in almost all cases. I love Object.freeze() myself and think it works well since I use it in cases that an object shouldn't have been mutated anyway but I want to be absolutely certain there isn't some condition it's done unintentionally via references. I'd rather be safe with loud errors to the user of their mistake.

const set = new Set();
// makes the sets mutating methods throw
// similar to Object.freeze()
Set.lock(set);

Has same issue you mention, but again, it's a pattern already in place without adding new concepts to understand.

ljharb commented 5 years ago

I would expect, in an ideal world, that new MutableSet instance of MutableSet, new Set instance of Set, new MutableSet instanceof Set, and that Set would be immutable and would not have any mutation method on it (no add or delete, eg), and that MutableSet would be what we think of as Set now.

Obviously that ship has sailed, but it would be unfortunate if an immutable set couldn’t be used with a mutable set’s non-mutation methods, and a mutable set with an immutable set’s methods.

hax commented 5 years ago

@ljharb

it would be unfortunate if an immutable set couldn’t be used with a mutable set’s non-mutation methods and a mutable set with an immutable set’s methods.

This should be possible (my previous proxy-based implementation satisfy this), actually ReadOnlySet could use same methods of Set, just exclude all mutation methods.

hax commented 5 years ago

@bradennapier

Is this not also the case today with Revocable Proxies or even Object.freeze()?

As my previous comment point out, the key difference is Revocable Proxies is not common cases in daily programming. We accept the complexity of revocability for special use cases because such complexity is essential in these cases (for example, remote objects), but immutable/readonly is too common so we'd better give programmers a simpler API without the unnecessary ability/burden of locking an object in any time.

I don't see much usage of Object.freeze() in the wild (maybe some libraries use it internally?). And it doesn't use const {freeze, o} = new FreezableObject() form. 😝

Though in theory any objects can become frozen in any time (i.e. Object is really FreezableObject), most JS programmers never worry about it because they mostly don't use freeze() 😅 . And we may assume even someone use it, they use it in the form return Object.freeze({...}) which means in most cases when you get an object, it is already frozen or never frozen. So no burden like LockableX in practice.

Basically, I always prefer x = LockedX() to {lock, x} = LockableX().

And ImmutableX or X.immutable() or X.prototype.snapshot() are better names than LockedX.

bradennapier commented 5 years ago

Agree on immutable but can see people complaining about use of the word.

I would argue Object.freeze() is starting to be used a good amount more as I've seen many posts lately using it as a default as well as Flow recommending it to make their typing work (significantly) better to represent their {| +value: string |} exact object semantics (personally would love for {| value: 'hi' |} to be supported syntax for freeze, but diff story!).

In my needs and with my concerns for making this post, I personally would only ever really use it as mentioned so Set.locked(set) or any version of it is actually the 100% ideal solution for me personally and has a clean simple api. Although I understand there may be other use cases where it may not be ideal to operate this way.

https://medium.freecodecamp.org/elegant-patterns-in-modern-javascript-ice-factory-4161859a0eee https://github.com/facebook/flow/releases/tag/v0.60.0

First attempt to use github search for such a concept but initial check on code search seems to return a ton of values and it cuts it off due to taking to long

https://github.com/search?q=%22Object.freeze%28%22+language%3AJavaScript&type=Code

but clearly a ton of those are clones - would be nice if it were possible to filter out duplicates :-P

bakkot commented 5 years ago

@hax:

apiReturnsLockableSet which means in most cases when you get an object, it is already frozen or never frozen

The assumption with LockableMap is that you would not generally return an unlocked version, so this wouldn't really be an issue. You'd lock it as soon as you were done constructing it and ready to hand it off.


The main thing the LockableMap API gives you which the {Const, ReadOnly, Flex} API does not is that it allows you to precisely express "this collection will not be modified", which is a much stronger guarantee than "this collection cannot be modified by you", without having to pay the cost of fully immutable data structures or a full clone.

The awkwardness in the design there was that I was attempting to satisfy the constraint that you could pass the map around in its mutable state without passing the ability to lock it. This has some niche applications, but the benefit above can be attained even if passing the structure in its mutable state implies passing the ability to lock it; the assumption is that this is not usually something you'd want to do and we could perhaps just say that the language won't provide support for it. So an alternative design:

let wip = Map.finalizable();
wip.set(a, b);
wip.get(a); // b
let ret = wip.finalize();
ret.get(a); // b
'set' in ret; // false
wip.get(a); // throws, `wip` is detatched

Common usage would be

let map = Map.finalizable();
for (let x of xs) {
  map.set(x, f(x));
}
return map.finalize();
bradennapier commented 5 years ago

In that circumstance I definitely would rather lean on semantics of Object.freeze and have

cosnt map = new Map();
// ... do stuff, mutate, do whatever
return Map.lock(map);

and as is the case with Object.freeze, the following is equivalent

cosnt map = new Map();
// ... do stuff, mutate, do whatever
Map.lock(map);
return map

It could remove the methods easily enough if needed, but it doesn't personally bother me to just have them throw similar to how mutating a frozen object does.

also can easily also include

Map.isLocked(map); // boolean

If the desire is to have a new underlying prototype without the mutating methods at all then i'd imagine its certainly possible to do that as well in a similar fashion to your example above

const map = new Map();
// do stuff
const lockedMap = Map.lock(map);
lockedMap instanceof ReadOnlyMap; // or LockedMap or whatever
'set' in lockedMap; // false
Map.isLocked(map);
map.set(a, b); // throws map is detached
lockedMap.set(a, b); // throws 'set' is not a function
bakkot commented 5 years ago

In that circumstance I definitely would rather learn on semantics of Object.freeze and have

I really really do not like allowing anyone to freeze any collection they have access to. I don't think it was necessarily a mistake that we allowed this for objects, given the rest of the language, but if we can possibly avoid doing that for richer collection types we should.

It could remove the methods easily enough if needed

... could it? By, what, mutating the prototype? That's generally a bad idea.

bradennapier commented 5 years ago

I don't dislike your idea (aside from the use of finalizable, heh ;-)) and get your concerns.

In the end it should and could always be opted-out of by cloning regardless but not ideal for your concerns so understandable (although I still like to lean on precedent and reduce the cognitive overhead of a new concept). I'm happy with any form and generally understand not everyone can be happy with whatever the final result is in most situations when it comes to extending a language :-).

if (Map.isLocked(map)) map = new Map(map)

Flow did recently introduce that the super types of collections do not have mutating methods $ReadOnlySet, $ReadOnlyMap, etc and their mutable methods are added through extension - although obviously a completely different situation ;-), it's actually what got me to make the post since I had already been concerned with using collections in multiple places over it.

https://github.com/facebook/flow/releases/tag/v0.86.0

hax commented 5 years ago

@bakkot

The main thing the LockableMap API gives you which the {Const, ReadOnly, Flex} API does not is that it allows you to precisely express "this collection will not be modified", which is a much stronger guarantee than "this collection cannot be modified by you"

ConstX (X.prototype.snapshot()) or ImmutableX is just what you want. Any usage of LockableMap or finalizable can be replaced by them.

let map = Map.finalizable();
for (let x of xs) {
  map.set(x, f(x));
}
return map.finalize();

=>

let map = new Map;
for (let x of xs) {
  map.set(x, f(x));
}
return map.snapshot(); // engines can optimize here by escape analysis, or use CoW.
hax commented 5 years ago

@bradennapier

I checked a project of my company which have many dependencies. There are about 1050 packages and 36000 js files (don't count the lines, but it should 1,000,000+ lines). And I found 100 (the number is very lucky 😆) usage of Object.freeze(), and if exclude shims like core-js, es5-shim and some test specs which test interoperability for frozen objects, there are only 64 usage in only 24 packages.

This rough search shows only 2% packages use it and if counting lines, the usage of it < 0.01%

Note, I don't mean Object.freeze() is not useful. It have good use cases like IceFactory you mentioned. Just want to point out Object.freeze() is a reflection api which programmers (even library authors) rarely use directly. Map/Set is very different.

bradennapier commented 5 years ago

@hax well if they don't program like i do then they aren't a good programmer anyway amiright? :-D (joking). Heh but yeah I've come to love IceFactory, although I wasn't aware it had such a stylish name until today.

hax commented 5 years ago

@bradennapier IceFactory is not a new idea, but I see such pattern rarely used in practice, because most js programmers don't care about some other could modify your prototype... What we really care is how we ourselves not do wrong thing accidently. So we use flow or TS (which only protect us but can't stop other do bad thing).

(IceFactory also drop class, but I don't want to discuss this aspect. 😛)

Flow did recently introduce that the super types of collections do not have mutating methods $ReadOnlySet, $ReadOnlyMap, etc and their mutable methods are added through extension - although obviously a completely different situation ;-)

I'm very glad flow add them! So both TS and Flow provide ReadOnlySet/Map which I think it's a good sign for introducing ReadOnlySet/Map in JS natively!

And it's obviously X.prototype.readOnlyView()/snapshot() are much more type-infer-friendly API than LockableX/X.finalize() for TS/Flow. It's hard for compiler/type checker to static analyze when a collection is locked and finalized, so you need to write type manually in many cases.

bakkot commented 5 years ago

ConstX (X.prototype.snapshot()) or ImmutableX is just what you want

No, it is not. I understand the usages of those APIs. I was careful to specify "without having to pay the cost of fully immutable data structures or a full clone". If you leave that part out, it changes the meaning of what I said.

Yes, some engines might optimize some usages of .snapshot some of the time. But these analyses are unpredictable and finicky and tend to lead to performance cliffs. There is advantage in doing things in a way which guarantees the properties you want. Maybe we'll decide it's not worth the trouble, but we should recognize the cost.

bradennapier commented 5 years ago

I’d be mildly to strongly against any solution that requires a clone since we can just ... clone at that point lol! Cloning collections is dead simple.

I do think in today’s environment selecting a solution that allows for type systems to better infer things is a good idea. Collections can be painful with flow and typescript today as it stands. So my vote definitely goes to whichever accomplishes this whole continuing to have a simple api.

By the way it would be easy using ‘Map.lock(map)’ as an example for flow as it does this today for Object.freeze. They’d simply extend that inference they do today for objects to maps to automatically infer their $ReadOnlyX types when it sees that.

hax commented 5 years ago

@bakkot

But these analyses are unpredictable and finicky and tend to lead to performance cliffs.

Don't understand why escape analysis is unpredictable.

Engines already use escape analysis and CoW. So if there will be performance cliffs, it is already there.

There is advantage in doing things in a way which guarantees the properties you want.

Considering complex optimizations in JS engines, it's hard to expect very same performance properties between different versions of different engines.

For example, even finalizable() api can not guarantee the properties you want --- nothing prevent the engine do CoW for new Set(set.finalize()) (except you want the spec explicitly forbidden such optimization.)

So if someone want such properties, they should use the languages like C++/Rust which give you the full control of everything and use wasm as target.

The practical criteria is:

  1. Is this optimizable?
  2. Is this the common case?

If both are true, then JS programmers can expect such optimization would eventually available on almost all engines.

Ginden commented 5 years ago

I would love to see FrozenSet and FrozenMap in JS, but this is definitely out of scope of this proposal.

bakkot commented 5 years ago

I created a repository to host further discussion and capture ideas from this thread: https://github.com/bakkot/proposal-frozen-set

erights commented 5 years ago

https://github.com/tc39/proposal-readonly-collections/ subsumes https://github.com/tc39/proposal-collection-methods/issues/22#issuecomment-444220555