New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range for object literal shorthand methods doesn't include params #1029

Closed
ariya opened this Issue Feb 13, 2015 · 1 comment

Comments

Projects
None yet
1 participant
@ariya
Contributor

ariya commented Feb 13, 2015

What steps will reproduce the problem?

On a fresh checkout of the harmony branch:

// test.js
var esprima = require('./');

var source = [
    '({',
    '    method1(arg1) {},',
    '})'
].join('\n');

var ast = esprima.parse(source, { range: true });

console.log(ast.body[0].expression.properties[0]);
console.log(ast.body[0].expression.properties[0].value.params[0]);
$ node test.js
{ type: 'Property',
  key: { type: 'Identifier', name: 'method1', range: [ 7, 14 ] },
  value:
   { type: 'FunctionExpression',
     id: null,
     params: [ [Object] ],
     defaults: [],
     body: { type: 'BlockStatement', body: [], range: [Object] },
     rest: null,
     generator: false,
     expression: false,
     range: [ 21, 23 ] },
  kind: 'init',
  method: true,
  shorthand: false,
  computed: false,
  range: [ 7, 23 ] }
{ type: 'Identifier', name: 'arg1', range: [ 15, 19 ] }

What is the expected output?

I would expect the FunctionExpression's range to include the method's parameters as well as the body as in (arg1) {}.

It makes sense that the method's name is not included, as that is the Property's key and does not belong to the FunctionExpression.

What do you see instead?

The FunctionExpression's range includes only the body, {}, leading to a scenario in which a traversal starting at the FunctionExpression, range 21-23, would reach the parameter, whose range, 15-19, lies outside that of its parent.

(Migrated from https://code.google.com/p/esprima/issues/detail?id=625, as reported by @btmills)

@ariya ariya added the defect label Feb 13, 2015

btmills added a commit to btmills/esprima that referenced this issue Feb 14, 2015

Expand property method range and loc to include params
Previously, the range and loc for get, set, and shorthand methods
inside object literal properties included only the method body,
meaning a method's parameters were outside the range of their parent
`FunctionExpression` node. This was happening because the
`FunctionExpression` node wasn't being created until
`parsePropertyFunction` was called, by which point the method's
parameters had already been parsed. This PR solves the issue by
creating the `FunctionExpression` node as soon as it determines that
the property is a get, set, or shorthand method, then passing the
node into `parsePropertyFunction`.

Closes jquery#1029
@ariya

This comment has been minimized.

Show comment
Hide comment
@ariya

ariya Feb 15, 2015

Contributor

I took a look at property setter and it seems to have the same problem: #1040.

Contributor

ariya commented Feb 15, 2015

I took a look at property setter and it seems to have the same problem: #1040.

btmills added a commit to btmills/esprima that referenced this issue Feb 16, 2015

Expand property method range and loc to include params
Previously, the range and loc for get, set, and shorthand methods
inside object literal properties included only the method body,
meaning a method's parameters were outside the range of their parent
`FunctionExpression` node. This was happening because the
`FunctionExpression` node wasn't being created until
`parsePropertyFunction` was called, by which point the method's
parameters had already been parsed. This PR solves the issue by
creating the `FunctionExpression` node as soon as it determines that
the property is a get, set, or shorthand method, then passing the
node into `parsePropertyFunction`.

Closes jquery#1029
Closes jquery#1040

@ariya ariya closed this in 3844589 Feb 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment