Merge validation code into a single traversal

actions-languageservice/src/complete.ts

@@ -117,7 +117,7 @@ async function getValues(
     (parent.definition?.key && defaultValueProviders[parent.definition.key]);
 
   if (valueProvider) {
-    const values = valueProvider(workflowContext);
+    const values = await valueProvider(workflowContext);
     return filterAndSortCompletionOptions(values, existingValues);
   }
 

actions-languageservice/src/validate.test.ts

@@ -1,6 +1,7 @@
-import {Diagnostic} from "vscode-languageserver-types";
+import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types";
 import {createDocument} from "./test-utils/document";
 import {validate} from "./validate";
+import {defaultValueProviders} from "./value-providers/default";
 
 describe("validation", () => {
   it("valid workflow", async () => {
@@ -57,4 +58,68 @@ jobs:
       }
     } as Diagnostic);
   });
+
+  it("single value not returned by value provider", async () => {
+    const result = await validate(
+      createDocument(
+        "wf.yaml",
+        `on: push
+jobs:
+  build:
+    runs-on: does-not-exist
+    steps:
+    - run: echo`
+      ),
+      defaultValueProviders
+    );
+
+    expect(result.length).toBe(1);
+    expect(result[0]).toEqual({
+      message: "Value 'does-not-exist' is not valid",
+      severity: DiagnosticSeverity.Error,
+      range: {
+        end: {
+          character: 27,
+          line: 3
+        },
+        start: {
+          character: 13,
+          line: 3
+        }
+      }
+    } as Diagnostic);
+  });
+
+  it("value in sequence not returned by value provider", async () => {
+    const result = await validate(
+      createDocument(
+        "wf.yaml",
+        `on: push
+jobs:
+  build:
+    runs-on:
+    - ubuntu-latest
+    - does-not-exist
+    steps:
+    - run: echo`
+      ),
+      defaultValueProviders
+    );
+
+    expect(result.length).toBe(1);
+    expect(result[0]).toEqual({
+      message: "Value 'does-not-exist' is not valid",
+      severity: DiagnosticSeverity.Error,
+      range: {
+        end: {
+          character: 20,
+          line: 5
+        },
+        start: {
+          character: 6,
+          line: 5
+        }
+      }
+    } as Diagnostic);
+  });
 });

actions-languageservice/src/validate.ts

@@ -10,6 +10,8 @@ import {
   WorkflowTemplate
 } from "@github/actions-workflow-parser";
 import {splitAllowedContext} from "@github/actions-workflow-parser/templates/allowed-context";
+import {BasicExpressionToken} from "@github/actions-workflow-parser/templates/tokens/basic-expression-token";
+import {StringToken} from "@github/actions-workflow-parser/templates/tokens/string-token";
 import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token";
 import {TokenRange} from "@github/actions-workflow-parser/templates/tokens/token-range";
 import {File} from "@github/actions-workflow-parser/workflows/file";
@@ -50,13 +52,15 @@ export async function validate(
       // Errors will be updated in the context
       const template = convertWorkflowTemplate(result.context, result.value);
 
-      // Validate with value providers
+      // Validate expressions and value providers
-      if (valueProviderConfig) {
+      await additionalValidations(
-        await validateValueProviders(diagnostics, textDocument.uri, template, result, valueProviderConfig);
+        diagnostics,
-      }
+        textDocument.uri,
-
+        template,
-      // Validate expressions
+        result.value,
-      validateExpressions(diagnostics, result, contextProviderConfig);
+        valueProviderConfig,
+        contextProviderConfig
+      );
     }
 
     // For now map parser errors directly to diagnostics
@@ -101,18 +105,89 @@ function mapRange(range: TokenRange | undefined): Range {
   };
 }
 
-function validateExpressions(
+async function additionalValidations(
   diagnostics: Diagnostic[],
-  result: ParseWorkflowResult,
+  documentUri: URI,
+  template: WorkflowTemplate,
+  root: TemplateToken,
+  valueProviderConfig: ValueProviderConfig | undefined,
   contextProviderConfig: ContextProviderConfig | undefined
 ) {
-  if (!result.value) {
+  for (const token of TemplateToken.traverse(root)) {
-    return;
+    // If this is an expression, validate it
+    if (isBasicExpression(token)) {
+      validateExpression(diagnostics, token, contextProviderConfig);
     }
 
-  // Iterate over the parsed workflow
+    // Allowed values coming from the schema have already been validated. Only check if
-  for (const token of TemplateToken.traverse(result.value)) {
+    // a value provider is defined for a token and if it is, validate the values match.
-    if (isBasicExpression(token)) {
+    if (valueProviderConfig && token.range && token.definition?.key) {
+      const defKey = token.definition.key;
+
+      let customValues: Value[] | undefined;
+
+      const customValueProvider = valueProviderConfig[defKey];
+      if (customValueProvider) {
+        customValues = await customValueProvider(getProviderContext(documentUri, template, root, token));
+      } else {
+        const defaultValueProvider = defaultValueProviders[defKey];
+        if (defaultValueProvider) {
+          customValues = await defaultValueProvider(getProviderContext(documentUri, template, root, token));
+        }
+      }
+
+      if (customValues) {
+        if (isSequence(token)) {
+          for (let i = 0; i < token.count; ++i) {
+            const entry = token.get(i);
+
+            if (isString(entry)) {
+              if (!customValues.map(x => x.label).includes(entry.value)) {
+                invalidValue(diagnostics, entry);
+              }
+            }
+          }
+        }
+
+        if (isString(token)) {
+          if (!customValues.map(x => x.label).includes(token.value)) {
+            invalidValue(diagnostics, token);
+          }
+        }
+      }
+    }
+  }
+}
+
+function invalidValue(diagnostics: Diagnostic[], token: StringToken) {
+  diagnostics.push({
+    message: `Value '${token.value}' is not valid`,
+    severity: DiagnosticSeverity.Error,
+    range: mapRange(token.range)
+  });
+}
+
+function getProviderContext(
+  documentUri: URI,
+  template: WorkflowTemplate,
+  root: TemplateToken,
+  token: TemplateToken
+): WorkflowContext {
+  const {parent, path} = findToken(
+    {
+      line: token.range!.start[0],
+      character: token.range!.start[1]
+    },
+    root
+  );
+  return getWorkflowContext(documentUri, template, path);
+}
+
+function validateExpression(
+  diagnostics: Diagnostic[],
+  token: BasicExpressionToken,
+  contextProviderConfig: ContextProviderConfig | undefined
+) {
   // Validate the expression
   for (const expression of token.originalExpressions || [token]) {
     const allowedContexts = token.definition?.readerContext || [];
@@ -151,82 +226,4 @@ function validateExpressions(
       }
     }
   }
-    }
-  }
-}
-
-async function validateValueProviders(
-  diagnostics: Diagnostic[],
-  documentUri: URI,
-  template: WorkflowTemplate,
-  result: ParseWorkflowResult,
-  valueProviderConfig: ValueProviderConfig
-) {
-  if (!result.value) {
-    return;
-  }
-
-  // Allowed values coming from the schema have already been validated. Only check if
-  // a value provider is defined for a token and if it is, validate the values match.
-  for (const token of TemplateToken.traverse(result.value)) {
-    if (token.range && token.definition?.key) {
-      const defKey = token.definition.key;
-
-      let customValues: Value[] | undefined;
-
-      const customValueProvider = valueProviderConfig[defKey];
-      if (customValueProvider) {
-        customValues = await customValueProvider(getProviderContext(documentUri, template, result.value, token));
-      } else {
-        const defaultValueProvider = defaultValueProviders[defKey];
-        if (defaultValueProvider) {
-          customValues = defaultValueProvider(getProviderContext(documentUri, template, result.value, token));
-        }
-      }
-
-      if (customValues) {
-        if (isSequence(token)) {
-          for (let i = 0; i < token.count; ++i) {
-            const entry = token.get(i);
-
-            if (isString(entry)) {
-              if (!customValues.map(x => x.label).includes(entry.value)) {
-                diagnostics.push({
-                  message: `Value '${entry.value}' is not allowed`,
-                  severity: DiagnosticSeverity.Error,
-                  range: mapRange(entry.range)
-                });
-              }
-            }
-          }
-        }
-
-        if (isString(token)) {
-          if (!customValues.map(x => x.label).includes(token.value)) {
-            diagnostics.push({
-              message: `Value '${token.value}' is not allowed`,
-              severity: DiagnosticSeverity.Error,
-              range: mapRange(token.range)
-            });
-          }
-        }
-      }
-    }
-  }
-}
-
-function getProviderContext(
-  documentUri: URI,
-  template: WorkflowTemplate,
-  root: TemplateToken,
-  token: TemplateToken
-): WorkflowContext {
-  const {parent, path} = findToken(
-    {
-      line: token.range!.start[0],
-      character: token.range!.start[1]
-    },
-    root
-  );
-  return getWorkflowContext(documentUri, template, path);
 }

actions-languageservice/src/value-providers/default.ts

@@ -1,10 +1,10 @@
 import {WorkflowContext} from "../context/workflow-context";
-import {Value} from "./config";
+import {Value, ValueProviderConfig} from "./config";
 import {needs} from "./needs";
 
-export const defaultValueProviders: {[key: string]: (workflowContext: WorkflowContext) => Value[]} = {
+export const defaultValueProviders: ValueProviderConfig = {
   needs,
-  "runs-on": () =>
+  "runs-on": async (_: WorkflowContext) =>
     stringsToValues([
       "ubuntu-latest",
       "ubuntu-18.04",

actions-languageservice/src/value-providers/needs.ts

@@ -1,7 +1,7 @@
 import {WorkflowContext} from "../context/workflow-context";
 import {Value} from "./config";
 
-export function needs(context: WorkflowContext): Value[] {
+export async function needs(context: WorkflowContext): Promise<Value[]> {
   if (!context.template) {
     return [];
   }