From 7c8bbe0391944e1f92e083a04715bf4b3fe6be5d Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Mon, 21 Oct 2019 16:17:13 +0100 Subject: [PATCH] Update: enforceForOrderingRelations no-unsafe-negation (fixes #12163) (#12414) * Update: add option to no-unsafe-negation (fixes #12163) * Chore: undo autoformatting + rename function * Update: code review changes - typos - clarity of docs - extra test cases * Fix: fix failing tests * Update: code review changes - add backticks * Chore: remove comma --- docs/rules/no-unsafe-negation.md | 40 ++++++++++++-- lib/rules/no-unsafe-negation.js | 35 ++++++++++-- tests/lib/rules/no-unsafe-negation.js | 78 ++++++++++++++++++++++++++- 3 files changed, 141 insertions(+), 12 deletions(-) diff --git a/docs/rules/no-unsafe-negation.md b/docs/rules/no-unsafe-negation.md index 880e9f64481..38bb9b02fd1 100644 --- a/docs/rules/no-unsafe-negation.md +++ b/docs/rules/no-unsafe-negation.md @@ -4,12 +4,10 @@ Just as developers might type `-a + b` when they mean `-(a + b)` for the negativ ## Rule Details -This rule disallows negating the left operand of [Relational Operators](https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Expressions_and_Operators#Relational_operators). +This rule disallows negating the left operand of the following relational operators: -Relational Operators are: - -- `in` operator. -- `instanceof` operator. +- [`in` operator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in). +- [`instanceof` operator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof). Examples of **incorrect** code for this rule: @@ -72,6 +70,38 @@ if (!(foo) in object) { } ``` +## Options + +This rule has an object option: + +- `"enforceForOrderingRelations": false` (default) allows negation of the left-hand side of ordering relational operators (`<`, `>`, `<=`, `>=`) +- `"enforceForOrderingRelations": true` disallows negation of the left-hand side of ordering relational operators + +### enforceForOrderingRelations + +With this option set to `true` the rule is additionally enforced for: + +- `<` operator. +- `>` operator. +- `<=` operator. +- `>=` operator. + +The purpose is to avoid expressions such as `! a < b` (which is equivalent to `(a ? 0 : 1) < b`) when what is really intended is `!(a < b)`. + +Examples of additional **incorrect** code for this rule with the `{ "enforceForOrderingRelations": true }` option: + +```js +/*eslint no-unsafe-negation: ["error", { "enforceForOrderingRelations": true }]*/ + +if (! a < b) {} + +while (! a > b) {} + +foo = ! a <= b; + +foo = ! a >= b; +``` + ## When Not To Use It If you don't want to notify unsafe logical negations, then it's safe to disable this rule. diff --git a/lib/rules/no-unsafe-negation.js b/lib/rules/no-unsafe-negation.js index 5226b367dee..5c8f569d7b8 100644 --- a/lib/rules/no-unsafe-negation.js +++ b/lib/rules/no-unsafe-negation.js @@ -16,14 +16,23 @@ const astUtils = require("./utils/ast-utils"); //------------------------------------------------------------------------------ /** - * Checks whether the given operator is a relational operator or not. + * Checks whether the given operator is `in` or `instanceof` * @param {string} op The operator type to check. - * @returns {boolean} `true` if the operator is a relational operator. + * @returns {boolean} `true` if the operator is `in` or `instanceof` */ -function isRelationalOperator(op) { +function isInOrInstanceOfOperator(op) { return op === "in" || op === "instanceof"; } +/** + * Checks whether the given operator is an ordering relational operator or not. + * @param {string} op The operator type to check. + * @returns {boolean} `true` if the operator is an ordering relational operator. + */ +function isOrderingRelationalOperator(op) { + return op === "<" || op === ">" || op === ">=" || op === "<="; +} + /** * Checks whether the given node is a logical negation expression or not. * @param {ASTNode} node The node to check. @@ -48,7 +57,18 @@ module.exports = { url: "https://eslint.org/docs/rules/no-unsafe-negation" }, - schema: [], + schema: [ + { + type: "object", + properties: { + enforceForOrderingRelations: { + type: "boolean", + default: false + } + }, + additionalProperties: false + } + ], fixable: null, messages: { unexpected: "Unexpected negating the left operand of '{{operator}}' operator." @@ -57,10 +77,15 @@ module.exports = { create(context) { const sourceCode = context.getSourceCode(); + const options = context.options[0] || {}; + const enforceForOrderingRelations = options.enforceForOrderingRelations === true; return { BinaryExpression(node) { - if (isRelationalOperator(node.operator) && + const orderingRelationRuleApplies = enforceForOrderingRelations && isOrderingRelationalOperator(node.operator); + + if ( + (isInOrInstanceOfOperator(node.operator) || orderingRelationRuleApplies) && isNegation(node.left) && !astUtils.isParenthesised(sourceCode, node.left) ) { diff --git a/tests/lib/rules/no-unsafe-negation.js b/tests/lib/rules/no-unsafe-negation.js index 6ac24541248..9493bbb7295 100644 --- a/tests/lib/rules/no-unsafe-negation.js +++ b/tests/lib/rules/no-unsafe-negation.js @@ -18,7 +18,26 @@ const rule = require("../../../lib/rules/no-unsafe-negation"), const ruleTester = new RuleTester(); const unexpectedInError = { messageId: "unexpected", data: { operator: "in" } }; -const unexpectedInstanceofError = { messageId: "unexpected", data: { operator: "instanceof" } }; +const unexpectedInstanceofError = { + messageId: "unexpected", + data: { operator: "instanceof" } +}; +const unexpectedLessThanOperatorError = { + messageId: "unexpected", + data: { operator: "<" } +}; +const unexpectedMoreThanOperatorError = { + messageId: "unexpected", + data: { operator: ">" } +}; +const unexpectedMoreThanOrEqualOperatorError = { + messageId: "unexpected", + data: { operator: ">=" } +}; +const unexpectedLessThanOrEqualOperatorError = { + messageId: "unexpected", + data: { operator: "<=" } +}; ruleTester.run("no-unsafe-negation", rule, { valid: [ @@ -29,7 +48,37 @@ ruleTester.run("no-unsafe-negation", rule, { "a instanceof b", "a instanceof b === false", "!(a instanceof b)", - "(!a) instanceof b" + "(!a) instanceof b", + + // tests cases for enforceForOrderingRelations option: + "if (! a < b) {}", + "while (! a > b) {}", + "foo = ! a <= b;", + "foo = ! a >= b;", + { + code: "! a <= b", + options: [{}] + }, + { + code: "foo = ! a >= b;", + options: [{ enforceForOrderingRelations: false }] + }, + { + code: "foo = (!a) >= b;", + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "a <= b", + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "!(a < b)", + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "foo = a > b;", + options: [{ enforceForOrderingRelations: true }] + } ], invalid: [ { @@ -55,6 +104,31 @@ ruleTester.run("no-unsafe-negation", rule, { { code: "!(a) instanceof b", errors: [unexpectedInstanceofError] + }, + { + code: "if (! a < b) {}", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedLessThanOperatorError] + }, + { + code: "while (! a > b) {}", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedMoreThanOperatorError] + }, + { + code: "foo = ! a <= b;", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedLessThanOrEqualOperatorError] + }, + { + code: "foo = ! a >= b;", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedMoreThanOrEqualOperatorError] + }, + { + code: "! a <= b", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedLessThanOrEqualOperatorError] } ] });