Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
New: Add no-setter-return rule (fixes #12285) (#12346)
  • Loading branch information
mdjermanovic authored and ilyavolodin committed Nov 10, 2019
1 parent 0afb518 commit 45aa6a3
Show file tree
Hide file tree
Showing 5 changed files with 840 additions and 0 deletions.
101 changes: 101 additions & 0 deletions docs/rules/no-setter-return.md
@@ -0,0 +1,101 @@
# Disallow returning values from setters (no-setter-return)

Setters cannot return values.

While returning a value from a setter does not produce an error, the returned value is being ignored. Therefore, returning a value from a setter is either unnecessary or a possible error, since the returned value cannot be used.

## Rule Details

This rule disallows returning values from setters and reports `return` statements in setter functions.

Only `return` without a value is allowed, as it's a control flow statement.

This rule checks setters in:

* Object literals.
* Class declarations and class expressions.
* Property descriptors in `Object.create`, `Object.defineProperty`, `Object.defineProperties`, and `Reflect.defineProperty` methods of the global objects.

Examples of **incorrect** code for this rule:

```js
/*eslint no-setter-return: "error"*/

var foo = {
set a(value) {
this.val = value;
return value;
}
};

class Foo {
set a(value) {
this.val = value * 2;
return this.val;
}
}

const Bar = class {
static set a(value) {
if (value < 0) {
this.val = 0;
return 0;
}
this.val = value;
}
};

Object.defineProperty(foo, "bar", {
set(value) {
if (value < 0) {
return false;
}
this.val = value;
}
});
```

Examples of **correct** code for this rule:

```js
/*eslint no-setter-return: "error"*/

var foo = {
set a(value) {
this.val = value;
}
};

class Foo {
set a(value) {
this.val = value * 2;
}
}

const Bar = class {
static set a(value) {
if (value < 0) {
this.val = 0;
return;
}
this.val = value;
}
};

Object.defineProperty(foo, "bar", {
set(value) {
if (value < 0) {
throw new Error("Negative value.");
}
this.val = value;
}
});
```

## Further Reading

* [MDN setter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set)

## Related Rules

* [getter-return](getter-return.md)
1 change: 1 addition & 0 deletions lib/rules/index.js
Expand Up @@ -186,6 +186,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({
"no-self-assign": () => require("./no-self-assign"),
"no-self-compare": () => require("./no-self-compare"),
"no-sequences": () => require("./no-sequences"),
"no-setter-return": () => require("./no-setter-return"),
"no-shadow": () => require("./no-shadow"),
"no-shadow-restricted-names": () => require("./no-shadow-restricted-names"),
"no-spaced-func": () => require("./no-spaced-func"),
Expand Down
227 changes: 227 additions & 0 deletions lib/rules/no-setter-return.js
@@ -0,0 +1,227 @@
/**
* @fileoverview Rule to disallow returning values from setters
* @author Milos Djermanovic
*/

"use strict";

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

const astUtils = require("./utils/ast-utils");
const { findVariable } = require("eslint-utils");

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

/**
* Determines whether the given identifier node is a reference to a global variable.
* @param {ASTNode} node `Identifier` node to check.
* @param {Scope} scope Scope to which the node belongs.
* @returns {boolean} True if the identifier is a reference to a global variable.
*/
function isGlobalReference(node, scope) {
const variable = findVariable(scope, node);

return variable !== null && variable.scope.type === "global" && variable.defs.length === 0;
}

/**
* Determines whether the given node is an argument of the specified global method call, at the given `index` position.
* E.g., for given `index === 1`, this function checks for `objectName.methodName(foo, node)`, where objectName is a global variable.
* @param {ASTNode} node The node to check.
* @param {Scope} scope Scope to which the node belongs.
* @param {string} objectName Name of the global object.
* @param {string} methodName Name of the method.
* @param {number} index The given position.
* @returns {boolean} `true` if the node is argument at the given position.
*/
function isArgumentOfGlobalMethodCall(node, scope, objectName, methodName, index) {
const parent = node.parent;

return parent.type === "CallExpression" &&
parent.arguments[index] === node &&
parent.callee.type === "MemberExpression" &&
astUtils.getStaticPropertyName(parent.callee) === methodName &&
parent.callee.object.type === "Identifier" &&
parent.callee.object.name === objectName &&
isGlobalReference(parent.callee.object, scope);
}

/**
* Determines whether the given node is used as a property descriptor.
* @param {ASTNode} node The node to check.
* @param {Scope} scope Scope to which the node belongs.
* @returns {boolean} `true` if the node is a property descriptor.
*/
function isPropertyDescriptor(node, scope) {
if (
isArgumentOfGlobalMethodCall(node, scope, "Object", "defineProperty", 2) ||
isArgumentOfGlobalMethodCall(node, scope, "Reflect", "defineProperty", 2)
) {
return true;
}

const parent = node.parent;

if (
parent.type === "Property" &&
parent.value === node
) {
const grandparent = parent.parent;

if (
grandparent.type === "ObjectExpression" &&
(
isArgumentOfGlobalMethodCall(grandparent, scope, "Object", "create", 1) ||
isArgumentOfGlobalMethodCall(grandparent, scope, "Object", "defineProperties", 1)
)
) {
return true;
}
}

return false;
}

/**
* Determines whether the given function node is used as a setter function.
* @param {ASTNode} node The node to check.
* @param {Scope} scope Scope to which the node belongs.
* @returns {boolean} `true` if the node is a setter.
*/
function isSetter(node, scope) {
const parent = node.parent;

if (
parent.kind === "set" &&
parent.value === node
) {

// Setter in an object literal or in a class
return true;
}

if (
parent.type === "Property" &&
parent.value === node &&
astUtils.getStaticPropertyName(parent) === "set" &&
parent.parent.type === "ObjectExpression" &&
isPropertyDescriptor(parent.parent, scope)
) {

// Setter in a property descriptor
return true;
}

return false;
}

/**
* Finds function's outer scope.
* @param {Scope} scope Function's own scope.
* @returns {Scope} Function's outer scope.
*/
function getOuterScope(scope) {
const upper = scope.upper;

if (upper.type === "function-expression-name") {
return upper.upper;
}

return upper;
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = {
meta: {
type: "problem",

docs: {
description: "disallow returning values from setters",
category: "Possible Errors",
recommended: false,
url: "https://eslint.org/docs/rules/no-setter-return"
},

schema: [],

messages: {
returnsValue: "Setter cannot return a value."
}
},

create(context) {
let funcInfo = null;

/**
* Creates and pushes to the stack a function info object for the given function node.
* @param {ASTNode} node The function node.
* @returns {void}
*/
function enterFunction(node) {
const outerScope = getOuterScope(context.getScope());

funcInfo = {
upper: funcInfo,
isSetter: isSetter(node, outerScope)
};
}

/**
* Pops the current function info object from the stack.
* @returns {void}
*/
function exitFunction() {
funcInfo = funcInfo.upper;
}

/**
* Reports the given node.
* @param {ASTNode} node Node to report.
* @returns {void}
*/
function report(node) {
context.report({ node, messageId: "returnsValue" });
}

return {

/*
* Function declarations cannot be setters, but we still have to track them in the `funcInfo` stack to avoid
* false positives, because a ReturnStatement node can belong to a function declaration inside a setter.
*
* Note: A previously declared function can be referenced and actually used as a setter in a property descriptor,
* but that's out of scope for this rule.
*/
FunctionDeclaration: enterFunction,
FunctionExpression: enterFunction,
ArrowFunctionExpression(node) {
enterFunction(node);

if (funcInfo.isSetter && node.expression) {

// { set: foo => bar } property descriptor. Report implicit return 'bar' as the equivalent for a return statement.
report(node.body);
}
},

"FunctionDeclaration:exit": exitFunction,
"FunctionExpression:exit": exitFunction,
"ArrowFunctionExpression:exit": exitFunction,

ReturnStatement(node) {

// Global returns (e.g., at the top level of a Node module) don't have `funcInfo`.
if (funcInfo && funcInfo.isSetter && node.argument) {
report(node);
}
}
};
}
};

0 comments on commit 45aa6a3

Please sign in to comment.