-
Notifications
You must be signed in to change notification settings - Fork 8
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
Addition for freezing collections #22
Comments
Continuing discussion from that issue:
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.
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. |
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 |
Also repeating my comment from there:
|
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 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 |
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 |
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. |
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 Here is a experimental implementation of |
Awesome yeah essentially what I did tc39/proposal-set-methods#30 here but I was going opposite way which is not as safe obviously. Thanks! |
I do personally like 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. |
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 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 :) |
Here's a summary of the issue as I see it:
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 So, following the example of 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 |
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. |
Because these patterns can be implemented in userland in terms of a (currently impossible) |
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. |
@bakkot @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. |
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 While the 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 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.
|
Actually I tried to trap
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
|
Definitley not against it but would be in favor of a
ATM collections are pretty painful when it comes to things like 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 |
One disadvantage of the I prefer API designs where I can generally use the things that I can see. IOW, when restrict visibility when restricting possible action. |
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.
It's not easy for average js programmers understand why |
@erights I agree, though of course |
Hi @bradennapier , let i = 0;
const set = new Set([i++, i++, i++]).readOnly();
set.forEach(value => { console.log(value); set.add(i++) }); As with |
As my experience, 90% js programmers never use Proxy directly, 90% js programmers even never know |
@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,
for all collections |
@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:
now since set is mutable but the user has a read only view - they may think it safer to do something like
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. |
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. |
@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 ;-). |
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 |
Is this not also the case today with Revocable Proxies or even I mean I would imagine the lockable sets would be used to pass a locked set in almost all cases. I love 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. |
I would expect, in an ideal world, that 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. |
This should be possible (my previous proxy-based implementation satisfy this), actually ReadOnlySet could use same methods of Set, just exclude all mutation methods. |
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 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 Basically, I always prefer And |
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 In my needs and with my concerns for making this post, I personally would only ever really use it as mentioned so https://medium.freecodecamp.org/elegant-patterns-in-modern-javascript-ice-factory-4161859a0eee 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 |
@hax:
The assumption with 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(); |
In that circumstance I definitely would rather lean on semantics of cosnt map = new Map();
// ... do stuff, mutate, do whatever
return Map.lock(map); and as is the case with 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 |
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.
... could it? By, what, mutating the prototype? That's generally a bad idea. |
I don't dislike your idea (aside from the use of 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 |
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. |
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 This rough search shows only 2% packages use it and if counting lines, the usage of it < 0.01% Note, I don't mean |
@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. |
@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. 😛)
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 |
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 |
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. |
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.
Considering complex optimizations in JS engines, it's hard to expect very same performance properties between different versions of different engines. For example, even 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:
If both are true, then JS programmers can expect such optimization would eventually available on almost all engines. |
I would love to see |
I created a repository to host further discussion and capture ideas from this thread: https://github.com/bakkot/proposal-frozen-set |
As mentioned in tc39/proposal-set-methods#30 it would be nice to have a way of "freezing" (or some other name if "freeze" doesn't match perfectly) collections seems ideal.
The text was updated successfully, but these errors were encountered: