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

Update: Fix accessor-pairs to enforce pairs per property in literals #12062

Merged
merged 3 commits into from Aug 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/rules/accessor-pairs.md
Expand Up @@ -143,6 +143,27 @@ Object.defineProperty(o, 'c', {

```

## Known Limitations

Due to the limits of static analysis, this rule does not account for possible side effects and in certain cases
might not report a missing pair for a getter/setter that has a computed key, like in the following example:

```js
/*eslint accessor-pairs: "error"*/

var a = 1;

// no warnings
var o = {
get [a++]() {
return this.val;
},
set [a++](value) {
this.val = value;
}
};
```

## When Not To Use It

You can turn this rule off if you are not concerned with the simultaneous presence of setters and getters on objects.
Expand Down
230 changes: 195 additions & 35 deletions lib/rules/accessor-pairs.js
Expand Up @@ -5,10 +5,87 @@

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Typedefs
//------------------------------------------------------------------------------

/**
* Property name if it can be computed statically, otherwise the list of the tokens of the key node.
* @typedef {string|Token[]} Key
*/

/**
* Accessor nodes with the same key.
* @typedef {Object} AccessorData
* @property {Key} key Accessor's key
* @property {ASTNode[]} getters List of getter nodes.
* @property {ASTNode[]} setters List of setter nodes.
*/

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

/**
* Checks whether or not the given lists represent the equal tokens in the same order.
* Tokens are compared by their properties, not by instance.
* @param {Token[]} left First list of tokens.
* @param {Token[]} right Second list of tokens.
* @returns {boolean} `true` if the lists have same tokens.
*/
function areEqualTokenLists(left, right) {
if (left.length !== right.length) {
return false;
}

for (let i = 0; i < left.length; i++) {
const leftToken = left[i],
rightToken = right[i];

if (leftToken.type !== rightToken.type || leftToken.value !== rightToken.value) {
return false;
}
}

return true;
}

/**
* Checks whether or not the given keys are equal.
* @param {Key} left First key.
* @param {Key} right Second key.
* @returns {boolean} `true` if the keys are equal.
*/
function areEqualKeys(left, right) {
if (typeof left === "string" && typeof right === "string") {

// Statically computed names.
return left === right;
}
if (Array.isArray(left) && Array.isArray(right)) {

// Token lists.
return areEqualTokenLists(left, right);
}

return false;
}

/**
* Checks whether or not a given node is of an accessor kind ('get' or 'set').
* @param {ASTNode} node - A node to check.
* @returns {boolean} `true` if the node is of an accessor kind.
*/
function isAccessorKind(node) {
return node.kind === "get" || node.kind === "set";
}

/**
* Checks whether or not a given node is an `Identifier` node which was named a given name.
* @param {ASTNode} node - A node to check.
Expand Down Expand Up @@ -97,69 +174,152 @@ module.exports = {
}],

messages: {
getter: "Getter is not present.",
setter: "Setter is not present."
missingGetterInPropertyDescriptor: "Getter is not present in property descriptor.",
missingSetterInPropertyDescriptor: "Setter is not present in property descriptor.",
missingGetterInObjectLiteral: "Getter is not present for {{ name }}.",
missingSetterInObjectLiteral: "Setter is not present for {{ name }}."
}
},
create(context) {
const config = context.options[0] || {};
const checkGetWithoutSet = config.getWithoutSet === true;
const checkSetWithoutGet = config.setWithoutGet !== false;
const sourceCode = context.getSourceCode();

/**
* Checks a object expression to see if it has setter and getter both present or none.
* @param {ASTNode} node The node to check.
* Reports the given node.
* @param {ASTNode} node The node to report.
* @param {string} messageKind "missingGetter" or "missingSetter".
* @returns {void}
* @private
*/
function checkLonelySetGet(node) {
let isSetPresent = false;
let isGetPresent = false;
const isDescriptor = isPropertyDescriptor(node);
function report(node, messageKind) {
if (node.type === "Property") {
context.report({
node,
messageId: `${messageKind}InObjectLiteral`,
loc: astUtils.getFunctionHeadLoc(node.value, sourceCode),
data: { name: astUtils.getFunctionNameWithKind(node.value) }
});
} else {
context.report({
node,
messageId: `${messageKind}InPropertyDescriptor`
});
}
}

for (let i = 0, end = node.properties.length; i < end; i++) {
const property = node.properties[i];
/**
* Reports each of the nodes in the given list using the same messageId.
* @param {ASTNode[]} nodes Nodes to report.
* @param {string} messageKind "missingGetter" or "missingSetter".
* @returns {void}
* @private
*/
function reportList(nodes, messageKind) {
for (const node of nodes) {
report(node, messageKind);
}
}

let propToCheck = "";
/**
* Creates a new `AccessorData` object for the given getter or setter node.
* @param {ASTNode} node A getter or setter node.
* @returns {AccessorData} New `AccessorData` object that contains the given node.
* @private
*/
function createAccessorData(node) {
const name = astUtils.getStaticPropertyName(node);
const key = (name !== null) ? name : sourceCode.getTokens(node.key);

if (property.kind === "init") {
if (isDescriptor && !property.computed) {
propToCheck = property.key.name;
}
} else {
propToCheck = property.kind;
}
return {
key,
getters: node.kind === "get" ? [node] : [],
setters: node.kind === "set" ? [node] : []
};
}

switch (propToCheck) {
case "set":
isSetPresent = true;
break;
/**
* Merges the given `AccessorData` object into the given accessors list.
* @param {AccessorData[]} accessors The list to merge into.
* @param {AccessorData} accessorData The object to merge.
* @returns {AccessorData[]} The same instance with the merged object.
* @private
*/
function mergeAccessorData(accessors, accessorData) {
const equalKeyElement = accessors.find(a => areEqualKeys(a.key, accessorData.key));

case "get":
isGetPresent = true;
break;
if (equalKeyElement) {
equalKeyElement.getters.push(...accessorData.getters);
equalKeyElement.setters.push(...accessorData.setters);
} else {
accessors.push(accessorData);
}

default:
return accessors;
}

// Do nothing
}
/**
* Checks accessor pairs in the given list of nodes.
* @param {ASTNode[]} nodes The list to check.
* @returns {void}
* @private
*/
function checkList(nodes) {
const accessors = nodes
.filter(isAccessorKind)
.map(createAccessorData)
.reduce(mergeAccessorData, []);

if (isSetPresent && isGetPresent) {
break;
for (const { getters, setters } of accessors) {
if (checkSetWithoutGet && setters.length && !getters.length) {
reportList(setters, "missingGetter");
}
if (checkGetWithoutSet && getters.length && !setters.length) {
reportList(getters, "missingSetter");
}
}
}

if (checkSetWithoutGet && isSetPresent && !isGetPresent) {
context.report({ node, messageId: "getter" });
} else if (checkGetWithoutSet && isGetPresent && !isSetPresent) {
context.report({ node, messageId: "setter" });
/**
* Checks accessor pairs in an object literal.
* @param {ASTNode} node `ObjectExpression` node to check.
* @returns {void}
* @private
*/
function checkObjectLiteral(node) {
checkList(node.properties.filter(p => p.type === "Property"));
}

/**
* Checks accessor pairs in a property descriptor.
* @param {ASTNode} node Property descriptor `ObjectExpression` node to check.
* @returns {void}
* @private
*/
function checkPropertyDescriptor(node) {
const namesToCheck = node.properties
.filter(p => p.type === "Property" && p.kind === "init" && !p.computed)
.map(({ key }) => key.name);

const hasGetter = namesToCheck.includes("get");
const hasSetter = namesToCheck.includes("set");

if (checkSetWithoutGet && hasSetter && !hasGetter) {
report(node, "missingGetter");
}
if (checkGetWithoutSet && hasGetter && !hasSetter) {
report(node, "missingSetter");
}
}

return {
ObjectExpression(node) {
if (checkSetWithoutGet || checkGetWithoutSet) {
checkLonelySetGet(node);
checkObjectLiteral(node);
if (isPropertyDescriptor(node)) {
checkPropertyDescriptor(node);
}
}
}
};
Expand Down