diff --git a/docs/rules/prefer-default-parameters.md b/docs/rules/prefer-default-parameters.md new file mode 100644 index 0000000000..7d89da6bf5 --- /dev/null +++ b/docs/rules/prefer-default-parameters.md @@ -0,0 +1,31 @@ +# Prefer default parameters over reassignment + +Instead of reassigning a function parameter, default parameters should be used. The `foo = foo || 123` statement evaluates to `123` when `foo` is falsy, possibly leading to confusing behavior, whereas default parameters only apply when passed an `undefined` value. This rule only reports reassignments to literal values. + +This rule is fixable. + +## Fail + +```js +function abc(foo) { + foo = foo || 'bar'; +} +``` + +```js +function abc(foo) { + const bar = foo || 'bar'; +} +``` + +## Pass + +```js +function abc(foo = 'bar') {} +``` + +```js +function abc(foo) { + foo = foo || bar(); +} +``` diff --git a/index.js b/index.js index 9c82f03253..f17af92ef5 100644 --- a/index.js +++ b/index.js @@ -57,6 +57,7 @@ module.exports = { 'unicorn/prefer-array-find': 'error', 'unicorn/prefer-dataset': 'error', 'unicorn/prefer-date-now': 'error', + 'unicorn/prefer-default-parameters': 'error', 'unicorn/prefer-event-key': 'error', // TODO: Enable this by default when targeting Node.js 12. 'unicorn/prefer-flat-map': 'off', diff --git a/readme.md b/readme.md index 5fcd7957d5..933096126a 100644 --- a/readme.md +++ b/readme.md @@ -72,6 +72,7 @@ Configure it in `package.json`. "unicorn/prefer-array-find": "error", "unicorn/prefer-dataset": "error", "unicorn/prefer-date-now": "error", + "unicorn/prefer-default-parameters": "error", "unicorn/prefer-event-key": "error", "unicorn/prefer-flat-map": "error", "unicorn/prefer-includes": "error", @@ -141,6 +142,7 @@ Configure it in `package.json`. - [prefer-array-find](docs/rules/prefer-array-find.md) - Prefer `.find(…)` over the first element from `.filter(…)`. *(partly fixable)* - [prefer-dataset](docs/rules/prefer-dataset.md) - Prefer using `.dataset` on DOM elements over `.setAttribute(…)`. *(fixable)* - [prefer-date-now](docs/rules/prefer-date-now.md) - Prefer `Date.now()` to get the number of milliseconds since the Unix Epoch. *(fixable)* +- [prefer-default-parameters](docs/rules/prefer-default-parameters.md) - Prefer default parameters over reassignment. *(fixable)* - [prefer-event-key](docs/rules/prefer-event-key.md) - Prefer `KeyboardEvent#key` over `KeyboardEvent#keyCode`. *(partly fixable)* - [prefer-flat-map](docs/rules/prefer-flat-map.md) - Prefer `.flatMap(…)` over `.map(…).flat()`. *(fixable)* - [prefer-includes](docs/rules/prefer-includes.md) - Prefer `.includes()` over `.indexOf()` when checking for existence or non-existence. *(fixable)* diff --git a/rules/prefer-default-parameters.js b/rules/prefer-default-parameters.js new file mode 100644 index 0000000000..165d003555 --- /dev/null +++ b/rules/prefer-default-parameters.js @@ -0,0 +1,213 @@ +'use strict'; +const {findVariable} = require('eslint-utils'); +const getDocumentationUrl = require('./utils/get-documentation-url'); + +const MESSAGE_ID = 'preferDefaultParameters'; +const MESSAGE_ID_SUGGEST = 'preferDefaultParametersSuggest'; + +const assignmentSelector = [ + 'ExpressionStatement', + '[expression.type="AssignmentExpression"]' +].join(''); + +const declarationSelector = [ + 'VariableDeclaration', + '[declarations.0.type="VariableDeclarator"]' +].join(''); + +const isDefaultExpression = (left, right) => + left && + right && + left.type === 'Identifier' && + right.type === 'LogicalExpression' && + (right.operator === '||' || right.operator === '??') && + right.left.type === 'Identifier' && + right.right.type === 'Literal'; + +const containsCallExpression = (source, node) => { + if (!node) { + return false; + } + + if (node.type === 'CallExpression') { + return true; + } + + for (const key of source.visitorKeys[node.type]) { + const value = node[key]; + + if (Array.isArray(value)) { + for (const element of value) { + if (containsCallExpression(source, element)) { + return true; + } + } + } else if (containsCallExpression(source, value)) { + return true; + } + } + + return false; +}; + +const hasSideEffects = (source, function_, node) => { + for (const element of function_.body.body) { + if (element === node) { + break; + } + + // Function call before default-assignment + if (containsCallExpression(source, element)) { + return true; + } + } + + return false; +}; + +const hasExtraReferences = (assignment, references, left) => { + // Parameter is referenced prior to default-assignment + if (assignment && references[0].identifier !== left) { + return true; + } + + // Old parameter is still referenced somewhere else + if (!assignment && references.length > 1) { + return true; + } + + return false; +}; + +const isLastParameter = (parameters, parameter) => { + const lastParameter = parameters[parameters.length - 1]; + + // See 'default-param-last' rule + return parameter && parameter === lastParameter; +}; + +const needsParentheses = (source, function_) => { + if (function_.type !== 'ArrowFunctionExpression' || function_.params.length > 1) { + return false; + } + + const [parameter] = function_.params; + const before = source.getTokenBefore(parameter); + const after = source.getTokenAfter(parameter); + + return !after || !before || before.value !== '(' || after.value !== ')'; +}; + +const fixDefaultExpression = (fixer, source, node) => { + const {line} = source.getLocFromIndex(node.range[0]); + const {column} = source.getLocFromIndex(node.range[1]); + const nodeText = source.getText(node); + const lineText = source.lines[line - 1]; + const isOnlyNodeOnLine = lineText.trim() === nodeText; + const endsWithWhitespace = lineText[column] === ' '; + + if (isOnlyNodeOnLine) { + return fixer.removeRange([ + source.getIndexFromLoc({line, column: 0}), + source.getIndexFromLoc({line: line + 1, column: 0}) + ]); + } + + if (endsWithWhitespace) { + return fixer.removeRange([ + node.range[0], + node.range[1] + 1 + ]); + } + + return fixer.removeRange(node.range); +}; + +const create = context => { + const source = context.getSourceCode(); + const functionStack = []; + + const checkExpression = (node, left, right, assignment) => { + const currentFunction = functionStack[functionStack.length - 1]; + + if (!currentFunction || !isDefaultExpression(left, right)) { + return; + } + + const {name: firstId} = left; + const { + left: {name: secondId}, + right: {raw: literal} + } = right; + + // Parameter is reassigned to a different identifier + if (assignment && firstId !== secondId) { + return; + } + + const {references} = findVariable(context.getScope(), secondId); + const {params} = currentFunction; + const parameter = params.find(parameter => + parameter.type === 'Identifier' && + parameter.name === secondId + ); + + if ( + hasSideEffects(source, currentFunction, node) || + hasExtraReferences(assignment, references, left) || + !isLastParameter(params, parameter) + ) { + return; + } + + const replacement = needsParentheses(source, currentFunction) ? + `(${firstId} = ${literal})` : + `${firstId} = ${literal}`; + + context.report({ + node, + messageId: MESSAGE_ID, + suggest: [{ + messageId: MESSAGE_ID_SUGGEST, + fix: fixer => [ + fixer.replaceText(parameter, replacement), + fixDefaultExpression(fixer, source, node) + ] + }] + }); + }; + + return { + ':function': node => { + functionStack.push(node); + }, + ':function:exit': () => { + functionStack.pop(); + }, + [assignmentSelector]: node => { + const {left, right} = node.expression; + + checkExpression(node, left, right, true); + }, + [declarationSelector]: node => { + const {id, init} = node.declarations[0]; + + checkExpression(node, id, init, false); + } + }; +}; + +module.exports = { + create, + meta: { + type: 'suggestion', + docs: { + url: getDocumentationUrl(__filename) + }, + fixable: 'code', + messages: { + [MESSAGE_ID]: 'Prefer default parameters over reassignment.', + [MESSAGE_ID_SUGGEST]: 'Replace reassignment with default parameter.' + } + } +}; diff --git a/test/prefer-default-parameters.js b/test/prefer-default-parameters.js new file mode 100644 index 0000000000..8f7ddb13e9 --- /dev/null +++ b/test/prefer-default-parameters.js @@ -0,0 +1,657 @@ +import test from 'ava'; +import avaRuleTester from 'eslint-ava-rule-tester'; +import {outdent} from 'outdent'; +import rule from '../rules/prefer-default-parameters'; + +const ruleTester = avaRuleTester(test, { + parserOptions: { + ecmaVersion: 2020 + } +}); + +const invalidTestCase = ({code, suggestions}) => { + if (!suggestions) { + return { + code, + output: code, + errors: [{ + messageId: 'preferDefaultParameters' + }] + }; + } + + return { + code, + output: code, + errors: suggestions.map(suggestion => ({ + messageId: 'preferDefaultParameters', + suggestions: [{ + messageId: 'preferDefaultParametersSuggest', + output: suggestion + }] + })) + }; +}; + +ruleTester.run('prefer-default-parameters', rule, { + valid: [ + 'function abc(foo = { bar: 123 }) { }', + 'function abc({ bar } = { bar: 123 }) { }', + 'function abc({ bar = 123 } = { bar }) { }', + 'function abc(foo = fooDefault) { }', + 'function abc(foo = {}) { }', + 'function abc(foo = \'bar\') { }', + 'function abc({ bar = 123 } = {}) { }', + 'const abc = (foo = \'bar\') => { };', + 'foo = foo || \'bar\';', + 'const bar = foo || \'bar\';', + 'const abc = function(foo = { bar: 123 }) { }', + 'const abc = function({ bar } = { bar: 123 }) { }', + 'const abc = function({ bar = 123 } = {}) { }', + outdent` + function abc(foo) { + foo = foo || bar(); + } + `, + outdent` + function abc(foo) { + foo = foo || {bar}; + } + `, + outdent` + function abc(foo) { + const {bar} = foo || 123; + } + `, + outdent` + function abc(foo, bar) { + bar = foo || 'bar'; + } + `, + outdent` + function abc(foo, bar) { + foo = foo || 'bar'; + baz(); + } + `, + outdent` + function abc(foo) { + foo = foo && 'bar'; + } + `, + outdent` + function abc(foo) { + foo = foo || 1 && 2 || 3; + } + `, + outdent` + function abc(foo) { + foo = !foo || 'bar'; + } + `, + outdent` + function abc(foo) { + foo = (foo && bar) || baz; + } + `, + outdent` + function abc(foo = 123) { + foo = foo || 'bar'; + } + `, + outdent` + function abc() { + let foo = 123; + foo = foo || 'bar'; + } + `, + outdent` + function abc() { + let foo = 123; + const bar = foo || 'bar'; + } + `, + outdent` + const abc = (foo, bar) => { + bar = foo || 'bar'; + }; + `, + outdent` + const abc = function(foo, bar) { + bar = foo || 'bar'; + } + `, + outdent` + const abc = function(foo) { + foo = foo || bar(); + } + `, + outdent` + function abc(foo) { + function def(bar) { + foo = foo || 'bar'; + } + } + `, + outdent` + function abc(foo) { + const bar = foo = foo || 123; + } + `, + outdent` + function abc(foo) { + bar(foo = foo || 1); + baz(foo); + } + `, + // The following tests check references and side effects + outdent` + function abc(foo) { + console.log(foo); + foo = foo || 123; + } + `, + outdent` + function abc(foo) { + console.log(foo); + foo = foo || 'bar'; + } + `, + outdent` + function abc(foo) { + const bar = foo || 'bar'; + console.log(foo, bar); + } + `, + outdent` + function abc(foo) { + let bar = 123; + bar = foo; + foo = foo || 123; + } + `, + outdent` + function abc(foo) { + bar(); + foo = foo || 123; + } + `, + outdent` + const abc = (foo) => { + bar(); + foo = foo || 123; + }; + `, + outdent` + const abc = function(foo) { + bar(); + foo = foo || 123; + }; + `, + outdent` + function abc(foo) { + sideEffects(); + foo = foo || 123; + + function sideEffects() { + foo = 456; + } + } + `, + outdent` + function abc(foo) { + const bar = sideEffects(); + foo = foo || 123; + + function sideEffects() { + foo = 456; + } + } + `, + outdent` + function abc(foo) { + const bar = sideEffects() + 123; + foo = foo || 123; + + function sideEffects() { + foo = 456; + } + } + `, + outdent` + function abc(foo) { + const bar = !sideEffects(); + foo = foo || 123; + + function sideEffects() { + foo = 456; + } + } + `, + outdent` + function abc(foo) { + const bar = function() { + foo = 456; + } + foo = foo || 123; + } + `, + // Last parameter is `RestElement` + outdent` + function abc(...foo) { + foo = foo || 'bar'; + } + `, + // Last parameter is `AssignmentPattern` + outdent` + function abc(foo = 'bar') { + foo = foo || 'baz'; + } + ` + ], + invalid: [ + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || 123; + } + `, + suggestions: [outdent` + function abc(foo = 123) { + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || true; + } + `, + suggestions: [outdent` + function abc(foo = true) { + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || 123; + console.log(foo); + } + `, + suggestions: [outdent` + function abc(foo = 123) { + console.log(foo); + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + const bar = foo || 'bar'; + } + `, + suggestions: [outdent` + function abc(bar = 'bar') { + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + let bar = foo || 'bar'; + } + `, + suggestions: [outdent` + function abc(bar = 'bar') { + } + `] + }), + invalidTestCase({ + code: outdent` + const abc = function(foo) { + foo = foo || 123; + } + `, + suggestions: [outdent` + const abc = function(foo = 123) { + } + `] + }), + invalidTestCase({ + code: outdent` + const abc = (foo) => { + foo = foo || 'bar'; + }; + `, + suggestions: [outdent` + const abc = (foo = 'bar') => { + }; + `] + }), + invalidTestCase({ + code: outdent` + const abc = foo => { + foo = foo || 'bar'; + }; + `, + suggestions: [outdent` + const abc = (foo = 'bar') => { + }; + `] + }), + invalidTestCase({ + code: outdent` + const abc = (foo) => { + const bar = foo || 'bar'; + }; + `, + suggestions: [outdent` + const abc = (bar = 'bar') => { + }; + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || 'bar'; + bar(); + baz(); + } + `, + suggestions: [outdent` + function abc(foo = 'bar') { + bar(); + baz(); + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo ?? 123; + } + `, + suggestions: [outdent` + function abc(foo = 123) { + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + const bar = foo || 'bar'; + console.log(bar); + } + `, + suggestions: [outdent` + function abc(bar = 'bar') { + console.log(bar); + } + `] + }), + invalidTestCase({ + code: outdent` + const abc = function(foo) { + const bar = foo || 'bar'; + console.log(bar); + } + `, + suggestions: [outdent` + const abc = function(bar = 'bar') { + console.log(bar); + } + `] + }), + invalidTestCase({ + code: outdent` + foo = { + abc(foo) { + foo = foo || 123; + } + }; + `, + suggestions: [outdent` + foo = { + abc(foo = 123) { + } + }; + `] + }), + invalidTestCase({ + code: outdent` + foo = { + abc(foo) { + foo = foo || 123; + }, + def(foo) { } + }; + `, + suggestions: [outdent` + foo = { + abc(foo = 123) { + }, + def(foo) { } + }; + `] + }), + invalidTestCase({ + code: outdent` + class Foo { + abc(foo) { + foo = foo || 123; + } + } + `, + suggestions: [outdent` + class Foo { + abc(foo = 123) { + } + } + `] + }), + invalidTestCase({ + code: outdent` + class Foo { + abc(foo) { + foo = foo || 123; + } + def(foo) { } + } + `, + suggestions: [outdent` + class Foo { + abc(foo = 123) { + } + def(foo) { } + } + `] + }), + // The following tests verify the correct code formatting + invalidTestCase({ + code: 'function abc(foo) { foo = foo || \'bar\'; }', + suggestions: ['function abc(foo = \'bar\') { }'] + }), + invalidTestCase({ + code: 'function abc(foo) { foo = foo || \'bar\';}', + suggestions: ['function abc(foo = \'bar\') { }'] + }), + invalidTestCase({ + code: 'const abc = function(foo) { foo = foo || \'bar\';}', + suggestions: ['const abc = function(foo = \'bar\') { }'] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || 'bar'; bar(); baz(); + } + `, + suggestions: [outdent` + function abc(foo = 'bar') { + bar(); baz(); + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo = foo || 'bar'; + function def(bar) { + bar = bar || 'foo'; + } + } + `, + suggestions: [outdent` + function abc(foo = 'bar') { + function def(bar) { + bar = bar || 'foo'; + } + } + `, outdent` + function abc(foo) { + foo = foo || 'bar'; + function def(bar = 'foo') { + } + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + foo += 'bar'; + function def(bar) { + bar = bar || 'foo'; + } + function ghi(baz) { + const bay = baz || 'bar'; + } + foo = foo || 'bar'; + } + `, + suggestions: [outdent` + function abc(foo) { + foo += 'bar'; + function def(bar = 'foo') { + } + function ghi(baz) { + const bay = baz || 'bar'; + } + foo = foo || 'bar'; + } + `, outdent` + function abc(foo) { + foo += 'bar'; + function def(bar) { + bar = bar || 'foo'; + } + function ghi(bay = 'bar') { + } + foo = foo || 'bar'; + } + `] + }), + invalidTestCase({ + code: outdent` + foo = { + abc(foo) { + foo = foo || 123; + }, + def(foo) { + foo = foo || 123; + } + }; + `, + suggestions: [outdent` + foo = { + abc(foo = 123) { + }, + def(foo) { + foo = foo || 123; + } + }; + `, outdent` + foo = { + abc(foo) { + foo = foo || 123; + }, + def(foo = 123) { + } + }; + `] + }), + invalidTestCase({ + code: outdent` + class Foo { + abc(foo) { + foo = foo || 123; + } + def(foo) { + foo = foo || 123; + } + } + `, + suggestions: [outdent` + class Foo { + abc(foo = 123) { + } + def(foo) { + foo = foo || 123; + } + } + `, outdent` + class Foo { + abc(foo) { + foo = foo || 123; + } + def(foo = 123) { + } + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + const noSideEffects = 123; + foo = foo || 123; + } + `, + suggestions: [outdent` + function abc(foo = 123) { + const noSideEffects = 123; + } + `] + }), + invalidTestCase({ + code: outdent` + const abc = function(foo) { + let bar = true; + bar = false; + + foo = foo || 123; + console.log(foo); + } + `, + suggestions: [outdent` + const abc = function(foo = 123) { + let bar = true; + bar = false; + + console.log(foo); + } + `] + }), + invalidTestCase({ + code: outdent` + function abc(foo) { + const bar = function() {}; + foo = foo || 123; + } + `, + suggestions: [outdent` + function abc(foo = 123) { + const bar = function() {}; + } + `] + }) + ] +});