From 85b5b1cc30d40489cc21834eae49dcb0c8273b2b Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Sun, 13 Oct 2019 08:16:18 +0100 Subject: [PATCH 1/6] Update: add option to no-unsafe-negation (fixes #12163) --- docs/rules/no-unsafe-negation.md | 44 ++++++++++++++++++++--- lib/rules/no-unsafe-negation.js | 35 +++++++++++++++--- tests/lib/rules/no-unsafe-negation.js | 52 +++++++++++++++++++++++++-- 3 files changed, 120 insertions(+), 11 deletions(-) diff --git a/docs/rules/no-unsafe-negation.md b/docs/rules/no-unsafe-negation.md index 880e9f64481..5f28236a0a8 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,42 @@ 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 + +The enforceForOrderingRelations option extends the rule to apply to all [Relational Operators](https://www.ecma-international.org/ecma-262/10.0/index.html#sec-relational-operators), not just `in` and `instanceof`. + +With this option set to true, the rule is enforced for all of the following: + +- `in` operator. +- `instanceof` operator. +- `<` 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-unneeded-ternary: ["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..3d763270890 100644 --- a/lib/rules/no-unsafe-negation.js +++ b/lib/rules/no-unsafe-negation.js @@ -24,6 +24,15 @@ function isRelationalOperator(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. @@ -42,25 +51,43 @@ module.exports = { type: "problem", docs: { - description: "disallow negating the left operand of relational operators", + description: + "disallow negating the left operand of relational operators", category: "Possible Errors", recommended: true, 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." + unexpected: + "Unexpected negating the left operand of '{{operator}}' operator." } }, 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 ( + (isRelationalOperator(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..59ab2f30cad 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,16 @@ ruleTester.run("no-unsafe-negation", rule, { "a instanceof b", "a instanceof b === false", "!(a instanceof b)", - "(!a) instanceof b" + "(!a) instanceof b", + "if (! a < b) {}", + "while (! a > b) {}", + "foo = ! a <= b;", + "foo = ! a >= b;", + { + code: "foo = (!a) >= b;", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedMoreThanOrEqualOperatorError] + } ], invalid: [ { @@ -55,6 +83,26 @@ 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] } ] }); From 849f295d42e07af1e0b82fe9b925d968d5e86299 Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Sun, 13 Oct 2019 08:35:12 +0100 Subject: [PATCH 2/6] Chore: undo autoformatting + rename function --- lib/rules/no-unsafe-negation.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/rules/no-unsafe-negation.js b/lib/rules/no-unsafe-negation.js index 3d763270890..5c8f569d7b8 100644 --- a/lib/rules/no-unsafe-negation.js +++ b/lib/rules/no-unsafe-negation.js @@ -16,11 +16,11 @@ 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"; } @@ -51,8 +51,7 @@ module.exports = { type: "problem", docs: { - description: - "disallow negating the left operand of relational operators", + description: "disallow negating the left operand of relational operators", category: "Possible Errors", recommended: true, url: "https://eslint.org/docs/rules/no-unsafe-negation" @@ -72,8 +71,7 @@ module.exports = { ], fixable: null, messages: { - unexpected: - "Unexpected negating the left operand of '{{operator}}' operator." + unexpected: "Unexpected negating the left operand of '{{operator}}' operator." } }, @@ -87,7 +85,7 @@ module.exports = { const orderingRelationRuleApplies = enforceForOrderingRelations && isOrderingRelationalOperator(node.operator); if ( - (isRelationalOperator(node.operator) || orderingRelationRuleApplies) && + (isInOrInstanceOfOperator(node.operator) || orderingRelationRuleApplies) && isNegation(node.left) && !astUtils.isParenthesised(sourceCode, node.left) ) { From 1e8af0024c95db76749a8c2d5a85536f08b9781a Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Fri, 18 Oct 2019 09:09:14 +0100 Subject: [PATCH 3/6] Update: code review changes - typos - clarity of docs - extra test cases --- docs/rules/no-unsafe-negation.md | 8 ++----- tests/lib/rules/no-unsafe-negation.js | 30 +++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/docs/rules/no-unsafe-negation.md b/docs/rules/no-unsafe-negation.md index 5f28236a0a8..59bcfbba445 100644 --- a/docs/rules/no-unsafe-negation.md +++ b/docs/rules/no-unsafe-negation.md @@ -79,12 +79,8 @@ This rule has an object option: ### enforceForOrderingRelations -The enforceForOrderingRelations option extends the rule to apply to all [Relational Operators](https://www.ecma-international.org/ecma-262/10.0/index.html#sec-relational-operators), not just `in` and `instanceof`. +With this option set to true, the rule is additionally enforced for: -With this option set to true, the rule is enforced for all of the following: - -- `in` operator. -- `instanceof` operator. - `<` operator. - `>` operator. - `<=` operator. @@ -95,7 +91,7 @@ The purpose is to avoid expressions such as `! a < b` (which is equivalent to `( Examples of additional **incorrect** code for this rule with the `{ "enforceForOrderingRelations": true }` option: ```js -/*eslint no-unneeded-ternary: ["error", { "enforceForOrderingRelations": true }]*/ +/*eslint no-unsafe-negation: ["error", { "enforceForOrderingRelations": true }]*/ if (! a < b) {} diff --git a/tests/lib/rules/no-unsafe-negation.js b/tests/lib/rules/no-unsafe-negation.js index 59ab2f30cad..448f5e32740 100644 --- a/tests/lib/rules/no-unsafe-negation.js +++ b/tests/lib/rules/no-unsafe-negation.js @@ -49,14 +49,35 @@ ruleTester.run("no-unsafe-negation", rule, { "a instanceof b === false", "!(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 }], - errors: [unexpectedMoreThanOrEqualOperatorError] + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "a <= b", + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "!(a < b)", + options: [{ enforceForOrderingRelations: true }] + }, + { + code: "foo = a > b);", + options: [{ enforceForOrderingRelations: true }] } ], invalid: [ @@ -103,6 +124,11 @@ ruleTester.run("no-unsafe-negation", rule, { code: "foo = ! a >= b;", options: [{ enforceForOrderingRelations: true }], errors: [unexpectedMoreThanOrEqualOperatorError] + }, + { + code: "! a <= b", + options: [{ enforceForOrderingRelations: true }], + errors: [unexpectedMoreThanOrEqualOperatorError] } ] }); From ff4e051c7904d3b66ec0e835fdcbe4f40b918eda Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Fri, 18 Oct 2019 09:16:24 +0100 Subject: [PATCH 4/6] Fix: fix failing tests --- tests/lib/rules/no-unsafe-negation.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/rules/no-unsafe-negation.js b/tests/lib/rules/no-unsafe-negation.js index 448f5e32740..9493bbb7295 100644 --- a/tests/lib/rules/no-unsafe-negation.js +++ b/tests/lib/rules/no-unsafe-negation.js @@ -76,7 +76,7 @@ ruleTester.run("no-unsafe-negation", rule, { options: [{ enforceForOrderingRelations: true }] }, { - code: "foo = a > b);", + code: "foo = a > b;", options: [{ enforceForOrderingRelations: true }] } ], @@ -128,7 +128,7 @@ ruleTester.run("no-unsafe-negation", rule, { { code: "! a <= b", options: [{ enforceForOrderingRelations: true }], - errors: [unexpectedMoreThanOrEqualOperatorError] + errors: [unexpectedLessThanOrEqualOperatorError] } ] }); From 796dba466cf7f0c4562990f97bf60fa6d9a3288f Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Sat, 19 Oct 2019 07:43:00 +0100 Subject: [PATCH 5/6] Update: code review changes - add backticks --- docs/rules/no-unsafe-negation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-unsafe-negation.md b/docs/rules/no-unsafe-negation.md index 59bcfbba445..a6ee039c62e 100644 --- a/docs/rules/no-unsafe-negation.md +++ b/docs/rules/no-unsafe-negation.md @@ -79,7 +79,7 @@ This rule has an object option: ### enforceForOrderingRelations -With this option set to true, the rule is additionally enforced for: +With this option set to `true`, the rule is additionally enforced for: - `<` operator. - `>` operator. From 69addd819b57006b422fd10bac4480253f18265a Mon Sep 17 00:00:00 2001 From: Sam Rae Date: Sat, 19 Oct 2019 07:44:15 +0100 Subject: [PATCH 6/6] Chore: remove comma --- docs/rules/no-unsafe-negation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-unsafe-negation.md b/docs/rules/no-unsafe-negation.md index a6ee039c62e..38bb9b02fd1 100644 --- a/docs/rules/no-unsafe-negation.md +++ b/docs/rules/no-unsafe-negation.md @@ -79,7 +79,7 @@ This rule has an object option: ### enforceForOrderingRelations -With this option set to `true`, the rule is additionally enforced for: +With this option set to `true` the rule is additionally enforced for: - `<` operator. - `>` operator.