wip: Function constructor evaluator #3000
Conversation
| `return (function () { | ||
| 'use strict'; | ||
| ${code} | ||
| }).bind(this)();`, |
There was a problem hiding this comment.
simply prepending 'use strict'; to code sadly doesn't work because of the destructuring of arguments
SyntaxError: Illegal 'use strict' directive in function with non-simple parameter list
|
I think I can see this being a valuable optimization (avoiding I think we keep the existing implementation, and if the non-extensible |
SwingSet and other things in agoric-sdk |
mhofman
left a comment
mhofman
left a comment
There was a problem hiding this comment.
Very much confused how dynamic updates of the global scope works with this approach. It seems it not only requires the global to be non extensible, but to be actually frozen (and for all module bindings to be similarly immutable).
| `); | ||
| context.evalScope.eval; // required by make-safe-evaluator. | ||
|
|
||
| return apply(evaluateFactory, context, []); |
There was a problem hiding this comment.
I think we haven't used the evaluate factory as a standalone thing in a while. I guess this removes the concept completely.
There was a problem hiding this comment.
that's not a problem. We can split into steps by adding some complexity if need be.
There was a problem hiding this comment.
Not a concern, just an observation. I added re-used of evaluator functions a while ago, so re-use of factory is pretty much not needed anymore.
| `{${MODULE_LEXICAL_KEYS}}`, | ||
| `return (function () { | ||
| 'use strict'; | ||
| ${code} |
There was a problem hiding this comment.
This means that code be allowed to utter new.target (undefined), which may be an acceptable fidelity concern.
I think super would still be illegal?
There was a problem hiding this comment.
Is that a concern on a dummy empty function?
Was eval the thing that prevented these issues before?
There was a problem hiding this comment.
Actually I think because of our direct eval in a function scope, new.target being allowed is an existing fidelity concern, however undocumented I believe.
| return apply(evaluateFactory, context, []); | ||
| return apply( | ||
| FERAL_FUNCTION( | ||
| `{${SCOPE_TERMINATOR_KEYS}}`, |
There was a problem hiding this comment.
I'm confused, how does it work for changing keys of the global object? Either the code changing a pre-existing global binding foo = 42, or someone changing an existing property of the global object and expecting the global binding to be updated. globalThis.bar = 42; console.log(bar).
There was a problem hiding this comment.
For new keys, globalThis is intended to be made non-extensible (but it was breaking tests even further so I commented it out)
as for updating existing fields... I forgot about that issue, but I have a vague memory of a solution for that existing. Did I hallucinate that?
There was a problem hiding this comment.
There is currently no way to hook into scope binding access (without this). Someday we may have let decorators that would do that. The refs proposal might also be able to do something here. Both of those would still require a non-extensible global object, but at least the bindings could be mutable (mutated either by the evaluated code, or reflect mutation of the source)
There was a problem hiding this comment.
We're talking about propagating updates between multiple evaluations within one compartment here I assume. So if we only ever had one, that would not be a problem. It'd be hard to pull off, but theoretically possible with a bundler.
Otherwise, manual propagation of individual globals seems to be the only option.
A code snippet that reads specified local variables and passes them on to a callback on demand could work.
There was a problem hiding this comment.
We're talking about propagating updates between multiple evaluations within one compartment here I assume.
Or within a single evaluation too. If bar exist on the global object and is writable/configurable, code would expect that doing globalThis.bar = 42 is immediately visible in calls to console.log(bar).
It'd be hard to pull off, but theoretically possible with a bundler.
If we're considering code rewrites, I assume the easiest is to transform all binding references that do not resolve to a surrounding scope into some kind of call access. E.g. console.log(bar) -> console.log(__getGlobal__('bar')) and bar = 42; -> __setGlobal__('bar', 42);.
|
@mhofman Clarification of intentions added to the description |
They would not be affected because this is intended to be an opt-in (I'm just prototyping it in place) - added a clarification |
ported the code from an old experiment within LavaMoat webpack plugin
It's still very broken in tests, but for what seems like minor features missing 😅
How many things (other than most tests) rely on
evaluatereturning the value of the last statement in code?Intentions
The goal here is to put together a PoC that can later be extracted into an evaluator factory that is used under Hermes (and potentially other cases where it's tempting and globalThis can be made non-extensible - eg. Node.js)
I'm not intending to replace the original entirely, but allow to opt-in to it.
I'm not implementing the opt-in part because I want to minimize work that's necessary to test the PoC and compare with the original implementation.