From 118e8e0acc816265426d678dc9e046dd79eb4a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Mon, 17 Aug 2020 14:28:51 -0400 Subject: [PATCH 1/2] fix: do not transform ClassPrivateMethods in estree --- .../test/integration/eslint/eslint.js | 22 ++++++ packages/babel-parser/src/plugins/estree.js | 13 ++++ .../class-private-method/basic/input.js | 3 + .../class-private-method/basic/options.json | 3 + .../class-private-method/basic/output.json | 68 +++++++++++++++++++ 5 files changed, 109 insertions(+) create mode 100644 packages/babel-parser/test/fixtures/estree/class-private-method/basic/input.js create mode 100644 packages/babel-parser/test/fixtures/estree/class-private-method/basic/options.json create mode 100644 packages/babel-parser/test/fixtures/estree/class-private-method/basic/output.json diff --git a/eslint/babel-eslint-tests/test/integration/eslint/eslint.js b/eslint/babel-eslint-tests/test/integration/eslint/eslint.js index 5a8ed44b23b9..533bd6c7d837 100644 --- a/eslint/babel-eslint-tests/test/integration/eslint/eslint.js +++ b/eslint/babel-eslint-tests/test/integration/eslint/eslint.js @@ -1758,6 +1758,28 @@ describe("verify", () => { { "no-unused-vars": 1 }, ); }); + + it("should visit params", () => { + verifyAndAssertMessages( + `export class C { + constructor() { this.#d(); } + #d(unused) {}; + } + `, + { "no-unused-vars": 1 }, + ); + }); + + it("should visit body", () => { + verifyAndAssertMessages( + `export class C { + constructor() { this.#d(); } + #d() { var unused; }; + } + `, + { "no-unused-vars": 1 }, + ); + }); }); }); diff --git a/packages/babel-parser/src/plugins/estree.js b/packages/babel-parser/src/plugins/estree.js index 579f45e0b279..5c4252ed642a 100644 --- a/packages/babel-parser/src/plugins/estree.js +++ b/packages/babel-parser/src/plugins/estree.js @@ -287,6 +287,19 @@ export default (superClass: Class): Class => type: string, inClassScope: boolean = false, ): T { + if (type === "ClassPrivateMethod") { + // todo: supports ClassPrivateMethod + // see https://github.com/estree/estree/blob/master/experimental/class-features.md + return super.parseMethod( + node, + isGenerator, + isAsync, + isConstructor, + allowDirectSuper, + type, + inClassScope, + ); + } let funcNode = this.startNode(); funcNode.kind = node.kind; // provide kind, so super method correctly sets state funcNode = super.parseMethod( 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 new file mode 100644 index 000000000000..83ead2348fae --- /dev/null +++ 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 new file mode 100644 index 000000000000..9a3d6c4ac482 --- /dev/null +++ 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 new file mode 100644 index 000000000000..71f9a7512d4b --- /dev/null +++ b/packages/babel-parser/test/fixtures/estree/class-private-method/basic/output.json @@ -0,0 +1,68 @@ +{ + "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", + "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": [] + } + } + ] + } + } + ] + } +} \ No newline at end of file From ce0451c056d6470bed9789247552e049817386f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hu=C3=A1ng=20J=C3=B9nli=C3=A0ng?= Date: Mon, 17 Aug 2020 20:04:58 -0400 Subject: [PATCH 2/2] fix: use MethodDefinition as ClassPrivateMethod visitor keys --- .../src/convert/convertAST.js | 34 +++++++------- .../babel-eslint-parser/src/visitor-keys.js | 33 +++++++------ .../test/integration/eslint/eslint.js | 14 ++++++ packages/babel-parser/src/plugins/estree.js | 13 ------ .../class-private-method/basic/output.json | 46 ++++++++++--------- 5 files changed, 74 insertions(+), 66 deletions(-) diff --git a/eslint/babel-eslint-parser/src/convert/convertAST.js b/eslint/babel-eslint-parser/src/convert/convertAST.js index 0d714ba5d00b..76f3c63fb420 100644 --- 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 index 3d28bde44cac..9cc7f0c315e8 100644 --- 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 index 533bd6c7d837..5ec83416f5ac 100644 --- a/eslint/babel-eslint-tests/test/integration/eslint/eslint.js +++ b/eslint/babel-eslint-tests/test/integration/eslint/eslint.js @@ -1767,6 +1767,7 @@ describe("verify", () => { } `, { "no-unused-vars": 1 }, + ["3:6 'unused' is defined but never used. no-unused-vars"], ); }); @@ -1778,6 +1779,19 @@ describe("verify", () => { } `, { "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/src/plugins/estree.js b/packages/babel-parser/src/plugins/estree.js index 5c4252ed642a..579f45e0b279 100644 --- a/packages/babel-parser/src/plugins/estree.js +++ b/packages/babel-parser/src/plugins/estree.js @@ -287,19 +287,6 @@ export default (superClass: Class): Class => type: string, inClassScope: boolean = false, ): T { - if (type === "ClassPrivateMethod") { - // todo: supports ClassPrivateMethod - // see https://github.com/estree/estree/blob/master/experimental/class-features.md - return super.parseMethod( - node, - isGenerator, - isAsync, - isConstructor, - allowDirectSuper, - type, - inClassScope, - ); - } let funcNode = this.startNode(); funcNode.kind = node.kind; // provide kind, so super method correctly sets state funcNode = super.parseMethod( 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 index 71f9a7512d4b..0c0275a5fe10 100644 --- 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 @@ -34,30 +34,34 @@ } }, "kind": "method", - "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": { + "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":25,"end":31,"loc":{"start":{"line":2,"column":15},"end":{"line":2,"column":21},"identifierName":"others"}, - "name": "others" + "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": [] } - ], - "body": { - "type": "BlockStatement", - "start":33,"end":35,"loc":{"start":{"line":2,"column":23},"end":{"line":2,"column":25}}, - "body": [] } } ]