diff --git a/languageservice/src/expression-hover/expression-pos.ts b/languageservice/src/expression-hover/expression-pos.ts index 2cde489..c01e659 100644 --- a/languageservice/src/expression-hover/expression-pos.ts +++ b/languageservice/src/expression-hover/expression-pos.ts @@ -1,6 +1,6 @@ import {Pos} from "@actions/expressions/lexer"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; -import {isBasicExpression} from "@actions/workflow-parser/templates/tokens/type-guards"; +import {isBasicExpression, isString} from "@actions/workflow-parser/templates/tokens/type-guards"; import {Position, Range as LSPRange} from "vscode-languageserver-textdocument"; import {mapRange} from "../utils/range"; import {posWithinRange} from "./pos-range"; @@ -16,12 +16,52 @@ export type ExpressionPos = { documentRange: LSPRange; }; +/** + * Maps a document position to an expression position for hover/completion features. + * + * This handles both explicit expressions (with ${{ }}) and implicit expressions (like if conditions). + * For if conditions without ${{ }}, this applies the same conversion as the parser's convertToIfCondition, + * wrapping them in `success() && (...)` when no status function is present. + * + * @param token The template token at the position + * @param position The position in the document + * @returns Expression and adjusted position, or undefined if not an expression + */ export function mapToExpressionPos(token: TemplateToken, position: Position): ExpressionPos | undefined { const pos: Pos = { line: position.line + 1, column: position.character + 1 }; + // Handle if conditions that are string tokens (job-if, step-if, snapshot-if) + const definitionKey = token.definition?.key; + if ( + isString(token) && + token.range && + (definitionKey === "job-if" || definitionKey === "step-if" || definitionKey === "snapshot-if") + ) { + const condition = token.value.trim(); + if (condition) { + // Check if the condition already contains a status function + const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); + const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`; + + const exprRange = mapRange(token.range); + + // Calculate offset for wrapping + const offset = hasStatusFunction ? 0 : "success() && (".length; + + return { + expression: finalCondition, + position: { + line: pos.line - exprRange.start.line - 1, + column: pos.column - exprRange.start.character - 1 + offset + }, + documentRange: exprRange + }; + } + } + if (!isBasicExpression(token)) { return undefined; } diff --git a/languageservice/src/hover.expressions.test.ts b/languageservice/src/hover.expressions.test.ts index 8b5efc2..a0b556f 100644 --- a/languageservice/src/hover.expressions.test.ts +++ b/languageservice/src/hover.expressions.test.ts @@ -155,8 +155,8 @@ jobs: contents: "Causes the step to always execute, and returns `true`, even when canceled. The `always` expression is best used at the step level or on tasks that you expect to run even when a job is canceled. For example, you can use `always` to send logs even when a job is canceled.", range: { - start: {line: 3, character: 11}, - end: {line: 3, character: 17} + start: {line: 3, character: 8}, + end: {line: 3, character: 14} } }); }); diff --git a/languageservice/src/utils/expression-detection.ts b/languageservice/src/utils/expression-detection.ts index 9b66ce4..e52de25 100644 --- a/languageservice/src/utils/expression-detection.ts +++ b/languageservice/src/utils/expression-detection.ts @@ -1,12 +1,11 @@ import {isString} from "@actions/workflow-parser"; -import {DefinitionType} from "@actions/workflow-parser/templates/schema/definition-type"; -import {StringDefinition} from "@actions/workflow-parser/templates/schema/string-definition"; import {OPEN_EXPRESSION} from "@actions/workflow-parser/templates/template-constants"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/index"; export function isPotentiallyExpression(token: TemplateToken): boolean { - const isAlwaysExpression = - token.definition?.definitionType === DefinitionType.String && (token.definition as StringDefinition).isExpression; const containsExpression = isString(token) && token.value != null && token.value.indexOf(OPEN_EXPRESSION) >= 0; - return isAlwaysExpression || containsExpression; + // If conditions are always expressions (job-if, step-if, snapshot-if) + const definitionKey = token.definition?.key; + const isIfCondition = definitionKey === "job-if" || definitionKey === "step-if" || definitionKey === "snapshot-if"; + return containsExpression || isIfCondition; } diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index 3bcbd0a..d250258 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -108,6 +108,40 @@ async function additionalValidations( ); } + // If this is a job-if, step-if, or snapshot-if field (which are strings that should be treated as expressions), validate it + const definitionKey = token.definition?.key; + if ( + isString(token) && + token.range && + (definitionKey === "job-if" || definitionKey === "step-if" || definitionKey === "snapshot-if") + ) { + // Convert the string to an expression token for validation + const condition = token.value.trim(); + if (condition) { + // Check if the condition already contains a status function + const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); + const finalCondition = hasStatusFunction ? condition : `success() && (${condition})`; + + // Create a BasicExpressionToken for validation + const expressionToken = new BasicExpressionToken( + token.file, + token.range, + finalCondition, + token.definitionInfo, + undefined, + token.source + ); + + await validateExpression( + diagnostics, + expressionToken, + validationToken.definitionInfo?.allowedContext || [], + config?.contextProviderConfig, + getProviderContext(documentUri, template, root, token.range) + ); + } + } + if (token.definition?.key === "regular-step" && token.range) { const context = getProviderContext(documentUri, template, root, token.range); await validateAction(diagnostics, token, context.step, config); diff --git a/workflow-parser/src/expressions.test.ts b/workflow-parser/src/expressions.test.ts index cf22c62..01758fe 100644 --- a/workflow-parser/src/expressions.test.ts +++ b/workflow-parser/src/expressions.test.ts @@ -194,10 +194,11 @@ jobs: const jobs = workflowRoot.get(1).value.assertMapping("jobs"); const build = jobs.get(0).value.assertMapping("job"); const ifToken = build.get(1).value; - expect(ifToken.toString()).toEqual("${{ github.event_name == 'push' }}"); + // Without isExpression: true, the value is kept as a string until convertToIfCondition processes it + expect(ifToken.toString()).toEqual("github.event_name == 'push'"); - if (!isBasicExpression(ifToken)) { - throw new Error("expected if to be a basic expression"); + if (!isString(ifToken)) { + throw new Error("expected if to be a string (will be converted to expression later)"); } }); }); diff --git a/workflow-parser/src/model/convert.test.ts b/workflow-parser/src/model/convert.test.ts index cd7f495..ed92dd1 100644 --- a/workflow-parser/src/model/convert.test.ts +++ b/workflow-parser/src/model/convert.test.ts @@ -74,7 +74,7 @@ jobs: { id: "build", if: { - expr: "success()", + expr: "success() && (true)", type: 3 }, name: "build", @@ -85,7 +85,7 @@ jobs: { id: "deploy", if: { - expr: "success()", + expr: "success() && (true)", type: 3 }, name: "deploy", diff --git a/workflow-parser/src/model/converter/if-condition.ts b/workflow-parser/src/model/converter/if-condition.ts new file mode 100644 index 0000000..d07c4a1 --- /dev/null +++ b/workflow-parser/src/model/converter/if-condition.ts @@ -0,0 +1,119 @@ +import {TemplateContext} from "../../templates/template-context"; +import {BasicExpressionToken, ExpressionToken, TemplateToken} from "../../templates/tokens"; + +/** + * Converts an if condition token to a BasicExpressionToken. + * Similar to Go's convertToIfCondition - treats the value as a string and parses it as an expression. + * Wraps the condition in success() && (...) if it doesn't already contain a status function. + * This allows both 'if: success()' and 'if: ${{ success() }}' to work correctly. + * + * @param context The template context for error reporting + * @param token The token containing the if condition + * @param allowedContext The allowed expression contexts (varies by job-if vs step-if vs snapshot-if) + * @returns A BasicExpressionToken with the processed condition, or undefined on error + */ +export function convertToIfCondition( + context: TemplateContext, + token: TemplateToken, + allowedContext: string[] +): BasicExpressionToken | undefined { + const scalar = token.assertScalar("if condition"); + + // If it's already an expression, use its value + let condition: string; + let source: string | undefined; + + if (scalar instanceof BasicExpressionToken) { + condition = scalar.expression; + source = scalar.source; + } else { + // Otherwise, treat it as a string + const stringToken = scalar.assertString("if condition"); + condition = stringToken.value.trim(); + source = stringToken.source; + } + + if (!condition) { + // Empty condition defaults to success() + return new BasicExpressionToken(token.file, token.range, "success()", token.definitionInfo, undefined, undefined); + } + + // Check if the condition already contains a status function (success, failure, cancelled, always) + // This is a simple check - just look for these function names + const hasStatusFunction = /\b(success|failure|cancelled|always)\s*\(/.test(condition); + + let finalCondition: string; + if (hasStatusFunction) { + finalCondition = condition; + } else { + // Wrap in success() && (condition) + finalCondition = `success() && (${condition})`; + } + + // Validate the expression before creating the token + try { + ExpressionToken.validateExpression(finalCondition, allowedContext); + } catch (err) { + context.error(token, err as Error); + return undefined; + } + + // Create a BasicExpressionToken with the final condition + return new BasicExpressionToken(token.file, token.range, finalCondition, token.definitionInfo, undefined, source); +} + +/** + * Allowed context for job-level if conditions + */ +export const JOB_IF_CONTEXT = [ + "github", + "inputs", + "vars", + "needs", + "always(0,0)", + "failure(0,MAX)", + "cancelled(0,0)", + "success(0,MAX)" +]; + +/** + * Allowed context for step-level if conditions + */ +export const STEP_IF_CONTEXT = [ + "github", + "inputs", + "vars", + "needs", + "strategy", + "matrix", + "steps", + "job", + "runner", + "env", + "always(0,0)", + "failure(0,0)", + "cancelled(0,0)", + "success(0,0)", + "hashFiles(1,255)" +]; + +/** + * Allowed context for snapshot-level if conditions + */ +export const SNAPSHOT_IF_CONTEXT = [ + "github", + "inputs", + "vars", + "needs", + "strategy", + "matrix", + "steps", + "job", + "runner", + "env", + "always(0,0)", + "failure(0,0)", + "cancelled(0,0)", + "success(0,0)", + "hashFiles(1,255)" +]; diff --git a/workflow-parser/src/model/converter/job.ts b/workflow-parser/src/model/converter/job.ts index 2ee2c70..5338b73 100644 --- a/workflow-parser/src/model/converter/job.ts +++ b/workflow-parser/src/model/converter/job.ts @@ -2,6 +2,7 @@ import {TemplateContext} from "../../templates/template-context"; import {BasicExpressionToken, MappingToken, ScalarToken, StringToken, TemplateToken} from "../../templates/tokens"; import {isSequence, isString} from "../../templates/tokens/type-guards"; import {Step, WorkflowJob} from "../workflow-template"; +import {convertToIfCondition, JOB_IF_CONTEXT} from "./if-condition"; import {convertConcurrency} from "./concurrency"; import {convertToJobContainer, convertToJobServices} from "./container"; import {handleTemplateTokenErrors} from "./handle-errors"; @@ -20,6 +21,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: container, env, environment, + ifCondition, name, outputs, runsOn, @@ -59,6 +61,10 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: environment = item.value; break; + case "if": + ifCondition = convertToIfCondition(context, item.value, JOB_IF_CONTEXT); + break; + case "name": name = item.value.assertScalar("job name"); break; @@ -134,7 +140,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: id: jobKey, name: jobName(name, jobKey), needs: needs || [], - if: new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), + if: ifCondition || new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), ref: workflowJobRef, "input-definitions": undefined, "input-values": workflowJobInputs, @@ -151,7 +157,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: id: jobKey, name: jobName(name, jobKey), needs, - if: new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), + if: ifCondition || new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), env, concurrency, environment, diff --git a/workflow-parser/src/model/converter/steps.ts b/workflow-parser/src/model/converter/steps.ts index 05793a5..ead5ea5 100644 --- a/workflow-parser/src/model/converter/steps.ts +++ b/workflow-parser/src/model/converter/steps.ts @@ -2,6 +2,7 @@ import {TemplateContext} from "../../templates/template-context"; import {BasicExpressionToken, MappingToken, ScalarToken, StringToken, TemplateToken} from "../../templates/tokens"; import {isSequence} from "../../templates/tokens/type-guards"; import {isActionStep} from "../type-guards"; +import {convertToIfCondition, STEP_IF_CONTEXT} from "./if-condition"; import {ActionStep, Step} from "../workflow-template"; import {handleTemplateTokenErrors} from "./handle-errors"; import {IdBuilder} from "./id-builder"; @@ -52,7 +53,7 @@ function convertStep(context: TemplateContext, idBuilder: IdBuilder, step: Templ let uses: StringToken | undefined; let continueOnError: boolean | ScalarToken | undefined; let env: MappingToken | undefined; - const ifCondition = new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined); + let ifCondition: BasicExpressionToken | undefined; for (const item of mapping) { const key = item.key.assertString("steps item key"); switch (key.value) { @@ -77,6 +78,9 @@ function convertStep(context: TemplateContext, idBuilder: IdBuilder, step: Templ case "env": env = item.value.assertMapping("step env"); break; + case "if": + ifCondition = convertToIfCondition(context, item.value, STEP_IF_CONTEXT); + break; case "continue-on-error": if (!item.value.isExpression) { continueOnError = item.value.assertBoolean("steps item continue-on-error").value; @@ -90,7 +94,7 @@ function convertStep(context: TemplateContext, idBuilder: IdBuilder, step: Templ return { id: id?.value || "", name, - if: ifCondition, + if: ifCondition || new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), "continue-on-error": continueOnError, env, run @@ -101,7 +105,7 @@ function convertStep(context: TemplateContext, idBuilder: IdBuilder, step: Templ return { id: id?.value || "", name, - if: ifCondition, + if: ifCondition || new BasicExpressionToken(undefined, undefined, "success()", undefined, undefined, undefined), "continue-on-error": continueOnError, env, uses @@ -134,3 +138,10 @@ function createActionStepId(step: ActionStep): string { return ""; } + +/** + * Converts an if condition token to a BasicExpressionToken. + * Similar to Go's convertToIfCondition - treats the value as a string and parses it as an expression. + * Wraps the condition in success() && (...) if it doesn't already contain a status function. + * This allows both 'if: success()' and 'if: ${{ success() }}' to work correctly. + */ diff --git a/workflow-parser/src/templates/template-reader.ts b/workflow-parser/src/templates/template-reader.ts index abeb362..44bce6f 100644 --- a/workflow-parser/src/templates/template-reader.ts +++ b/workflow-parser/src/templates/template-reader.ts @@ -8,7 +8,6 @@ import {DefinitionType} from "./schema/definition-type"; import {MappingDefinition} from "./schema/mapping-definition"; import {ScalarDefinition} from "./schema/scalar-definition"; import {SequenceDefinition} from "./schema/sequence-definition"; -import {StringDefinition} from "./schema/string-definition"; import {ANY, CLOSE_EXPRESSION, INSERT_DIRECTIVE, OPEN_EXPRESSION} from "./template-constants"; import {TemplateContext} from "./template-context"; import { @@ -456,14 +455,7 @@ class TemplateReader { let startExpression: number = raw.indexOf(OPEN_EXPRESSION); if (startExpression < 0) { - // Doesn't contain "${{" - // Check if value should still be evaluated as an expression - if (definitionInfo.definition instanceof StringDefinition && definitionInfo.definition.isExpression) { - const expression = this.parseIntoExpressionToken(token.range!, raw, allowedContext, token, definitionInfo); - if (expression) { - return expression; - } - } + // Doesn't contain "{{" return token; } diff --git a/workflow-parser/src/workflow-v1.0.json b/workflow-parser/src/workflow-v1.0.json index 6e1c23a..ba3d154 100644 --- a/workflow-parser/src/workflow-v1.0.json +++ b/workflow-parser/src/workflow-v1.0.json @@ -1842,9 +1842,7 @@ "cancelled(0,0)", "success(0,MAX)" ], - "string": { - "is-expression": true - } + "string": {} }, "job-if-result": { "context": [ @@ -1949,9 +1947,7 @@ "matrix" ], "description": "Use the if conditional to prevent a snapshot from being taken unless a condition is met. Any supported context and expression can be used to create a conditional. Expressions in an `if` conditional do not require the bracketed expression syntax. When you use expressions in an `if` conditional, you may omit the expression syntax because GitHub automatically evaluates the `if` conditional as an expression.", - "string": { - "is-expression": true - } + "string": {} }, "runs-on": { "description": "Use `runs-on` to define the type of machine to run the job on.\n* The destination machine can be either a GitHub-hosted runner, larger runner, or a self-hosted runner.\n* You can target runners based on the labels assigned to them, or their group membership, or a combination of these.\n* You can provide `runs-on` as a single string or as an array of strings.\n* If you specify an array of strings, your workflow will execute on any runner that matches all of the specified `runs-on` values.\n* If you would like to run your workflow on multiple machines, use `jobs..strategy`.", @@ -2216,9 +2212,7 @@ "hashFiles(1,255)" ], "description": "Use the `if` conditional to prevent a step from running unless a condition is met. Any supported context and expression can be used to create a conditional. Expressions in an `if` conditional do not require the bracketed expression syntax. When you use expressions in an `if` conditional, you may omit the expression syntax because GitHub automatically evaluates the `if` conditional as an expression.", - "string": { - "is-expression": true - } + "string": {} }, "step-if-result": { "context": [ diff --git a/workflow-parser/testdata/skipped-tests.txt b/workflow-parser/testdata/skipped-tests.txt index 7da9375..7afa98c 100644 --- a/workflow-parser/testdata/skipped-tests.txt +++ b/workflow-parser/testdata/skipped-tests.txt @@ -50,7 +50,6 @@ errors-step-uses-syntax.yml errors-unclosed-tokens.yml errors-yaml-invalid-style.yml errors-yaml-tags-explicit-unsupported.yml -escape-html-values.yml float-folded-style.yml insert.yml is-partial-rerun.yml @@ -59,7 +58,6 @@ job-cancel-timeout-minutes.yml job-concurrency.yml job-continue-on-error.yml job-defaults.yml -job-if.yml job-permissions.yml job-timeout-minutes.yml matrix-basic.yml