Refactor avoidCapture function (#1399)

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"
 	},

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;
 		}
 	};
 };

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;
 				}
 			}

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;

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

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)) {

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,15 +415,15 @@ const create = context => {
 			variable.scope
 		];
 		variableReplacements.samples = variableReplacements.samples.map(
-			name => avoidCapture(name, scopes, ecmaVersion, isSafeName)
+			name => avoidCapture(name, scopes, isSafeName)
 		);
 
 		const problem = {
 			node: definition.name,
 			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) {

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,31 +97,22 @@ 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;
 
 /**
 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;
 };

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) {}',

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
 		{

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()
 		}
 	]
 });