prefer-conditional-expression: avoid error on nested if statement (pa…

…lantir#3528) [bugfix] `prefer-conditional-expression`: don't repeat error on nested if statements Fixes: palantir#3350
mavriel · Nov 30, 2017 · 3db5fcf · 3db5fcf
1 parent 372408e
commit 3db5fcf
Show file tree

Hide file tree

Showing 3 changed files with 131 additions and 25 deletions.
diff --git a/src/rules/preferConditionalExpressionRule.ts b/src/rules/preferConditionalExpressionRule.ts
@@ -60,44 +60,56 @@ function walk(ctx: Lint.WalkContext<Options>): void {
     const { sourceFile, options: { checkElseIf } } = ctx;
     return ts.forEachChild(sourceFile, function cb(node: ts.Node): void {
         if (isIfStatement(node)) {
-            const assigned = detect(node, sourceFile, checkElseIf);
+            const assigned = detectAssignment(node, sourceFile, checkElseIf);
             if (assigned !== undefined) {
                 ctx.addFailureAtNode(
                     node.getChildAt(0, sourceFile),
                     Rule.FAILURE_STRING(assigned.getText(sourceFile)));
             }
             if (assigned !== undefined || !checkElseIf) {
                 // Be careful not to fail again for the "else if"
-                ts.forEachChild(node.expression, cb);
-                ts.forEachChild(node.thenStatement, cb);
-                if (node.elseStatement !== undefined) {
-                    ts.forEachChild(node.elseStatement, cb);
-                }
-                return;
+                do {
+                    ts.forEachChild(node.expression, cb);
+                    ts.forEachChild(node.thenStatement, cb);
+                    if (node.elseStatement === undefined) {
+                        return;
+                    }
+                    node = node.elseStatement;
+                    while (isBlock(node) && node.statements.length === 1) {
+                        node = node.statements[0];
+                    }
+                } while (isIfStatement(node));
             }
         }
         return ts.forEachChild(node, cb);
     });
 }
 
-function detect({ thenStatement, elseStatement }: ts.IfStatement, sourceFile: ts.SourceFile, elseIf: boolean): ts.Expression | undefined {
-    if (elseStatement === undefined || !elseIf && elseStatement.kind === ts.SyntaxKind.IfStatement) {
-        return undefined;
-    }
-    const elze = isIfStatement(elseStatement) ? detect(elseStatement, sourceFile, elseIf) : getAssigned(elseStatement, sourceFile);
-    if (elze === undefined) {
-        return undefined;
-    }
-    const then = getAssigned(thenStatement, sourceFile);
-    return then !== undefined && nodeEquals(elze, then, sourceFile) ? then : undefined;
-}
-
-/** Returns the left side of an assignment. */
-function getAssigned(node: ts.Statement, sourceFile: ts.SourceFile): ts.Expression | undefined {
-    if (isBlock(node)) {
-        return node.statements.length === 1 ? getAssigned(node.statements[0], sourceFile) : undefined;
-    } else if (isExpressionStatement(node) && isBinaryExpression(node.expression)) {
-        const { operatorToken: { kind }, left, right } = node.expression;
+/**
+ * @param inElse `undefined` when this is the top level if statement, `false` when inside the then branch, `true` when inside else
+ */
+function detectAssignment(
+    statement: ts.Statement,
+    sourceFile: ts.SourceFile,
+    checkElseIf: boolean,
+    inElse?: boolean,
+): ts.Expression | undefined {
+    if (isIfStatement(statement)) {
+        if (inElse === false || !checkElseIf && inElse || statement.elseStatement === undefined) {
+            return undefined;
+        }
+        const then = detectAssignment(statement.thenStatement, sourceFile, checkElseIf, false);
+        if (then === undefined) {
+            return undefined;
+        }
+        const elze = detectAssignment(statement.elseStatement, sourceFile, checkElseIf, true);
+        return elze !== undefined && nodeEquals(then, elze, sourceFile) ? then : undefined;
+    } else if (isBlock(statement)) {
+        return statement.statements.length === 1
+            ? detectAssignment(statement.statements[0], sourceFile, checkElseIf, inElse)
+            : undefined;
+    } else if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) {
+        const { operatorToken: { kind }, left, right } = statement.expression;
         return kind === ts.SyntaxKind.EqualsToken && isSameLine(sourceFile, right.getStart(sourceFile), right.end) ? left : undefined;
     } else {
         return undefined;

diff --git a/test/rules/prefer-conditional-expression/check-else-if/test.ts.lint b/test/rules/prefer-conditional-expression/check-else-if/test.ts.lint
@@ -51,4 +51,56 @@ if (true) {
     foo(bar).baz = 1;
 }
 
+if (length === 0) {
+~~ [err % ('x')]
+    x = "foo";
+} else if (length === 1) {
+    x = "bar";
+} else if (length === 2) {
+    x = "something else";
+} else {
+    x = "unknown";
+}
+
+if (length === 0) {
+~~ [err % ('x')]
+    x = "foo";
+} else {
+    if (length === 1) {
+        x = "bar";
+    } else {
+        if (length === 2) {
+            x = "something else";
+        } else {
+            x = "unknown";
+        }
+    }
+}
+
+if (length === 0) {
+    y = "foo";
+} else if (length === 1) {
+       ~~ [err % ('x')]
+    x = "bar";
+} else if (length === 2) {
+    x = "something else";
+} else {
+    x = "unknown";
+}
+
+if (length === 0) {
+    y = "foo";
+} else {
+    if (length === 1) {
+    ~~ [err % ('x')]
+        x = "bar";
+    } else {
+        if (length === 2) {
+            x = "something else";
+        } else {
+            x = "unknown";
+        }
+    }
+}
+
 [err]: Use a conditional expression instead of assigning to '%s' in multiple places.
diff --git a/test/rules/prefer-conditional-expression/default/test.ts.lint b/test/rules/prefer-conditional-expression/default/test.ts.lint
@@ -50,4 +50,46 @@ if (true) {
     foo(bar).baz = 1;
 }
 
+// leave nested if statements alone
+if (length === 0) {
+    x = "foo";
+} else if (length === 1) {
+    x = "bar";
+} else if (length === 2) {
+    x = "something else";
+} else {
+    x = "unknown";
+}
+
+if (length === 0) {
+    x = "foo";
+} else {
+    if (length === 1) {
+        x = "bar";
+    } else {
+        if (length === 2) {
+            x = "something else";
+        } else {
+            x = "unknown";
+        }
+    }
+}
+
+if (length === 0) {
+    x = "foo";
+} else if (length === 1) {
+    x = "bar";
+} else if (length === 2) {
+    x = "something else";
+} else {
+    foo();
+    // detects nested if statements when not direct child of else
+    if (bar) {
+    ~~ [err % ('x')]
+        x = 1;
+    } else {
+        x = 2;
+    }
+}
+
 [err]: Use a conditional expression instead of assigning to '%s' in multiple places.