1149333 - Reflect.parse for ArrowFunctionExpression returns incorrect loc.start.column (off by one)

Status: NEW

(Core :: JavaScript Engine, defect, P5)

Description

[James Burke [:jrburke]]

Example:

    var content = "a(() => {});";
    var ast = Reflect.parse(content, {
      loc: true
    });
    dump(JSON.stringify(ast, null, '  '));

The ArrowFunctionExpression has this loc: structure:

            "loc": {
              "start": {
                "line": 1,
                "column": 1
              },
              "end": {
                "line": 1,
                "column": 10
              },
              "source": null
            },

Where the loc.start.column says "1", but it should be "2". Esprima 2.1.0 gives "2" for that value. The loc.end.column value is correct.

This was found when trying to support AMD modules that may be used in Gaia of the form `define((require) => {})`, where the location of the function passed to define needs to be found for proper insertion of module IDs for module bundling.

Found using xulrunner 40.0a1, downloaded from:
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/latest-mozilla-central/

Comment 1

[James Burke [:jrburke]]

Full Reflect.parse result for context:

{
  "loc": {
    "start": {
      "line": 1,
      "column": 0
    },
    "end": {
      "line": 1,
      "column": 12
    },
    "source": null
  },
  "type": "Program",
  "body": [
    {
      "loc": {
        "start": {
          "line": 1,
          "column": 0
        },
        "end": {
          "line": 1,
          "column": 12
        },
        "source": null
      },
      "type": "ExpressionStatement",
      "expression": {
        "loc": {
          "start": {
            "line": 1,
            "column": 0
          },
          "end": {
            "line": 1,
            "column": 11
          },
          "source": null
        },
        "type": "CallExpression",
        "callee": {
          "loc": {
            "start": {
              "line": 1,
              "column": 0
            },
            "end": {
              "line": 1,
              "column": 1
            },
            "source": null
          },
          "type": "Identifier",
          "name": "a"
        },
        "arguments": [
          {
            "loc": {
              "start": {
                "line": 1,
                "column": 1
              },
              "end": {
                "line": 1,
                "column": 10
              },
              "source": null
            },
            "type": "ArrowFunctionExpression",
            "id": null,
            "params": [],
            "defaults": [],
            "body": {
              "loc": {
                "start": {
                  "line": 1,
                  "column": 8
                },
                "end": {
                  "line": 1,
                  "column": 9
                },
                "source": null
              },
              "type": "BlockStatement",
              "body": []
            },
            "rest": null,
            "generator": false,
            "expression": false
          }
        ]
      }
    }
  ]
}

Comment 2

[Tooru Fujisawa [:arai]]

It's because FullParseHandler::newFunctionDefinition uses the position of the token just before the arguments for the beginning position of the function.
it's wrong for arrow function, since it starts with arguments, and previous token is not a part of it.

So, the possible solution will be following:
 A. pass function kind to newFunctionDefinition, and use token next to current token if it's arrow function
 B. overwrite the position later (maybe while parsing function argument) if it's arrow function


btw, current implementation's behavior is inconsistent for other function's case.
  * function f() {}
    returns the position of "f"
  * function() {}
    returns the position of "function"
  * function* f() {}
    returns the position of "f"
  * function*() {}
    returns the position of "*"

so, if the function has a name, it returns the position of the name, but if not, it depends on the case.
I guess it would be better to returning the position of "function" in all cases, providing other way to get the position or node of function name if needed (maybe in separated bug)

also, method definition has more complicated situation...
  * ({ f() {} })
    returns the position of "f"
  * ({ ["f"]() {} })
    returns the position of "]"

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Set the start position of arrow function node correctly.

Implement following, with passing boolean value isArrow instead of kind, since required information there is "arrow function or not".
> A. pass function kind to newFunctionDefinition, and use token next to current token if it's arrow function

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ceb39e8c7c

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8586083 [details] [diff] [review]
Set the start position of arrow function node correctly.

Review of attachment 8586083 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FullParseHandler.h
@@ +569,5 @@
> +            return new_<CodeNode>(pos());
> +
> +        TokenPos pos;
> +        if (!tokenStream.peekTokenPos(&pos, TokenStream::Operand))
> +            return nullptr;

This might be consistent (I haven't looked).  But really, this is pretty spooky behavior -- when the caller calls this function determines the position it uses, and not just the position it's currently parked on.  The latter is IMO dodgy, but the former is too much.

I think we need to clean up when we call newFunctionDefinition, so that we're always calling it parked at a consistent location.  Or really, since a generic method calls that function, that calling function needs to be fixed, to be called later.

Or, if that's not possible, the worst case -- explicitly set the function's position after the node's created.

Anyway, please do one of these instead of making this method even more calling-instant tokenstream-position-dependent.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing.

Sorry if I'm misunderstanding.

if we always use FullParseHandler::pos() for the postion of newly created function node, newFunctionDefinition should be called on different timing for different syntaxes, right?
  * () => 1
    after getting TOK_LP, it happens at the top of Parser::functionArguments
    http://hg.mozilla.org/mozilla-central/file/18a8ea7c2c62/js/src/frontend/Parser.cpp#l1560
  * x => 1
    after getting TOK_NAME, it happens in the loop of Parser::functionArguments
    http://hg.mozilla.org/mozilla-central/file/18a8ea7c2c62/js/src/frontend/Parser.cpp#l1606

Also, to keep current behavior for other function's case (at the top of Parser::functionDef), newFunctionDefinition will be called from 3 different locations. I guess it's already too complicated, and it will make it harder to fix other function and method's cases (if we need).

So, setting it later will be simpler solution.
I'll try to implement it.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Set the start position of arrow function node correctly.

Fortunatelly, there are similar code for starting position for funbox, so added code there.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd70fedeb1e

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> Sorry if I'm misunderstanding.
> 
> if we always use FullParseHandler::pos() for the postion of newly created
> function node, newFunctionDefinition should be called on different timing
> for different syntaxes, right?

That's what would happen sticking to the current code.  This *behavior* isn't tolerable, as you rightly point out.  It's the nature of the fix to that, that's the concern.

> So, setting it later will be simpler solution.
> I'll try to implement it.

That probably is simpler.  As a stopgap, good enough.

But frankly, in the long view, what *should* in my view happen is this.  When parsing, you take explicit note of the position of each node that's to be created.  You compute all the data needed to create the function -- child nodes and the like.  Then, as the *final* step, you create the actual node passing in the children and any extra bits specific to the node itself.

So then, for functions you might have something basically like:

  MOZ_ASSERT(tokenStream.isCurrentTokenType(TOK_FUNCTION);
  uint32_t begin = pos().begin, end;
  GeneratorType gen;
  PropertyName* name;
  if (!parseStarAndName(&gen, &name))
    return null();
  MOZ_ASSERT(tokenStream.isCurrentTokenType(TOK_LP);
  Node arguments = parseFunctionArguments();
  if (!arguments)
    return null();
  Node body = parseFunctionBody(&end);
  if (!body)
    return null();
  return handler.newFunction(gen, name, arguments, body, TokenPos(begin, end));

So if you want a function node, you state exactly what position you want recorded for it, when you create it.

Right now we have a mix of "use the current position when creating a node", "take a begin offset and use the current position end", and "mutate the node position to the right value as a final step, separate from creating the node".  None of those are any good (except the first for single-token nodes).  Rather, creating a node should explicitly take the position to ascribe to it -- the functional approach, rather than an imperative or context-determined approach, if you will.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Create function node after parsed function.

Draft patch here :)
Can I get a feedback for current changes?

Parser
 * add ParseHandler::newFunction which takes opcode, arguments, body, position, and blockid
   FullParseHandler::newFunction pass them to CodeNode ctor

 * pass a combination of (begin, end, PNK_ARGSBODY node, FunctionBox)
   instead of PNK_FUNCTION node
   + ParseHandler::setLastFunctionArgumentDefault
   + ParseHandler::addFunctionArgument
   + Parser::defineArg
   + Parser::leaveFunction
   + Parser::checkFunctionDefinition
   + Parser::finishFunctionDefinition
   + Parser::functionArgsAndBody
   + Parser::functionArgsAndBodyGeneric

 * pass lazy->column() to standaloneLazyFunction as a beginning offset of the function node
   (this points TOK_LC, instead of TOK_NAME, but it's same as current behavior)

 * return PNK_ARGSBODY node from following
   + Parser::functionArgsAndBodyGeneric
   + Parser::functionArguments
   + Parser::functionArgsAndBody (as an out parameter, since it may return nullptr for lazy function on successful case)

 * add Parser::starAndName
   called from Parser::functionStmt and Parser::functionExpr

 * add Parser::checkStrictBindingWithOffset as an alternative for Parser::checkStrictBinding with uint32_t offset
   (might be better to make this and error reporting functions templates or overloading)

 * separate out definition part from Parser::checkFunctionDefinition to Parser::defineFunction
   Parser::checkFunctionDefinition is called before PNK_FUNCTION node is created
   Parser::defineFunction is called after PNK_FUNCTION node is created

 * pass beginning offset to Parser::functionDef to solve arrow function's issue
   this also make it easier to fix other function/method's case
   but not yet touched for compatibility with other places ("// to keep old behavior" comments)

 * remove following
   + ParseHandler::newFunctionDefinition
     replaced by ParseHandler::newFunction
   + ParseHandler::setFunctionBody
     pn_body is initialized in CodeNode ctor
   + ParseHandler::setFunctionBox
     pn_funbox is initialized in CodeNode ctor
   + ParseNode::isLikelyIIFE
     directly use 'invoked' in Parser::functionArgsAndBody

SharedContext
 * replace maybeFunction with maybeFunctionOffset and maybeArgsBody
   since ParseContext is created before PNK_FUNCTION node is created
     maybeFunctionOffset : passed as a ctor argument, instead of maybeFunction
     maybeArgsBody : set after PNK_ARGSBODY node is created

asm.js
 * remove reference to maybeFunction
   + parser.pc->maybeFunction->pn_pos.begin
     => parser.pc->maybeFunctionOffset
   + parser.pc->maybeFunction->pn_body
     => parser.pc->maybeArgsBody
   + parser.pc->maybeFunction->pn_funbox
     => parser.pc->sc->asFunctionBox()

 * convert some functions to template functions (take ParseNode* or uint32_t as offset)
   since those functions may be called before creating PNK_FUNCTION node
   + ModuleCompiler::failfVA
   + ModuleCompiler::failf
   + ModuleCompiler::failName
   + CheckIdentifier
   + CheckModuleLevelName
   + CheckFunctionHead

 * overload ModuleCompiler::fail with ParseNode* and int32_t (rename failOffset to fail)
   to make it callable from above templates

 * pass combination of (offset, PNK_ARGSBODY node, FunctionBox) instead of PNK_FUNCTION node
   + FunctionArgsList
   + FunctionObject
   + FunctionName
   + CheckIdentifier
   + CheckModuleLevelName
   + CheckFunctionHead
   + CheckModuleArguments

functionArguments and finishFunctionDefinition could be made simpler after bug 1018628 is solved (now rebasing and testing). it removes prelude.

Comment 9

[Tooru Fujisawa [:arai]]

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9154e3eb58dd

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

AST columns look like this,

            "type": "ArrowFunctionExpression",
            "id": null,
            "params": [],
            "defaults": [],
            "body": {
              "loc": {
                "start": {
                  "line": 1,
                  "column": 9
                },
                "end": {
                  "line": 1,
                  "column": 11
                },
                "source": null
              },
              "type": "BlockStatement",
              "body": []
            },

So today ArrowFunctionExpression doesn't have the location, but the body does.