From e8dc017377b3e90281bb9349263db22616242e41 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Tue, 6 Jul 2021 14:29:19 +0800 Subject: [PATCH] Refactor `avoidCapture` function (#1399) --- package.json | 2 +- rules/catch-error-name.js | 17 ++-- rules/consistent-destructuring.js | 3 +- rules/no-for-loop.js | 2 +- rules/prefer-array-find.js | 2 +- rules/prefer-ternary.js | 2 +- rules/prevent-abbreviations.js | 5 +- rules/utils/avoid-capture.js | 153 +++++++++++++++++++----------- test/catch-error-name.mjs | 17 ++++ test/prefer-array-find.mjs | 22 +++++ test/prevent-abbreviations.mjs | 12 ++- 11 files changed, 164 insertions(+), 73 deletions(-) diff --git a/package.json b/package.json index 136aa956ee..9923f443f5 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "xo" ], "dependencies": { + "@babel/helper-validator-identifier": "^7.14.5", "ci-info": "^3.2.0", "clean-regexp": "^1.0.0", "eslint-template-visitor": "^2.3.2", @@ -46,7 +47,6 @@ "pluralize": "^8.0.0", "read-pkg-up": "^7.0.1", "regexp-tree": "^0.1.23", - "reserved-words": "^0.1.2", "safe-regex": "^2.1.1", "semver": "^7.3.5" }, diff --git a/rules/catch-error-name.js b/rules/catch-error-name.js index a244144717..9cbf22d216 100644 --- a/rules/catch-error-name.js +++ b/rules/catch-error-name.js @@ -33,8 +33,6 @@ const selector = matches([ ]); const create = context => { - const {ecmaVersion} = context.parserOptions; - const options = { name: 'error', ignore: [], @@ -79,17 +77,22 @@ const create = context => { variable.scope, ...variable.references.map(({from}) => from) ]; - const fixedName = avoidCapture(expectedName, scopes, ecmaVersion); + const fixedName = avoidCapture(expectedName, scopes); - return { + const problem = { node, messageId: MESSAGE_ID, data: { originalName, - fixedName - }, - fix: fixer => renameVariable(variable, fixedName, fixer) + fixedName: fixedName || expectedName + } }; + + if (fixedName) { + problem.fix = fixer => renameVariable(variable, fixedName, fixer); + } + + return problem; } }; }; diff --git a/rules/consistent-destructuring.js b/rules/consistent-destructuring.js index 7a44d22695..3b743a17bd 100644 --- a/rules/consistent-destructuring.js +++ b/rules/consistent-destructuring.js @@ -52,7 +52,6 @@ const isChildInParentScope = (child, parent) => { }; const create = context => { - const {ecmaVersion} = context.parserOptions; const source = context.getSourceCode(); const declarations = new Map(); @@ -108,7 +107,7 @@ const create = context => { } // Destructured member collides with an existing identifier - if (avoidCapture(member, [memberScope], ecmaVersion) !== member) { + if (avoidCapture(member, [memberScope]) !== member) { return; } } diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index 133714d0af..277763007b 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -355,7 +355,7 @@ const create = context => { const index = indexIdentifierName; const element = elementIdentifierName || - avoidCapture(singular(arrayIdentifierName) || defaultElementName, getChildScopesRecursive(bodyScope), context.parserOptions.ecmaVersion); + avoidCapture(singular(arrayIdentifierName) || defaultElementName, getChildScopesRecursive(bodyScope)); const array = arrayIdentifierName; let declarationElement = element; diff --git a/rules/prefer-array-find.js b/rules/prefer-array-find.js index b4d1f24c9a..d2a17b36f4 100644 --- a/rules/prefer-array-find.js +++ b/rules/prefer-array-find.js @@ -299,7 +299,7 @@ const create = context => { const singularName = singular(node.id.name); if (singularName) { // Rename variable to be singularized now that it refers to a single item in the array instead of the entire array. - const singularizedName = avoidCapture(singularName, getChildScopesRecursive(scope), context.parserOptions.ecmaVersion); + const singularizedName = avoidCapture(singularName, getChildScopesRecursive(scope)); yield * renameVariable(variable, singularizedName, fixer); // Prevent possible variable conflicts diff --git a/rules/prefer-ternary.js b/rules/prefer-ternary.js index 7537abb11f..a8a3c7a94b 100644 --- a/rules/prefer-ternary.js +++ b/rules/prefer-ternary.js @@ -216,7 +216,7 @@ const create = context => { let generateNewVariables = false; if (type === 'ThrowStatement') { const scopes = getScopes(scope); - const errorName = avoidCapture('error', scopes, context.parserOptions.ecmaVersion, isSafeName); + const errorName = avoidCapture('error', scopes, isSafeName); for (const scope of scopes) { if (!scopeToNamesGeneratedByFixer.has(scope)) { diff --git a/rules/prevent-abbreviations.js b/rules/prevent-abbreviations.js index 4fab188410..2628d06837 100644 --- a/rules/prevent-abbreviations.js +++ b/rules/prevent-abbreviations.js @@ -313,7 +313,6 @@ const isInternalImport = node => { }; const create = context => { - const {ecmaVersion} = context.parserOptions; const options = prepareOptions(context.options[0]); const filenameWithExtension = context.getPhysicalFilename(); @@ -416,7 +415,7 @@ const create = context => { variable.scope ]; variableReplacements.samples = variableReplacements.samples.map( - name => avoidCapture(name, scopes, ecmaVersion, isSafeName) + name => avoidCapture(name, scopes, isSafeName) ); const problem = { @@ -424,7 +423,7 @@ const create = context => { message: formatMessage(definition.name.name, variableReplacements, 'variable') }; - if (variableReplacements.total === 1 && shouldFix(variable)) { + if (variableReplacements.total === 1 && shouldFix(variable) && variableReplacements.samples[0]) { const [replacement] = variableReplacements.samples; for (const scope of scopes) { diff --git a/rules/utils/avoid-capture.js b/rules/utils/avoid-capture.js index 9df03e93bd..1f87a04c33 100644 --- a/rules/utils/avoid-capture.js +++ b/rules/utils/avoid-capture.js @@ -1,33 +1,84 @@ 'use strict'; -const reservedWords = require('reserved-words'); +const { + isIdentifierName, + isStrictReservedWord, + isKeyword +} = require('@babel/helper-validator-identifier'); const resolveVariableName = require('./resolve-variable-name.js'); -const indexifyName = (name, index) => name + '_'.repeat(index); - -const scopeHasArgumentsSpecial = scope => { - while (scope) { - /* istanbul ignore next: `someScopeHasVariableName` seems already handle this */ - if (scope.taints.get('arguments')) { - return true; - } - - scope = scope.upper; - } - - return false; -}; - -const someScopeHasVariableName = (name, scopes) => scopes.some(scope => resolveVariableName(name, scope)); - -const someScopeIsStrict = scopes => scopes.some(scope => scope.isStrict); - -const nameCollidesWithArgumentsSpecial = (name, scopes, isStrict) => { - if (name !== 'arguments') { - return false; - } - - return isStrict || scopes.some(scope => scopeHasArgumentsSpecial(scope)); -}; +// https://github.com/microsoft/TypeScript/issues/2536#issuecomment-87194347 +const typescriptReservedWords = new Set([ + 'break', + 'case', + 'catch', + 'class', + 'const', + 'continue', + 'debugger', + 'default', + 'delete', + 'do', + 'else', + 'enum', + 'export', + 'extends', + 'false', + 'finally', + 'for', + 'function', + 'if', + 'import', + 'in', + 'instanceof', + 'new', + 'null', + 'return', + 'super', + 'switch', + 'this', + 'throw', + 'true', + 'try', + 'typeof', + 'var', + 'void', + 'while', + 'with', + 'as', + 'implements', + 'interface', + 'let', + 'package', + 'private', + 'protected', + 'public', + 'static', + 'yield', + 'any', + 'boolean', + 'constructor', + 'declare', + 'get', + 'module', + 'require', + 'number', + 'set', + 'string', + 'symbol', + 'type', + 'from', + 'of' +]); + +// Copied from https://github.com/babel/babel/blob/fce35af69101c6b316557e28abf60bdbf77d6a36/packages/babel-types/src/validators/isValidIdentifier.ts#L7 +// Use this function instead of `require('@babel/types').isIdentifier`, since `@babel/helper-validator-identifier` package is much smaller +const isValidIdentifier = name => + typeof name === 'string' && + !isKeyword(name) && + !isStrictReservedWord(name, true) && + isIdentifierName(name) && + name !== 'arguments' && + !typescriptReservedWords.has(name); /* Unresolved reference is probably from the global scope. We should avoid using that name. @@ -46,21 +97,12 @@ function unicorn() { } ``` */ -const isUnresolvedName = (name, scopes) => scopes.some(scope => +const isUnresolvedName = (name, scope) => scope.references.some(reference => reference.identifier && reference.identifier.name === name && !reference.resolved) || - isUnresolvedName(name, scope.childScopes) -); - -const isSafeName = (name, scopes, ecmaVersion, isStrict) => { - ecmaVersion = Math.min(6, ecmaVersion); // 6 is the latest version understood by `reservedWords` - - return ( - !someScopeHasVariableName(name, scopes) && - !reservedWords.check(name, ecmaVersion, isStrict) && - !nameCollidesWithArgumentsSpecial(name, scopes, isStrict) && - !isUnresolvedName(name, scopes) - ); -}; + scope.childScopes.some(scope => isUnresolvedName(name, scope)); + +const isSafeName = (name, scopes) => + !scopes.some(scope => resolveVariableName(name, scope) || isUnresolvedName(name, scope)); const alwaysTrue = () => true; @@ -68,9 +110,9 @@ const alwaysTrue = () => true; Rule-specific name check function. @callback isSafe -@param {string} indexifiedName - The generated candidate name. +@param {string} name - The generated candidate name. @param {Scope[]} scopes - The same list of scopes you pass to `avoidCapture`. -@returns {boolean} - `true` if the `indexifiedName` is ok. +@returns {boolean} - `true` if the `name` is ok. */ /** @@ -84,22 +126,21 @@ Useful when you want to rename a variable (or create a new variable) while being @param {string} name - The desired name for a new variable. @param {Scope[]} scopes - The list of scopes the new variable will be referenced in. -@param {number} ecmaVersion - The language version, get it from `context.parserOptions.ecmaVersion`. @param {isSafe} [isSafe] - Rule-specific name check function. @returns {string} - Either `name` as is, or a string like `${name}_` suffixed with underscores to make the name unique. */ -module.exports = (name, scopes, ecmaVersion, isSafe = alwaysTrue) => { - const isStrict = someScopeIsStrict(scopes); - - let index = 0; - let indexifiedName = indexifyName(name, index); - while ( - !isSafeName(indexifiedName, scopes, ecmaVersion, isStrict) || - !isSafe(indexifiedName, scopes) - ) { - index++; - indexifiedName = indexifyName(name, index); +module.exports = (name, scopes, isSafe = alwaysTrue) => { + if (!isValidIdentifier(name)) { + name += '_'; + + if (!isValidIdentifier(name)) { + return; + } + } + + while (!isSafeName(name, scopes) || !isSafe(name, scopes)) { + name += '_'; } - return indexifiedName; + return name; }; diff --git a/test/catch-error-name.mjs b/test/catch-error-name.mjs index d70364f1cb..56818d6b6f 100644 --- a/test/catch-error-name.mjs +++ b/test/catch-error-name.mjs @@ -508,6 +508,23 @@ test({ options: [{name: 'exception'}] }, + // Should not run into an infinity loop + { + code: 'try {} catch (e) {}', + errors: [generateError('e', 'has_space_after ')], + options: [{name: 'has_space_after '}] + }, + { + code: 'try {} catch (e) {}', + errors: [generateError('e', '1_start_with_a_number')], + options: [{name: '1_start_with_a_number'}] + }, + { + code: 'try {} catch (e) {}', + errors: [generateError('e', '_){} evilCode; if(false')], + options: [{name: '_){} evilCode; if(false'}] + }, + // `ignore` { code: 'try {} catch (notMatching) {}', diff --git a/test/prefer-array-find.mjs b/test/prefer-array-find.mjs index 3f82207ac0..dab9ec5f8f 100644 --- a/test/prefer-array-find.mjs +++ b/test/prefer-array-find.mjs @@ -784,6 +784,28 @@ test({ `, errors: [{messageId: ERROR_DECLARATION}] }, + { + code: outdent` + const packages = array.filter(bar); + console.log(packages[0]); + `, + output: outdent` + const package_ = array.find(bar); + console.log(package_); + `, + errors: [{messageId: ERROR_DECLARATION}] + }, + { + code: outdent` + const symbols = array.filter(bar); + console.log(symbols[0]); + `, + output: outdent` + const symbol_ = array.find(bar); + console.log(symbol_); + `, + errors: [{messageId: ERROR_DECLARATION}] + }, // Not fixable { diff --git a/test/prevent-abbreviations.mjs b/test/prevent-abbreviations.mjs index f39a18ce24..dee49a5803 100644 --- a/test/prevent-abbreviations.mjs +++ b/test/prevent-abbreviations.mjs @@ -1295,7 +1295,7 @@ test({ { code: 'var y', - output: 'var yield', + output: 'var yield_', options: customOptions, errors: createErrors() }, @@ -1316,6 +1316,16 @@ test({ output: 'function a() {try {} catch(arguments_) {}}', options: customOptions, errors: createErrors() + }, + { + code: 'var one', + options: [{replacements: {one: {1: true}}}], + errors: createErrors() + }, + { + code: 'var one_two', + options: [{replacements: {one: {first: true, 1: true}}}], + errors: createErrors() } ] });