[camelcase] false positive when parsing object pattern with a default value, and using a non-default parser.

[bradzacher]

Tell us about your environment

• ESLint Version: 6.7.0
• Node Version: 12

What parser (default, Babel-ESLint, etc.) are you using?
default, typescript-eslint

Please show your full configuration:

Configuration

{
  rules: {
    camelcase: ['error', {
      "properties": "never",
      "ignoreDestructuring": true
    }],
  },
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Context: typescript-eslint/typescript-eslint#1686
A user of typescript-eslint received a linting error with the following code, even though it should be covered by the options above.

  const res = {a_b:123}
  const {a_b=0} = res
//       ^^^ Error Identifier 'a_b' is not in camel case

This error occurs when parsing with typescript-eslint, and not with the default parser.
Thinking this was a bug in the typescript-eslint parser, I did some digging, and it turns out that the acorn reuses the AST node in two places (i.e. the two a_b Identifiers are referentially equal).
acornjs/acorn#928

This causes the parent references to be set incorrectly, as the node's parent will be that of whichever node was traversed last.

Logging this here because:

• the rule doesn't work with the "correct" parent pointers,
• the rule will have to be updated for when the bug in acorn is fixed.

[kaicataldo]

@bradzacher Thanks for the report. I was able to verify this.

[mdjermanovic]

We should probably check some other rules, too.

Also, what's expected AST for each of these cases:

({ a });

({ a } = {});

import { a } from 'foo';

export { a };

Referentially equal identifier nodes, or not referentially equal identifier nodes (i.e. two different nodes), or the parser can choose either.

[bradzacher]

IMO there should never be referentially equal identifier nodes - every AST node should always be a brand new object, regardless of similarities to other nodes in the AST.

If you share nodes, then it means you have to be weary of mutating the nodes in case you put incorrect data in the AST (as is what's happening with the parent right now).

Allowing the parser to choose either just creates a headache for consumers, as they don't know what will happen if they mutate the object.

I believe that's the direction they're going to go toward in the acorn issue.

[mdjermanovic]

I think it's reasonable to always have different objects.

But I'm not sure what the spec for ImportSpecifier actually says:

If it is a basic named import, such as in import {foo} from "mod", both imported and local are equivalent Identifier nodes; in this case an Identifier node representing foo. If it is an aliased import, such as in import {foo as bar} from "mod", the imported field is an Identifier node representing foo, and the local field is an Identifier node representing bar.

Sounds like it should be the same object ("an Identifier node representing foo")? Though, "equivalent" usually doesn't mean "same". Does typescript-eslint parser create two nodes in this case?

[bradzacher]

Does typescript-eslint parser create two nodes in this case

yes, it does.

> ast = tsestree.parse("import { a } from 'foo';")
{
  type: 'Program',
  body: [
    {
      type: 'ImportDeclaration',
      source: [Object],
      specifiers: [Array],
      importKind: 'value',
      range: [Array],
      loc: [Object]
    }
  ],
  sourceType: 'module',
  range: [ 0, 24 ],
  loc: { start: { line: 1, column: 0 }, end: { line: 1, column: 24 } }
}
> ast.body[0].specifiers[0].imported === ast.body[0].specifiers[0].local
false

babel also creates two separate nodes:

> ast = require('@babel/core').parse("import { a } from 'foo';")
Node {
  type: 'File',
  start: 0,
  end: 24,
  loc: SourceLocation {
    start: Position { line: 1, column: 0 },
    end: Position { line: 1, column: 24 }
  },
  errors: [],
  program: Node {
    type: 'Program',
    start: 0,
    end: 24,
    loc: SourceLocation { start: [Position], end: [Position] },
    sourceType: 'module',
    interpreter: null,
    body: [ [Node] ],
    directives: []
  },
  comments: []
}
> ast.program.body[0].specifiers[0].imported === ast.program.body[0].specifiers[0].local
false

[mdjermanovic]

Then, I misread the estree spec while working on #12831 so we should probably fix that as well. Tests didn't catch it because acorn creates just 1 node.

I was sure it's the same node because there is no something like shorthand attribute, so it isn't possible to distinguish between import { a } from "foo" and import { a as a } from "foo" without comparing locations.

[mdjermanovic]

Actually, #12831 seems to work well by chance. The name of the function isRenamedImport is wrong if parser always creates two nodes, but the code inside works okay and prevents reporting the same location twice.

It is, however, difficult to think about and cover both variations with same code.