search

tc39 / proposal-array-is-template-object

TC39 proposal to identify tagged template string array objects
https://tc39.es/proposal-array-is-template-object/
MIT License
39 stars 7 forks source link

Stage 2 Review #9

Closed mikesamuel closed 3 years ago

mikesamuel commented 5 years ago

@erights @jridgewell, thanks for volunteering to be stage 2 reviewers.

I think this is ready for review. Of interest might be

ljharb commented 5 years ago
erights commented 5 years ago

Hi Mike, I object to using a realm-agnostic internal property for the branding. The way you first presented this, with a per-realm branding-on-creation weakset-equivalent is strictly superior. It should be observably identical except that it is per-realm.

Making this inter-realm fails to make trust distinctions between realms with different evaluation policies, which kinda negates the whole purpose of this proposal.

Further, membrane boundaries are an almost perfect emulation of realm boundaries. The only thing breaking transparency is cross-realm internal slots. isTemplateObject must not work across a membrane. Therefore it should not work between realms, preserving membrane transparency.

ljharb commented 5 years ago

Why? Internal slots almost all behave this way - what’s to stop you from wrapping isTemplateObject inside your membrane and applying the restrictions you want?

erights commented 5 years ago

@ljharb I don't understand the question.

ljharb commented 5 years ago

My understanding is that membranes only work if you have the ability to wrap all the intrinsics/primordials/builtins. So, the original behavior of them - like Function.prototype.toString.call, for example - is irrelevant (except in how it dictates what you need to virtualize). Thus, you can virtualize isTemplateObject to have any same-realm behavior you want - including denying cross-realm detection across a membrane.

It’s entirely likely I’m misunderstanding something, in which case, please help me understand why having it be cross-realm (like every other internal slot) is a problem?

mikesamuel commented 5 years ago

@erights said

I object to using a realm-agnostic internal property for the branding.

Brand checks would use isTemplateObject and a realm check.

The difference is that a realm-agnostic isTemplateObject allows flexible brand checks via isTemplateObject(x) && setOfArrayPrototypes.has(Object.getPrototypeOf(x)).

So Array.isTemplateObject is not a brand check in the same way that Array.isArray is not a type check; neither tells you little about how the input responds to any particular message, but they enable other things.

erights commented 5 years ago

We special cased Array.isArray in proxy. I do not want to add a special proxy request for this one. We need to keep it realm local for this reason, but even more so, for eval-policy reasons.

mikesamuel commented 5 years ago

@ljharb said

  1. in the static method, i'd probably just do Return ! isTemplateObject(value), ie both inlining and adding the !
  2. isTemplateObject seems like it could be simplified to If Type(value) is Object and value.[[TemplateObject]] is true, return true, else return false, or similar?

Done

mikesamuel commented 5 years ago

We special cased Array.isArray in proxy. I do not want to add a special proxy request for this one. We need to keep it realm local for this reason, but even more so, for eval-policy reasons.

I agree re eval-policy, but am unsure how that relates to this.

By special proxy handling do you mean step 3 of https://tc39.es/ecma262/#sec-isarray

7.2.2 IsArray ( argument )

  1. ...
  2. ...
  3. If argument is a Proxy exotic object, then a. If argument.[[ProxyHandler]] is null, throw a TypeError exception. b. Let target be argument.[[ProxyTarget]]. c. Return ? IsArray(target).

If so, there is currently no special proxy handling for isTemplateObject and no requests for it.

jridgewell commented 5 years ago

LGTM.

littledan commented 4 years ago

What more is needed in this proposal before Stage 3? It seems to be in pretty good shape to me.

koto commented 3 years ago

It seems to be blocked on #10.

ljharb commented 3 years ago

@koto the rendered output still says "Stage 0", which suggests the spec is not up to date. Modulo #10, is it?

koto commented 3 years ago

Yes, I don't yet have admin access to the repository (https://github.com/tc39/Admin-and-Business/issues/97) and didn't regenerate the spec. In the meantime the ES GetTemplateObject step ordering changed, so this needs just a tiny editorial fix before that, but the logic is up to date modulo #10.

ljharb commented 3 years ago

It's very difficult to review the raw ecmarkup; please ping the thread once the rendered spec has been updated :-)

koto commented 3 years ago

Ready to review. I assume I should put the current stage in the spec draft, not the one I intend to advance to?

ljharb commented 3 years ago

Regarding the spec: we don’t add slots to objects post-creation, so if you want this array to have a [[TemplateObject]] slot, then every array must have one.

jridgewell commented 3 years ago

Yah, we'll need to modify ArrayCreate to initialize with [[TemplateObject]] and set that to false. Otherwise looks fine.

koto commented 3 years ago

@erights, could you review the current proposal? I wanted to apply for advancement to stage 3 in the January meeting. It seems like the contentious part might be the brand check, there's already a brand check discussion planned for that meeting proposed by @littledan. Is it reasonable to resolve the brand check concerns in that discussion? Do you have any other outstanding concerns?

koto commented 3 years ago

Closing this issue, as it's currently redundant with #15.