diff --git a/expressions/src/evaluator.ts b/expressions/src/evaluator.ts index 8f5964bf..2b4cea4b 100644 --- a/expressions/src/evaluator.ts +++ b/expressions/src/evaluator.ts @@ -32,7 +32,7 @@ export class Evaluator implements ExprVisitor { return this.eval(this.n); } - private eval(n: Expr): data.ExpressionData { + protected eval(n: Expr): data.ExpressionData { return n.accept(this); } diff --git a/languageservice/src/expression-validation/evaluator.ts b/languageservice/src/expression-validation/evaluator.ts new file mode 100644 index 00000000..28a7afe9 --- /dev/null +++ b/languageservice/src/expression-validation/evaluator.ts @@ -0,0 +1,61 @@ +import {Evaluator, ExpressionEvaluationError, data} from "@actions/expressions"; +import {Expr, Logical} from "@actions/expressions/ast"; +import {ExpressionData} from "@actions/expressions/data/expressiondata"; +import {TokenType} from "@actions/expressions/lexer"; +import {falsy, truthy} from "@actions/expressions/result"; +import {AccessError} from "./error-dictionary"; + +export type ValidationError = { + message: string; + severity: "error" | "warning"; +}; + +export class ValidationEvaluator extends Evaluator { + public readonly errors: ValidationError[] = []; + + public validate() { + super.evaluate(); + } + + protected override eval(n: Expr): ExpressionData { + try { + return super.eval(n); + } catch (e) { + // Record error + if (e instanceof AccessError) { + this.errors.push({ + message: `Context access might be invalid: ${e.keyName}`, + severity: "warning" + }); + } else if (e instanceof ExpressionEvaluationError) { + this.errors.push({ + message: `Expression might be invalid: ${e.message}`, + severity: "error" + }); + } + } + + // Return null but continue with the validation + return new data.Null(); + } + + override visitLogical(logical: Logical): ExpressionData { + let result: data.ExpressionData | undefined; + + for (const arg of logical.args) { + const r = this.eval(arg); + + // Simulate short-circuit behavior but continue to evalute all arguments for validation purposes + if ( + !result && + ((logical.operator.type === TokenType.AND && falsy(r)) || (logical.operator.type === TokenType.OR && truthy(r))) + ) { + result = r; + } + } + + // result is always assigned before we return here + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return result!; + } +} diff --git a/languageservice/src/validate.expressions.test.ts b/languageservice/src/validate.expressions.test.ts index 2704d757..94f50413 100644 --- a/languageservice/src/validate.expressions.test.ts +++ b/languageservice/src/validate.expressions.test.ts @@ -46,6 +46,52 @@ jobs: ]); }); + it("access invalid context field in short-circuited expression", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: push +run-name: name-\${{ github.does-not-exist || github.does-not-exist2 }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo` + ) + ); + + expect(result).toEqual([ + { + message: "Context access might be invalid: does-not-exist", + range: { + end: { + character: 69, + line: 1 + }, + start: { + character: 15, + line: 1 + } + }, + severity: DiagnosticSeverity.Warning + }, + { + message: "Context access might be invalid: does-not-exist2", + range: { + end: { + character: 69, + line: 1 + }, + start: { + character: 15, + line: 1 + } + }, + severity: DiagnosticSeverity.Warning + } + ]); + }); + it("partial skip access invalid context on incomplete", async () => { const contextProviderConfig: ContextProviderConfig = { getContext: (context: string) => { diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index 34ddea59..3bcbd0af 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -1,6 +1,6 @@ -import {Evaluator, ExpressionEvaluationError, Lexer, Parser} from "@actions/expressions"; +import {Lexer, Parser} from "@actions/expressions"; import {Expr} from "@actions/expressions/ast"; -import {isBasicExpression, isString, ParseWorkflowResult, WorkflowTemplate} from "@actions/workflow-parser"; +import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context"; import {BasicExpressionToken} from "@actions/workflow-parser/templates/tokens/basic-expression-token"; @@ -13,9 +13,10 @@ import {TextDocument} from "vscode-languageserver-textdocument"; import {Diagnostic, DiagnosticSeverity, URI} from "vscode-languageserver-types"; import {ActionMetadata, ActionReference} from "./action"; import {ContextProviderConfig} from "./context-providers/config"; -import {getContext, Mode} from "./context-providers/default"; -import {getWorkflowContext, WorkflowContext} from "./context/workflow-context"; -import {AccessError, wrapDictionary} from "./expression-validation/error-dictionary"; +import {Mode, getContext} from "./context-providers/default"; +import {WorkflowContext, getWorkflowContext} from "./context/workflow-context"; +import {wrapDictionary} from "./expression-validation/error-dictionary"; +import {ValidationEvaluator} from "./expression-validation/evaluator"; import {validatorFunctions} from "./expression-validation/functions"; import {error} from "./log"; import {findToken} from "./utils/find-token"; @@ -202,28 +203,17 @@ async function validateExpression( continue; } - try { - const context = await getContext(namedContexts, contextProviderConfig, workflowContext, Mode.Validation); - - const e = new Evaluator(expr, wrapDictionary(context), validatorFunctions); - e.evaluate(); - - // Any invalid context access would've thrown an error via the `ErrorDictionary`, for now we don't have to check the actual - // result of the evaluation. - } catch (e) { - if (e instanceof AccessError) { - diagnostics.push({ - message: `Context access might be invalid: ${e.keyName}`, - severity: DiagnosticSeverity.Warning, - range: mapRange(expression.range) - }); - } else if (e instanceof ExpressionEvaluationError) { - diagnostics.push({ - message: `Expression might be invalid: ${e.message}`, - severity: DiagnosticSeverity.Error, - range: mapRange(expression.range) - }); - } - } + const context = await getContext(namedContexts, contextProviderConfig, workflowContext, Mode.Validation); + + const e = new ValidationEvaluator(expr, wrapDictionary(context), validatorFunctions); + e.validate(); + + diagnostics.push( + ...e.errors.map(e => ({ + message: e.message, + range: mapRange(expression.range), + severity: e.severity === "error" ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning + })) + ); } }