Merge branch 'main' into graph-script-diagnostics

Author: tmr232

CHANGELOG.md

@@ -6,6 +6,15 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
 
 ## [Unreleased]
 
+
+### Fixed
+
+- Comment-tests now fail if an unsupported test type is present instead of passing silently.
+
+### Changed
+
+- Empty switch-cases with fallthrough now chain in a cleaner way in flat-switches.
+
 ## [0.0.14] - 2025-02-17
 
 ### Added

src/components/utils.ts

@@ -69,10 +69,7 @@ export function runTest(record: TestFuncRecord): TestResults[] {
   };
   const testResults = [];
   for (const [key, value] of Object.entries(testFunc.reqs)) {
-    const reqHandler = requirementTests[key];
-    if (!reqHandler) {
-      continue;
-    }
+    const reqHandler = requirementTests[key as keyof typeof testFunc.reqs];
     testResults.push({
       reqName: key,
       reqValue: value,

src/control-flow/cfg-defs.ts

@@ -303,15 +303,6 @@ export function mergeNodeAttrs(
     startOffset: startOffset,
   };
 }
-export interface Case {
-  conditionEntry: string;
-  conditionExit: string;
-  consequenceEntry: string;
-  consequenceExit: string | null;
-  alternativeExit: string;
-  hasFallthrough: boolean;
-  isDefault: boolean;
-}
 
 export interface BuilderOptions {
   /**

src/control-flow/switch-utils.ts

@@ -1,38 +1,89 @@
 import type Parser from "web-tree-sitter";
-import type { Case, EdgeType } from "./cfg-defs";
+import type { EdgeType } from "./cfg-defs";
 import type { Context } from "./generic-cfg-builder";
 import { pairwise } from "./itertools.ts";
+
 export interface SwitchOptions {
   /// A Go `select` blocks until one of the branches matches.
   /// This means that we never add an alternative edge from the
   /// head to the merge node. There is no implicit-default.
   noImplicitDefault?: boolean;
 }
 
+/**
+ * Represents a single case inside a switch statement.
+ *
+ * Cases are build of a condition and a consequence,
+ * where the condition _may_ lead to the consequence.
+ */
+export interface Case {
+  /**
+   * The condition entry node
+   */
+  conditionEntry: string;
+  /**
+   * The condition exit node
+   */
+  conditionExit: string;
+  consequenceEntry: string;
+  consequenceExit: string | null;
+  alternativeExit: string;
+  /**
+   * Does this case fall-through to the next one?
+   */
+  hasFallthrough: boolean;
+  /**
+   * Is this the default case?
+   */
+  isDefault: boolean;
+  /**
+   * A case may be entirely empty
+   */
+  isEmpty: boolean;
+}
+
 export function buildSwitch(
   cases: Case[],
   mergeNode: string,
   switchHeadNode: string,
   options: SwitchOptions,
   ctx: Context,
 ) {
+  // Fallthrough from the previous case
   let fallthrough: string | null = null;
+  let hasDefaultCase = false;
   let previous: string | null = switchHeadNode;
   for (const thisCase of cases) {
     if (ctx.options.flatSwitch) {
       ctx.builder.addEdge(switchHeadNode, thisCase.conditionEntry);
-      ctx.builder.addEdge(thisCase.conditionExit, thisCase.consequenceEntry);
-      if (fallthrough) {
-        ctx.builder.addEdge(fallthrough, thisCase.consequenceEntry);
-      }
-      if (thisCase.isDefault) {
-        // If we have any default node - then we don't connect the head to the merge node.
-        previous = null;
+      if (thisCase.isEmpty && thisCase.hasFallthrough) {
+        // When we have an empty fallthrough case, we ignore its consequence node.
+        // Instead, we link it directly to the condition node of the next case.
+        // This allows for nice chaining while avoiding the tree-like artifacts
+        // found in https://github.com/tmr232/function-graph-overview/issues/77
+        if (fallthrough) {
+          ctx.builder.addEdge(fallthrough, thisCase.conditionEntry);
+        }
+        fallthrough = thisCase.conditionExit;
+      } else {
+        ctx.builder.addEdge(thisCase.conditionExit, thisCase.consequenceEntry);
+
+        if (fallthrough) {
+          ctx.builder.addEdge(fallthrough, thisCase.consequenceEntry);
+        }
+
+        if (!thisCase.hasFallthrough && thisCase.consequenceExit) {
+          ctx.builder.addEdge(thisCase.consequenceExit, mergeNode, "regular");
+        }
+        // Update for next case
+        fallthrough = thisCase.hasFallthrough ? thisCase.consequenceExit : null;
       }
     } else {
+      /* Model the switch as an if-elif-else chain */
       if (fallthrough) {
         ctx.builder.addEdge(fallthrough, thisCase.consequenceEntry);
       }
+
       if (previous && thisCase.conditionEntry) {
         ctx.builder.addEdge(
           previous,
@@ -50,18 +101,19 @@ export function buildSwitch(
 
       // Update for next case
       previous = thisCase.isDefault ? null : thisCase.alternativeExit;
-    }
 
-    // Fallthrough is the same for both flat and non-flat layouts.
-    if (!thisCase.hasFallthrough && thisCase.consequenceExit) {
-      ctx.builder.addEdge(thisCase.consequenceExit, mergeNode, "regular");
+      if (!thisCase.hasFallthrough && thisCase.consequenceExit) {
+        ctx.builder.addEdge(thisCase.consequenceExit, mergeNode, "regular");
+      }
+      // Update for next case
+      fallthrough = thisCase.hasFallthrough ? thisCase.consequenceExit : null;
     }
-    // Update for next case
-    fallthrough = thisCase.hasFallthrough ? thisCase.consequenceExit : null;
+
+    hasDefaultCase ||= thisCase.isDefault;
   }
   // Connect the last node to the merge node.
   // No need to handle `fallthrough` here as it is not allowed for the last case.
-  if (previous && !options.noImplicitDefault) {
+  if (previous && !hasDefaultCase && !options.noImplicitDefault) {
     ctx.builder.addEdge(previous, mergeNode, "alternative");
   }
 
@@ -113,6 +165,15 @@ export function collectCases(
       );
     }
 
+    // We want to mark empty nodes, so that we can avoid linking their
+    // consequence nodes.
+    // It is true that Go's cases are "empty" even if they have a `fallthrough`
+    // keyword as their only statement, but we can safely ignore those.
+    // That is because a Go switch allows multiple conditions, making
+    // the common case of a huge switch with many cases less common.
+    // If it comes up in practice - we'll address it.
+    const isEmpty = consequence.length === 0;
+
     cases.push({
       conditionEntry: conditionNode,
       conditionExit: conditionNode,
@@ -121,6 +182,7 @@ export function collectCases(
       alternativeExit: conditionNode,
       hasFallthrough,
       isDefault,
+      isEmpty,
     });
   }