endojs
feat(ses): Shim compatible with Hermes compiler
Description
- Refs: https://github.com/endojs/endo/issues/1891
- SES shim compatible with Hermes compiler
Add lockdown shim compatible with Hermes
Make current shim compatible with Hermes compiler
for building React Native (RN) prod apps with SES
Generating the release AAB (Android App Bundle)
i.e. npx react-native build-android --mode=release via RN CLI
calls Gradle's bundleRelease task under the hood (bundle build task on release variant)
which calls RNGP (React Native Gradle Plugin) React task createBundleReleaseJsAndAssets and fails
after Metro finishes writing the release bundle (bundle, sourcemaps, assets)
Gradle emits these Hermes errors
async functions are unsupported- https://github.com/facebook/hermes/issues/1395
async generators are unsupported
(at runtime we can see both are SyntaxErrors)
Resulting in vague java.lang.StackOverflowError (no error message)
The try/catch approach testing language ft support via a new fn works since RNGP no longer emits the Hermes errors in the task after Metro bundles and we're conditionally using parts of SES compatible with the Hermes engine
The initial approach involved building a new shim via an env var
which involved a lot of duplicates /src files
Nb: clean before bundling to see changes reflected (avoid cache)
i.e. ./gradlew clean :app:bundleRelease
Nb: async function* a() {}; alone won't emit an error
but using/referencing it and beyond const b = a; will
Nb: eventually we hit RNGP BundleHermesCTask.kt > run > detectedHermesCommand > detectOSAwareHermesCommand from PathUtils.kt, which calls the Hermes compiler default command hermesc - the path of the binary file, to create the optimised bytecode bundle to load/exec at runtime
TODO
- [x] use standard async functions without arrows (module-load.js)
- [x] remove async generators from whitelist (permits.js)
- [x] remove async generators from intrinsics (get-anonymous-intrinsics-hermes.js)
- [x] remove repairing async generators during taming (tame-function-constructors.js)
- [x] remove async generator tests (ses, harden, tame-function-unit)
- [x] ensure 379 ses tests passing
- [x] test new shim in React Native repro
- [x] test android bundle via gradle:
./gradlew :app:createBundleReleaseJsAndAssets - [x] test android build/runtime via gradle:
./gradlew :app:installRelease -PreactNativeArchitectures=arm64-v8a - [x] test android/ios build/runtime via RN CLI:
yarn <android/ios> --mode release
- [x] test android bundle via gradle:
- [x] add new entry points to hook into get-anonymous-intrinsics-hermes.js
- original: bundle.js > index.js > lockdown-shim.js > lockdown.js
- hermes: bundle.js > index-hermes.js > lockdown-shim-hermes.js > lockdown-hermes.js
- [x] support env var to bundle new ses/lockdown shim bundle/terse variants (bundle.js)
- [x] bundle new shims in prepare npm lifecycle script (package.json)
- [x] add new shim exports entry points (package.json)
- [x] conditionally get intrinsics (get-anonymous-intrinsics.js)
- [x] conditionally repair async generators (tame-function-constructors.js)
- [x] conditionally add async generators to whitelist (permits.js)
- [x] revert new publishing changes
- [x] revert new entry points
- [x] revert new bundling with env var changes
- [x] ensure 379 ses tests still passing
- [x] restore original tests
- [x] suggested change
- [x] revert permits.js changes
- [x] asyncTrampoline non-arrow async function
- [x] console info msg x2
- [x] improved catch block
- do nothing only on the SyntaxError
- otherwise re-throw
- add to commons.js and use
- [x] AsyncGeneratorFunctionInstance
- AsyncArrowFunctionInstance?
- CI (new PR, stacked)
Follow-up, CI testing options discussed
- ubuntu: RN app + SES,
cd android && ./gradlew :app:bundleRelease- add Java CI docs if included
- react-native-community/docker-android is available to use
CI(macos): RN app test + SES, Xcode release- not needed, Xcode does not emit Hermes errors like Gradle
test262:hermesscript (liketest262:xs)- Hermes maintains test262 tests (testsuite.py, testsuite_skiplist.py)
- (eshost) > Hermes CLI v0.12.0 (few years old, newer being discussed)
- or build and run Hermes (and/or Static Hermes) as a standalone compiler and VM
- add docs or at least link to Hermes docs
Security Considerations
Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?
Scaling Considerations
Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?
Documentation Considerations
Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?
Testing Considerations
Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?
Compatibility Considerations
Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?
Upgrade Considerations
What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?
Include
*BREAKING*:in the commit message with migration instructions for any breaking change.
Update
NEWS.mdfor user-facing changes.
Delete guidance from pull request description before merge (including this!)
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small
Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow.
One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.
Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in master with a single merge commit.
Glad to see this!
I would like to make sure @erights looks at these changes as well.
Hopefully tomorrow.
Nb: interestingly this isn't surfaced nor fixed running lint scripts within the ses folder
edit: ah it's a Prettier problem, unrelated to ESLint, i.e. fixed with yarn format
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small
Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow.
One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.
Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in
masterwith a single merge commit.
created the test changes in PR based on this branch to work on here
- https://github.com/leotm/endo/pull/1
One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.
@kriskowal 120ish commits groomed into 6 ^ and all requested changes addressed
What is the status of this? Is it still needed?
yes this is still needed for CI feedback keeping SES compatible with Hermes
after rebasing off master i've added https://github.com/endojs/endo/pull/2334/commits/7b9eca2277b90328a78036a9ff9b77c5554890fc for now (tbd)
otherwise 5 failing tests left to resolve and https://github.com/endojs/endo/pull/2334#discussion_r1860412296 (tbd) before ready for final re-review/merge
(unless any further comments on the open threads)
I plan to add draft release notes to NEWS.md separately after the last 2 issues are addressed (removeUnpermittedIntrinsics and assertDirectEvalAvailable) which close https://github.com/endojs/endo/issues/1891 and enable https://github.com/LavaMoat/LavaMoat/pull/1438
Not blocking: I would like a SES-specific lint rule, like the we have for
@endo/polymorphic*, so that we get a lint error if we usethisinside an arrow function, since evidently that will break the Hermes version.
added to tracking issue under 'Non blocking' header
endo sync: the current error in the transform that provides feedback in CI is adequate
I think there is a risk under maintenance that we add a new syntactic async generator in the SES-shim, that would be unexpectedly transformed under Hermes.
Let's add a lint rule that warn on async generator usages that Hermes requires special handling, and silence the lint warning in the EvalError fallback case, which has the special handling.
I think there is a risk under maintenance that we add a new syntactic async generator in the SES-shim, that would be unexpectedly transformed under Hermes.
Let's add a lint rule that warn on async generator usages that Hermes requires special handling, and silence the lint warning in the EvalError fallback case, which has the special handling.
added to tracking issue under 'Non blocking' header