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

Initial API design #13

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Initial API design #13

merged 6 commits into from
Feb 7, 2022

Conversation

Jack-Works
Copy link
Member

I tried to design the API based on the feedback from the stage-1 presentation on tc39. And I also try to match the API with readonly collection proposal.

Hope to get early feedback from the following people so I can iterate on the API design to resolve concerns. Thanks!

cc @erights, @phoddie (read-only collection proposal champions), @syg, @codehag, @kmiller68 (implementors), @bmeck, @hax, and @littledan (people have interest at the meeting)

I want to have feedback, especially from implementors because this proposal will create new paths on ArrayBuffers.

@Jack-Works
Copy link
Member Author

I have updated the design based on feedback from @hax:

  • Add %TypedArray%.prototype.{snapshot,diverge}() because it's not possible to access %TypedArray%.prototype.buffer to create a snapshot/diverge.
  • %TypedArray%.prototype.buffer can return an ArrayBufferSlice (if it is accepted) for a read-only view.
  • Clearify that calling freeze() on an ArrayBufferSlice will throw a TypeError.
  • Added ArrayBuffer.prototype.slice(offset, length) to create an ArrayBufferSlice.

@ghost
Copy link

ghost commented Dec 12, 2021

. . .

* Added `ArrayBuffer.prototype.slice(offset, length)` to create an `ArrayBufferSlice`.

This will create a breaking change, as ArrayBuffer#slice already exists, and returns ArrayBuffer objects, which are copies of the original object's data. I lack statistical data, yet the fact that it copies instead of creates a view is actively relied upon by others.
(i.e. all you should need to do is choose a different name?)

@Jack-Works
Copy link
Member Author

Oh, I didn't notice that. We can select another name.

@Jack-Works
Copy link
Member Author

Ok, no reviews since then. I may merge this and make a non-advance presentation to notify this change.

Copy link

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jack-Works , I like this approach.

LGTM , thanks!

@erights
Copy link

erights commented Feb 6, 2022

Why not change the spec of Object.freeze and Object.isFrozen so that it has these semantics on ArrayBuffers, rather than defining new ArrayBuffer.prototype methods?

@ljharb
Copy link
Member

ljharb commented Feb 6, 2022

@erights that sounds like it might not be web compatible. Object.freeze already has the semantics it has, which affect no internal slots of builtins (except [[IntegrityLevel]], ofc) - and frozen builtins thus remain mutable.

@Jack-Works Jack-Works merged commit 725dbeb into main Feb 7, 2022
@Jack-Works Jack-Works deleted the design-1 branch February 7, 2022 02:23
@Jack-Works
Copy link
Member Author

I'll make a presentation in the future and hope I can get feedback from implementors.

@phoddie
Copy link

phoddie commented Feb 7, 2022

I strongly support a path to immutable ArrayBuffers in JavaScript and welcome this discussion. Immutable ArrayBuffers have long been supported in the XS engine as part of preparing a virtual machine to run from read-only flash storage. That said, Moddable's view is that immutable ArrayBuffers should done as part of addressing immutability in the language in a consistent manner, not on an object-by-object basis.

This proposal isn't freeze. As @ljharb notes, freeze has well defined behavior with respect to the IntegrityLevel. This is a new IntegrityLevel beyond frozen and should have a different name to make that clear.

Having just been through implementation of resizable ArrayBuffer, the engine overhead for an inescapable slice seems high. That could be acceptable if the value is also high. I'd want to understand the need more clearly than is described in #11 before exploring solutions.

@syg
Copy link

syg commented Feb 7, 2022

Is the idea that snapshot() copies the backing store? I imagine most uses of immutable ABs would prefer mprotect semantics of freezing in-place than getting a new immutable copy.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2022

For what it's worth, adding a true immutable integrity level, beyond "frozen" - one that worked on Maps, Sets, Promises, RegExps, Dates, as well as ArrayBuffers - would be a wonderful proposal.

@phoddie
Copy link

phoddie commented Feb 8, 2022

For what it's worth, adding a true immutable integrity level, beyond "frozen" - one that worked on Maps, Sets, Promises, RegExps, Dates, as well as ArrayBuffers - would be a wonderful proposal.

I completely agree. And we do "petrify" for all of those in the Moddable SDK. I've considered proposing it here, but the impression I get from discussions is that it wouldn't fly yet. I would be happy to be wrong about that.; )

@Jack-Works
Copy link
Member Author

Is the idea that snapshot() copies the backing store?

Yes, this API is intended to match the readonly collection proposal. I would expect the common use case of this API is no changes to the snapshot at all so a Copy-on-Write would be a better way to implement this.

For what it's worth, adding a true immutable integrity level, beyond "frozen" - one that worked on Maps, Sets, Promises, RegExps, Dates, as well as ArrayBuffers - would be a wonderful proposal.

A new integrity level is interesting, maybe the Host APIs can get benefits from it too. I'm curious how Promises can be frozen, once it is frozen, it becomes useless (if it's pending, it's forever pending, otherwise they can not make a state transition already).

Should we try to merge all tries on this route and make a unified proposal to define a new integrity level and a unified API?

@ljharb
Copy link
Member

ljharb commented Feb 8, 2022

@Jack-Works yes, a truly immutable Promise would either be already-fulfilled (and thus immutable anyways) or forever-pending (and thus be useless).

I think a unified proposal would be great.

@Jack-Works
Copy link
Member Author

What do you think? cc @erights @phoddie

@erights
Copy link

erights commented Feb 8, 2022

Peter and I have talked alot about harden and petrify, and I like where we've gotten. It is indeed that petrify that we should consider standardizing. The key thing about it is that it breaks encapsulation only informationally, by detecting whether there is any mutable state left after hardening. It does not harden anything that cannot be hardened today from user code because the state is encapsulated.

Indeed, anything that did reach through an encapsulation barrier to freeze internals that should have been protected would be an attack we must not enable.

@erights
Copy link

erights commented Feb 8, 2022

On this arraybuffer proposal, @ljharb is indeed correct, and I was confused. I was confusing ArrayBuffers with TypedArrays. TypedArrays currently cannot be frozen either. However, unlike Array, applying Object.freeze to a non-empty TypedArray throws. It cannot do otherwise without freezing the backing ArrayBuffer. Which leads to an interesting idea:

harden on a TypedArray, were it successful, would freeze both the TypedArray and its backing buffer. Currently, the only sense in which it would freeze the backing buffer is the vacuous sense that Jordan pointed out. But having harden on a TypedArray also do the arraybuffer freeze proposed here would be in the spirit of the transitivity of harden.

@phoddie
Copy link

phoddie commented Feb 8, 2022

@erights I want to clarify a couple points specifically about petrify as we've gone through some name changes over time. There is the imperative petrify which does roughly what @ljharb describes - for example making a Date or ArrayBuffer immutable. This is what XS does to put objects in ROM. There is also the query, isPetrified. (These are not the names in XS today, but they are what we agreed on). In your comment above, isPetrified checks for immutability but does not enforce it. I believe you are referring to isPetrified when you say "It is indeed that petrify that we should consider standardizing " and your objection ("would be an attack") refers to the imperative petrify. Is that correct?

@erights
Copy link

erights commented Feb 8, 2022

@phoddie that is correct. As you say, we've gone through some changes over time ;).

harden and isPetrified (by whatever name) are what we should standardize. The stronger petrify would indeed be an attack we must disallow.

On the naming, I thought we settled on something other than isSomething because it wouldn't return true or false. Rather it would return a list of paths to the impure state. We rejected isPure on the same grounds, but I don't remember what we settled on.

However, as with the TypedArray / ArrayBuffer above, we can consider making harden stronger and different from what user code can do, as long as we don't venture into attack territory. Thus, Date and ArrayBuffer could be in harden territory if we widen it this way. Where harden must not go is freezing variables captured by closures or their contents, and likewise, private fields of instances and their contents. But isPetrified would detect that there exists such remaining mutable state of a hardened graph of objects. Being petrified (or pure) is a transitive and permanent condition, and is likely consequence of hardening. We could make hardened and pure distinct integrity levels. We would need the bookkeeping internally anyway, so that harden stops when it reaches previously hardened, and so that isPetrified returns is small amortized time.

Were hardened a distinct integrity level, we could also entertain the possibility that properties of hardened objects do not trigger the override mistake. Though I realize this step will be more controversial.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2022

Why would the verb be an attack? I want to be able to make builtin instances immutable.

@erights
Copy link

erights commented Feb 8, 2022

If your class C creates an instance, I'd certainly love it if C could make the instance immutable. But having made, say, a defensive instance guarding mutable state, and giving me access to the instance, I can subvert the purpose of the instance --- cause it to misbehave --- by petrifying it in ways that violate the assumption made by the author of C.

@erights
Copy link

erights commented Feb 8, 2022

For instances of most builtins, like Date and ArrayBuffer, in the rest of my message above I propose that locking down their state should not be considered an attack, and so we can consider stretching the behavior of harden to cover those cases.

Promises, WeakMaps, Proxies are examples of builtin instances that encapsulate. Making them immutable from the outside should be prohibited, as it would subvert the way these are used to encapsulate state.

@erights
Copy link

erights commented Feb 8, 2022

@ljharb I suspect you're also thinking of a non-transitive operation. The transitive petrify on an instance of a class would also freeze the class itself and anything reachable from the class. If we go with the weakmap model of private state, that would include the hidden weakmap and therefore all the other instances as well. Further, it would prevent the class from making new instances, since that would require mutation of the hidden weakmap.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2022

Oh certainly, just like freeze and everything else in the language, it’d need to be shallow.

@syg
Copy link

syg commented Feb 8, 2022

Yes, this API is intended to match the readonly collection proposal. I would expect the common use case of this API is no changes to the snapshot at all so a Copy-on-Write would be a better way to implement this.

I don't think the COW and snapshot API will be a good fit for WebAssembly and lower-level web APIs, as normal usage for this kind of API promotes making new ArrayBuffers.

@Jack-Works Jack-Works mentioned this pull request Mar 20, 2022
@Jack-Works
Copy link
Member Author

About new integrity level, please move the discussion to #15
thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants