search

tc39 / proposal-shadowrealm

ECMAScript Proposal, specs, and reference implementation for Realms
https://tc39.es/proposal-shadowrealm/
1.43k stars 67 forks source link

ValidateShadowRealmObject: checking both slots? #362

Closed bathos closed 8 months ago

bathos commented 2 years ago

The ValidateShadowRealmObject AO currently has two steps:

  1. Perform ? RequireInternalSlot(O, [[ShadowRealm]]).
  2. Perform ? RequireInternalSlot(O, [[ExecutionContext]]).

Given the only path specified for creating ShadowRealm instances allocates « [[ShadowRealm]], [[ExecutionContext]] » up front (and given there’s no way, as far as I’m aware, for an internal slot to itself be “removed”), it seems like the second step is redundant. While it’s harmless to list both in terms of observable effects, it creates the impression that it’s possible for either of the slots to be present without the other (that’s what led me here).

I think this might be a vestige of an earlier iteration of the spec because when ValidateShadowRealmObject was introduced (as ValidateRealmObject), it looked like this, using the generic [[Realm]] slot rather than the API-specific [[ShadowRealm]] slot that was introduced later:

1. Perform ? RequireInternalSlot(_O_, [[Realm]]).
2. Perform ? RequireInternalSlot(_O_, [[ExecutionContext]]).
caridy commented 8 months ago

@ptomato can you clean this up?

ljharb commented 8 months ago

Typically we try to check the presence of any slots used in the algorithm. If both aren’t consistently used, one technique would be to make an IsShadowRealm AO that checks both slots, and use it in the various places needed.

If this AO is called from places that in aggregate use both slots, it’s probably best to keep both checks here, redundant or no.

caridy commented 8 months ago

Ok, fair enough, closing.

ptomato commented 8 months ago

@caridy @leobalter There's also #320 which I cleaned up and rebased recently, it removes [[ExecutionContext]] which would make this redundant.