Reuse of Identifier when parsing ObjectPattern with AssignmentPattern

[bradzacher]

Context: typescript-eslint/typescript-eslint#1686
cc @kaicataldo

When parsing an assignment within an object pattern, acorn reuses the identifier object for both the Property.key, and the AssignmentPattern.left. Best shown by the example below.

This is a problem because it means that any mutations applied to one node also apply to the other node.
For example, ESLint adds a parent property as it traverses the AST. This node reuse means that the parent will be wrong for one of the two locations (whichever one is traversed first).

It has exposed a subtle bug in ESLint's camelcase rule.
When the code is parsed by espree (and thus acorn), the rule does not fire an error, because it does not detect the correct lineage for the node.
However, when the code is parsed by a parser that does not reuse objects (like typescript-estree), the rule fires an error, because the parent pointers are correct.

Is this intentional? To me it seems like a bug in acorn, as I would assume that every AST node is a brand new object, no matter how similar to an existing node they might be. It seems like ESLint makes the same assumption as well.

---

const acorn = require('acorn');
const ast = acorn.parse("const {foo=1} = {};");

/*
  {
    "type": "Property",
    "start": 7,
    "end": 12,
    "method": false,
    "shorthand": true,
    "computed": false,
    "key": {
      "type": "Identifier",
      "start": 7,
      "end": 10,
      "name": "foo"
    },
    "kind": "init",
    "value": {
      "type": "AssignmentPattern",
      "start": 7,
      "end": 12,
      "left": {
        "type": "Identifier",
        "start": 7,
        "end": 10,
        "name": "foo"
      },
      "right": {
        "type": "Literal",
        "start": 11,
        "end": 12,
        "value": 1,
        "raw": "1"
      }
    }
  }
*/
const node = ast.body[0].declarations[0].id.properties[0];
console.log(node.key === node.value.left) // true

[marijnh]

Mutation of AST nodes wasn't really something we accounted for when writing this, but I agree it's a reasonable thing to do. I'd be happy with a pull request that copies the identifier node in this case. (It'd be a good idea to use a for/in loop to copy the properties of the node object, in case a plugin or future language extension ends up adding additional properties to identifiers.)

[RReverser]

in case a plugin or future language extension ends up adding additional properties to identifiers

Counter-point: a plugin might want to put some properties only on one side of a shorthand expression (e.g. set isReference: true on LHS), in which case we explicitly don't want them to be the same.

[marijnh]

Yeah, our plugin approach remains a wild, dangerous ride. I think adding a designated 'copyIdentifierNode` method for this would be overkill, since it's relatively unlikely for this to actually become a real problem. What do you think?

[RReverser]

I think for now we can just copy nodes as raw Identifiers and then plugins will either continue to work as expected in common cases, or can switch to use Object.assign or deep cloning or whatever is most appropriate for their use-case. (Basically I'm suggesting not to do anything special here and see how this works out - it might fix more issues than break.)

[marijnh]

I don't care too much either way, really.

[marijnh]

Implemented in 85f4150. I did go for a by-property copy, since even estree's own type-annotation doc extends Identifier to add another property.

[sanex3339]

It would be nice to describe it in the CHANGELOG.md