Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prefer-destructuring-in-parameters rule #1045

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b9f5126
Add `prefer-destructuring-in-parameters` rule
fisker Jan 21, 2021
c1f16d5
Check effects
fisker Jan 21, 2021
ff01e56
Style
fisker Jan 21, 2021
d36b112
Refactor
fisker Jan 21, 2021
db2d17a
More tests
fisker Jan 21, 2021
c01b302
Add another case
fisker Jan 21, 2021
1a029a5
Add test
fisker Jan 21, 2021
7801f77
Add more tests
fisker Jan 21, 2021
72947d5
Fix parentheses
fisker Jan 21, 2021
86a33f4
ignore unreachable
fisker Jan 21, 2021
e778758
More tests
fisker Jan 21, 2021
8a4bc33
test `constructor` and `setter`
fisker Jan 21, 2021
e87ae3d
Fix functions has `"use strict"`
fisker Jan 21, 2021
a8ed118
Fix `delete bar.x`
fisker Jan 21, 2021
22cc069
One more test
fisker Jan 21, 2021
4882743
Improve coverage
fisker Jan 21, 2021
2972162
Test private fields
fisker Jan 21, 2021
d8f8ab5
Fix crash on typescript parser
fisker Jan 21, 2021
c5c62b1
Simplify
fisker Jan 21, 2021
14cad62
Test curried
fisker Jan 21, 2021
11b6d23
Add docs
fisker Jan 21, 2021
6921209
Fix async function
fisker Jan 21, 2021
66ad0af
Style
fisker Jan 21, 2021
76fcf7f
Minor refactor
fisker Jan 21, 2021
3b879a0
Rename
fisker Jan 21, 2021
51ab3bb
Exclude `NewExpression` from `isNodeEffectThis`
fisker Jan 21, 2021
24f7d0b
Fix `hasParenthesesAroundParametersList` function
fisker Jan 21, 2021
4cd6d05
Add comment
fisker Jan 21, 2021
43669b0
Turn off rule on `run-rules-on-codebase` job
fisker Jan 21, 2021
37e24d1
Fix crash on typescript projects
fisker Jan 21, 2021
c7709e7
Extend fix range
fisker Jan 21, 2021
58e9d59
Update has-parentheses-around-parameters-list.js
sindresorhus Jan 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions docs/rules/prefer-destructuring-in-parameters.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Prefer destructuring in parameters over accessing properties

Makes your code shorter and nicer.

This rule is fixable.
fisker marked this conversation as resolved.
Show resolved Hide resolved

## Fail

```js
const getObjectProperty = object => object.property;
```

```js
const removeEmptyValues = object => Object.fromEntries(
Object.entries(object).filter(keyValuePair => Boolean(keyValuePair[1]))
);
```

## Pass

```js
const getFoo = ({property}) => property;
```

```js
const removeEmptyValues = object => Object.fromEntries(
Object.entries(object).filter(([, value]) => Boolean(value))
);
```

```js
// Used property and index together
function foo(array) {
if (array.length > 0) {
return bar(array[0]);
}
}
```
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ module.exports = {
'unicorn/prefer-array-some': 'error',
'unicorn/prefer-date-now': 'error',
'unicorn/prefer-default-parameters': 'error',
'unicorn/prefer-destructuring-in-parameters': 'error',
'unicorn/prefer-dom-node-append': 'error',
'unicorn/prefer-dom-node-dataset': 'error',
'unicorn/prefer-dom-node-remove': 'error',
Expand Down
2 changes: 2 additions & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ Configure it in `package.json`.
"unicorn/prefer-array-some": "error",
"unicorn/prefer-date-now": "error",
"unicorn/prefer-default-parameters": "error",
"unicorn/prefer-destructuring-in-parameters": "error",
"unicorn/prefer-dom-node-append": "error",
"unicorn/prefer-dom-node-dataset": "error",
"unicorn/prefer-dom-node-remove": "error",
Expand Down Expand Up @@ -158,6 +159,7 @@ Configure it in `package.json`.
- [prefer-array-some](docs/rules/prefer-array-some.md) - Prefer `.some(…)` over `.find(…)`.
- [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-destructuring-in-parameters](docs/rules/prefer-destructuring-in-parameters.md) - Prefer destructuring in parameters over accessing properties. *(fixable)*
- [prefer-dom-node-append](docs/rules/prefer-dom-node-append.md) - Prefer `Node#append()` over `Node#appendChild()`. *(fixable)*
- [prefer-dom-node-dataset](docs/rules/prefer-dom-node-dataset.md) - Prefer using `.dataset` on DOM elements over `.setAttribute(…)`. *(fixable)*
- [prefer-dom-node-remove](docs/rules/prefer-dom-node-remove.md) - Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`. *(fixable)*
Expand Down
270 changes: 270 additions & 0 deletions rules/prefer-destructuring-in-parameters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,270 @@
'use strict';
const {upperFirst} = require('lodash');
const {findVariable} = require('eslint-utils');
const getDocumentationUrl = require('./utils/get-documentation-url');
const avoidCapture = require('./utils/avoid-capture');
const hasParenthesesAroundParametersList = require('./utils/has-parentheses-around-parameters-list');
const extendFixRange = require('./utils/extend-fix-range');

const MESSAGE_ID = 'prefer-destructuring-in-parameters';
const messages = {
[MESSAGE_ID]: '`{{member}}` should be destructed in parameter `{{parameter}}`.'
};

const indexVariableNamePrefixes = ['first', 'second'];

function isNodeEffectThis(node) {
const {parent} = node;

/* istanbul ignore next: Not sure if this is needed */
if (
parent.type === 'ChainExpression' &&
parent.expression === node
) {
return isNodeEffectThis(parent);
}

if (
parent.type === 'CallExpression' &&
parent.callee === node
) {
return true;
}

return false;
}

function isModifyingNode(node) {
const {parent} = node;

if (
parent.type === 'AssignmentExpression' &&
parent.left === node
) {
return true;
}

if (
parent.type === 'UpdateExpression' &&
parent.argument === node
) {
return true;
}

if (
parent.type === 'UnaryExpression' &&
parent.operator === 'delete' &&
parent.argument === node
) {
return true;
}

return false;
}

function getMemberExpression(node) {
const memberExpression = node.parent;
if (
memberExpression.type !== 'MemberExpression' ||
isNodeEffectThis(memberExpression) ||
isModifyingNode(memberExpression)
) {
return;
}

const {computed, optional, object, property} = memberExpression;

if (optional || object !== node) {
return;
}

const data = {};
if (computed) {
if (property.type !== 'Literal') {
return;
}

const index = property.value;
if (
typeof index !== 'number' ||
!Number.isInteger(index) ||
!(index >= 0 && index < indexVariableNamePrefixes.length)
) {
return;
}

data.type = 'index';
data.index = index;
} else {
if (property.type !== 'Identifier') {
return;
}

data.type = 'property';
data.property = property.name;
}

data.node = memberExpression;
return data;
}

function fix({sourceCode, functionNode, parameter, properties, type}) {
function * fixArrowFunctionParentheses(fixer) {
if (!hasParenthesesAroundParametersList(functionNode, sourceCode)) {
yield fixer.insertTextBefore(parameter, '(');
yield fixer.insertTextAfter(parameter, ')');
}
}

function fixParameter(fixer) {
const variables = [];
for (const [indexOrProperty, {variable}] of properties.entries()) {
if (type === 'index') {
variables[indexOrProperty] = variable;
} else {
variables.push(variable);
}
}

const text = variables.join(', ');

return fixer.replaceText(parameter, type === 'index' ? `[${text}]` : `{${text}}`);
}

return function * (fixer) {
yield * fixArrowFunctionParentheses(fixer);
yield fixParameter(fixer);

for (const {variable, memberExpressions} of properties.values()) {
for (const node of memberExpressions) {
yield fixer.replaceText(node, variable);
}
}

// Prevent possible conflicts
yield * extendFixRange(fixer, functionNode.range);
};
}

function hasDirectiveInFunction(functionNode) {
const {body} = functionNode;
if (body.type !== 'BlockStatement') {
return false;
}

return body.body.some(({directive}) => directive === 'use strict');
}

const create = context => {
const {ecmaVersion} = context.parserOptions;
const sourceCode = context.getSourceCode();
return {
':function > Identifier.params'(parameter) {
const {name, parent: functionNode} = parameter;

// If "use strict" directive used, it should not reported
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Strict_Non_Simple_Params
if (hasDirectiveInFunction(functionNode)) {
return;
}

const scope = context.getScope();
const variable = findVariable(scope, parameter);

/* istanbul ignore next: This should not happen, but integration test fails on some typescript project, and can't reproduce in tests */
if (!variable) {
return;
}

const identifiers = variable.references.map(({identifier}) => identifier);

const properties = new Map();
let propertyType;
let firstExpression;
for (const identifier of identifiers) {
const memberExpression = getMemberExpression(identifier);
if (!memberExpression) {
return;
}

const {node, type} = memberExpression;
if (propertyType) {
// Avoid case like `foo[0] === foo.length`
if (type !== propertyType) {
return;
}
} else {
propertyType = type;
}

if (
!firstExpression ||
node.range[0] < firstExpression.node.range[0]
) {
firstExpression = memberExpression;
}

const indexOrProperty = memberExpression[type];
if (properties.has(indexOrProperty)) {
properties.get(indexOrProperty).memberExpressions.push(node);
} else {
properties.set(indexOrProperty, {memberExpressions: [node]});
}
}

if (properties.size === 0) {
return;
}

const scopes = [
variable.scope,
...variable.references.map(({from}) => from)
];
for (const [indexOrProperty, data] of properties.entries()) {
let variableName;
if (propertyType === 'index') {
variableName = avoidCapture(
`${indexVariableNamePrefixes[indexOrProperty]}ElementOf${upperFirst(name)}`,
scopes,
ecmaVersion
);
} else {
variableName = avoidCapture(indexOrProperty, scopes, ecmaVersion);
if (variableName !== indexOrProperty) {
return;
}
}

data.variable = variableName;
}

context.report({
node: firstExpression.node,
messageId: MESSAGE_ID,
data: {
member: `${name}${propertyType === 'index' ? `[${firstExpression.index}]` : `.${firstExpression.property}`}`,
parameter: name
},
fix: fix({
sourceCode,
functionNode,
parameter,
properties,
type: propertyType
})
});
}
};
};

module.exports = {
fisker marked this conversation as resolved.
Show resolved Hide resolved
create,
meta: {
type: 'suggestion',
docs: {
url: getDocumentationUrl(__filename)
},
fixable: 'code',
messages
}
};
27 changes: 27 additions & 0 deletions rules/utils/has-parentheses-around-parameters-list.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';
const {isOpeningParenToken} = require('eslint-utils');

/**
Check if a function has parentheses around its parameter list.

@param {Node} node - The AST node to check.
@param {SourceCode} sourceCode - The source code object.
@returns {boolean}
*/
function hasParenthesesAroundParametersList(node, sourceCode) {
if (
node.type !== 'ArrowFunctionExpression' ||
node.params.length !== 1
) {
return true;
}

const [onlyArgument] = node.params;
const tokenBefore = sourceCode.getTokenBefore(onlyArgument);
return tokenBefore &&
// `(` may not belong to the function. For example: `array.map(x => x)`
tokenBefore.range[0] >= node.range[0] &&
isOpeningParenToken(tokenBefore);
}

module.exports = hasParenthesesAroundParametersList;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foo.forEach(btn => console.log(btn.name))
Loading