fix: do not transform ClassPrivateMethods in estree (#11973)

* fix: do not transform ClassPrivateMethods in estree * fix: use MethodDefinition as ClassPrivateMethod visitor keys
babel · Aug 18, 2020 · cdada58 · cdada58
1 parent 028a051
commit cdada58
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 32 deletions.
diff --git a/eslint/babel-eslint-parser/src/convert/convertAST.js b/eslint/babel-eslint-parser/src/convert/convertAST.js
@@ -1,5 +1,5 @@
 import { types as t, traverse } from "@babel/core";
-import VISITOR_KEYS from "../visitor-keys";
+import { newTypes, conflictTypes } from "../visitor-keys";
 
 function convertNodes(ast, code) {
   const astTransformVisitor = {
@@ -81,30 +81,30 @@ function convertNodes(ast, code) {
     },
   };
   const state = { source: code };
-
-  const oldExportAllDeclarationKeys = t.VISITOR_KEYS.ExportAllDeclaration;
+  const oldVisitorKeys = new Map();
 
   try {
-    // Monkey patch visitor keys in order to be able to traverse the estree nodes
-    t.VISITOR_KEYS.ChainExpression = VISITOR_KEYS.ChainExpression;
-    t.VISITOR_KEYS.ImportExpression = VISITOR_KEYS.ImportExpression;
-    t.VISITOR_KEYS.Property = VISITOR_KEYS.Property;
-    t.VISITOR_KEYS.MethodDefinition = VISITOR_KEYS.MethodDefinition;
+    for (const [type, visitorKey] of Object.entries(conflictTypes)) {
+      // backup conflicted visitor keys
+      oldVisitorKeys.set(type, t.VISITOR_KEYS[type]);
 
-    // Make sure we visit `exported` key to remove `identifierName` from loc node
-    t.VISITOR_KEYS.ExportAllDeclaration = t.VISITOR_KEYS.ExportAllDeclaration.concat(
-      "exported",
-    );
+      t.VISITOR_KEYS[type] = visitorKey;
+    }
+    for (const [type, visitorKey] of Object.entries(newTypes)) {
+      t.VISITOR_KEYS[type] = visitorKey;
+    }
 
     traverse(ast, astTransformVisitor, null, state);
   } finally {
     // These can be safely deleted because they are not defined in the original visitor keys.
-    delete t.VISITOR_KEYS.ChainExpression;
-    delete t.VISITOR_KEYS.ImportExpression;
-    delete t.VISITOR_KEYS.MethodDefinition;
-    delete t.VISITOR_KEYS.Property;
+    for (const type of Object.keys(newTypes)) {
+      delete t.VISITOR_KEYS[type];
+    }
 
-    t.VISITOR_KEYS.ExportAllDeclaration = oldExportAllDeclarationKeys;
+    // These should be restored
+    for (const type of Object.keys(conflictTypes)) {
+      t.VISITOR_KEYS[type] = oldVisitorKeys.get(type);
+    }
   }
 }
 

diff --git a/eslint/babel-eslint-parser/src/visitor-keys.js b/eslint/babel-eslint-parser/src/visitor-keys.js
@@ -1,19 +1,22 @@
 import { types as t } from "@babel/core";
 import { KEYS as ESLINT_VISITOR_KEYS } from "eslint-visitor-keys";
 
-/*eslint no-unused-vars: ["error", { "ignoreRestSiblings": true }]*/
-const { ExportAllDeclaration, ...BABEL_VISITOR_KEYS } = t.VISITOR_KEYS;
+// AST Types that are not presented in Babel AST
+export const newTypes = {
+  ChainExpression: ESLINT_VISITOR_KEYS.ChainExpression,
+  ImportExpression: ESLINT_VISITOR_KEYS.ImportExpression,
+  Literal: ESLINT_VISITOR_KEYS.Literal,
+  MethodDefinition: ["decorators"].concat(ESLINT_VISITOR_KEYS.MethodDefinition),
+  Property: ["decorators"].concat(ESLINT_VISITOR_KEYS.Property),
+};
 
-export default Object.assign(
-  {
-    ChainExpression: ESLINT_VISITOR_KEYS.ChainExpression,
-    ExportAllDeclaration: ESLINT_VISITOR_KEYS.ExportAllDeclaration,
-    ImportExpression: ESLINT_VISITOR_KEYS.ImportExpression,
-    Literal: ESLINT_VISITOR_KEYS.Literal,
-    MethodDefinition: ["decorators"].concat(
-      ESLINT_VISITOR_KEYS.MethodDefinition,
-    ),
-    Property: ["decorators"].concat(ESLINT_VISITOR_KEYS.Property),
-  },
-  BABEL_VISITOR_KEYS,
-);
+// AST Types that shares `"type"` property with Babel but have different shape
+export const conflictTypes = {
+  // todo: remove this when class features are supported
+  ClassPrivateMethod: ["decorators"].concat(
+    ESLINT_VISITOR_KEYS.MethodDefinition,
+  ),
+  ExportAllDeclaration: ESLINT_VISITOR_KEYS.ExportAllDeclaration,
+};
+
+export default Object.assign(newTypes, t.VISITOR_KEYS, conflictTypes);
diff --git a/eslint/babel-eslint-tests/test/integration/eslint/eslint.js b/eslint/babel-eslint-tests/test/integration/eslint/eslint.js
@@ -1758,6 +1758,42 @@ describe("verify", () => {
           { "no-unused-vars": 1 },
         );
       });
+
+      it("should visit params", () => {
+        verifyAndAssertMessages(
+          `export class C {
+              constructor() { this.#d(); }
+              #d(unused) {};
+            }
+          `,
+          { "no-unused-vars": 1 },
+          ["3:6 'unused' is defined but never used. no-unused-vars"],
+        );
+      });
+
+      it("should visit body", () => {
+        verifyAndAssertMessages(
+          `export class C {
+              constructor() { this.#d(); }
+              #d() { var unused; };
+            }
+          `,
+          { "no-unused-vars": 1 },
+          ["3:14 'unused' is defined but never used. no-unused-vars"],
+        );
+      });
+
+      it("should work with no-unreachable", () => {
+        verifyAndAssertMessages(
+          `class C {
+  #a() {
+    return;
+  }
+  #b() {} // no-unreachable should not bail here
+}`,
+          { "no-unreachable": 1 },
+        );
+      });
     });
   });
 

diff --git a/packages/babel-parser/test/fixtures/estree/class-private-method/basic/input.js b/packages/babel-parser/test/fixtures/estree/class-private-method/basic/input.js
@@ -0,0 +1,3 @@
+class A {
+  #foo(arg, ...others) {}
+}
diff --git a/packages/babel-parser/test/fixtures/estree/class-private-method/basic/options.json b/packages/babel-parser/test/fixtures/estree/class-private-method/basic/options.json
@@ -0,0 +1,3 @@
+{
+  "plugins": ["flow", "jsx", "estree", "classPrivateMethods"]
+}
diff --git a/packages/babel-parser/test/fixtures/estree/class-private-method/basic/output.json b/packages/babel-parser/test/fixtures/estree/class-private-method/basic/output.json
@@ -0,0 +1,72 @@
+{
+  "type": "File",
+  "start":0,"end":37,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
+  "program": {
+    "type": "Program",
+    "start":0,"end":37,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
+    "sourceType": "script",
+    "interpreter": null,
+    "body": [
+      {
+        "type": "ClassDeclaration",
+        "start":0,"end":37,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
+        "id": {
+          "type": "Identifier",
+          "start":6,"end":7,"loc":{"start":{"line":1,"column":6},"end":{"line":1,"column":7},"identifierName":"A"},
+          "name": "A"
+        },
+        "superClass": null,
+        "body": {
+          "type": "ClassBody",
+          "start":8,"end":37,"loc":{"start":{"line":1,"column":8},"end":{"line":3,"column":1}},
+          "body": [
+            {
+              "type": "ClassPrivateMethod",
+              "start":12,"end":35,"loc":{"start":{"line":2,"column":2},"end":{"line":2,"column":25}},
+              "static": false,
+              "key": {
+                "type": "PrivateName",
+                "start":12,"end":16,"loc":{"start":{"line":2,"column":2},"end":{"line":2,"column":6}},
+                "id": {
+                  "type": "Identifier",
+                  "start":13,"end":16,"loc":{"start":{"line":2,"column":3},"end":{"line":2,"column":6},"identifierName":"foo"},
+                  "name": "foo"
+                }
+              },
+              "kind": "method",
+              "value": {
+                "type": "FunctionExpression",
+                "start":16,"end":35,"loc":{"start":{"line":2,"column":6},"end":{"line":2,"column":25}},
+                "id": null,
+                "generator": false,
+                "async": false,
+                "expression": false,
+                "params": [
+                  {
+                    "type": "Identifier",
+                    "start":17,"end":20,"loc":{"start":{"line":2,"column":7},"end":{"line":2,"column":10},"identifierName":"arg"},
+                    "name": "arg"
+                  },
+                  {
+                    "type": "RestElement",
+                    "start":22,"end":31,"loc":{"start":{"line":2,"column":12},"end":{"line":2,"column":21}},
+                    "argument": {
+                      "type": "Identifier",
+                      "start":25,"end":31,"loc":{"start":{"line":2,"column":15},"end":{"line":2,"column":21},"identifierName":"others"},
+                      "name": "others"
+                    }
+                  }
+                ],
+                "body": {
+                  "type": "BlockStatement",
+                  "start":33,"end":35,"loc":{"start":{"line":2,"column":23},"end":{"line":2,"column":25}},
+                  "body": []
+                }
+              }
+            }
+          ]
+        }
+      }
+    ]
+  }
+}