913617 - Reflect.parse: rename ArrowExpression to ArrowFunctionExpression to align with esprima

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Identifiers in Reflect.parse but not in esprima:
    ArrowExpression
    SpreadExpression
    GeneratorExpression
    LetExpression
    LetStatement
    Property

Identifiers in esprima but not Reflect.parse:
    ArrowFunctionExpression
    ClassBody
    ClassDeclaration
    ClassExpression
    ClassHeritage
    ExportDeclaration
    ExportBatchSpecifier
    ExportSpecifier
    ImportDeclaration
    ImportSpecifier
    MethodDefinition
    SpreadElement
    TaggedTemplateExpression
    TemplateElement
    TemplateLiteral

This is based on the following two files:
  https://hg.mozilla.org/integration/mozilla-inbound/file/77e2eaaf2fbb/js/src/jsast.tbl
  https://github.com/ariya/esprima/blob/6c8079ab261757590c40d3c82e9f775bb9935615/esprima.js

The above suggests renaming:
    ArrowExpression -> ArrowFunctionExpression
    SpreadExpression -> SpreadElement
and probably Property as well. I don't even know exactly what that is, honestly.

LetExpression and LetStatement are SpiderMonkey extensions that we don't expect to see in Esprima. GeneratorExpression currently reflects legacy SpiderMonkey-only non-ES6 syntax.

I'm not thrilled to see ClassHeritage as a separate node; that is some extra weight that doesn't seem necessary.

Comment 2

[Dave Herman [:dherman]]

Attachment: renames ArrowFunction to ArrowFunctionExpression

I am extremely rusty at hacking on SpiderMonkey but this one seemed easy. Nick, I believe this shouldn't be a problem for the front-end but I'd appreciate your feedback.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8574300 [details] [diff] [review]
renames ArrowFunction to ArrowFunctionExpression

I don't see any references to either ArrowFunction or ArrowFunctionExpression in the devtools AST walker -- I suspect it just doesn't support ES6 arow functions at all yet.

Comment 4

[Dave Herman [:dherman]]

Hm, I just found this:

http://mxr.mozilla.org/mozilla-central/search?string=ArrowExpression

So it looks like ScratchPad is slightly affected.

Dave

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Dave Herman [:dherman] from comment #4)
> Hm, I just found this:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=ArrowExpression
> 
> So it looks like ScratchPad is slightly affected.
> 
> Dave

Ah, the patch (but not the bug) is mis-labeled and I searched for "ArrowFunction" not "ArrowExpression".

I can rubber stamp a find/replace in the devtools code.

Comment 6

[Dave Herman [:dherman]]

Attachment: renames ArrowFunction to ArrowFunctionExpression

Waiting on try server. If it passes I'll request review.

Dave

Comment 7

[Dave Herman [:dherman]]

Comment on attachment 8575416 [details] [diff] [review]
renames ArrowFunction to ArrowFunctionExpression

Asking for Jason's review and Nick's feedback.

Thanks,
Dave

Comment 8

[Dave Herman [:dherman]]

Should mention I had a pretty clean test run at

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4772e82829f4

Dave

Comment 9

[Dave Herman [:dherman]]

Also, for reference, here's the ESTree issue where it was decided to go with ArrowFunctionExpression:

https://github.com/estree/estree/issues/2

Dave

Comment 10

[Dave Herman [:dherman]]

I re-ran the two oranges and they failed again. Jason, if/when you have a chance I'd love your help trying to figure out what's going wrong.

Thanks again,
Dave

Comment 11

[Dave Herman [:dherman]]

Turns out I mis-read the treeherder output and the oranges were green on second run, so we're good to land. Marking checkin-needed.

Dave

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e29988051b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2e29988051b