-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat(ses): Support global lexicals #356
Conversation
c4b2773
to
ceb8788
Compare
2aa1019
to
facae46
Compare
ceb8788
to
f311422
Compare
facae46
to
abb8f9d
Compare
7d2f5a8
to
f888723
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but it'd be nice to have those extra tests. And maybe have @erights take a look at the evaluator changes just in case.
packages/ses/src/scope-constants.js
Outdated
// Collect all valid & immutable identifiers from the endowments. | ||
const localConstants = localNames.filter( | ||
name => | ||
isValidIdentifierName(name) && isImmutableDataProperty(localObject, name), | ||
!lexicalConstants.includes(name) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this check is to keep the name from showing up in the constants list twice, right? (if the caller provided the same name for both a global and a lexical?) The way this is written made me think that maybe we're enforcing some important security property.
Not recommending a change that's outside the scope of this PR, but at some point would it be better (or maybe worse..) to just collect all these names into a Set?
packages/ses/src/scope-handler.js
Outdated
// Global lexicals. | ||
if (prop in globalLexicals) { | ||
// Use reflect to defeat accessors that could be present on the | ||
// globalLexicals object itself as `this`. XXX? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, do we not need this protection in this one case because we created globalLexicals
ourselves, and know that it doesn't contain any accessors? I'm happy to retain the reflectGet
for safety, but we might amend the comment if we know that's why we're doing it, and point out the difference between globalLexicals
(which we create, and is static) and endowments
(which.. might come from the user, and might have accessors, which we're happy to let execute, but not let those accessors use our this
object?)
Also, just confirming my understanding.. when we say "endowments", do we strictly mean properties of Before we needed this distinction for SES, did we use "endowments" as a more generic term to encompass names that were visible in the global scope, without much consideration as to whether they could or could not be denied to code through static analysis and rejection of matching free-variable names? Did E have any notion of |
a3eb545
to
76ae82e
Compare
f888723
to
4067992
Compare
I can fill in some of this. "endowments" used to be "global lexicals" in SES-0.6. It was when Moddable created the |
74c59bf
to
788b190
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I didn't see anything about the constants optimization in this PR. Did I miss it, or does this PR no longer mess with that?
788b190
to
761e32c
Compare
The constants optimization no longer requires changes. The optimization still occurs by virtue of the compartment endowments, compartment global lexicals, and module live bindings all coëxisting on the Endowment has indeed become a very confusing name internally. There are things named |
daa837c
to
2f22de6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I no longer understand how things are wired up. I need you to walk me through this verbally while I ask questions. Tomorrow (Friday) afternoon?
2f22de6
to
8f6c714
Compare
8f6c714
to
719896f
Compare
I’ve tacked on a few incremental commits. I’ve moved back to Copying globalLexicals and per-evaluate endowments by property descriptors turned out to be necessary. The per-evaluate endowments are mutable, per the existing tests, and global lexicals are not. The localObject for modules also needs to copy by property descriptor because the globalLexicals are also frozen there, because they are required to be immutable, but localObject must remain extensible until all of the live binding traps are defined. |
719896f
to
a318e41
Compare
7177213
to
196ee78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
196ee78
to
d30ae04
Compare
As distinct from endowments, extra properties of `globalThis`, global lexicals are constant free variables that are not reachable by computing properties of `globalThis`.
d30ae04
to
32269f2
Compare
As distinct from endowments, extra properties of
globalThis
, global lexicals are constant free variables that are not reachable by computing properties ofglobalThis
.