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

feat(ses): Support global lexicals #356

Merged
merged 14 commits into from
Jul 5, 2020
Merged

feat(ses): Support global lexicals #356

merged 14 commits into from
Jul 5, 2020

Conversation

kriskowal
Copy link
Member

As distinct from endowments, extra properties of globalThis, global lexicals are constant free variables that are not reachable by computing properties of globalThis.

Copy link
Contributor

@warner warner left a 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.

// Collect all valid & immutable identifiers from the endowments.
const localConstants = localNames.filter(
name =>
isValidIdentifierName(name) && isImmutableDataProperty(localObject, name),
!lexicalConstants.includes(name) &&
Copy link
Contributor

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?

// Global lexicals.
if (prop in globalLexicals) {
// Use reflect to defeat accessors that could be present on the
// globalLexicals object itself as `this`. XXX?
Copy link
Contributor

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?)

packages/ses/test/global-lexicals-modules.test.js Outdated Show resolved Hide resolved
@warner
Copy link
Contributor

warner commented Jun 28, 2020

Also, just confirming my understanding.. when we say "endowments", do we strictly mean properties of globalThis? I know we needed a term for "global lexicals" to make it clear that they're distinct from (enumerable, unavoidably reachable through computed property lookup) properties of the global. But I'm wondering what our historical definition has been (maybe this is a question for @erights).

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 globalThis, or is this distinction a new requirement, driven by JS having a name for the collection of global names?

@kriskowal kriskowal force-pushed the kris/ses-rollup-commonjs branch 3 times, most recently from a3eb545 to 76ae82e Compare June 30, 2020 20:12
@kriskowal kriskowal changed the base branch from kris/ses-rollup-commonjs to master June 30, 2020 21:58
@michaelfig
Copy link
Member

Also, just confirming my understanding.. when we say "endowments", do we strictly mean properties of globalThis? I know we needed a term for "global lexicals" to make it clear that they're distinct from (enumerable, unavoidably reachable through computed property lookup) properties of the global. But I'm wondering what our historical definition has been (maybe this is a question for @erights).

I can fill in some of this. "endowments" used to be "global lexicals" in SES-0.6. It was when Moddable created the Compartment API that they made "endowments" a synonym for "globalThis properties". We then adopted this meaning, which necessitated introducing "global lexicals" as a new categorisation for what we had before.

@kriskowal kriskowal force-pushed the kris/global-lexicals branch 4 times, most recently from 74c59bf to 788b190 Compare July 2, 2020 00:09
Copy link
Contributor

@erights erights left a 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?

packages/ses/src/compartment-shim.js Show resolved Hide resolved
packages/ses/src/compartment-shim.js Outdated Show resolved Hide resolved
packages/ses/src/compartment-shim.js Show resolved Hide resolved
packages/ses/src/scope-handler.js Outdated Show resolved Hide resolved
packages/ses/src/scope-handler.js Outdated Show resolved Hide resolved
@kriskowal
Copy link
Member Author

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?

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 localObject (as it’s named in the scope handler) or endowments (as it’s named by the evaluator).

Endowment has indeed become a very confusing name internally. There are things named endowment throughout the evaluator machinery that do not correspond to the compartment constructor option. I would like to straighten those out in this change.

@kriskowal kriskowal force-pushed the kris/global-lexicals branch 4 times, most recently from daa837c to 2f22de6 Compare July 3, 2020 00:08
Copy link
Contributor

@erights erights left a 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?

packages/ses/package.json Outdated Show resolved Hide resolved
packages/ses/src/compartment-shim.js Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Show resolved Hide resolved
packages/ses/src/scope-handler.js Outdated Show resolved Hide resolved
packages/ses/src/compartment-shim.js Show resolved Hide resolved
packages/ses/src/module-instance.js Show resolved Hide resolved
packages/ses/src/scope-handler.js Show resolved Hide resolved
@kriskowal
Copy link
Member Author

I’ve tacked on a few incremental commits.

I’ve moved back to tape @ 4. This version doesn’t have a match method, but we probably shouldn’t be validating error messages anyway, especially to the extent I had, which may have made the tests brittle between engines.

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.

packages/ses/src/compartment-shim.js Show resolved Hide resolved
packages/ses/src/compartment-shim.js Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
@kriskowal kriskowal force-pushed the kris/global-lexicals branch 4 times, most recently from 7177213 to 196ee78 Compare July 4, 2020 01:00
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kriskowal kriskowal merged commit aefefbf into master Jul 5, 2020
@kriskowal kriskowal deleted the kris/global-lexicals branch July 5, 2020 01:59
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

Successfully merging this pull request may close these issues.

4 participants