Skip to content

Commit

Permalink
feat(eslint-plugin): [no-unnecessary-condition] remove option `ignore…
Browse files Browse the repository at this point in the history 
…RHS` (#1163)
  • Loading branch information
@Retsam @bradzacher
Retsam authored and bradzacher committed May 21, 2020
1 parent 8e31d5d commit ee8dd8f
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 65 deletions.
45 changes: 13 additions & 32 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Expand Up @@ -60,7 +60,6 @@ const isLiteral = (type: ts.Type): boolean =>
export type Options = [
{
allowConstantLoopConditions?: boolean;
ignoreRhs?: boolean;
checkArrayPredicates?: boolean;
},
];
Expand Down Expand Up @@ -93,9 +92,6 @@ export default createRule<Options, MessageId>({
allowConstantLoopConditions: {
type: 'boolean',
},
ignoreRhs: {
type: 'boolean',
},
checkArrayPredicates: {
type: 'boolean',
},
Expand Down Expand Up @@ -124,14 +120,10 @@ export default createRule<Options, MessageId>({
defaultOptions: [
{
allowConstantLoopConditions: false,
ignoreRhs: false,
checkArrayPredicates: false,
},
],
create(
context,
[{ allowConstantLoopConditions, checkArrayPredicates, ignoreRhs }],
) {
create(context, [{ allowConstantLoopConditions, checkArrayPredicates }]) {
const service = getParserServices(context);
const checker = service.program.getTypeChecker();
const sourceCode = context.getSourceCode();
Expand Down Expand Up @@ -176,6 +168,12 @@ export default createRule<Options, MessageId>({
return;
}

// When checking logical expressions, only check the right side
// as the left side has been checked by checkLogicalExpressionForUnnecessaryConditionals
if (node.type === AST_NODE_TYPES.LogicalExpression) {
return checkNode(node.right);
}

const type = getNodeType(node);

// Conditional is always necessary if it involves:
Expand Down Expand Up @@ -259,20 +257,6 @@ export default createRule<Options, MessageId>({
}
}

/**
* Checks that a testable expression is necessarily conditional, reports otherwise.
* Filters all LogicalExpressions to prevent some duplicate reports.
*/
function checkIfTestExpressionIsNecessaryConditional(
node: TSESTree.ConditionalExpression | TSESTree.IfStatement,
): void {
if (node.test.type === AST_NODE_TYPES.LogicalExpression) {
return;
}

checkNode(node.test);
}

/**
* Checks that a logical expression contains a boolean, reports otherwise.
*/
Expand All @@ -283,10 +267,9 @@ export default createRule<Options, MessageId>({
checkNodeForNullish(node.left);
return;
}
// Only checks the left side, since the right side might not be "conditional" at all.
// The right side will be checked if the LogicalExpression is used in a conditional context
checkNode(node.left);
if (!ignoreRhs) {
checkNode(node.right);
}
}

/**
Expand All @@ -298,10 +281,8 @@ export default createRule<Options, MessageId>({
| TSESTree.ForStatement
| TSESTree.WhileStatement,
): void {
if (
node.test === null ||
node.test.type === AST_NODE_TYPES.LogicalExpression
) {
if (node.test === null) {
// e.g. `for(;;)`
return;
}

Expand Down Expand Up @@ -480,10 +461,10 @@ export default createRule<Options, MessageId>({
return {
BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
CallExpression: checkCallExpression,
ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
ConditionalExpression: (node): void => checkNode(node.test),
DoWhileStatement: checkIfLoopIsNecessaryConditional,
ForStatement: checkIfLoopIsNecessaryConditional,
IfStatement: checkIfTestExpressionIsNecessaryConditional,
IfStatement: (node): void => checkNode(node.test),
LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals,
WhileStatement: checkIfLoopIsNecessaryConditional,
OptionalMemberExpression: checkOptionalMemberExpression,
Expand Down
73 changes: 40 additions & 33 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
View file
Expand Up @@ -56,6 +56,14 @@ for (let i = 0; b1 && b2; i++) {
break;
}
const t1 = b1 && b2 ? 'yes' : 'no';
if (b1 && b2) {
}
while (b1 && b2) {}
for (let i = 0; b1 && b2; i++) {
break;
}
const t1 = b1 && b2 ? 'yes' : 'no';
for (;;) {}
`,
necessaryConditionTest('false | 5'), // Truthy literal and falsy literal
necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal
Expand Down Expand Up @@ -203,15 +211,14 @@ returnsArr?.()[42]?.toUpperCase();
declare const arr: string[][];
arr[x] ?? [];
`,
// Supports ignoring the RHS
// Doesn't check the right-hand side of a logical expression
// in a non-conditional context
{
code: `
declare const b1: boolean;
declare const b2: true;
if (b1 && b2) {
}
const x = b1 && b2;
`,
options: [{ ignoreRhs: true }],
},
{
code: `
Expand Down Expand Up @@ -289,19 +296,26 @@ const t1 = b1 && b2;
const t2 = b1 || b2;
if (b1 && b2) {
}
if (b2 && b1) {
}
while (b1 && b2) {}
while (b2 && b1) {}
for (let i = 0; b1 && b2; i++) {
break;
}
const t1 = b1 && b2 ? 'yes' : 'no';
const t1 = b2 && b1 ? 'yes' : 'no';
`,
errors: [
ruleError(4, 12, 'alwaysTruthy'),
ruleError(5, 12, 'alwaysTruthy'),
ruleError(6, 5, 'alwaysTruthy'),
ruleError(8, 8, 'alwaysTruthy'),
ruleError(9, 17, 'alwaysTruthy'),
ruleError(12, 12, 'alwaysTruthy'),
ruleError(8, 11, 'alwaysTruthy'),
ruleError(10, 8, 'alwaysTruthy'),
ruleError(11, 14, 'alwaysTruthy'),
ruleError(12, 17, 'alwaysTruthy'),
ruleError(15, 12, 'alwaysTruthy'),
ruleError(16, 18, 'alwaysTruthy'),
],
},
// Ensure that it's complaining about the right things
Expand All @@ -314,6 +328,25 @@ const t1 = b1 && b2 ? 'yes' : 'no';
unnecessaryConditionTest('void', 'alwaysFalsy'),
unnecessaryConditionTest('never', 'never'),

// More complex logical expressions
{
code: `
declare const b1: boolean;
declare const b2: boolean;
if (true && b1 && b2) {
}
if (b1 && false && b2) {
}
if (b1 || b2 || true) {
}
`,
errors: [
ruleError(4, 5, 'alwaysTruthy'),
ruleError(6, 11, 'alwaysFalsy'),
ruleError(8, 17, 'alwaysTruthy'),
],
},

// Generic type params
{
code: `
Expand Down Expand Up @@ -497,32 +530,6 @@ function falsy() {}
// `,
// errors: [ruleError(6, 23, 'alwaysTruthyFunc')],
// },

// Still errors on in the expected locations when ignoring RHS
{
options: [{ ignoreRhs: true }],
code: `
const b1 = true;
const b2 = false;
const t1 = b1 && b2;
const t2 = b1 || b2;
if (b1 && b2) {
}
while (b1 && b2) {}
for (let i = 0; b1 && b2; i++) {
break;
}
const t1 = b1 && b2 ? 'yes' : 'no';
`,
errors: [
ruleError(4, 12, 'alwaysTruthy'),
ruleError(5, 12, 'alwaysTruthy'),
ruleError(6, 5, 'alwaysTruthy'),
ruleError(8, 8, 'alwaysTruthy'),
ruleError(9, 17, 'alwaysTruthy'),
ruleError(12, 12, 'alwaysTruthy'),
],
},
{
code: `
while (true) {}
Expand Down

0 comments on commit ee8dd8f

Please sign in to comment.