evaluators should all use the same `evaluate` flavor

[warner]

As @jfparadis explained to me, there are basically six ways to evaluate javascript code: (program/script, expression, eval) times (strict-mode, sloppy-mode). program behaves like <script> tags in an HTML file: the evaluated strings can contain multiple ;-separated statements, and each one modifies a shared global environment (so top-level let and const and function definitions accumulate and are visible to subsequent evaluations). The expression mode takes only a single expression. The eval mode handles both.

SES-0.6.x (soon to be known as "old SES") offered an s.evaluate() that basically operated in the third "eval" mode. SES-0.7.x (known as "new SES") will offer a c.evaluate() that's pretty much only in "expression" mode. We couldn't emulate program/script mode accurately enough, so it seemed best to not even pretend to support it. If that changes in the future, you'd use c.evaluate(source, { type: "program" }) to access it. Everything is strict-mode.

To make it easier to program both inside and outside of a SES environment, the @agoric/evaluate package provides a number of named exports (evaluateProgram, evaluateExpr, evaluateModule), and a default export that is equal to evaluateProgram. It also provides makeEvaluators which offers even more control.

Several agoric-sdk packages use @agoric/evaluate. The default export is used by SwingSet, cosmic-swingset, pixeldemo, spawner, and zoe. evaluateProgram is imported by name in agoric-cli and cosmic-swingset, and makeEvaluators is imported by cosmic-swingset to set up the REPL.

Code that passes e.g. function foo() { stuff } to evaluateProgram needs to switch to passing ( function foo() { stuff } ) into evaluateExpr.

As we move to new-SES, evaluateProgram is going away. Callers need to switch to evaluateExpression. This maybe a little messy because evaluateProgram was the default export, and in the future evaluateExpression wants to be the default export (since evaluateProgram is going away). I'm thinking the sequence should be:

• change all callers to import { evaluateExpr } from '@agoric/evaluate', and change the strings the pass in by wrapping them with parentheses
• (bump semver), remove the default export from @agoric/evaluate
• (probably bump semver), replace @agoric/evaluate to use new-SES
• (maybe) (definitely bump semver) change @agoric/evaluate to have a default export that matches evaluateExpr
• (maybe) change callers to import evaluateExpr from '@agoric/evaluate'

@jfparadis @michaelfig @erights did I get this right?

[jfparadis]

Simpler:

• program/script should perform an evaluation in the global lexical scope, which we can't correctly emulate using the evaluators available in JS (eval and Function) without parsing. It should not return a completion value.
• expression is not well defined but should be single expression (because eval('1;2') doesn't make sense) and it should return the completion value. It makes sense that expressions are evaluated in a new lexical scope, but the global object can be affected in sloppy mode.
• eval() is a mix of both, plus the capacity to perform direct mode, BUT always executes in a new lexical scope, and always returns a completion value.

new Compartment().evaluate() is proposed normative to behave like eval(), except with indirect and strict modes always enabled because we want the Compartment API to be forward looking, like modules.

[warner]

Ok, so program/script mode is basically only for side-effects, just like <script> tags.

In the one "eval" mode we're implementing, the completion values look like:

source	completion value
1+2	3
1; 2	2
1; 2;	2
function foo() { .. }	undefined (foo is defined but then discarded along with the rest of the lexical scope)
(function foo() {..})	a function named foo

(edited to incorporate @erights 's correction below)

[erights]

The completion value of 1; 2; is 2. The others look right.

[erights]

There was controversy about this in a meeting just now, about what actually happens. Using the SES-shim, I just verified that, when c is a compartment instance,

c.evaluate('5;6;')

evaluates to 6 as expected. Also

c.evaluate('5;7;const y = 9;;')

evaluates to 7, since neither declarations nor empty statements affect the "current" completion value, i.e., the completion value prior to the declaration or empty statement. All of these are exactly consistent with the behavior of JavaScript's own eval.

[erights]

c.evaluate('(function foo(){}); function bar(){}');

evaluates to the foo function, since, above, we have a foo function expression, which does have a completion value, and a bar function declaration, which does not affect the completion value.

[Chris-Hibbert]

As we're doing the migration, it would be nice to have a pointer from the source (.../evaluate/src/index.js) to this issue describing the evolving behavior.

[erights]

describing the evolving behavior.

This has always been the behavior. The behavior is confusing and surprising, but has always exactly mirrored JavaScript's (confusing and surprising) eval behavior. What's evolved is that this behavior is now more widely understood.

[warner]

Ok so I think I was wrong when I said:

SES-0.7.x (known as "new SES") will offer a c.evaluate() that's pretty much only in "expression" mode.

And in fact new SES's c.evaluate behaves in the third hybrid "eval" mode, just like old-SES.

@erights are you ok with the plan to have @agoric/evaluate only offer evaluateExpr? And is that name acceptable? This is the user-facing API for evaluating things on our platform, when they aren't being spawned off into a separate dynamic vat.

(we might also consider making new Compartment and c.evaluate() be the user-facing API, but that requires changing the way we run tests to always produce a SES environment before allowing the test code to run, which is a bit more work, and potentially interferes with downstream applications using our libraries in other environments)

[erights]

@erights are you ok with the plan to have @agoric/evaluate only offer evaluateExpr?

First, just to check we're talking about the same thing, the relevant code starts at https://github.com/Agoric/agoric-sdk/blob/master/packages/evaluate/src/index.js#L75 ?

If so, no. I think the insecure evaluate we offer, if we need to offer one, should stay as close to the compartment api's evaluate as possible, to encourage porting of code from running insecurely to running securely.

[warner]

That's the relevant code for the "insecure" version, yes. The relevant code for the secure version is in my swingset-on-ses2 branch, and currently looks like this:

function makeSESEvaluator() {
  let c;
  function evaluate(src) {
    return c.evaluate(src);
  }
  function evaluateWithEndowments(src, endowments) {
    return c.evaluate(src, { endowments });
  }
  function req(what) {
    if (what === '@agoric/harden') {
      return harden;
    }
    if (what === '@agoric/evaluate') {
      // what precisely should this return?
      return evaluateWithEndowments;
      //return { default: evaluateWithEndowments,
      //         evaluateProgram: evaluateWithEndowments,
      //       };
    }
    throw Error(`unknown require(${what})`);
  }

  const endowments = {
    console: makeConsole(console),
    require: req,
    evaluate,
    HandledPromise,
  };
  const modules = undefined; // unimplemented
  const transforms = makeDefaultEvaluateOptions().transforms;
  c = new Compartment(endowments, modules, { transforms });
  harden(c.global);
  return src => {
    return c.evaluate(src);
  };
}

We want these two versions to contain the same names and provide the same behavior for each name.

What I'm hearing from @jfparadis is that the Compartment cannot provide evaluateProgram, and it sounds like it's not really providing a pure expression evaluator either (c.evaluate() accepts "1; 2", which isn't an expression).

For that matter, it sounds like Javascript itself doesn't have any pure-expression evaluators: the base eval accepts "1; 2" too.

If I've got that right, then the "insecure" library should be offering just one option. When the transition is complete, we don't need a special name for this option: it will just be the default export of @agoric/evaluate, and there will be no named exports. But during the transition, while we're moving clients from evaluateProgram to this new behavior, we need a distinct name to move them to:

1. import evaluate from '@agoric/evaluate' (meaning evaluateProgram)
2. import { evaluateProgram } from '@agoric/evaluate' (make it explicit)
3. import { evaluateXXX } from '@agoric/evaluate' (change client to new behavior)
4. modify @agoric/evaluate to change default export
5. import evaluate from '@agoric/evaluate' (now means evaluateXXX)

So I guess I'm asking how to spell this evaluateXXX option, and whether this plan seems correct. evaluateProgram is obviously wrong, and evaluateExpr implies that it's an expression-only evaluator, which it's not. If I named it evaluate (as in import { evaluate } from '@agoric/evaluate'), would that be too confusing?

[jfparadis]

@warner, I think it's important keep things simple. But focusing on the evaluator seems incorrect.

So I guess I'm asking how to spell this evaluateXXX option, and whether this plan seems correct. evaluateProgram is obviously wrong, and evaluateExpr implies that it's an expression-only evaluator, which it's not. If I named it evaluate (as in import { evaluate } from '@agoric/evaluate'), would that be too confusing?

You should name it evaluate because you are not changing the source (the what), just creating a context for it, a custom compartment (the where):

  return src => {
    return c.evaluate(src);
  };

Instead, I would change makeSESEvaluator to something else, like createImmutableCompartment and return the compartment instance, not a function. Then in your code, you get this very expressive API:

immutableCompartment.evaluate(src);

All state is held by the immutableCompartment object, it's easier to debug, and its a much richer API to use in your code than an evaluator. In addition, your @agoric/evaluate loses its role as a singleton as it is doing now.

[warner]

I'm happy to make that change, but what I'm currently concerned about is the other code that wants to use a two-argument evaluator, like in Zoe and ERTP. Examples like these:

packages/cosmic-swingset/lib/ag-solo/vats/repl.js:

import { makeEvaluators } from '@agoric/evaluate';
...
const { evaluateProgram } = makeEvaluators({ sloppyGlobals: true });
...
      const endowments = {
        console,
        E,
        commands,
        history,
        home: homeObjects,
        harden,
      };
      let r;
      try {
        r = evaluateProgram(body, endowments);
 ...

packages/cosmic-swingset/lib/ag-solo/bundle.js

import { evaluateProgram } from '@agoric/evaluate';
...
const mainNS = evaluateProgram(actualSources, { require })();
...

packages/zoe/src/evalContractCode.js:

import evaluate from '@agoric/evaluate';
...
const evaluateStringToFn = (functionSrcString, endowments) => {
  assert.typeof(functionSrcString, 'string');
  const fn = evaluate(functionSrcString, endowments);
  assert.typeof(
    fn,
    'function',
    details`"${functionSrcString}" must be a string for a function, but produced ${typeof fn}`,
  );
  return fn;
};

packages/spawner/test/swingsetTests/escrow/vat-host.js

import evaluate from '@agoric/evaluate';
import { makeContractHost } from '../../../src/contractHost';
...
          return harden(makeContractHost(E, evaluate));

Those files get their evaluator by importing a module, which lets them be tested outside of a SES environment (in addition to being executed inside a SES environment). We've gone back and forth a bit about whether to change our required test environment to provide things like evaluate and harden as globals, or continue to use this sometimes-from-NPM/sometimes-injected @agoric/evaluate module so this code can be tested from normal environments too. I'm assuming we're going to continue with the module approach for now, so I need to update that module's exports to match the new (smaller) API of the new SES compartments.

[jfparadis]

I would change those tests to import your makeImmutableCompartment.

On one end, you can better test your immutableCompartment instance in its package. And the package itself is much more useful. Those tests that you mention above will benefit, for example they can put spies on the global of the compartment if design the makeImmutableCompartment to accept additional globals.

On the other hand, we need to get rid of @agoric/evaluate because it's unsafe, incorrect, and it's a duplication of the compartment code. We had several issues with it when testing the module system that was built on it. I'm very concerned when I see in your example zoe importing it.

Those files get their evaluator by importing a module, which lets them be tested outside of a SES environment.

I see, so we have created a parallel @agoric/evaluate and @agoric/harden to test outside of a SES environment, and now we are stuck because we have code and tests depending on it, and we can't publish harden to NPM because it can give to others a false sense of security?

Could it be that those early architecture decisions are finally showing their limitations? Something has to change, but the division of concerns was not well established, so when you try to refactor a part of the code it ends up impacting a lot more than you envisioned?

My advise is to address the problem right now in steps. Deprecate '@agoric/evaluate`, don't try to fix it. Create the immutable compartment. Use it in Zoe. You can also use it in some tests, that will give you a much better and much simpler system in terms of size, complexity, and security.

[warner]

Ok, here's a proposal that might incorporate JF's suggestion, and maybe shifts the burden of effort/surprise in a way that still works:

• first we change all users of @agoric/evaluate (swingset, agoric-cli/lib/deploy.js, cosmic-swingset (REPL and contract-host vat), spawner, zoe (evalContractCode)) to use the same flavor of evaluation, which means adjusting the code they submit (maybe adding parenthesis). This flavor isn't evaluateProgram because that's going away, and it isn't evaluateExpr because we don't really have anything limited to just expressions, and is instead the sort of generically-named evaluate flavor that accepts both programs and expressions. So maybe @agoric/evaluate will have only the default export and no named exports, since we've only got one flavor to offer.
• second: rewrite @agoric/evaluate to import (new-)SES, and internally call lockdown() if it hasn't been already (i.e. if harden and Compartment aren't in scope), then create a new Compartment, harden its global, then use c.evaluate(src, endowments) and return the result. If harden/Compartment are already present (i.e. if evaluate() has already been called in this process), we don't call it a second time, but we still create a new Compartment to do the evaluation.
• third: change the @agoric/evaluate which swingset provides to require() inside the kernel's compartment (which is used by all vats too) to only offer the same flavor of evaluation, which works by just calling c.evaluate from the kernel compartment. So it doesn't create a new compartment for evaluation.

Code inside a swingset environment will never use the @agoric/evaluate from NPM: bundle-source will continue to label it as an "external" exit from the rollup graph, and SwingSet's require('@agoric/evaluate') routes to the kernel Compartment's c.evaluate. Code inside a swingset environment will never load the SES shim, because it's already running in a SES environment.

The surprise this approach imposes is that any program that includes @agoric/evaluate in its dependency graph (maybe as some far-off transitive dependency, maybe just a devDependency used for a couple of unit tests) might suddenly discover itself in a SES environment, the first time someone calls evaluate. If we manage to implement https://github.com/Agoric/SES-shim/issues/202 (to leave Date.now and Math.random available to the start compartment), and nothing tries to modify the globals, then they might not notice.

The second alternative would be: the @agoric/evaluate on NPM assumes it is already being run inside a SES environment, does not import SES or call lockdown, but still uses new Compartment and harden for the evaluation. SwingSet operates the same way as the first approach. The surprise imposed by this approach would be that all programs which include @agoric/evaluate in their dependency graph must start by importing SES and calling lockdown(), else they'll get surprising errors when evaluation finally happens.

[Chris-Hibbert]

I like the three-step proposal, though I don't understand where that would leave tests. I think we already got rid of the non-SES tests.

• Is there a way to test contract (spawner or zoe) that presume they'll be run inside a new vat?
• Will contracts that need imports get them the same way in tests and normal use?

Do we use @agoric/evaluate outside spawner and zoe? I think if it's just those two, then there shouldn't be surprises with code that sometimes runs inside and sometimes outside SES.

[warner]

We did indeed get rid of the tests that ask for a non-SES SwingSet environment. I'm thinking of tests that don't ask for a SwingSet environment at all (maybe packages/zoe/tests/unitTests/ as opposed to packages/zoe/tests/swingsetTests?). If any of those tests still do evaluation, they'll be using the @agoric/evaluate that comes from NPM, rather than the one SwingSet injects into the environment it provides.

However both vats (passed into swingset's makeVatController configuration argument) and non-vats (strings passed into @agoric/evaluate) will be running in a SES environment, in this proposed scheme.

Contract code that wants to use @agoric/evaluate should just import it as usual. If that code gets run inside a swingset vat, it will wind up using the SES evaluator provided by swingset. If it gets run normally, it will get the @agoric/evaluate package from NPM, which will create a new SES environment (if necessary) and use an evaluator from that.

I see imports of @agoric/evaluate from zoe, agoric-cli/lib/deploy.js, spawner, and a few places in cosmic-swingset, so I think you're right that there aren't too many opportunities for surprise.

[warner]

With my updated understanding of the evaluators, I'm changing the title/purpose of this ticket from evaluators should use evaluateExpr, not evaluateProgram, to evaluators should all use the same flavor. The other two steps of my proposal in https://github.com/Agoric/agoric-sdk/issues/602#issuecomment-594797528 will go into separate tickets.

All agoric-sdk users of @agoric/evaluate will change to use only the default export. I now believe that this "one true eval" flavor is the one named evaluateProgram in the current version of @agoric/evaluate. The @agoric/evaluate code will be changed to export evaluateProgram as its default, and the other flavors (evaluateExpr and evaluateModule) will be removed. The evaluateExpr flavor wraps parentheses around its argument, and sometimes removes an extra semicolon if Babel added one. The evaluateModule simply throws an error.

Part of this effort requires unification of the options available to our various evaluators. For example, I'm looking at https://github.com/Agoric/agoric-sdk/blob/d5df3157801ee506ef611a9b4b57122c6a6f0453/packages/cosmic-swingset/lib/ag-solo/vats/repl.js#L77 and https://github.com/Agoric/agoric-sdk/blob/d5df3157801ee506ef611a9b4b57122c6a6f0453/packages/cosmic-swingset/lib/ag-solo/vats/repl.js#L118

to understand what our REPL needs. It uses makeEvaluators (which I think goes away) to capture a sloppyGlobals: true option, and then calls a two-argument form evaluateProgram(body, endowments).

In new-SES, the API is new Compartment(endowments, modules, options) and c.evaluate(src, options). The only Compartment option recognized is transforms, but the evaluate options can include endowments, transforms, and sloppyGlobalsMode.

So if the REPL were using the Compartment API directly, it would create a single shared compartment (c = new Compartment()) and then for each submitted string it would call c.evaluate(src, { endowments, sloppyGlobalsMode: true }). That Compartment was created from within an existing Compartment on which the tildot and metering transforms had been imposed, so the new Compartment gets them too.

[warner]

In the short term, we'll keep using @agoric/evaluate. In the long term, when ses-adapter is fully functional, downstream code will either use import { importBundle } from '@agoric/import-bundle' (for full bundles), or import { Compartment } from 'ses-adapter' and create/use their own compartmsnts (for single strings). In the longer term, import-bundle will use a module-loader API on Compartment, but I suspect the way downstream code interacts with it will remain the same.

I'm now thinking that we align on import { evaluateProgram } from '@agoric/evaluate', rather than using the default export. Outside of swingset and cosmic-swingset (both of which will be changed in my #477 branch), I see the following uses of @agoric/evaluate:

• zoe/src/evalContractCode.js: import { evaluateProgram }
• agoric-cli/lib/deploy.js: import { evaluateProgram }
• bundle-source/test/sanity.js: import { evaluateProgram as evaluate }`
• spawner/test/swingsetTests/contractHost/vat-host.js: import evaluate
• spawner/test/swingsetTests/escrow/vat-host.js: import evaluate

So the two spawner tests are the ones that might need renaming and/or behavioral changes.

[erights]

We should kill the name evaluateProgram and just call it evaluate.

[warner]

I think we can close this now, as irrelevant. We've deleted @agoric/evaluate and @agoric/ses-adapter. Going forward, any code which wants to do dynamic evaluation must either 1: use new Compartment() and c.evaluate(), or 2: use importBundle(). The behavior of the former is defined by SES and the Compartment shim. The latter takes a module graph bundle, so its behavior is defined by JS itself (module syntax).

Both options require a SES environment (since importBundle uses Compartment internally), so the program that contains this evaluating code must start with import '@agoric/install-ses' to set that up.

If anyone disagrees, please feel free to re-open.