tc39 / proposal-shadowrealm

ECMAScript Proposal, specs, and reference implementation for Realms
https://tc39.es/proposal-shadowrealm/
1.44k stars 68 forks source link

New evaluators should always have suffix clarifying their start production #183

Closed erights closed 4 years ago

erights commented 6 years ago

Cross-posting from https://github.com/Agoric/SES/issues/14 but copying the initial message in its entirety.

Both realm.evaluate(src, endowments) and SES.confine(src, endowments) parse source as a Program and evaluate it to its completion value. Because many JS expressions, when parsed as Programs, will parse as expression-statements, and because the completion value of an expression statement is the value of the expression, we often mistakenly use them to evaluate expressions. We have repeatedly stubbed our toe on expressions that begin with { or function. These expressions, when parsed as programs, parse as programs that mean something else and have different completion values.

Even after introducing the SES.confineExpr(exprSrc, endowments) convenience, we still accidentally used SES.confine on an inappropriate expression. Suggestion:

All new evaluators should come overloaded by explicit suffix clarifying what their start symbol is. Retire realm.evaluate and SES.confine and instead have:

caridy commented 4 years ago

More info about the difference between the different evaluations: https://github.com/FUDCo/proposal-frozen-realms/wiki/Differences-between-SES,-SES5,-and-ES5#top-level-declarations

caridy commented 4 years ago

Part of this goes into the evaluator, but we are still questioning whether or not realm.evaluate() should be a thing, and if so, what will be the API, that's why this is marked as Stage 3 blocker.

littledan commented 4 years ago

It's a little hard for me to understand how forcing evaluate to be for either a script or module addresses the issue. Do you want people to use modules for programs, and scripts for expressions? It seems like that would still lead to the completion value footguns @erights identified. Should evaluate have an expression mode?

erights commented 4 years ago

I no longer believe evaluate should have an expression mode. Using

const result = evaluate(`(${expr})`);

is good enough, is easy to teach, and is already what people need to do with eval.

littledan commented 4 years ago

I want to note, I'm concerned about #224, for reasons I explained in https://github.com/tc39/proposal-realms/pull/224#discussion_r386337648 . I still haven't seen an explanation of the relation between #224 and this issue.

@erights Could you explain the reason for the change of heart? This issue was opened entirely about these particular concerns with eval. What made things better, when people already could've put those parentheses in themselves?

caridy commented 4 years ago

closed by PR #235