Skip to content
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

[@typescript-eslint/camelcase] There is a bug When I destructure and assign default values #1686

Closed
2016geek opened this issue Mar 6, 2020 · 8 comments
Labels
bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@2016geek
Copy link

2016geek commented Mar 6, 2020

What were you trying to do?
There is a bug When I destructure and assign default values

{
  "rules": {
    "@typescript-eslint/camelcase": ['error',
      {
        "properties": "never",
        "ignoreDestructuring": true,
        "genericType": "never"
      }]
  }
}
const res = {a_b:123}
const {a_b=0} = res

What did you expect to happen?
There is no error
What actually happened?
Error Identifier 'a_b' is not in camel case
Versions
2.22.0

package version
@typescript-eslint/eslint-plugin ^2.22.0
@typescript-eslint/parser ^2.22.0
TypeScript ^3.5.3
node v10.15.0
npm 6.4.1
@bradzacher bradzacher added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-estree Issues related to @typescript-eslint/typescript-estree and removed package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Mar 6, 2020
@bradzacher
Copy link
Member

Hi there,

Please make sure you use the issue templates when filing a bug.
We have created these templates to make sure you include the relevant information for each package.


Hmmm, this looks like a bug in the base rule due to how espree parses.

Doing a parse in both our parser, and in espree will produce the same json output.
However, in espree, it appears that the parser reuses the identifier node between the Property and the AssignmentPattern, which means that the parent chain is incorrect for one of the locations.

ESPREE:
image

TS-ESTREE:
image

cc @kaicataldo who has more context on espree - this looks like a bug in espree? Or is this intentional? Sharing the node in multiple places in the AST seems really fragile, as mutating the node causes both instances to be mutated, which means the tree is not consistent.

@kaicataldo
Copy link
Member

I don't think happens at the parser level. We add the parent reference to a node during traversal in ESLint core, and I don't think we do any "dereferencing", so it would be the same node in this case. Are you logging during the ESLint run above?

@bradzacher
Copy link
Member

sorry to clarify - the problem isn't the parent references, it's that an AST node has been shared in two places in the AST.

const {foo=1} = {};

is parsed into the following AST by both parsers:

- VariableDeclaration
  - VariableDeclarator
    - ObjectPattern
      - Property
        - Identifier (1)
        - AssignmentPattern
          - Identifier (2)
          - Literal
// ... init ...

The problem is that when parsed with espree, Identifier (1) and (2) are referentially equal (as per the above screenshot).
OTOH when parsed with ts-estree, the two are not referentially equal.

@kaicataldo
Copy link
Member

Thanks for clarifying. I checked this in Acorn (Espree's underlying parser) and it displays the same behavior.

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

It's probably worth making an issue an Acorn to discuss - I'm honestly not sure what the correct behavior should be here.

@2016geek
Copy link
Author

2016geek commented Mar 9, 2020

Thanks for your troubleshooting, so is there a solution now

@bradzacher
Copy link
Member

no. this looks like a bug in acorn.
suggestion is to not use the camelcase rule anyways, and instead use the naming-convention rule, as it covers more cases and is more configurable.

@2016geek
Copy link
Author

2016geek commented Mar 9, 2020

thank you!I try it

@armano2 armano2 changed the title 【@typescript-eslint/camelcase】There is a bug When I destructure and assign default values [@typescript-eslint/camelcase] here is a bug When I destructure and assign default values Mar 9, 2020
@armano2 armano2 changed the title [@typescript-eslint/camelcase] here is a bug When I destructure and assign default values [@typescript-eslint/camelcase] There is a bug When I destructure and assign default values Mar 9, 2020
@bradzacher
Copy link
Member

our camelcase rule no longer exists

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Development

No branches or pull requests

3 participants