feat: report errors on actions circular dependencies

src/analysis/blocks-analyzer.ts

@@ -4,8 +4,9 @@
 
 import { BoundNodeVisitor } from "../binding/bound-node-visitor";
 import { DiagnosticBag } from "../util/diagnostics";
-import { BoundDocument, BoundAction, BoundWorkflow } from "../binding/bound-nodes";
+import { BoundDocument, BoundAction, BoundWorkflow, BoundStringValue } from "../binding/bound-nodes";
 import { MAXIMUM_SUPPORTED_ACTIONS } from "../util/constants";
+import { TextRange } from "../scanning/tokens";
 
 export function analyzeBlocks(
   document: BoundDocument,
@@ -22,41 +23,67 @@ export function analyzeBlocks(
 }
 
 class BlocksAnalyzer extends BoundNodeVisitor {
-  private readonly allWorkflows = new Set<string>();
-  private readonly allActions = new Set<string>();
   private actionsExceededMaximum = false;
+  private circularDependenciesFound = false;
+  private readonly dependencies = new Map<string, ReadonlyArray<BoundStringValue>>();
+
+  public readonly workflows = new Set<string>();
+  public readonly actions = new Set<string>();
 
   public constructor(document: BoundDocument, private readonly bag: DiagnosticBag) {
     super();
     this.visit(document);
   }
 
-  public get workflows(): ReadonlySet<string> {
-    return this.allWorkflows;
-  }
-
-  public get actions(): ReadonlySet<string> {
-    return this.allActions;
-  }
-
   protected visitAction(node: BoundAction): void {
-    if (this.allActions.has(node.name) || this.allWorkflows.has(node.name)) {
+    if (this.actions.has(node.name) || this.workflows.has(node.name)) {
       this.bag.duplicateBlock(node.name, node.syntax.name.range);
     } else {
-      this.allActions.add(node.name);
+      this.actions.add(node.name);
     }
 
-    if (!this.actionsExceededMaximum && this.allActions.size > MAXIMUM_SUPPORTED_ACTIONS) {
+    if (!this.actionsExceededMaximum && this.actions.size > MAXIMUM_SUPPORTED_ACTIONS) {
       this.bag.tooManyActions(node.syntax.name.range);
       this.actionsExceededMaximum = true;
     }
+
+    if (node.needs) {
+      this.dependencies.set(node.name, node.needs.actions);
+    } else {
+      this.dependencies.set(node.name, []);
+    }
+
+    this.checkCircularDependencies(node.name, node.syntax.name.range, new Set<string>());
   }
 
   protected visitWorkflow(node: BoundWorkflow): void {
-    if (this.allActions.has(node.name) || this.allWorkflows.has(node.name)) {
+    if (this.actions.has(node.name) || this.workflows.has(node.name)) {
       this.bag.duplicateBlock(node.name, node.syntax.name.range);
     } else {
-      this.allWorkflows.add(node.name);
+      this.workflows.add(node.name);
     }
   }
+
+  private checkCircularDependencies(action: string, range: TextRange, visited: Set<string>): void {
+    if (this.circularDependenciesFound) {
+      return;
+    }
+
+    if (visited.has(action)) {
+      this.circularDependenciesFound = true;
+      this.bag.circularDependency(action, range);
+      return;
+    }
+
+    visited.add(action);
+
+    const dependencies = this.dependencies.get(action);
+    if (dependencies) {
+      for (const dependency of dependencies) {
+        this.checkCircularDependencies(dependency.value, dependency.syntax.range, visited);
+      }
+    }
+
+    visited.delete(action);
+  }
 }

src/analysis/properties-analyzer.ts

@@ -13,8 +13,8 @@ export function analyzeProperties(document: BoundDocument, actions: ReadonlySet<
 }
 
 class PropertiesAnalyzer extends BoundNodeVisitor {
+  private readonly secrets = new Set<string>();
   private exceededMaximum = false;
-  private allSecrets = new Set<string>();
 
   public constructor(
     document: BoundDocument,
@@ -53,8 +53,8 @@ class PropertiesAnalyzer extends BoundNodeVisitor {
 
     if (!this.exceededMaximum) {
       for (const secret of node.secrets) {
-        this.allSecrets.add(secret.value);
-        if (this.allSecrets.size > MAXIMUM_SUPPORTED_SECRETS) {
+        this.secrets.add(secret.value);
+        if (this.secrets.size > MAXIMUM_SUPPORTED_SECRETS) {
           this.bag.tooManySecrets(secret.syntax.range);
           this.exceededMaximum = true;
           break;

src/util/diagnostics.ts

@@ -30,13 +30,14 @@ export enum DiagnosticCode {
   // Block Analysis
   TooManyActions,
   DuplicateBlock,
+  CircularDependency,
 
   // Property Analysis
+  ActionDoesNotExist,
   TooManySecrets,
   DuplicateSecrets,
-  ActionDoesNotExist,
-  UnrecognizedEvent,
   ReservedEnvironmentVariable,
+  UnrecognizedEvent,
 }
 
 export interface Diagnostic {
@@ -180,51 +181,59 @@ export class DiagnosticBag {
     });
   }
 
-  public reservedEnvironmentVariable(range: TextRange): void {
+  public tooManyActions(range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.ReservedEnvironmentVariable,
-      message: `Environment variables starting with 'GITHUB_' are reserved.`,
+      code: DiagnosticCode.TooManyActions,
+      message: `Too many actions defined. The maximum currently supported is '${MAXIMUM_SUPPORTED_ACTIONS}'.`,
     });
   }
 
-  public tooManySecrets(range: TextRange): void {
+  public duplicateBlock(duplicate: string, range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.TooManySecrets,
-      message: `Too many secrets defined. The maximum currently supported is '${MAXIMUM_SUPPORTED_SECRETS}'.`,
+      code: DiagnosticCode.DuplicateBlock,
+      message: `This file already defines another workflow or action with the name '${duplicate}'.`,
     });
   }
 
-  public duplicateSecrets(duplicate: string, range: TextRange): void {
+  public circularDependency(action: string, range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.DuplicateSecrets,
-      message: `This 'secrets' property has duplicate '${duplicate}' secrets.`,
+      code: DiagnosticCode.CircularDependency,
+      message: `The action '${action}' has a circular dependency on itself.`,
     });
   }
 
-  public tooManyActions(range: TextRange): void {
+  public actionDoesNotExist(action: string, range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.TooManyActions,
-      message: `Too many actions defined. The maximum currently supported is '${MAXIMUM_SUPPORTED_ACTIONS}'.`,
+      code: DiagnosticCode.ActionDoesNotExist,
+      message: `The action '${action}' does not exist in the same workflow file.`,
     });
   }
 
-  public duplicateBlock(duplicate: string, range: TextRange): void {
+  public tooManySecrets(range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.DuplicateBlock,
-      message: `This file already defines another workflow or action with the name '${duplicate}'.`,
+      code: DiagnosticCode.TooManySecrets,
+      message: `Too many secrets defined. The maximum currently supported is '${MAXIMUM_SUPPORTED_SECRETS}'.`,
     });
   }
 
-  public actionDoesNotExist(action: string, range: TextRange): void {
+  public duplicateSecrets(duplicate: string, range: TextRange): void {
     this.items.push({
       range,
-      code: DiagnosticCode.ActionDoesNotExist,
-      message: `The action '${action}' does not exist in the same workflow file.`,
+      code: DiagnosticCode.DuplicateSecrets,
+      message: `This 'secrets' property has duplicate '${duplicate}' secrets.`,
+    });
+  }
+
+  public reservedEnvironmentVariable(range: TextRange): void {
+    this.items.push({
+      range,
+      code: DiagnosticCode.ReservedEnvironmentVariable,
+      message: `Environment variables starting with 'GITHUB_' are reserved.`,
     });
   }
 

test/blocks-analysis.spec.ts

@@ -188,6 +188,26 @@ ERROR: Too many actions defined. The maximum currently supported is '100'.
     |        ^^^^^^^^^^^
 103 | 
 "
+`);
+  });
+
+  it("reports error on cycling dependencies in actions", () => {
+    expectDiagnostics(`
+action "a" { uses = "./ci" }
+action "b" { uses = "./ci" needs = ["a"] }
+action "c" { uses = "./ci" needs = ["a", "b", "e"] }
+action "d" { uses = "./ci" needs = ["c"] }
+action "e" { uses = "./ci" needs = ["c"] }
+`).toMatchInlineSnapshot(`
+"
+ERROR: The action 'e' has a circular dependency on itself.
+  2 | action \\"a\\" { uses = \\"./ci\\" }
+  3 | action \\"b\\" { uses = \\"./ci\\" needs = [\\"a\\"] }
+  4 | action \\"c\\" { uses = \\"./ci\\" needs = [\\"a\\", \\"b\\", \\"e\\"] }
+    |                                               ^^^
+  5 | action \\"d\\" { uses = \\"./ci\\" needs = [\\"c\\"] }
+  6 | action \\"e\\" { uses = \\"./ci\\" needs = [\\"c\\"] }
+"
 `);
   });
 });