diff --git a/src/analysis/blocks-analyzer.ts b/src/analysis/blocks-analyzer.ts index 7abd310..7aaa948 100644 --- a/src/analysis/blocks-analyzer.ts +++ b/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(); - private readonly allActions = new Set(); private actionsExceededMaximum = false; + private circularDependenciesFound = false; + private readonly dependencies = new Map>(); + + public readonly workflows = new Set(); + public readonly actions = new Set(); public constructor(document: BoundDocument, private readonly bag: DiagnosticBag) { super(); this.visit(document); } - public get workflows(): ReadonlySet { - return this.allWorkflows; - } - - public get actions(): ReadonlySet { - 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()); } 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): 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); + } } diff --git a/src/analysis/properties-analyzer.ts b/src/analysis/properties-analyzer.ts index 7766637..1dd6aaf 100644 --- a/src/analysis/properties-analyzer.ts +++ b/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(); private exceededMaximum = false; - private allSecrets = new Set(); 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; diff --git a/src/util/diagnostics.ts b/src/util/diagnostics.ts index 1d69749..3339569 100644 --- a/src/util/diagnostics.ts +++ b/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.`, }); } diff --git a/test/blocks-analysis.spec.ts b/test/blocks-analysis.spec.ts index 9e1623e..a087680 100644 --- a/test/blocks-analysis.spec.ts +++ b/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\\"] } +" `); }); });