From 9f260c4658789de718f6bec7aaae2db16aa625d4 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Wed, 7 Jan 2026 21:41:51 +0000 Subject: [PATCH] Add format string validation in expressions package Format string validation now happens at parse time in the Parser: - Invalid format string syntax throws ErrorInvalidFormatString - Argument count mismatch throws ErrorFormatArgCountMismatch This catches errors early and automatically for all consumers. --- expressions/src/errors.ts | 12 +- expressions/src/index.ts | 3 +- expressions/src/parser.ts | 25 +++ expressions/src/validate-format.test.ts | 63 +++++++ expressions/src/validate-format.ts | 101 +++++++++++ expressions/testdata/format.json | 68 ++++---- .../src/validate.format-string.test.ts | 164 ++++++++++++++++++ 7 files changed, 398 insertions(+), 38 deletions(-) create mode 100644 expressions/src/validate-format.test.ts create mode 100644 expressions/src/validate-format.ts create mode 100644 languageservice/src/validate.format-string.test.ts diff --git a/expressions/src/errors.ts b/expressions/src/errors.ts index 967113f..ffcf448 100644 --- a/expressions/src/errors.ts +++ b/expressions/src/errors.ts @@ -13,12 +13,14 @@ export enum ErrorType { ErrorTooFewParameters, ErrorTooManyParameters, ErrorUnrecognizedContext, - ErrorUnrecognizedFunction + ErrorUnrecognizedFunction, + ErrorInvalidFormatString, + ErrorFormatArgCountMismatch } export class ExpressionError extends Error { - constructor(private typ: ErrorType, private tok: Token) { - super(`${errorDescription(typ)}: '${tokenString(tok)}'`); + constructor(private typ: ErrorType, private tok: Token, customMessage?: string) { + super(customMessage ?? `${errorDescription(typ)}: '${tokenString(tok)}'`); this.pos = this.tok.range.start; } @@ -46,6 +48,10 @@ function errorDescription(typ: ErrorType): string { return "Unrecognized named-value"; case ErrorType.ErrorUnrecognizedFunction: return "Unrecognized function"; + case ErrorType.ErrorInvalidFormatString: + return "Invalid format string"; + case ErrorType.ErrorFormatArgCountMismatch: + return "Format string argument count mismatch"; default: // Should never reach here. return "Unknown error"; } diff --git a/expressions/src/index.ts b/expressions/src/index.ts index 4b91e1d..5d72a87 100644 --- a/expressions/src/index.ts +++ b/expressions/src/index.ts @@ -2,9 +2,10 @@ export {Expr} from "./ast.js"; export {complete, CompletionItem} from "./completion.js"; export {DescriptionDictionary, DescriptionPair, isDescriptionDictionary} from "./completion/descriptionDictionary.js"; export * as data from "./data/index.js"; -export {ExpressionError, ExpressionEvaluationError} from "./errors.js"; +export {ErrorType, ExpressionError, ExpressionEvaluationError} from "./errors.js"; export {Evaluator} from "./evaluator.js"; export {ExperimentalFeatureKey, ExperimentalFeatures, FeatureFlags} from "./features.js"; export {wellKnownFunctions} from "./funcs.js"; export {Lexer, Result} from "./lexer.js"; export {Parser} from "./parser.js"; +export {validateFormatString} from "./validate-format.js"; diff --git a/expressions/src/parser.ts b/expressions/src/parser.ts index 965af74..49efe3f 100644 --- a/expressions/src/parser.ts +++ b/expressions/src/parser.ts @@ -15,6 +15,7 @@ import {ErrorType, ExpressionError, MAX_PARSER_DEPTH} from "./errors.js"; import {ParseContext, validateFunction} from "./funcs.js"; import {FunctionInfo} from "./funcs/info.js"; import {Token, TokenType} from "./lexer.js"; +import {validateFormatString} from "./validate-format.js"; export class Parser { private extContexts: Map; @@ -261,6 +262,30 @@ export class Parser { validateFunction(this.context, identifier, args.length); + // Validate format() calls + if (identifier.lexeme.toLowerCase() === "format" && args.length > 0) { + const firstArg = args[0]; + if (firstArg instanceof Literal && firstArg.literal.kind === data.Kind.String) { + const formatString = firstArg.literal.coerceString(); + const result = validateFormatString(formatString); + + if (!result.valid) { + throw new ExpressionError(ErrorType.ErrorInvalidFormatString, identifier); + } + + // Check argument count: format string uses {0} to {N}, so need N+1 args after format string + const providedArgs = args.length - 1; + const requiredArgs = result.maxArgIndex + 1; + if (requiredArgs > providedArgs) { + throw new ExpressionError( + ErrorType.ErrorFormatArgCountMismatch, + identifier, + `Format string references {${result.maxArgIndex}} but only ${providedArgs} argument(s) provided` + ); + } + } + } + return new FunctionCall(identifier, args); } diff --git a/expressions/src/validate-format.test.ts b/expressions/src/validate-format.test.ts new file mode 100644 index 0000000..b814dba --- /dev/null +++ b/expressions/src/validate-format.test.ts @@ -0,0 +1,63 @@ +import {validateFormatString} from "./validate-format.js"; + +describe("validateFormatString", () => { + it("returns valid for simple placeholder", () => { + const result = validateFormatString("{0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for multiple placeholders", () => { + const result = validateFormatString("{0} {1} {2}"); + expect(result).toEqual({valid: true, maxArgIndex: 2}); + }); + + it("returns valid for text with placeholder", () => { + const result = validateFormatString("hello {0} world"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for escaped left braces", () => { + const result = validateFormatString("{{0}} {0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for escaped right braces", () => { + const result = validateFormatString("{0}}}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); + + it("returns valid for no placeholders", () => { + const result = validateFormatString("hello world"); + expect(result).toEqual({valid: true, maxArgIndex: -1}); + }); + + it("returns invalid for missing closing brace", () => { + const result = validateFormatString("{0"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for empty placeholder", () => { + const result = validateFormatString("{}"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for non-numeric placeholder", () => { + const result = validateFormatString("{abc}"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("returns invalid for unescaped closing brace", () => { + const result = validateFormatString("text } more"); + expect(result).toEqual({valid: false, maxArgIndex: -1}); + }); + + it("handles out-of-order placeholders", () => { + const result = validateFormatString("{2} {0} {1}"); + expect(result).toEqual({valid: true, maxArgIndex: 2}); + }); + + it("handles repeated placeholders", () => { + const result = validateFormatString("{0} {0} {0}"); + expect(result).toEqual({valid: true, maxArgIndex: 0}); + }); +}); diff --git a/expressions/src/validate-format.ts b/expressions/src/validate-format.ts new file mode 100644 index 0000000..42cc220 --- /dev/null +++ b/expressions/src/validate-format.ts @@ -0,0 +1,101 @@ +/** + * Format string validation for format() function calls. + * Validates format string syntax and argument count at parse time. + */ + +/** + * Validates a format string and returns the maximum placeholder index. + * + * @param formatString The format string to validate + * @returns { valid: boolean, maxArgIndex: number } where maxArgIndex is -1 if no placeholders + */ +export function validateFormatString(formatString: string): {valid: boolean; maxArgIndex: number} { + let maxIndex = -1; + let i = 0; + + while (i < formatString.length) { + // Find next left brace + let lbrace = -1; + for (let j = i; j < formatString.length; j++) { + if (formatString[j] === "{") { + lbrace = j; + break; + } + } + + // Find next right brace + let rbrace = -1; + for (let j = i; j < formatString.length; j++) { + if (formatString[j] === "}") { + rbrace = j; + break; + } + } + + // No more braces + if (lbrace < 0 && rbrace < 0) { + break; + } + + // Left brace comes first (or only left brace exists) + if (lbrace >= 0 && (rbrace < 0 || lbrace < rbrace)) { + // Check if it's escaped + if (lbrace + 1 < formatString.length && formatString[lbrace + 1] === "{") { + // Escaped left brace + i = lbrace + 2; + continue; + } + + // This is a placeholder opening - find the closing brace + rbrace = -1; + for (let j = lbrace + 1; j < formatString.length; j++) { + if (formatString[j] === "}") { + rbrace = j; + break; + } + } + + if (rbrace < 0) { + // Missing closing brace + return {valid: false, maxArgIndex: -1}; + } + + // Validate placeholder content (must be digits only) + if (rbrace === lbrace + 1) { + // Empty placeholder {} + return {valid: false, maxArgIndex: -1}; + } + + // Parse the index and validate it's all digits + let index = 0; + for (let j = lbrace + 1; j < rbrace; j++) { + const c = formatString[j]; + if (c < "0" || c > "9") { + // Non-numeric character + return {valid: false, maxArgIndex: -1}; + } + index = index * 10 + (c.charCodeAt(0) - "0".charCodeAt(0)); + } + + if (index > maxIndex) { + maxIndex = index; + } + + i = rbrace + 1; + continue; + } + + // Right brace comes first (or only right brace exists) + // Check if it's escaped + if (rbrace + 1 < formatString.length && formatString[rbrace + 1] === "}") { + // Escaped right brace + i = rbrace + 2; + continue; + } + + // Unescaped right brace outside of placeholder + return {valid: false, maxArgIndex: -1}; + } + + return {valid: true, maxArgIndex: maxIndex}; +} diff --git a/expressions/testdata/format.json b/expressions/testdata/format.json index 24dc64f..24bb228 100644 --- a/expressions/testdata/format.json +++ b/expressions/testdata/format.json @@ -87,120 +87,120 @@ { "expr": "format('{0')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {0" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{0', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {0" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{0}}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {0}}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{0}}}}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {0}}}}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('0}')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: 0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('0}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: 0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{{0}')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{{0}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{{{{0}')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {{{{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{{{{0}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {{{{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('}0{')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: }0{" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('}0{', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: }0{" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('}{0}')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: }{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('}{0}', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: }{0}" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{0}{', '')", "err": { - "kind": "evaluation", - "value": "The following format string is invalid: {0}{" + "kind": "parsing", + "value": "Invalid format string" } }, { "expr": "format('{0}')", "err": { - "kind": "evaluation", - "value": "The following format string references more arguments than were supplied: {0}" + "kind": "parsing", + "value": "Format string references {0} but only 0 argument(s) provided" } }, { "expr": "format('{0}{1}', 'abc')", "err": { - "kind": "evaluation", - "value": "The following format string references more arguments than were supplied: {0}{1}" + "kind": "parsing", + "value": "Format string references {1} but only 1 argument(s) provided" } } ] diff --git a/languageservice/src/validate.format-string.test.ts b/languageservice/src/validate.format-string.test.ts new file mode 100644 index 0000000..0581696 --- /dev/null +++ b/languageservice/src/validate.format-string.test.ts @@ -0,0 +1,164 @@ +import {Diagnostic} from "vscode-languageserver-types"; +import {createDocument} from "./test-utils/document.js"; +import {validate} from "./validate.js"; +import {clearCache} from "./utils/workflow-cache.js"; + +beforeEach(() => { + clearCache(); +}); + +function hasMessageContaining(results: Diagnostic[], substring: string): boolean { + return results.some(r => r.message.includes(substring)); +} + +describe("format string validation", () => { + describe("InvalidFormatString workflow validation", () => { + it("errors on missing closing brace", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Invalid format string")).toBe(true); + }); + + it("errors on empty braces", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Invalid format string")).toBe(true); + }); + + it("errors on non-numeric placeholder", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{abc}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Invalid format string")).toBe(true); + }); + + it("allows valid format strings", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0} {1}', github.event_name, github.ref) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Invalid format string")).toBe(false); + }); + + it("allows escaped braces", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{{0}} {0}', github.event_name) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Invalid format string")).toBe(false); + }); + }); + + describe("FormatArgCountMismatch workflow validation", () => { + it("errors when placeholder exceeds arg count", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{2}', 'arg0', 'arg1') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Format string references {2}")).toBe(true); + }); + + it("errors when referencing arg 0 with no args", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0}') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Format string references {0}")).toBe(true); + }); + + it("allows when arg count matches", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0} {1} {2}', 'a', 'b', 'c') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Format string references")).toBe(false); + }); + + it("handles no placeholders correctly", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('hello world') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + expect(hasMessageContaining(result, "Format string references")).toBe(false); + }); + + it("skips validation for dynamic format strings", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format(env.FORMAT_STRING, 'arg') }} +`; + const result = await validate(createDocument("wf.yaml", input)); + // Should not have format errors since we can't validate dynamic strings + expect(hasMessageContaining(result, "Invalid format string")).toBe(false); + expect(hasMessageContaining(result, "Format string references")).toBe(false); + }); + + it("validates nested format calls", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo \${{ format('{0}', format('{2}', 'a')) }} +`; + const result = await validate(createDocument("wf.yaml", input)); + // The inner format call has an error + expect(hasMessageContaining(result, "Format string references {2}")).toBe(true); + }); + }); +});