diff --git a/docs/rules/prevent-abbreviations.md b/docs/rules/prevent-abbreviations.md new file mode 100644 index 0000000000..cf4b3c6947 --- /dev/null +++ b/docs/rules/prevent-abbreviations.md @@ -0,0 +1,222 @@ +# Prevent abbreviations + +Using complete words results in more readable code. Not everyone knows all your abbreviations. Code is written only once, but read many times. + +This rule can also be used to replace terms, disallow words, etc. See the [`replacements`](#replacements) and [`extendDefaultReplacements`](#extenddefaultreplacements) options. + +You can find the default replacements [here](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/rules/prevent-abbreviations.js#L13). + +This rule is fixable only for variable names with exactly one replacement defined. + + +## Fail + +```js +const e = new Error(); +``` + +```js +const e = document.createEvent('Event'); +``` + +```js +const levels = { + err: 0 +}; +``` + +```js +this.evt = 'click'; +``` + +```js +class Btn {} +``` + + +## Pass + +```js +const error = new Error(); +``` + +```js +const event = document.createEvent('Event'); +``` + +```js +const levels = { + error: 0 +}; +``` + +```js +this.event = 'click'; +``` + +```js +class Button {} +``` + + +## Options + +Type: `Object` + +### replacements + +Type: `Object` + +You can extend default replacements by passing the `replacements` option. + +Lowercase replacements will match both camelcase and pascalcase identifiers. For example, `err` will match both `err` and `Err`. `errCb` will match both `errCb` and `ErrCb`. + +Lowercase replacements will match both complete identifiers and separate words inside identifiers. For example, `cmd` will match all of `cmd`, `createCmd` and `CmdFactory`. + +Camelcase replacements will only match complete identifiers. For example `errCb` will only match `errCb` and `ErrCb`. It will not match `fooErrCb` or `errCbFoo`. + +The example below: +- disables the default `e` → `event` replacement (leaving `e` → `error` enabled), +- disables `res` replacement completely (both `res` → `response` and `res` → `result` from defaults are disabled), +- adds a custom `cmd` → `command` replacement, +- adds a custom `errCb` → `handleError` replacement. + +```js +"unicorn/prevent-abbreviations": [ + "error", + { + "replacements": { + "e": { + "event": false + }, + "res": false, + "cmd": { + "command": true + }, + "errCb": { + "handleError": true + } + } + } +] +``` + +### extendDefaultReplacements + +Type: `boolean`
+Default: `true` + +Pass `"extendDefaultReplacements": false` to override the default `replacements` completely. + +The example below disables all the default replacements and enables a custom `cmd` → `command` one. + +```js +"unicorn/prevent-abbreviations": [ + "error", + { + "extendDefaultReplacements": false, + "replacements": { + "cmd": { + "command": true + } + } + } +] +``` + +### whitelist + +Type: `Object` + +You can extend the default whitelist by passing the `whitelist` option. + +Unlike the `replacements` option, `whitelist` matches full identifier names case-sensitively. + +For example, if you want to report `props` → `properties` (enabled by default), but allow `getInitialProps`, you could use the following configuration. + +```js +"unicorn/prevent-abbreviations": [ + "error", + { + "whitelist": { + "getInitialProps": true + } + } +] +``` + +### extendDefaultWhitelist + +Type: `boolean`
+Default: `true` + +Pass `"extendDefaultWhitelist": false` to override the default `whitelist` completely. + +### checkDefaultAndNamespaceImports + +Type: `boolean`
+Default: `false` + +Pass `"checkDefaultAndNamespaceImports": true` to check variables declared in default or namespace import. + +With this set to `true` the following code will be reported. + +```js +import tempWrite from 'temp-write'; +``` + +```js +import * as err from 'err'; +``` + +```js +const err = require('err'); +``` + +### checkShorthandImports + +Type: `boolean`
+Default: `false` + +Pass `"checkShorthandImports": true` to check variables declared in shorthand import. + +With this set to `true` the following code will be reported. + +```js +import {prop} from 'ramda'; +``` + +### checkShorthandProperties + +Type: `boolean`
+Default: `false` + +Pass `"checkShorthandProperties": true` to check variables declared as shorhand properties in object destructuring. + +With this set to `true` the following code will be reported. + +```js +const {prop} = require('ramda'); +``` + +```js +const {err} = foo; +``` + +```js +function f({err}) {} +``` + +### checkProperties + +Type: `boolean`
+Default: `true` + +Pass `"checkProperties": false` to disable checking property names. + +### checkVariables + +Type: `boolean`
+Default: `true` + +Pass `"checkVariables": false` to disable checking variable names. diff --git a/index.js b/index.js index 5a07ad24b7..83c7080592 100644 --- a/index.js +++ b/index.js @@ -47,7 +47,8 @@ module.exports = { 'unicorn/prefer-query-selector': 'error', 'unicorn/prefer-node-remove': 'error', 'unicorn/prefer-text-content': 'error', - 'unicorn/no-for-loop': 'error' + 'unicorn/no-for-loop': 'error', + 'unicorn/prevent-abbreviations': 'error' } } } diff --git a/package.json b/package.json index ffc06548c4..991c493224 100644 --- a/package.json +++ b/package.json @@ -35,9 +35,12 @@ "eslint-ast-utils": "^1.0.0", "import-modules": "^1.1.0", "lodash.camelcase": "^4.1.1", + "lodash.defaultsdeep": "^4.6.0", "lodash.kebabcase": "^4.0.1", "lodash.snakecase": "^4.0.1", + "lodash.topairs": "^4.3.0", "lodash.upperfirst": "^4.2.0", + "reserved-words": "^0.1.2", "safe-regex": "^2.0.1" }, "devDependencies": { diff --git a/readme.md b/readme.md index 0cbef5e6b0..3827b3fbd2 100644 --- a/readme.md +++ b/readme.md @@ -64,7 +64,8 @@ Configure it in `package.json`. "unicorn/prefer-query-selector": "error", "unicorn/prefer-node-remove": "error", "unicorn/prefer-text-content": "error", - "unicorn/no-for-loop": "error" + "unicorn/no-for-loop": "error", + "unicorn/prevent-abbreviations": "error" } } } @@ -104,6 +105,7 @@ Configure it in `package.json`. - [prefer-node-remove](docs/rules/prefer-node-remove.md) - Prefer `remove` over `parentNode.removeChild` and `parentElement.removeChild`. *(fixable)* - [prefer-text-content](docs/rules/prefer-text-content.md) - Prefer `textContent` over `innerText`. *(fixable)* - [no-for-loop](docs/rules/no-for-loop.md) - Do not use a `for` loop that can be replaced with a `for-of` loop. *(fixable)* +- [prevent-abbreviations](docs/rules/prevent-abbreviations.md) - Prevent abbreviations *(partly fixable)* ## Recommended config diff --git a/rules/catch-error-name.js b/rules/catch-error-name.js index 9347b4fbc2..d6d9b577c5 100644 --- a/rules/catch-error-name.js +++ b/rules/catch-error-name.js @@ -25,6 +25,7 @@ function isLintablePromiseCatch(node) { return arg0.type === 'FunctionExpression' || arg0.type === 'ArrowFunctionExpression'; } +// TODO: Use `./utils/avoid-capture.js` instead function indexifyName(name, scope) { const variables = scope.variableScope.set; diff --git a/rules/prevent-abbreviations.js b/rules/prevent-abbreviations.js new file mode 100644 index 0000000000..7f02c1702f --- /dev/null +++ b/rules/prevent-abbreviations.js @@ -0,0 +1,727 @@ +'use strict'; +const astUtils = require('eslint-ast-utils'); +const defaultsDeep = require('lodash.defaultsdeep'); +const toPairs = require('lodash.topairs'); +const camelCase = require('lodash.camelcase'); +const kebabCase = require('lodash.kebabcase'); +const upperfirst = require('lodash.upperfirst'); + +const getDocsUrl = require('./utils/get-docs-url'); +const avoidCapture = require('./utils/avoid-capture'); + +const pascalCase = string => upperfirst(camelCase(string)); +const isPascalCase = string => string === pascalCase(string); + +const isUpperCase = string => string === string.toUpperCase(); + +const defaultReplacements = { + err: { + error: true + }, + cb: { + callback: true + }, + opts: { + options: true + }, + str: { + string: true + }, + obj: { + object: true + }, + num: { + number: true + }, + val: { + value: true + }, + e: { + event: true, + error: true + }, + evt: { + event: true + }, + el: { + element: true + }, + req: { + request: true + }, + res: { + response: true, + result: true + }, + btn: { + button: true + }, + msg: { + message: true + }, + len: { + length: true + }, + env: { + environment: true + }, + dev: { + development: true + }, + prod: { + production: true + }, + tmp: { + temporary: true + }, + arg: { + argument: true + }, + args: { + arguments: true + }, + tbl: { + table: true + }, + db: { + database: true + }, + ctx: { + context: true + }, + mod: { + module: true + }, + prop: { + property: true + }, + arr: { + array: true + }, + ret: { + returnValue: true + }, + retval: { + returnValue: true + }, + ext: { + extension: true + }, + exts: { + extensions: true + }, + lib: { + library: true + }, + dir: { + directory: true + }, + dirs: { + directories: true + }, + ref: { + reference: true + }, + refs: { + references: true + }, + pkg: { + package: true + }, + sep: { + separator: true + }, + doc: { + document: true + }, + docs: { + documents: true + }, + elem: { + element: true + }, + src: { + source: true + }, + dest: { + destination: true + }, + prev: { + previous: true + }, + rel: { + relative: true, + related: true, + relationship: true + }, + conf: { + config: true + }, + temp: { + temporary: true + }, + props: { + properties: true + }, + attr: { + attribute: true + }, + attrs: { + attributes: true + } +}; + +const defaultWhitelist = { + propTypes: true, + defaultProps: true, + getDerivedStateFromProps: true, + stdDev: true +}; + +const prepareOptions = ({ + checkProperties = true, + checkVariables = true, + + checkDefaultAndNamespaceImports = false, + checkShorthandImports = false, + checkShorthandProperties = false, + + extendDefaultReplacements = true, + replacements = {}, + + extendDefaultWhitelist = true, + whitelist = {} +} = {}) => { + const mergedReplacements = extendDefaultReplacements ? + defaultsDeep({}, replacements, defaultReplacements) : + replacements; + + const mergedWhitelist = extendDefaultWhitelist ? + defaultsDeep({}, whitelist, defaultWhitelist) : + whitelist; + + return { + checkProperties, + checkVariables, + + checkDefaultAndNamespaceImports, + checkShorthandImports, + checkShorthandProperties, + + replacements: new Map(toPairs(mergedReplacements).map(([discouragedName, replacements]) => { + return [discouragedName, new Map(toPairs(replacements))]; + })), + whitelist: new Map(toPairs(mergedWhitelist)) + }; +}; + +const normalizeName = name => { + let originalLeadingUnderscores; + let originalTrailingUnderscores; + ([, originalLeadingUnderscores, name, originalTrailingUnderscores] = /^(_*)(.*?)(_*)$/.exec(name)); + + const originalIsInPascalCase = isPascalCase(name); + if (originalIsInPascalCase) { + name = camelCase(name); + } + + return { + originalLeadingUnderscores, + originalTrailingUnderscores, + originalIsInPascalCase, + normalizedName: name + }; +}; + +const createApplyOriginalUnderscores = (originalLeadingUnderscores, originalTrailingUnderscores) => name => { + return originalLeadingUnderscores + name + originalTrailingUnderscores; +}; + +const splitNormalizedName = normalizedName => { + return kebabCase(normalizedName).split('-'); +}; + +const getWordReplacements = (replacements, word) => { + const wordReplacements = replacements.get(word); + + if (!wordReplacements) { + return []; + } + + return [...wordReplacements.keys()].filter(name => wordReplacements.get(name)); +}; + +/* + * This function has terrible big O complexity, so we limit it by `limit`. + * This is fine since result of the function is used only to check if there is zero, one or more + * replacements and when formating the message. + * Example: `[[1, 2], [3, 4]]` -> `[[1, 3], [1, 4], [2, 3], [2, 4]]` + */ +const getWordByWordReplacementsCombinations = (wordByWordReplacements, limit = 16) => { + if (wordByWordReplacements.length === 0) { + return []; + } + + if (wordByWordReplacements.length === 1) { + return wordByWordReplacements[0]; + } + + if (limit <= 1) { + return wordByWordReplacements[0]; + } + + const [wordReplacements, ...tailWordReplacements] = wordByWordReplacements; + const tailCombinations = getWordByWordReplacementsCombinations(tailWordReplacements, limit / wordReplacements.length); + + const result = []; + for (const name of wordReplacements) { + for (const combination of tailCombinations) { + result.push([name].concat(combination)); + } + } + + return result; +}; + +const getWordByWordReplacements = (replacements, normalizedName, originalIsInPascalCase) => { + const words = splitNormalizedName(normalizedName); + + let wordByWordReplacements = words.map(word => getWordReplacements(replacements, word)); + + const someWordsHaveReplacements = wordByWordReplacements.some(wordReplacements => wordReplacements.length > 0); + if (!someWordsHaveReplacements) { + return []; + } + + wordByWordReplacements = wordByWordReplacements + .map((wordReplacements, i) => wordReplacements.length > 0 ? wordReplacements : [words[i]]); + + return getWordByWordReplacementsCombinations(wordByWordReplacements) + .map(originalIsInPascalCase ? pascalCase : camelCase) + .sort(); +}; + +const getExactReplacements = (replacements, normalizedName, originalIsInPascalCase) => { + const variableNameReplacements = replacements.get(normalizedName); + + if (!variableNameReplacements) { + return []; + } + + return [...variableNameReplacements.keys()] + .filter(name => variableNameReplacements.get(name)) + .map(originalIsInPascalCase ? pascalCase : name => name) + .sort(); +}; + +const getNameReplacements = (replacements, whitelist, name) => { + if (whitelist.get(name)) { + return []; + } + + if (isUpperCase(name)) { + return []; + } + + const { + originalLeadingUnderscores, + originalTrailingUnderscores, + originalIsInPascalCase, + normalizedName + } = normalizeName(name); + + const applyOriginalUnderscores = createApplyOriginalUnderscores(originalLeadingUnderscores, originalTrailingUnderscores); + + const exactReplacements = getExactReplacements(replacements, normalizedName, originalIsInPascalCase); + + if (exactReplacements.length > 0) { + return exactReplacements.map(applyOriginalUnderscores); + } + + return getWordByWordReplacements(replacements, normalizedName, originalIsInPascalCase).map(applyOriginalUnderscores); +}; + +const anotherNameMessage = 'A more descriptive name will do too.'; + +const formatMessage = (discouragedName, replacements, nameTypeText, replacementsLimit = 3) => { + const message = []; + + if (replacements.length === 1) { + message.push(`The ${nameTypeText} \`${discouragedName}\` should be named \`${replacements[0]}\`.`); + } else { + let replacementsText = replacements.slice(0, replacementsLimit) + .map(replacement => `\`${replacement}\``) + .join(', '); + + const omittedReplacementsCount = replacements.length - replacementsLimit; + if (omittedReplacementsCount > 0) { + replacementsText += `, ... (${omittedReplacementsCount} more omitted)`; + } + + message.push(`Please rename the ${nameTypeText} \`${discouragedName}\`.`); + message.push(`Suggested names are: ${replacementsText}.`); + } + + message.push(anotherNameMessage); + + return message.join(' '); +}; + +const variableIdentifiers = variable => [...(new Set([ + ...variable.identifiers, + ...variable.references + .map(reference => reference.identifier) +])).values()]; + +const isExportedIdentifier = identifier => { + if (identifier.parent.type === 'VariableDeclarator' && + identifier.parent.id === identifier + ) { + return identifier.parent.parent.type === 'VariableDeclaration' && + identifier.parent.parent.parent.type === 'ExportNamedDeclaration'; + } + + if (identifier.parent.type === 'FunctionDeclaration' && + identifier.parent.id === identifier + ) { + return identifier.parent.parent.type === 'ExportNamedDeclaration'; + } + + if (identifier.parent.type === 'ClassDeclaration' && + identifier.parent.id === identifier + ) { + return identifier.parent.parent.type === 'ExportNamedDeclaration'; + } + + return false; +}; + +const shouldFix = variable => { + return !variableIdentifiers(variable).some(isExportedIdentifier); +}; + +const isShorthandPropertyIdentifier = identifier => { + return identifier.parent.type === 'Property' && + identifier.parent.shorthand; +}; + +const isShorthandImportIdentifier = identifier => { + return identifier.parent.type === 'ImportSpecifier' && + identifier.parent.imported.name === identifier.name && + identifier.parent.local.name === identifier.name; +}; + +const isShorthandExportIdentifier = identifier => { + return identifier.parent.type === 'ExportSpecifier' && + identifier.parent.exported.name === identifier.name && + identifier.parent.local.name === identifier.name; +}; + +const fixIdentifier = (fixer, replacement) => identifier => { + if (isShorthandPropertyIdentifier(identifier)) { + return fixer.replaceText(identifier, `${identifier.name}: ${replacement}`); + } + + if (isShorthandImportIdentifier(identifier)) { + return fixer.replaceText(identifier, `${identifier.name} as ${replacement}`); + } + + if (isShorthandExportIdentifier(identifier)) { + return fixer.replaceText(identifier, `${replacement} as ${identifier.name}`); + } + + return fixer.replaceText(identifier, replacement); +}; + +const isDefaultOrNamespaceImportName = identifier => { + if (identifier.parent.type === 'ImportDefaultSpecifier' && + identifier.parent.local === identifier + ) { + return true; + } + + if (identifier.parent.type === 'ImportNamespaceSpecifier' && + identifier.parent.local === identifier + ) { + return true; + } + + if (identifier.parent.type === 'ImportSpecifier' && + identifier.parent.local === identifier && + identifier.parent.imported.type === 'Identifier' && + identifier.parent.imported.name === 'default' + ) { + return true; + } + + if (identifier.parent.type === 'VariableDeclarator' && + identifier.parent.id === identifier && + astUtils.isStaticRequire(identifier.parent.init) + ) { + return true; + } + + return false; +}; + +const isClassVariable = variable => { + if (variable.defs.length !== 1) { + return false; + } + + const [definition] = variable.defs; + + return definition.type === 'ClassName'; +}; + +const shouldReportIdentifierAsProperty = identifier => { + if (identifier.parent.type === 'MemberExpression' && + identifier.parent.property === identifier && + !identifier.parent.computed && + identifier.parent.parent.type === 'AssignmentExpression' && + identifier.parent.parent.left === identifier.parent + ) { + return true; + } + + if (identifier.parent.type === 'Property' && + identifier.parent.key === identifier && + !identifier.parent.computed && + !identifier.parent.shorthand && // Shorthand properties are reported and fixed as variables + identifier.parent.parent.type === 'ObjectExpression' + ) { + return true; + } + + if (identifier.parent.type === 'ExportSpecifier' && + identifier.parent.exported === identifier && + identifier.parent.local !== identifier // Same as shorthand properties above + ) { + return true; + } + + if (identifier.parent.type === 'MethodDefinition' && + identifier.parent.key === identifier && + !identifier.parent.computed + ) { + return true; + } + + if (identifier.parent.type === 'ClassProperty' && + identifier.parent.key === identifier && + !identifier.parent.computed + ) { + return true; + } + + return false; +}; + +const create = context => { + const { + ecmaVersion + } = context.parserOptions; + const options = prepareOptions(context.options[0]); + + // A `class` declaration produces two variables in two scopes: + // the inner class scope, and the outer one (whereever the class is declared). + // This map holds the outer ones to be later processed when the inner one is encountered. + // For why this is not a eslint issue see https://github.com/eslint/eslint-scope/issues/48#issuecomment-464358754 + const identifierToOuterClassVariable = new WeakMap(); + + const checkPossiblyWeirdClassVariable = variable => { + if (isClassVariable(variable)) { + if (variable.scope.type === 'class') { // The inner class variable + const [definition] = variable.defs; + const outerClassVariable = identifierToOuterClassVariable.get(definition.name); + + if (!outerClassVariable) { + return checkVariable(variable); + } + + // Create a normal-looking variable (like a `var` or a `function`) + // For which a single `variable` holds all references, unline with `class` + const combinedReferencesVariable = { + name: variable.name, + scope: variable.scope, + defs: variable.defs, + identifiers: variable.identifiers, + references: variable.references.concat(outerClassVariable.references) + }; + + // Call the common checker with the newly forged normalized class variable + return checkVariable(combinedReferencesVariable); + } + + // The outer class variable, we save it for later, when it's inner counterpart is encountered + const [definition] = variable.defs; + identifierToOuterClassVariable.set(definition.name, variable); + + return; + } + + return checkVariable(variable); + }; + + // Holds a map from a `Scope` to a `Set` of new variable names generated by our fixer. + // Used to avoid generating duplicate names, see for instance `let errCb, errorCb` test. + const scopeToNamesGeneratedByFixer = new WeakMap(); + const isSafeName = (name, scopes) => scopes.every(scope => { + const generatedNames = scopeToNamesGeneratedByFixer.get(scope); + return !generatedNames || !generatedNames.has(name); + }); + + const checkVariable = variable => { + if (variable.defs.length === 0) { + return; + } + + const [definition] = variable.defs; + + if (!options.checkDefaultAndNamespaceImports && isDefaultOrNamespaceImportName(definition.name)) { + return; + } + + if (!options.checkShorthandImports && isShorthandImportIdentifier(definition.name)) { + return; + } + + if (!options.checkShorthandProperties && isShorthandPropertyIdentifier(definition.name)) { + return; + } + + const variableReplacements = getNameReplacements(options.replacements, options.whitelist, variable.name); + + if (variableReplacements.length === 0) { + return; + } + + const scopes = variable.references.map(reference => reference.from).concat(variable.scope); + + const problem = { + node: definition.name, + message: formatMessage(definition.name.name, variableReplacements, 'variable') + }; + + if (variableReplacements.length === 1 && shouldFix(variable)) { + const [replacement] = variableReplacements; + const captureAvoidingReplacement = avoidCapture(replacement, scopes, ecmaVersion, isSafeName); + + for (const scope of scopes) { + if (!scopeToNamesGeneratedByFixer.has(scope)) { + scopeToNamesGeneratedByFixer.set(scope, new Set()); + } + + const generatedNames = scopeToNamesGeneratedByFixer.get(scope); + generatedNames.add(captureAvoidingReplacement); + } + + problem.fix = fixer => { + return variableIdentifiers(variable) + .map(fixIdentifier(fixer, captureAvoidingReplacement)); + }; + } + + context.report(problem); + }; + + const checkVariables = scope => { + scope.variables.forEach(checkPossiblyWeirdClassVariable); + }; + + const checkChildScopes = scope => { + scope.childScopes.forEach(checkScope); + }; + + const checkScope = scope => { + checkVariables(scope); + + return checkChildScopes(scope); + }; + + return { + Identifier(node) { + if (!options.checkProperties) { + return; + } + + if (node.name === '__proto__') { + return; + } + + const identifierReplacements = getNameReplacements(options.replacements, options.whitelist, node.name); + + if (identifierReplacements.length === 0) { + return; + } + + if (!shouldReportIdentifierAsProperty(node)) { + return; + } + + const problem = { + node, + message: formatMessage(node.name, identifierReplacements, 'property') + }; + + context.report(problem); + }, + + 'Program:exit'() { + if (!options.checkVariables) { + return; + } + + checkScope(context.getScope()); + } + }; +}; + +const schema = [{ + type: 'object', + properties: { + checkProperties: {type: 'boolean'}, + checkVariables: {type: 'boolean'}, + + checkDefaultAndNamespaceImports: {type: 'boolean'}, + checkShorthandImports: {type: 'boolean'}, + checkShorthandProperties: {type: 'boolean'}, + + extendDefaultReplacements: {type: 'boolean'}, + replacements: {$ref: '#/items/0/definitions/abbreviations'}, + + extendDefaultWhitelist: {type: 'boolean'}, + whitelist: {$ref: '#/items/0/definitions/booleanObject'} + }, + additionalProperties: false, + definitions: { + abbreviations: { + type: 'object', + additionalProperties: {$ref: '#/items/0/definitions/replacements'} + }, + replacements: { + anyOf: [ + { + enum: [false] + }, + {$ref: '#/items/0/definitions/booleanObject'} + ] + }, + booleanObject: { + type: 'object', + additionalProperties: {type: 'boolean'} + } + } +}]; + +module.exports = { + create, + meta: { + type: 'suggestion', + docs: { + url: getDocsUrl(__filename) + }, + fixable: 'code', + schema + } +}; diff --git a/rules/utils/avoid-capture.js b/rules/utils/avoid-capture.js new file mode 100644 index 0000000000..759ad33d0f --- /dev/null +++ b/rules/utils/avoid-capture.js @@ -0,0 +1,77 @@ +'use strict'; +const reservedWords = require('reserved-words'); +const resolveVariableName = require('./resolve-variable-name'); + +const indexifyName = (name, index) => name + '_'.repeat(index); + +const scopeHasArgumentsSpecial = scope => { + while (scope) { + 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(scopeHasArgumentsSpecial); +}; + +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); +}; + +const alwaysTrue = () => true; + +/** + * Rule-specific name check function + * + * @callback isSafe + * @param {string} indexifiedName - The generated candidate name. + * @param {Scope[]} scopes - The same list of scopes you pass to `avoidCapture`. + * @return {boolean} - `true` if the `indexifiedName` is ok. + */ + +/** + * Generates a unique name prefixed with `name` such that: + * - it is not defined in any of the `scopes`, + * - it is not a reserved word, + * - it is not `arguments` in strict scopes (where `arguments` is not allowed), + * - it does not collide with the actual `arguments` (which is always defined in function scopes). + * + * Useful when you want to rename a variable (or create a new variable) + * while being sure not to shadow any other variables in the code. + * + * @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 undescores 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); + } + + return indexifiedName; +}; diff --git a/rules/utils/resolve-variable-name.js b/rules/utils/resolve-variable-name.js new file mode 100644 index 0000000000..d604b94cdd --- /dev/null +++ b/rules/utils/resolve-variable-name.js @@ -0,0 +1,22 @@ +'use strict'; + +/** + * Finds a variable named `name` in the scope `scope` (or it's parents). + * + * @param {string} name - The variable name to be resolve. + * @param {Scope} scope - The scope to look for the variable in. + * @returns {?Variable} - The found variable, if any. + */ +module.exports = (name, scope) => { + while (scope) { + const variable = scope.set.get(name); + + if (variable) { + return variable; + } + + scope = scope.upper; + } + + return undefined; +}; diff --git a/test/prevent-abbreviations.js b/test/prevent-abbreviations.js new file mode 100644 index 0000000000..758f495cc7 --- /dev/null +++ b/test/prevent-abbreviations.js @@ -0,0 +1,1161 @@ +import test from 'ava'; +import avaRuleTester from 'eslint-ava-rule-tester'; +import rule from '../rules/prevent-abbreviations'; + +const ruleTester = avaRuleTester(test, { + env: { + es6: true + } +}); + +const browserES5RuleTester = avaRuleTester(test, { + parserOptions: { + ecmaVersion: 5 + }, + env: { + browser: true + } +}); + +const moduleRuleTester = avaRuleTester(test, { + parserOptions: { + ecmaVersion: 2019, + sourceType: 'module' + } +}); + +const babelRuleTester = avaRuleTester(test, { + parser: 'babel-eslint' +}); + +const noFixingTestCase = test => Object.assign({}, test, { + output: test.code +}); + +const createErrors = message => { + const error = { + ruleId: 'prevent-abbreviations' + }; + + if (message) { + error.message = message; + } + + return [error]; +}; + +const extendedOptions = [{ + replacements: { + e: false, + c: { + custom: true + }, + cb: { + callback: false, + circuitBreacker: true + } + } +}]; + +const customOptions = [{ + checkProperties: false, + + checkDefaultAndNamespaceImports: true, + checkShorthandImports: true, + checkShorthandProperties: true, + + extendDefaultReplacements: false, + replacements: { + args: { + arguments: true + }, + e: { + error: true, + event: true, + element: true + }, + err: { + error: true + }, + y: { + yield: true + }, + errCb: { + handleError: true + }, + proto: { + prototype: true + } + } +}]; + +const dontCheckVariablesOptions = [{ + checkVariables: false +}]; + +ruleTester.run('prevent-abbreviations', rule, { + valid: [ + 'let x', + '({x: 1})', + '({x() {}})', + '({[err]() {}})', + 'let error', + 'const {error} = {}', + '({error: 1})', + '({[err]: 1})', + 'error => {}', + '({error}) => ({error})', + 'httpError => {}', + '({httpError}) => ({httpError})', + '(function (error) {})', + '(function ({error}) {})', + 'function f(x) {}', + 'function f({x: y}) {}', + '(class {})', + '(class C {})', + 'class C {}', + ` + (class { + error() {} + }) + `, + ` + (class { + [err]() {} + }) + `, + ` + class C { + x() {} + } + `, + 'foo.error', + 'foo.x', + 'let E', + 'let NODE_ENV', + + // Accessing banned names is allowed (as in `camelcase` rule) + 'foo.err', + 'foo.err()', + 'foo.bar.err', + 'foo.err.bar', + 'this.err', + '({err: error}) => error', + 'const {err: httpError} = {}', + + // `err` in not defined, should not be report (could be reported by `no-unused-vars`) + 'console.log(err)', + + // `err` would be reported by `quote-props` if it's a problem for user + '({"err": 1})', + + // Testing that options apply + 'let c', + { + code: 'var c', + options: customOptions + }, + + { + code: 'class cb {}', + options: customOptions + }, + + { + code: 'function e() {}', + options: extendedOptions + }, + + { + code: '({err: 1})', + options: customOptions + }, + + { + code: 'let err', + options: dontCheckVariablesOptions + }, + + { + code: '({__proto__: null})', + options: customOptions + } + ], + + invalid: [ + noFixingTestCase({ + code: 'let e', + errors: createErrors('Please rename the variable `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'let eCbOpts', + errors: createErrors('Please rename the variable `eCbOpts`. Suggested names are: `errorCallbackOptions`, `eventCallbackOptions`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '({e: 1})', + errors: createErrors('Please rename the property `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.e = 1', + errors: createErrors('Please rename the property `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '({e() {}})', + errors: createErrors('Please rename the property `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '(class {e() {}})', + errors: createErrors('Please rename the property `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.eResDir = 1', + errors: createErrors('Please rename the property `eResDir`. Suggested names are: `errorResponseDirectory`, `errorResultDirectory`, `eventResponseDirectory`, ... (1 more omitted). A more descriptive name will do too.') + }), + + { + code: 'let err', + output: 'let error', + errors: createErrors('The variable `err` should be named `error`. A more descriptive name will do too.') + }, + { + code: 'let errCbOptsObj', + output: 'let errorCallbackOptionsObject', + errors: createErrors('The variable `errCbOptsObj` should be named `errorCallbackOptionsObject`. A more descriptive name will do too.') + }, + noFixingTestCase({ + code: '({err: 1})', + errors: createErrors('The property `err` should be named `error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.err = 1', + errors: createErrors('The property `err` should be named `error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '({err() {}})', + errors: createErrors('The property `err` should be named `error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '(class {err() {}})', + errors: createErrors('The property `err` should be named `error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.errCbOptsObj = 1', + errors: createErrors('The property `errCbOptsObj` should be named `errorCallbackOptionsObject`. A more descriptive name will do too.') + }), + + { + code: 'let successCb', + output: 'let successCallback', + errors: createErrors() + }, + { + code: 'let btnColor', + output: 'let buttonColor', + errors: createErrors() + }, + + noFixingTestCase({ + code: 'this.successCb = 1', + errors: createErrors() + }), + noFixingTestCase({ + code: 'this.btnColor = 1', + errors: createErrors() + }), + + // This tests that the rule does not hang up on combinatoric explosion of possible replacements + { + code: 'let ' + 'CbE'.repeat(1024), + errors: createErrors() + }, + + { + code: 'let evt', + errors: createErrors('The variable `evt` should be named `event`. A more descriptive name will do too.') + }, + noFixingTestCase({ + code: '({evt: 1})', + errors: createErrors('The property `evt` should be named `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'foo.evt = 1', + errors: createErrors('The property `evt` should be named `event`. A more descriptive name will do too.') + }), + + // Testing that options apply + { + code: 'let args', + output: 'let arguments', + errors: createErrors() + }, + { + code: 'let args', + output: 'let arguments', + options: extendedOptions, + errors: createErrors() + }, + { + code: 'let args', + output: 'let arguments', + options: customOptions, + errors: createErrors() + }, + + { + code: 'let c', + output: 'let custom', + options: extendedOptions, + errors: createErrors() + }, + + { + code: 'function cb() {}', + output: 'function callback() {}', + errors: createErrors() + }, + { + code: 'class cb {}', + output: 'class circuitBreacker {}', + options: extendedOptions, + errors: createErrors() + }, + + noFixingTestCase({ + code: 'let e', + errors: createErrors() + }), + noFixingTestCase({ + code: 'let e', + options: customOptions, + errors: createErrors() + }), + + { + code: 'let err', + output: 'let error', + errors: createErrors() + }, + { + code: 'let err', + output: 'let error', + options: extendedOptions, + errors: createErrors() + }, + { + code: 'let err', + output: 'let error', + options: customOptions, + errors: createErrors() + }, + + noFixingTestCase({ + code: '({err: 1})', + errors: createErrors() + }), + noFixingTestCase({ + code: '({err: 1})', + options: extendedOptions, + errors: createErrors() + }), + + { + code: 'let errCb', + output: 'let errorCallback', + errors: createErrors() + }, + { + code: 'let errCb', + output: 'let errorCircuitBreacker', + options: extendedOptions, + errors: createErrors() + }, + { + code: 'let errCb', + output: 'let handleError', + options: customOptions, + errors: createErrors() + }, + { + code: 'let ErrCb', + output: 'let HandleError', + options: customOptions, + errors: createErrors() + }, + { + code: 'let ErrCb', + output: 'let ErrorCallback', + errors: createErrors() + }, + { + code: 'let ErrCb', + output: 'let ErrorCircuitBreacker', + options: extendedOptions, + errors: createErrors() + }, + { + code: 'let ErrCb', + output: 'let HandleError', + options: customOptions, + errors: createErrors() + }, + + // `errCb` should not match this + { + code: 'let fooErrCb', + output: 'let fooErrorCb', + options: customOptions, + errors: createErrors() + }, + { + code: 'let errCbFoo', + output: 'let errorCbFoo', + options: customOptions, + errors: createErrors() + }, + + { + code: 'class Err {}', + output: 'class Error_ {}', + errors: createErrors() + }, + + noFixingTestCase({ + code: '({err: 1})', + options: dontCheckVariablesOptions, + errors: createErrors() + }), + + noFixingTestCase({ + code: ` + let e; + console.log(e); + `, + errors: createErrors() + }), + + { + code: ` + let err; + console.log(err); + `, + output: ` + let error; + console.log(error); + `, + errors: createErrors() + }, + + { + code: ` + let error; + { + let err; + } + `, + output: ` + let error; + { + let error_; + } + `, + errors: createErrors() + }, + { + code: ` + let error; + let error_; + { + let err; + } + `, + output: ` + let error; + let error_; + { + let error__; + } + `, + errors: createErrors() + }, + { + code: ` + let error; + { + let err; + console.log(error); + } + `, + output: ` + let error; + { + let error_; + console.log(error); + } + `, + errors: createErrors() + }, + { + code: ` + let error; + { + let err; + console.log(error, err); + } + `, + output: ` + let error; + { + let error_; + console.log(error, error_); + } + `, + errors: createErrors() + }, + + { + code: 'err => err', + output: 'error => error', + errors: createErrors() + }, + + { + code: ` + const opts = {}; + console.log(opts); + `, + output: ` + const options = {}; + console.log(options); + `, + errors: createErrors() + }, + { + code: ` + var opts = 1; + var opts = 2; + console.log(opts); + `, + output: ` + var options = 1; + var options = 2; + console.log(options); + `, + errors: createErrors() + }, + + { + code: ` + const err = {}; + const foo = {err}; + `, + output: ` + const error = {}; + const foo = {err: error}; + `, + errors: createErrors() + }, + { + code: ` + const err = {}; + const foo = { + a: 1, + err, + b: 2 + }; + `, + output: ` + const error = {}; + const foo = { + a: 1, + err: error, + b: 2 + }; + `, + errors: createErrors() + }, + + { + code: '({err}) => err', + output: '({err: error}) => error', + options: customOptions, + errors: createErrors() + }, + + noFixingTestCase({ + code: 'const foo = {err: 1}', + errors: createErrors() + }), + noFixingTestCase({ + code: ` + const foo = { + err() {} + }; + `, + errors: createErrors() + }), + noFixingTestCase({ + code: 'foo.err = 1', + errors: createErrors() + }), + noFixingTestCase({ + code: 'foo.bar.err = 1', + errors: createErrors() + }), + noFixingTestCase({ + code: 'this.err = 1', + errors: createErrors() + }), + + noFixingTestCase({ + code: ` + class C { + err() {} + } + `, + errors: createErrors() + }), + + noFixingTestCase({ + code: 'this._err = 1', + errors: createErrors('The property `_err` should be named `_error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.__err__ = 1', + errors: createErrors('The property `__err__` should be named `__error__`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: 'this.e_ = 1', + errors: createErrors('Please rename the property `e_`. Suggested names are: `error_`, `event_`. A more descriptive name will do too.') + }), + + { + code: 'let err_', + output: 'let error_', + errors: createErrors() + }, + { + code: 'let __err__', + output: 'let __error__', + errors: createErrors() + }, + noFixingTestCase({ + code: 'let _e', + errors: createErrors('Please rename the variable `_e`. Suggested names are: `_error`, `_event`. A more descriptive name will do too.') + }), + + { + code: 'class Err {}', + output: 'class Error_ {}', + errors: createErrors('The variable `Err` should be named `Error`. A more descriptive name will do too.') + }, + { + code: 'class Cb {}', + output: 'class Callback {}', + errors: createErrors('The variable `Cb` should be named `Callback`. A more descriptive name will do too.') + }, + noFixingTestCase({ + code: 'class Res {}', + errors: createErrors('Please rename the variable `Res`. Suggested names are: `Response`, `Result`. A more descriptive name will do too.') + }), + { + code: 'const Err = 1;', + output: 'const Error_ = 1;', + errors: createErrors() + }, + { + code: 'const _Err_ = 1;', + output: 'const _Error_ = 1;', + errors: createErrors() + }, + noFixingTestCase({ + code: '({Err: 1})', + errors: createErrors('The property `Err` should be named `Error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '({Res: 1})', + errors: createErrors('Please rename the property `Res`. Suggested names are: `Response`, `Result`. A more descriptive name will do too.') + }), + + { + code: 'let doc', + output: 'let document', + errors: createErrors() + }, + + // `package` is a reserved word in strict mode + { + code: 'let pkg', + output: 'let package', + errors: createErrors() + }, + { + code: ` + "use strict"; + let pkg; + `, + output: ` + "use strict"; + let package_; + `, + errors: createErrors() + }, + + { + code: 'let y', + output: 'let yield_', + options: customOptions, + errors: createErrors() + }, + + { + code: 'let errCb, errorCb', + output: 'let errorCallback, errorCallback_', + errors: createErrors().concat(createErrors()) + }, + { + code: '{ let errCb }; { let errorCb }', + output: '{ let errorCallback }; { let errorCallback }', + errors: createErrors().concat(createErrors()) + }, + + // The following test should have looked like this (commented one), but eslint's `RuleTester` + // does not apply all fixes at once. See https://github.com/eslint/eslint/issues/11187#issuecomment-446380824 + /* + { + code: ` + let errCb; + { + let errorCb; + console.log(errCb, errorCb); + } + `, + output: ` + let errorCallback; + { + let errorCallback_; + console.log(errorCallback, errorCallback_); + } + `, + errors: createErrors().concat(createErrors()) + }, + */ + { + code: ` + let errCb; + { + let errorCb; + console.log(errCb, errorCb); + } + `, + output: ` + let errorCallback; + { + let errorCb; + console.log(errorCallback, errorCb); + } + `, + errors: createErrors().concat(createErrors()) + }, + + { + code: ` + let err; + ({a: err = 1} = 2); + `, + output: ` + let error; + ({a: error = 1} = 2); + `, + errors: createErrors() + }, + + // Renaming to `arguments` would result in a `SyntaxError`, so it should rename to `arguments_` + { + code: ` + 'use strict'; + let args; + `, + output: ` + 'use strict'; + let arguments_; + `, + errors: createErrors() + }, + { + code: ` + class A { + method(...args) { + return super.method(...args) + 1; + } + } + `, + output: ` + class A { + method(...arguments_) { + return super.method(...arguments_) + 1; + } + } + `, + errors: createErrors() + }, + { + code: ` + class A extends B { + constructor(...args) { + super(...args); + } + } + `, + output: ` + class A extends B { + constructor(...arguments_) { + super(...arguments_); + } + } + `, + errors: createErrors() + }, + + // TODO: This could be renamed to `arguments` safely in non-strict mode, + // but it is currently impractical due to a bug in `eslint-scope`. + // https://github.com/eslint/eslint-scope/issues/49 + { + code: ` + const f = (...args) => { + return args; + } + `, + output: ` + const f = (...arguments_) => { + return arguments_; + } + `, + errors: createErrors() + }, + { + code: ` + let args; + const f = () => { + return args; + } + `, + output: ` + let arguments_; + const f = () => { + return arguments_; + } + `, + errors: createErrors() + }, + + // Renaming to `arguments` whould result in `f` returning it's arguments instead of the outer variable + { + code: ` + let args; + function f() { + return args; + } + `, + output: ` + let arguments_; + function f() { + return arguments_; + } + `, + errors: createErrors() + }, + { + code: ` + let args; + function f() { + return arguments + args; + } + `, + output: ` + let arguments_; + function f() { + return arguments + arguments_; + } + `, + errors: createErrors() + }, + { + code: ` + let args; + function f() { + return g.apply(this, arguments) + args; + } + `, + output: ` + let arguments_; + function f() { + return g.apply(this, arguments) + arguments_; + } + `, + errors: createErrors() + } + ] +}); + +browserES5RuleTester.run('prevent-abbreviations', rule, { + valid: [], + invalid: [ + { + code: 'var doc', + output: 'var document_', + errors: createErrors() + }, + { + code: ` + var doc; + document.querySelector(doc); + `, + output: ` + var document_; + document.querySelector(document_); + `, + errors: createErrors() + }, + + { + code: 'var y', + output: 'var yield', + options: customOptions, + errors: createErrors() + }, + { + code: ` + "use strict"; + var y; + `, + output: ` + "use strict"; + var yield_; + `, + options: customOptions, + errors: createErrors() + } + ] +}); + +moduleRuleTester.run('prevent-abbreviations', rule, { + valid: [ + 'import {err as foo} from "foo"', + + // Default import names are allowed + 'import err from "err"', + 'import err, {foo as bar} from "err"', + 'import {default as err, foo as bar} from "err"', + + // Namespace import names are allowed + 'import * as err from "err"', + 'import foo, * as err from "err"', + 'const err = require("err")', + + // Named import name is allowed + 'import {err} from "err"', + 'import foo, {err} from "err"', + 'import {default as foo, err} from "err"', + + // Names from object destructuring are allowed + 'const {err} = require("err")', + 'const {err} = foo', + 'function f({err}) {}' + ], + + invalid: [ + { + code: ` + import err from 'err'; + `, + output: ` + import error from 'err'; + `, + options: customOptions, + errors: createErrors() + }, + { + code: ` + import {err} from 'err'; + `, + output: ` + import {err as error} from 'err'; + `, + options: customOptions, + errors: createErrors() + }, + { + code: ` + import { + bar, + err, + buz, + } from 'foo'; + `, + output: ` + import { + bar, + err as error, + buz, + } from 'foo'; + `, + options: customOptions, + errors: createErrors() + }, + + { + code: ` + import {err as cb} from 'err'; + `, + output: ` + import {err as callback} from 'err'; + `, + errors: createErrors() + }, + { + code: ` + const {err: cb} = foo; + `, + output: ` + const {err: callback} = foo; + `, + errors: createErrors() + }, + + { + code: ` + let err; + export {err}; + `, + output: ` + let error; + export {error as err}; + `, + errors: createErrors() + }, + + noFixingTestCase({ + code: 'export const err = {}', + errors: createErrors() + }), + noFixingTestCase({ + code: 'export let err', + errors: createErrors() + }), + noFixingTestCase({ + code: 'export var err', + errors: createErrors() + }), + noFixingTestCase({ + code: 'export function err() {}', + errors: createErrors() + }), + noFixingTestCase({ + code: 'export class err {}', + errors: createErrors() + }), + + { + code: ` + const err = {}; + export const error = err; + `, + output: ` + const error_ = {}; + export const error = error_; + `, + errors: createErrors() + }, + + { + code: ` + class err {}; + console.log(err); + `, + output: ` + class error {}; + console.log(error); + `, + errors: createErrors() + }, + + { + code: ` + class err { + constructor() { + console.log(err); + } + }; + console.log(err); + `, + output: ` + class error { + constructor() { + console.log(error); + } + }; + console.log(error); + `, + errors: createErrors() + }, + + noFixingTestCase({ + code: ` + let foo; + export {foo as err}; + `, + errors: createErrors() + }) + ] +}); + +babelRuleTester.run('prevent-abbreviations', rule, { + valid: [ + // Whitelisted names + 'Foo.defaultProps = {}', + `class Foo { + static propTypes = {}; + static getDerivedStateFromProps() {} + }` + ], + + invalid: [ + { + code: 'Foo.customProps = {}', + errors: createErrors() + }, + { + code: ` + class Foo { + static propTypesAndStuff = {}; + } + `, + errors: createErrors() + }, + { + code: ` + class Foo { + static getDerivedContextFromProps() {} + } + `, + errors: createErrors() + }, + + noFixingTestCase({ + code: '(class {e = 1})', + errors: createErrors('Please rename the property `e`. Suggested names are: `error`, `event`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: '(class {err = 1})', + errors: createErrors('The property `err` should be named `error`. A more descriptive name will do too.') + }), + noFixingTestCase({ + code: ` + class C { + err = () => {} + } + `, + errors: createErrors() + }) + ] +});