From 8581f4fb8bf0bc9f49dee5fb06b00a0c78ae4b2b Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Fri, 5 Aug 2016 03:53:24 +0900 Subject: [PATCH] Fix: `no-invalid-this` false positive (fixes #6824) (#6827) --- docs/rules/no-invalid-this.md | 3 +- lib/ast-utils.js | 65 +++++++++++++++++++++--------- tests/lib/rules/no-invalid-this.js | 56 ++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 21 deletions(-) diff --git a/docs/rules/no-invalid-this.md b/docs/rules/no-invalid-this.md index 92623f3fd77..87d5387ac60 100644 --- a/docs/rules/no-invalid-this.md +++ b/docs/rules/no-invalid-this.md @@ -11,12 +11,13 @@ Basically this rule checks whether or not a function which are containing `this` This rule judges from following conditions whether or not the function is a constructor: * The name of the function starts with uppercase. +* The function is assigned to a variable which starts with an uppercase letter. * The function is a constructor of ES2015 Classes. This rule judges from following conditions whether or not the function is a method: * The function is on an object literal. -* The function assigns to a property. +* The function is assigned to a property. * The function is a method/getter/setter of ES2015 Classes. (excepts static methods) And this rule allows `this` keywords in functions below: diff --git a/lib/ast-utils.js b/lib/ast-utils.js index 346873ccf4d..9fcab25b4af 100644 --- a/lib/ast-utils.js +++ b/lib/ast-utils.js @@ -49,16 +49,23 @@ function isModifyingReference(reference, index, references) { ); } +/** + * Checks whether the given string starts with uppercase or not. + * + * @param {string} s - The string to check. + * @returns {boolean} `true` if the string starts with uppercase. + */ +function startsWithUpperCase(s) { + return s[0] !== s[0].toLocaleLowerCase(); +} + /** * Checks whether or not a node is a constructor. * @param {ASTNode} node - A function node to check. * @returns {boolean} Wehether or not a node is a constructor. */ function isES5Constructor(node) { - return ( - node.id && - node.id.name[0] !== node.id.name[0].toLocaleLowerCase() - ); + return (node.id && startsWithUpperCase(node.id.name)); } /** @@ -344,9 +351,9 @@ module.exports = { * If the location is below, this judges `this` is valid. * * - The location is not on an object literal. - * - The location does not assign to a property. + * - The location is not assigned to a variable which starts with an uppercase letter. * - The location is not on an ES2015 class. - * - The location does not call its `bind`/`call`/`apply` method directly. + * - Its `bind`/`call`/`apply` method is not called directly. * - The function is not a callback of array methods (such as `.forEach()`) if `thisArg` is given. * * @param {ASTNode} node - A function node to check. @@ -357,6 +364,7 @@ module.exports = { if (isES5Constructor(node) || hasJSDocThisTag(node, sourceCode)) { return false; } + const isAnonymous = node.id === null; while (node) { const parent = node.parent; @@ -391,25 +399,44 @@ module.exports = { // e.g. // var obj = { foo() { ... } }; // var obj = { foo: function() { ... } }; - case "Property": - return false; - - // e.g. - // obj.foo = foo() { ... }; - case "AssignmentExpression": - return ( - parent.right !== node || - parent.left.type !== "MemberExpression" - ); - - // e.g. // class A { constructor() { ... } } // class A { foo() { ... } } // class A { get foo() { ... } } // class A { set foo() { ... } } // class A { static foo() { ... } } + case "Property": case "MethodDefinition": - return false; + return parent.value !== node; + + // e.g. + // obj.foo = function foo() { ... }; + // Foo = function() { ... }; + // [obj.foo = function foo() { ... }] = a; + // [Foo = function() { ... }] = a; + case "AssignmentExpression": + case "AssignmentPattern": + if (parent.right === node) { + if (parent.left.type === "MemberExpression") { + return false; + } + if (isAnonymous && + parent.left.type === "Identifier" && + startsWithUpperCase(parent.left.name) + ) { + return false; + } + } + return true; + + // e.g. + // var Foo = function() { ... }; + case "VariableDeclarator": + return !( + isAnonymous && + parent.init === node && + parent.id.type === "Identifier" && + startsWithUpperCase(parent.id.name) + ); // e.g. // var foo = function foo() { ... }.bind(obj); diff --git a/tests/lib/rules/no-invalid-this.js b/tests/lib/rules/no-invalid-this.js index 540870e4271..e35f1c7cb0c 100644 --- a/tests/lib/rules/no-invalid-this.js +++ b/tests/lib/rules/no-invalid-this.js @@ -529,7 +529,61 @@ const patterns = [ parserOptions: { ecmaVersion: 6 }, valid: [NORMAL, USE_STRICT, IMPLIED_STRICT, MODULES], invalid: [] - } + }, + + // https://github.com/eslint/eslint/issues/6824 + { + code: "var Ctor = function() { console.log(this); z(x => console.log(x, this)); }", + parserOptions: { ecmaVersion: 6 }, + valid: [NORMAL, USE_STRICT, IMPLIED_STRICT, MODULES], + invalid: [] + }, + { + code: "var func = function() { console.log(this); z(x => console.log(x, this)); }", + parserOptions: { ecmaVersion: 6 }, + errors: errors, + valid: [NORMAL], + invalid: [USE_STRICT, IMPLIED_STRICT, MODULES] + }, + { + code: "Ctor = function() { console.log(this); z(x => console.log(x, this)); }", + parserOptions: { ecmaVersion: 6 }, + valid: [NORMAL, USE_STRICT, IMPLIED_STRICT, MODULES], + invalid: [] + }, + { + code: "func = function() { console.log(this); z(x => console.log(x, this)); }", + parserOptions: { ecmaVersion: 6 }, + errors: errors, + valid: [NORMAL], + invalid: [USE_STRICT, IMPLIED_STRICT, MODULES] + }, + { + code: "function foo(Ctor = function() { console.log(this); z(x => console.log(x, this)); }) {}", + parserOptions: { ecmaVersion: 6 }, + valid: [NORMAL, USE_STRICT, IMPLIED_STRICT, MODULES], + invalid: [] + }, + { + code: "function foo(func = function() { console.log(this); z(x => console.log(x, this)); }) {}", + parserOptions: { ecmaVersion: 6 }, + errors: errors, + valid: [NORMAL], + invalid: [USE_STRICT, IMPLIED_STRICT, MODULES] + }, + { + code: "[obj.method = function() { console.log(this); z(x => console.log(x, this)); }] = a", + parserOptions: { ecmaVersion: 6 }, + valid: [NORMAL, USE_STRICT, IMPLIED_STRICT, MODULES], + invalid: [] + }, + { + code: "[func = function() { console.log(this); z(x => console.log(x, this)); }] = a", + parserOptions: { ecmaVersion: 6 }, + errors: errors, + valid: [NORMAL], + invalid: [USE_STRICT, IMPLIED_STRICT, MODULES] + }, ]; const ruleTester = new RuleTester();