579230 - Builder pattern refactor of the narcissus parser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre
Build Identifier: 

Refactor of the narcissus parser into the builder pattern so we can interchange the vanilla builder with other builders that do analysis, such as SSA.

Reproducible: Always

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Builder w/o support for let

Current un-niceties: the bottom-up expression parser doesn't play nicely with doing transforms of the AST via the builder.

Comment 2

[Andreas Gal :gal]

make this bug depend on your other bug

Comment 3

[Andreas Gal :gal]

actually other way around, your other bug depends on this one

Comment 4

[Brendan Eich [:brendan]]

Maybe we should use recursive descent for expression parsing. Separate bug.

/be

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Builder refactor + expr parser rewrite to maintain parity with TM

The new expression parser handles the following:
 - generators and generator expressions
 - expression closures
 - destructuring
 - let

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Builder refactor + expr parser rewrite to maintain parity with TM

Previous patch was based against an old TM tree.

Comment 7

[Tom Austin]

The testing framework does not start up now.  Here is the error message:
$ python jstests.py -d -j 4 ../njs -m narcissus.list -x narcissus-failures.txt 
Traceback (most recent call last):
  File "jstests.py", line 282, in <module>
    test_list = manifest.parse(OPTIONS.manifest, xul_tester)
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 104, in parse
    ans += parse(os.path.join(dir, include_file), xul_tester, include_reldir)
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 104, in parse
    ans += parse(os.path.join(dir, include_file), xul_tester, include_reldir)
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 104, in parse
    ans += parse(os.path.join(dir, include_file), xul_tester, include_reldir)
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 104, in parse
    ans += parse(os.path.join(dir, include_file), xul_tester, include_reldir)
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 136, in parse
    if xul_tester.test(cond):
  File "/Users/taustin/mozilla/tracemonkey/js/src/tests/manifest.py", line 78, in test
    raise Exception("Failed to test XUL condition '%s'"%cond)
Exception: Failed to test XUL condition '!xulRuntime.shell&&xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(/x86_64/)'


Evaluating the condition from the error message gives an error about a missing operand:

  $ ../njs -e '!xulRuntime.shell&&xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(/x86_64/)'
  :1: Missing operand


Previously, this gave an error that xulRuntime was not defined:
  $ ../njs -e '!xulRuntime.shell&&xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(/x86_64/)'
  :1: xulRuntime is not defined

The regular expression might be the culprit. Removing that gets the same error message as pre-patch:
  $ ../njs -e '!xulRuntime.shell&&xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(0)'
  :1: xulRuntime is not defined

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Builder refactor + expr parser rewrite to maintain parity with TM

The following tests not in narcissus-failures fail, mostly for reasons not
related to parsing.

Run out of memory; don't know what's wrong or how to fix.
  - js1_5/Regress/regress-159334.js
  - js1_5/Regress/regress-367561-01.js
  - js1_5/Regress/regress-367561-03.js

Array comprehensions not implemented in jsexec.js.
  - js1_5/Regress/regress-352009.js
  - js1_7/block/regress-349653.js

Destructuring not implemented in jsexec.js.
  - js1_7/expressions/regress-346203.js
  - js1_8/regress/regress-459185.js

No real decompiler.
  - js1_5/extensions/regress-355736.js
  - js1_7/decompilation/regress-352079.js
  - js1_8_1/decompilation/regress-352011.js
  - js1_8_1/decompilation/regress-380237-04.js
  - js1_8_1/regress/regress-452498-038.js

Error reporting isn't the same as SpiderMonkey
  - ecma_3/LexicalConventions/7.9.1.js

Array isn't being reflected properly (Array.name is undefined?)
  - ecma_3_1/Object/regress-444787.js

Comment 9

[Tom Austin]

When I run it locally, I don't get the out of memory errors.  For the others, can we add them to narcissus-failures.txt?

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Builder refactor + expr parser rewrite to maintain parity with TM

Flatten the builder structure to avoid scary pinInnerThis magic.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Builder

Fixes bugs with parsing lets in for heads.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Builder

Per discussion with gal, applied the following fixes:

 - Rename COMPREHENSION to COMP
 - Rename saveRewindPoint to just save
 - Rename returnsNull/returnsValue to hasEmptyReturn/hasReturnWithValue
 - Change property name keyword check to use the keywords hash

Not fixed:

 - Move SyntaxError exception checking to the parse function. Exceptions need to be overhauled anyways.

Comment 14

[Dave Herman [:dherman]]

The FunctionDefinition function in jsparse is pretty hairy. I've opened bug 584436 with some additional review.

Dave

Comment 15

[Andreas Gal :gal]

Lets use 584436 to patch it. shu, want to grab that bug? dherman can review your fixes in that bug.

Comment 16

[Shu-yu Guo [:shu]]

Sure, I'll reply in that bug.

Comment 17

[Brendan Eich [:brendan]]

Fly-by comment:

Why are there JSON-style quoted property names in the Narcissus .jsparse = ... module pattern object initialiser return value?

Why jsparse and not parse as the method name?

/be

Comment 18

[Tom Austin]

No good reason on the JSON-style property names.

jsparse is an object with the exported properties.  Maybe 'parser' would be a better name?  Perhaps these changes:

Narcissus.jslex -> Narcissus.lexer
Narcissus.jsparse -> Narcissus.parser
Narcissus.jsexec -> Narcissus.interpreter
Narcissus.jsdefs -> Narcissus.definitions

Comment 19

[Brendan Eich [:brendan]]

Yes, nouns for data (and infallible getters), verbs for methods. Thanks,

/be