fix: Ensure correct code path for && followed by ?? (#17618)

nzakas · mdjermanovic · web-flow · commit d2f68019b888 · 2023-10-04T11:30:06.000+02:00
* fix: Ensure correct code path for && followed by ?? fixes #13614 * Remove .only * Update lib/linter/code-path-analysis/code-path-state.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update tests/lib/linter/code-path-analysis/code-path-analyzer.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * Update variable names in comments --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
diff --git a/lib/linter/code-path-analysis/code-path-state.js b/lib/linter/code-path-analysis/code-path-state.js
@@ -121,7 +121,7 @@ class ChainContext {
  * true go one way and if false go the other (tracked by `trueForkContext` and
  * `falseForkContext`). The `??` operator doesn't operate on true/false because
  * the left expression is evaluated to be nullish or not, so only if nullish do
- * we fork to the right expression (tracked by `qqForkcontext`).
+ * we fork to the right expression (tracked by `nullishForkContext`).
  */
 class ChoiceContext {
 
@@ -170,14 +170,14 @@ class ChoiceContext {
         this.falseForkContext = ForkContext.newEmpty(forkContext);
 
         /**
-         * The fork context for the right side of the `??` path of the choice.
+         * The fork context for when the choice result is `null` or `undefined`.
          * @type {ForkContext}
          */
-        this.qqForkContext = ForkContext.newEmpty(forkContext);
+        this.nullishForkContext = ForkContext.newEmpty(forkContext);
 
         /**
          * Indicates if any of `trueForkContext`, `falseForkContext`, or
-         * `qqForkContext` have been updated with segments from a child context.
+         * `nullishForkContext` have been updated with segments from a child context.
          * @type {boolean}
          */
         this.processed = false;
@@ -849,7 +849,7 @@ function finalizeTestSegmentsOfFor(context, choiceContext, head) {
     if (!choiceContext.processed) {
         choiceContext.trueForkContext.add(head);
         choiceContext.falseForkContext.add(head);
-        choiceContext.qqForkContext.add(head);
+        choiceContext.nullishForkContext.add(head);
     }
 
     /*
@@ -1095,31 +1095,31 @@ class CodePathState {
 
     /**
      * Pops the last choice context and finalizes it.
+     * This is called upon leaving a node that represents a choice.
      * @throws {Error} (Unreachable.)
      * @returns {ChoiceContext} The popped context.
      */
     popChoiceContext() {
-        const context = this.choiceContext;
-
-        this.choiceContext = context.upper;
-
+        const poppedChoiceContext = this.choiceContext;
         const forkContext = this.forkContext;
-        const headSegments = forkContext.head;
+        const head = forkContext.head;
 
-        switch (context.kind) {
+        this.choiceContext = poppedChoiceContext.upper;
+
+        switch (poppedChoiceContext.kind) {
             case "&&":
             case "||":
             case "??":
 
                 /*
-                 * The `headSegments` are the path of the right-hand operand.
+                 * The `head` are the path of the right-hand operand.
                  * If we haven't previously added segments from child contexts,
                  * then we add these segments to all possible forks.
                  */
-                if (!context.processed) {
-                    context.trueForkContext.add(headSegments);
-                    context.falseForkContext.add(headSegments);
-                    context.qqForkContext.add(headSegments);
+                if (!poppedChoiceContext.processed) {
+                    poppedChoiceContext.trueForkContext.add(head);
+                    poppedChoiceContext.falseForkContext.add(head);
+                    poppedChoiceContext.nullishForkContext.add(head);
                 }
 
                 /*
@@ -1128,38 +1128,38 @@ class CodePathState {
                  * then we take the segments for this context and move them up
                  * to the parent context.
                  */
-                if (context.isForkingAsResult) {
+                if (poppedChoiceContext.isForkingAsResult) {
                     const parentContext = this.choiceContext;
 
-                    parentContext.trueForkContext.addAll(context.trueForkContext);
-                    parentContext.falseForkContext.addAll(context.falseForkContext);
-                    parentContext.qqForkContext.addAll(context.qqForkContext);
+                    parentContext.trueForkContext.addAll(poppedChoiceContext.trueForkContext);
+                    parentContext.falseForkContext.addAll(poppedChoiceContext.falseForkContext);
+                    parentContext.nullishForkContext.addAll(poppedChoiceContext.nullishForkContext);
                     parentContext.processed = true;
 
                     // Exit early so we don't collapse all paths into one.
-                    return context;
+                    return poppedChoiceContext;
                 }
 
                 break;
 
             case "test":
-                if (!context.processed) {
+                if (!poppedChoiceContext.processed) {
 
                     /*
                      * The head segments are the path of the `if` block here.
                      * Updates the `true` path with the end of the `if` block.
                      */
-                    context.trueForkContext.clear();
-                    context.trueForkContext.add(headSegments);
+                    poppedChoiceContext.trueForkContext.clear();
+                    poppedChoiceContext.trueForkContext.add(head);
                 } else {
 
                     /*
                      * The head segments are the path of the `else` block here.
                      * Updates the `false` path with the end of the `else`
                      * block.
                      */
-                    context.falseForkContext.clear();
-                    context.falseForkContext.add(headSegments);
+                    poppedChoiceContext.falseForkContext.clear();
+                    poppedChoiceContext.falseForkContext.add(head);
                 }
 
                 break;
@@ -1170,7 +1170,7 @@ class CodePathState {
                  * Loops are addressed in `popLoopContext()` so just return
                  * the context without modification.
                  */
-                return context;
+                return poppedChoiceContext;
 
             /* c8 ignore next */
             default:
@@ -1180,71 +1180,116 @@ class CodePathState {
         /*
          * Merge the true path with the false path to create a single path.
          */
-        const combinedForkContext = context.trueForkContext;
+        const combinedForkContext = poppedChoiceContext.trueForkContext;
 
-        combinedForkContext.addAll(context.falseForkContext);
+        combinedForkContext.addAll(poppedChoiceContext.falseForkContext);
         forkContext.replaceHead(combinedForkContext.makeNext(0, -1));
 
-        return context;
+        return poppedChoiceContext;
     }
 
     /**
-     * Makes a code path segment of the right-hand operand of a logical
+     * Creates a code path segment to represent right-hand operand of a logical
      * expression.
+     * This is called in the preprocessing phase when entering a node.
      * @throws {Error} (Unreachable.)
      * @returns {void}
      */
     makeLogicalRight() {
-        const context = this.choiceContext;
+        const currentChoiceContext = this.choiceContext;
         const forkContext = this.forkContext;
 
-        if (context.processed) {
+        if (currentChoiceContext.processed) {
 
             /*
-             * This got segments already from the child choice context.
-             * Creates the next path from own true/false fork context.
+             * This context was already assigned segments from a child
+             * choice context. In this case, we are concerned only about
+             * the path that does not short-circuit and so ends up on the
+             * right-hand operand of the logical expression.
              */
             let prevForkContext;
 
-            switch (context.kind) {
+            switch (currentChoiceContext.kind) {
                 case "&&": // if true then go to the right-hand side.
-                    prevForkContext = context.trueForkContext;
+                    prevForkContext = currentChoiceContext.trueForkContext;
                     break;
                 case "||": // if false then go to the right-hand side.
-                    prevForkContext = context.falseForkContext;
+                    prevForkContext = currentChoiceContext.falseForkContext;
                     break;
-                case "??": // Both true/false can short-circuit, so needs the third path to go to the right-hand side. That's qqForkContext.
-                    prevForkContext = context.qqForkContext;
+                case "??": // Both true/false can short-circuit, so needs the third path to go to the right-hand side. That's nullishForkContext.
+                    prevForkContext = currentChoiceContext.nullishForkContext;
                     break;
                 default:
                     throw new Error("unreachable");
             }
 
+            /*
+             * Create the segment for the right-hand operand of the logical expression
+             * and adjust the fork context pointer to point there. The right-hand segment
+             * is added at the end of all segments in `prevForkContext`.
+             */
             forkContext.replaceHead(prevForkContext.makeNext(0, -1));
+
+            /*
+             * We no longer need this list of segments.
+             *
+             * Reset `processed` because we've removed the segments from the child
+             * choice context. This allows `popChoiceContext()` to continue adding
+             * segments later.
+             */
             prevForkContext.clear();
-            context.processed = false;
+            currentChoiceContext.processed = false;
+
         } else {
 
             /*
-             * This did not get segments from the child choice context.
-             * So addresses the head segments.
-             * The head segments are the path of the left-hand operand.
+             * This choice context was not assigned segments from a child
+             * choice context, which means that it's a terminal logical
+             * expression.
+             *
+             * `head` is the segments for the left-hand operand of the
+             * logical expression.
+             *
+             * Each of the fork contexts below are empty at this point. We choose
+             * the path(s) that will short-circuit and add the segment for the
+             * left-hand operand to it. Ultimately, this will be the only segment
+             * in that path due to the short-circuting, so we are just seeding
+             * these paths to start.
              */
-            switch (context.kind) {
-                case "&&": // the false path can short-circuit.
-                    context.falseForkContext.add(forkContext.head);
+            switch (currentChoiceContext.kind) {
+                case "&&":
+
+                    /*
+                     * In most contexts, when a && expression evaluates to false,
+                     * it short circuits, so we need to account for that by setting
+                     * the `falseForkContext` to the left operand.
+                     *
+                     * When a && expression is the left-hand operand for a ??
+                     * expression, such as `(a && b) ?? c`, a nullish value will
+                     * also short-circuit in a different way than a false value,
+                     * so we also set the `nullishForkContext` to the left operand.
+                     * This path is only used with a ?? expression and is thrown
+                     * away for any other type of logical expression, so it's safe
+                     * to always add.
+                     */
+                    currentChoiceContext.falseForkContext.add(forkContext.head);
+                    currentChoiceContext.nullishForkContext.add(forkContext.head);
                     break;
                 case "||": // the true path can short-circuit.
-                    context.trueForkContext.add(forkContext.head);
+                    currentChoiceContext.trueForkContext.add(forkContext.head);
                     break;
                 case "??": // both can short-circuit.
-                    context.trueForkContext.add(forkContext.head);
-                    context.falseForkContext.add(forkContext.head);
+                    currentChoiceContext.trueForkContext.add(forkContext.head);
+                    currentChoiceContext.falseForkContext.add(forkContext.head);
                     break;
                 default:
                     throw new Error("unreachable");
             }
 
+            /*
+             * Create the segment for the right-hand operand of the logical expression
+             * and adjust the fork context pointer to point there.
+             */
             forkContext.replaceHead(forkContext.makeNext(-1, -1));
         }
     }
@@ -1265,7 +1310,7 @@ class CodePathState {
         if (!context.processed) {
             context.trueForkContext.add(forkContext.head);
             context.falseForkContext.add(forkContext.head);
-            context.qqForkContext.add(forkContext.head);
+            context.nullishForkContext.add(forkContext.head);
         }
 
         context.processed = false;
diff --git a/tests/fixtures/code-path-analysis/assignment--nested-and-3.js b/tests/fixtures/code-path-analysis/assignment--nested-and-3.js
@@ -1,7 +1,8 @@
 /*expected
 initial->s1_1->s1_2->s1_3->s1_4;
-s1_1->s1_4;
-s1_2->s1_4->final;
+s1_1->s1_3;
+s1_2->s1_4;
+s1_1->s1_4->final;
 */
 (a &&= b) ?? c;
 
@@ -15,7 +16,8 @@ digraph {
     s1_3[label="Identifier (c)"];
     s1_4[label="LogicalExpression:exit\nExpressionStatement:exit\nProgram:exit"];
     initial->s1_1->s1_2->s1_3->s1_4;
-    s1_1->s1_4;
-    s1_2->s1_4->final;
+    s1_1->s1_3;
+    s1_2->s1_4;
+    s1_1->s1_4->final;
 }
 */
diff --git a/tests/fixtures/code-path-analysis/logical--and-qq.js b/tests/fixtures/code-path-analysis/logical--and-qq.js
@@ -0,0 +1,22 @@
+/*expected
+initial->s1_1->s1_2->s1_3->s1_4;
+s1_1->s1_3;
+s1_2->s1_4;
+s1_1->s1_4->final;
+*/
+(a && b) ?? c;
+
+/*DOT
+digraph {
+    node[shape=box,style="rounded,filled",fillcolor=white];
+    initial[label="",shape=circle,style=filled,fillcolor=black,width=0.25,height=0.25];
+    final[label="",shape=doublecircle,style=filled,fillcolor=black,width=0.25,height=0.25];
+    s1_1[label="Program:enter\nExpressionStatement:enter\nLogicalExpression:enter\nLogicalExpression:enter\nIdentifier (a)"];
+    s1_2[label="Identifier (b)\nLogicalExpression:exit"];
+    s1_3[label="Identifier (c)"];
+    s1_4[label="LogicalExpression:exit\nExpressionStatement:exit\nProgram:exit"];
+    initial->s1_1->s1_2->s1_3->s1_4;
+    s1_1->s1_3;
+    s1_2->s1_4;
+    s1_1->s1_4->final;
+}*/
diff --git a/tests/fixtures/code-path-analysis/logical--if-mix-and-qq-1.js b/tests/fixtures/code-path-analysis/logical--if-mix-and-qq-1.js
@@ -1,8 +1,9 @@
 /*expected
 initial->s1_1->s1_2->s1_3->s1_4->s1_6;
-s1_1->s1_5->s1_6;
+s1_1->s1_3;
 s1_2->s1_4;
-s1_3->s1_5;
+s1_3->s1_5->s1_6;
+s1_1->s1_5;
 s1_2->s1_5;
 s1_6->final;
 */
@@ -24,9 +25,10 @@ digraph {
     s1_6[label="IfStatement:exit\nProgram:exit"];
     s1_5[label="BlockStatement\nExpressionStatement\nCallExpression\nIdentifier (bar)\nIdentifier:exit (bar)\nCallExpression:exit\nExpressionStatement:exit\nBlockStatement:exit"];
     initial->s1_1->s1_2->s1_3->s1_4->s1_6;
-    s1_1->s1_5->s1_6;
+    s1_1->s1_3;
     s1_2->s1_4;
-    s1_3->s1_5;
+    s1_3->s1_5->s1_6;
+    s1_1->s1_5;
     s1_2->s1_5;
     s1_6->final;
 }
