ESC needs better eval support

VERIFIED WONTFIX

Status

VERIFIED WONTFIX
11 years ago
9 years ago

People

(Reporter: markh, Assigned: lhansen)

Tracking

Details

Attachments

(4 attachments)

ESC can currently compile 'scripts' but has no way to compile and evaluate expressions.  Pedantically, the names in main.es (evalString/evalFile) should possibly be renamed to execString/execFile, to reflect the fact they execute script code rather than evaluate an expression
(Assignee)

Comment 1

11 years ago
Just a quick note about how full ES3-style "eval" might be supported without changing Tamarin much.

The central problem is that (a) code is loaded into the global environment as a script (by means of Domain.loadBytes) and any code in the script is closed in the global environment, and (b) that script needs access to the scope chain of the calling code.

Ergo, ES3 eval can be implemented as a top-level function in the script, having a unique name, that receives an array of scope objects.  It pushes those objects onto its scope chain, and then runs the code to be evaluated, which is compiled into the function.  The scope-pushing code can know whether each object is a regular scope object or a "with" object, because that's all statically known when the script is compiled.  All the caller needs to do is push the live scope objects as arguments and invoke the script function.

In other words,

   ...
   eval(s)

becomes first a script

   function .EVAL.127(...args) {
      // pushscope args[0]
      // pushscope args[1]
      ...
      <s, compiled>
   }

and the calling site becomes

   [name,file] = compileScriptForEval(s, <info about the current scope>)
   domain.loadBytes(file)  // this defined name globally
   // push scope object
   ...
   // push scope object
   getglobalscope
   findprop name 
   call

There are two problems remaining.

One is that the eval code can introduce bindings into the innermost VAR object, but activation records are non-dynamic in Tamarin.  So there needs to be a bit on the MethodInfo or thereabouts that allows the activation object to be dynamic, and that searches properly.  It's possible we can use OP_pushwith, but that's dangerous because of prototype properties.  At a minimum we'd need to be able to create a plain object.

The other is that we must detect uses of eval statically in order not to impair early binding to lexical variables.  ESC does not implement early binding yet but it must (soon).  Fortunately, calls to eval can be lexically determined in ES3 and ES4, and though eg Firefox allows more general use of eval, that is not necessary for web compatibility.
(Assignee)

Updated

11 years ago
Assignee: nobody → lhansen
(Assignee)

Comment 2

11 years ago
Created attachment 304203 [details] [diff] [review]
Patch 1: cleaned up program analysis and code generation

This is the first part of the "eval" puzzle: analyzing the code for static occurences of "eval" and propagating attributes.  This patch also cleans up the context stack in the code generator, moving from a per-function stack to a global stack.
Attachment #304203 - Flags: review?(tierney)
(Assignee)

Comment 3

11 years ago
Created attachment 304204 [details] [diff] [review]
Patch 2: OP_getouterscope instruction

This is the second piece of the "eval" puzzle: a "getouterscope" instruction that allows code to pick up objects in its outer scope stack.
Attachment #304204 - Flags: review?(edwsmith)

Comment 4

11 years ago
Comment on attachment 304204 [details] [diff] [review]
Patch 2: OP_getouterscope instruction

Looks good.  Simple, eh?  Strictly optional but consider also updating utils/abcdump.as
Attachment #304204 - Flags: review?(edwsmith) → review+

Updated

11 years ago
Attachment #304203 - Flags: review?(tierney) → review+
(Assignee)

Comment 5

11 years ago
Patches 1 and 2 pushed.  Will fix abcdump.as (and ditto in ESC) in separate patch.
(Assignee)

Comment 6

11 years ago
Created attachment 305470 [details] [diff] [review]
Patch 3: bug fixes to OP_getouterscope

I forgot to set the operand count in opcodes.tbl.  Also, this patch includes a fix to abcdump.as.
Attachment #305470 - Flags: review?(edwsmith)
(Assignee)

Comment 7

11 years ago
Created attachment 305477 [details] [diff] [review]
Patch 4: eval parts 1 and 2

Biggish patch that does three things (but too messy to separate them):

(1) Support for eval that 

  - handles expressions and statements
  - captures the statement result value and returns it
  - is available to see its caller's lexical scope

This eval has some shortcomings:

  - it does *not* allow bindings to be introduced in the caller's scope.  That requires a VM fix and I'm still pondering the best way to do that.
  - there may be subtle bugs wrt to how scope is handled (esp in class methods)

It seemed more important to ship this code even in its imperfect state than to get these things right ATM, it'll be a couple of weeks before I have time to finish it, most likely.

(2) Refactoring of esc.{es,sh} and main.sh so that main.sh is a more useful shell; if it is passed filenames it compiles and loads them and then exits, otherwise it enters the REPL, like now.

main.sh now uses core in esc-core.es instead of having its own notion of how the compiler works.

(3) A very rudimentary sketch for how to run the ES4RI spidermonkey test suite; UTSL.
Attachment #305477 - Flags: review?(tierney)
(Assignee)

Updated

11 years ago
Attachment #305477 - Flags: review?(mhammond)
There seems to be a bit of a bootstrap problem here.  After applying patches 3 and 4, ESC can't build itself. 'make' and 'comp-esc.sh' both generate:

cannot open file: ../bin/esc-core.es.abc
cannot open file: ../bin/esc-core.es.abc
...
(Assignee)

Comment 9

11 years ago
Yeah, I should have noted that.  If you want to apply this patch without waiting for my binaries, you need to remove the line that loads esc-core.es.abc in esc.sh (that's all), then add it once you've rebuilt.  The refactoring introduced that file, but of course it's not part of the patch as Mercurial is deficient wrt to shipping patches to binary files around.

Comment 10

11 years ago
Lars, do you have git-style patches turned on in mercurial? I believe they can properly represent moving binary files around. Also, if you wanted precision you could attach the changeset as a bundle.
(Assignee)

Comment 11

11 years ago
Git-style patches: no.

Bundle?  I have tried "hg export" but I'm not all that impressed.  Do you have something else in mind?  I'm anything but sophisticated wrt hg.

Comment 12

11 years ago
I recommend always turning on git-style patches: you can do this in ~/.hgrc with

[diff]
git=1

As for bundles, they are binary files that represent one or more changesets. They aren't directly readable. You can create them with `hg bundle` and apply them with `hg unbundle`.

Updated

11 years ago
Attachment #305470 - Flags: review?(edwsmith) → review+

Updated

11 years ago
Attachment #305477 - Flags: review?(tierney) → review+
(Assignee)

Comment 13

11 years ago
Patches 3 and 4 pushed, with new binaries.
I'm struggling to make this work with ScreamingMonkey.  SM now has eval implemented:

"""
	namespace ESC = "ESC"; // implicit namespace from esc-env.ast

	public function eval(s)
	{
		use namespace ESC;
		ESC::evaluateInScopeArray(s, "", []);
	}
"""

It is probably relevant that the above code is compiled by the ASC compiler, not ESC (it is in axscript/mscom.as).

When this function is called, I get:

C:\temp\delme.js2(0, 1) (null): SyntaxError: esc-env.ast:1: Internal: evalIdentExprToNamespace: case not implemented [object ReservedNamespace]
        at syntaxError()[../src/util.es:59]
        at internalError()[../src/util.es:68]
        at internalError()[../src/parse.es:187]
        at Parse::Context/evalIdentExprToNamespace()[../src/parse.es:487]
        at Parse::Context/openNamespace()[../src/parse.es:339]
        at Parse::Parser/Parse::program()[../src/parse.es:5420]
        at boot()[../src/esc-core.es:66]
        at evaluateInScopeArray()[../src/eval-support.es:15]
        at global$init()[<script 1>:7]  <------ this line has eval('1+1')
        at axtam::Domain/loadBytes()
        at ScriptEngine/executeScriptBlock()
        at ScriptEngine/executePendingScriptBlocks()
        at ScriptEngine/setStateStarted()
        at <anonymous>()
        at ScriptEngine/transitionTo()
        at ScriptEngine/SetScriptState()

This can be reproduced by building ScreamingMonkey, then creating a file with extension ".js2" containing the single line "print(eval('1+1'))", then execute 'cscript.exe filename.js2'.  Note that you will not see the complete traceback, but will see the exception.
Blocks: 436029
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Summary: ESC needs eval support → ESC needs better eval support
(Assignee)

Updated

10 years ago
Attachment #305477 - Flags: review?(mhammond)
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.