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

Shims of well-known symbols #27

Closed
mhofman opened this issue Jun 1, 2022 · 27 comments
Closed

Shims of well-known symbols #27

mhofman opened this issue Jun 1, 2022 · 27 comments

Comments

@mhofman
Copy link
Member

mhofman commented Jun 1, 2022

In the SES call today we discussed how a shim of a well-known symbol would have to be implemented to be transparent with ShadowRealm.

There are basically 2 approaches, each with their caveats:

  • The shim uses a unique symbol and adds it to the realm's global Symbol. However to ensure that the identity of that symbol is consistent across realms, shims loaded in multiple realms have to coordinate somehow. One way would be to wrap the ShadowRealm constructor.
  • The shim uses a registered symbol. It similarly adds it to the realm's global Symbol, but because it uses a registered symbol, no coordination is necessary. However if well-known symbols are allowed as WeakMap key, then the shim needs to wrap WeakMap, and store entries keyed by this fake well-known symbol into a side Map.

Both shims approaches have fidelity issues. In hindsight, I wish well-known symbols were all registered. However if well-known symbols were disallowed as WeakMap keys, it'd allow greater shim fidelity without increased complexity. They could use registered symbols as the synchronization mechanism across realms, with only Symbol.keyFor remaining as fidelity discrepancy for shimmed well-known symbols.

@ljharb
Copy link
Member

ljharb commented Jun 1, 2022

You would never want to use a registered symbol, because then Symbol.keyFor(polyfilled) would return "not undefined", which is a much bigger problem than "not being able to use a polyfilled WKS in a weak context" imo.

@acutmore
Copy link
Contributor

acutmore commented Jun 1, 2022

Could you expand the initial premises a little, perhaps with some code?

What is there to shim? The protocols associated with the well know symbols will only work with the real values.

@acutmore
Copy link
Contributor

acutmore commented Jun 1, 2022

Oh I follow now, I was thinking this was about a form of virtualisation. The situation is that there is some well-known symbol that the host does not currently implement, and the intention is to be able to polyfill the symbols and any associated APIs.

@ljharb
Copy link
Member

ljharb commented Jun 1, 2022

@acutmore shimming a WKS also means you shim all of the methods that care about it.

@acutmore
Copy link
Contributor

acutmore commented Jun 1, 2022

I think the analysis is correct, to fully shim well known symbols in a spec compliant way. The library would also need to replace Symbol.for, Symbol.keyFor, WeakMap, WeakSet, WeakRef and FinalizationRegistry.

IMO it is very unlikely that someone will be attempting to use a well-known symbol with the weak & GC APIs so this extra conformance could be seen as unnecessary, and the library could choose to disallow the shimmed well-known symbol as a WMK.

I’ll also note that the required changes to the APIs could be wrapped up into one reusable library. Rather than re-implemented for each shim.

@ljharb
Copy link
Member

ljharb commented Jun 1, 2022

Exactly right.

It's impossible to shim cross-realm things reliably, so you have to choose your caveats between:

  1. use the symbol registry, and risk other-realm Symbol.keyFor giving the wrong answer
  2. use a unique symbol, and risk other-realm Weak collections not being able to hold the symbol

The first both has existed for 7+ years and is much more common than the second, which doesn't exist yet and will be very rare anyways.

@mhofman
Copy link
Member Author

mhofman commented Jun 1, 2022

Right I don't think there is a way to shim Symbol.keyFor in this case.

Maybe in the future we can come up with a solution like "registered init modules" that would automatically be executed when creating a ShadowRealm, at which point there would be a way for shims to coordinate between realm.

@Jack-Works
Copy link
Member

I think the registered symbol is the solution that has the least problem. You can always use "__DO NOT USE OR YOU WILL BE FIRED__POLYFILL__WELL KNOWN SYMBOL__SYMBOL NAME" as your registered symbol name right?

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

@Jack-Works the issue is that there’s code that uses “Symbol.keyFor is undefined” to filter out symbols that aren’t unique or well-known, so it’s pretty problematic to polyfill using that, unless you also wrap keyFor to return undefined for those.

@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

This is what I'd imagine someone would need to do to shim well-known symbols using registered symbols:

https://gist.github.com/acutmore/45bf04c0079d0ec7bb4a9f36939057cf

It uses the prefix idea as mentioned by @Jack-Works to avoid clashes and updates Symbol.keyFor, Symbol.prototype.description and Symbol.prototype.toString to hide the shim.

Adding additional logic to also fix WeakMap, WeakSet, WeakRef and FinalizationRegistry to be fully spec compliant and allow the use of the shimmed well-known symbol does fall into the 'non-trivial' category of amount of code required.

@mhofman
Copy link
Member Author

mhofman commented Jun 2, 2022

Adding additional logic to also fix WeakMap, WeakSet, WeakRef and FinalizationRegistry to be fully spec compliant and allow the use of the shimmed well-known symbol does fall into the 'non-trivial' category of amount of code required.

Only if this proposal allows well-known symbols as WeakMaps keys. If it doesn't, there is no need for further "non-trivial" shimming.

@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

Only if this proposal allows well-known symbols as WeakMaps keys. If it doesn't, there is no need for further "non-trivial" shimming.

There is the flip-side, for a single-realm application it could shim a well-known symbol in a single-line: Object.defineProperty(Symbol, 'thenable', { value: Symbol("Symbol.thenable") }). No need to patch anything else, unless the proposal disallows well-known symbols as WeakMap keys - it then needs to do the non-trivial shimming.

@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

This is what I'd imagine someone would need to do to shim well-known symbols using registered symbols:

https://gist.github.com/acutmore/45bf04c0079d0ec7bb4a9f36939057cf

And here is what I'd imagine someone would need to do to shim well-known symbols using unique symbols:

// Current realm forms the 'root' where the unique symbol is initially created
Object.defineProperty(Symbol, 'thenable', { value: Symbol("Symbol.thenable") });

// Bootstrap recursively passing this value down into the 'child' realms
(function realmInit() {
  const _shadowRealm = ShadowRealm;
  const src = realmInit.toString(); 
  globalThis.ShadowRealm = function ShadowRealm() {
    const sr = new _shadowRealm();
    sr.evaluate(`s => Object.defineProperty(Symbol, 'thenable', { value: s });`)(Symbol.thenable);
    sr.evaluate(`(0,${src})`)();
    return sr; 
  };
})();

Testing:

new ShadowRealm().evaluate('Symbol.thenable') === Symbol.thenable; // true
new ShadowRealm().evaluate(`new ShadowRealm().evaluate('Symbol.thenable')`) === Symbol.thenable; // true

@bathos
Copy link

bathos commented Jun 2, 2022

I’m curious if folks would expect this kind of patching to occur in polyfill libraries themselves. Ultimately that’s gonna be up to the ecosystem I realize, but I’d hate to see the endless bogus unsafe-eval csp reports that globalThis finally vanquished return.

@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

I’m curious if folks would expect this kind of patching to occur in polyfill libraries themselves. Ultimately that’s gonna be up to the ecosystem I realize, but I’d hate to see the endless bogus unsafe-eval csp reports that globalThis finally vanquished return.

https://github.com/tc39/proposal-js-module-blocks might nicely replace the evaluate calls.

@bathos
Copy link

bathos commented Jun 2, 2022

Sweet! Pretty sure module blocks as ShadowRealm input would permit some pretty major improvements (i.e. deleting code I’d written) in a tentative call boundary adaptation. It’s great learn this is part of the bigger picture of where the API might be heading.

@mhofman
Copy link
Member Author

mhofman commented Jun 2, 2022

No need to patch anything else, unless the proposal disallows well-known symbols as WeakMap keys - it then needs to do the non-trivial shimming.

Arguably using a value that should be disallowed, while not entirely faithful, is less of an issue than using a value which would break and requires further shimming.

That said, I think I do agree with you, and it may be worth ignoring the complexity of faithful multi-realm compatible shims for this proposal.

And here is what I'd imagine someone would need to do to shim well-known symbols using unique symbols:

Your example is actually not sufficient, as you need to wrap the ShadowRealm in the child realms as well. This shows how much of a footgun multi-realm shims are, which I'm hoping we can fix with a future proposal.

https://github.com/tc39/proposal-js-module-blocks might nicely replace the evaluate calls.

That's exactly my idea, something based on module blocks. However the problem is that importing modules is asynchronous for now, so shimming a realm will still require evaluate until that changes. I believe we'll need a way to import modules synchronously in realms (and in Compartments when those show up) if the graph of dependencies is can be fully resolved synchronously and already ready to be evaluated, which should be the case for module blocks imported in the same agent. Something like importNow(moduleBlock) which either completes synchronously or throws if any dependency cannot be resolved and evaluated synchronously.

Anyway, I think at this point besides changing existing well-known symbols to be registered symbols, which would likely be web incompatible, keeping well-known symbols behavior aligned with unique symbols is unfortunately the more program friendly thing to do. I'll close this issue.

@mhofman mhofman closed this as not planned Won't fix, can't repro, duplicate, stale Jun 2, 2022
@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

Your example is actually not sufficient, as you need to wrap the ShadowRealm in the child realms as well.

I think it does do this btw, it sets itself up recursively 🙂

@caridy
Copy link
Collaborator

caridy commented Jun 2, 2022

@mhofman is right, if you polyfill the well-known symbol with a regular symbol, then the challenge is to propagate it to other realms, which will probably require a lot more patching :( and it all comes down to the fact the well-known symbols are, in itself, a type of registered symbols, with small variations. If there is a stronger desire to get well-knowns to work as WMK, we should continue, but if that's not the case, then we should probably stick to the original idea of preventing registered and well-knowns all together.

@caridy
Copy link
Collaborator

caridy commented Jun 2, 2022

Your example is actually not sufficient, as you need to wrap the ShadowRealm in the child realms as well.

I think it does do this btw, it sets itself up recursively 🙂

Not just ShadowRoot, but any other realm that can be accessed from the Realm that you're patching (that includes iframes), because a non-registered symbol will have to match the identity of the symbol on the other side.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

I think it's important that WKS work weakly.

@mhofman
Copy link
Member Author

mhofman commented Jun 2, 2022

I think it does do this btw, it sets itself up recursively 🙂

Oops, I misread when I wasn't fully awake.

@mhofman
Copy link
Member Author

mhofman commented Jun 2, 2022

but any other realm that can be accessed from the Realm that you're patching (that includes iframes)

I wish we could mandate that hosts are not allowed to expose any APIs in a ShadowRealm that allow the program to reach the objects of any other realms (shadow or legacy), and the corollary that any legacy realm (whether incubator or iframe) shouldn't be allowed to ever reach objects of ShadowRealms through host APIs.

See tc39/proposal-shadowrealm#324

@acutmore
Copy link
Contributor

acutmore commented Jun 2, 2022

Your example is actually not sufficient, as you need to wrap the ShadowRealm in the child realms as well.

I think it does do this btw, it sets itself up recursively 🙂

Not just ShadowRoot, but any other realm that can be accessed from the Realm that you're patching (that includes iframes), because a non-registered symbol will have to match the identity of the symbol on the other side.

Absolutely! The intention of my code was to demonstrate the pattern - a library would naturally implement things in more of a “production quality” manner.

For iframes the initial script tags that install any polyfills could copy across well known symbols from their parent using something like:

Object.defineProperty(Symbol, 'thenable', { value: window.parent.Symbol.thenable });

This pattern of creating a well-known symbol once in a root, and then ensuring they are passed down to child realms is actually how SpiderMonkey implements them (ref). Viewed in this way well-known symbols are more like unique-symbols than registered symbols as there is no global cross realm lookup table being utilised; the values are passed around explicitly.

It’s a holiday here in the UK, but when I get the time I will put together a complete gist of what would need to be shimmed for libraries using Symbol.for to shim well-known symbols (and running in a future host where this proposal is implemented) so there would be a document authors could use as a reference.

@caridy
Copy link
Collaborator

caridy commented Jun 2, 2022

@acutmore can you also point to the benefits of allowing well-known symbols? I understand that implementers are ok with that, but for registered symbols, but that on itself is not sufficient indication, IMO, to allow well-known symbols as WMK.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

For one, it makes the mental model of understanding what symbols can be held weakly much simpler - since users (who realize the difference exists) already understand "registered vs everything else", this way they aren't forced to learn about all three categories.

It also minimizes which things can't be used, which imo is an important goal. We shouldn't restrict anything we aren't required to.

@caridy
Copy link
Collaborator

caridy commented Jun 3, 2022

For one, it makes the mental model of understanding what symbols can be held weakly much simpler - since users (who realize the difference exists) already understand "registered vs everything else", this way they aren't forced to learn about all three categories.

There are only 13 of them specified in 262. They are rarely used (we can get some metrics here), less likely to use those symbols as WMK. This is, IMO, a weak use-case.

It also minimizes which things can't be used, which imo is an important goal. We shouldn't restrict anything we aren't required to.

This is a more strong reason, specially when looking at this from the language purist point of view.

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

No branches or pull requests

6 participants