Merge pull request #45 from github/joshmgross/matrix-context-validation

Improve validation for `matrix` context

actions-languageservice/src/complete.ts

@@ -84,7 +84,7 @@ export async function complete(
 
       const expressionInput = (getExpressionInput(currentInput, relCharPos) || "").trim();
 
-      const allowedContext = getAllowedContext(token, parent!);
+      const allowedContext = getAllowedContext(token, parent);
       const context = await getContext(allowedContext, contextProviderConfig, workflowContext);
 
       return completeExpression(expressionInput, context, []);

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

@@ -1,4 +1,5 @@
 import {data} from "@github/actions-expressions";
+import {Kind} from "@github/actions-expressions/data/expressiondata";
 import {WorkflowContext} from "../context/workflow-context";
 import {ContextProviderConfig} from "./config";
 import {getInputsContext} from "./inputs";
@@ -7,6 +8,10 @@ import {getNeedsContext} from "./needs";
 import {getStepsContext} from "./steps";
 import {getStrategyContext} from "./strategy";
 
+// ContextValue is the type of the value returned by a context provider
+// Null indicates that the context provider doesn't have any value to provide
+export type ContextValue = data.Dictionary | data.Null;
+
 export async function getContext(
   names: string[],
   config: ContextProviderConfig | undefined,
@@ -17,6 +22,10 @@ export async function getContext(
   const filteredNames = filterContextNames(names, workflowContext);
   for (const contextName of filteredNames) {
     let value = (await getDefaultContext(contextName, workflowContext)) || new data.Dictionary();
+    if (value.kind === Kind.Null) {
+      context.add(contextName, value);
+      continue;
+    }
 
     value = (await config?.getContext(contextName, value)) || value;
 
@@ -26,7 +35,7 @@ export async function getContext(
   return context;
 }
 
-async function getDefaultContext(name: string, workflowContext: WorkflowContext): Promise<data.Dictionary | undefined> {
+async function getDefaultContext(name: string, workflowContext: WorkflowContext): Promise<ContextValue | undefined> {
   switch (name) {
     case "runner":
       return objectToDictionary({

actions-languageservice/src/context-providers/matrix.test.ts

@@ -88,7 +88,7 @@ describe("matrix context", () => {
       const workflowContext = contextFromStrategy(strategy);
 
       const context = getMatrixContext(workflowContext);
-      expect(context).toEqual(new data.Dictionary());
+      expect(context).toEqual(new data.Null());
     });
 
     it("matrix is not a mapping token", () => {
@@ -97,7 +97,7 @@ describe("matrix context", () => {
       const workflowContext = contextFromStrategy(strategy);
 
       const context = getMatrixContext(workflowContext);
-      expect(context).toEqual(new data.Dictionary());
+      expect(context).toEqual(new data.Null());
     });
 
     it("empty matrix", () => {
@@ -118,7 +118,7 @@ describe("matrix context", () => {
       const workflowContext = contextFromStrategy(strategy);
       const context = getMatrixContext(workflowContext);
 
-      expect(context).toEqual(new data.Dictionary());
+      expect(context).toEqual(new data.Null());
     });
 
     it("matrix with include expression", () => {
@@ -138,7 +138,7 @@ describe("matrix context", () => {
       const workflowContext = contextFromStrategy(strategy);
       const context = getMatrixContext(workflowContext);
 
-      expect(context).toEqual(new data.Dictionary());
+      expect(context).toEqual(new data.Null());
     });
 
     it("matrix with expression within property", () => {

actions-languageservice/src/context-providers/matrix.ts

@@ -4,8 +4,9 @@ import {KeyValuePair} from "@github/actions-workflow-parser/templates/tokens/key
 import {MappingToken} from "@github/actions-workflow-parser/templates/tokens/mapping-token";
 import {SequenceToken} from "@github/actions-workflow-parser/templates/tokens/sequence-token";
 import {WorkflowContext} from "../context/workflow-context";
+import {ContextValue} from "./default";
 
-export function getMatrixContext(workflowContext: WorkflowContext): data.Dictionary {
+export function getMatrixContext(workflowContext: WorkflowContext): ContextValue {
   // https://docs.github.com/en/actions/learn-github-actions/contexts#matrix-context
   const strategy = workflowContext.job?.strategy;
   if (!strategy || !isMapping(strategy)) {
@@ -15,13 +16,13 @@ export function getMatrixContext(workflowContext: WorkflowContext): data.Diction
   const matrix = strategy.find("matrix");
   if (!matrix || !isMapping(matrix)) {
     // Matrix could be an expression, so there's no context we can provide
-    return new data.Dictionary();
+    return new data.Null();
   }
 
   const properties = matrixProperties(matrix);
   if (!properties) {
     // Matrix included an expression, so there's no context we can provide
-    return new data.Dictionary();
+    return new data.Null();
   }
 
   const d = new data.Dictionary();

actions-languageservice/src/utils/allowed-context.ts

@@ -1,11 +1,11 @@
 import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token";
 
-export function getAllowedContext(token: TemplateToken, parent: TemplateToken): string[] {
+export function getAllowedContext(token: TemplateToken, parent: TemplateToken | null | undefined): string[] {
   // Workaround for https://github.com/github/c2c-actions-experience/issues/6876
   // Context is inherited from the parent
   const allowedContext = new Set<string>();
   for (const t of [token, parent]) {
-    if (t.definition?.readerContext) {
+    if (t?.definition?.readerContext) {
       for (const context of t.definition.readerContext) {
         allowedContext.add(context);
       }

actions-languageservice/src/validate.expressions.test.ts

@@ -305,4 +305,354 @@ jobs:
       ]);
     });
   });
+
+  describe("matrix context", () => {
+    it("reference within a matrix job", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        node: [14, 16]
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("reference outside of a matrix job", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([
+        {
+          message: "Context access might be invalid: matrix",
+          range: {
+            end: {
+              character: 36,
+              line: 8
+            },
+            start: {
+              character: 18,
+              line: 8
+            }
+          },
+          severity: DiagnosticSeverity.Warning
+        }
+      ]);
+    });
+
+    it("basic matrix", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: \${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        node: [14, 16]
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("invalid property reference", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: \${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        node: [14, 16]
+        include:
+          - os: macos-latest
+            node: 14
+        exclude:
+          - os: windows-latest
+            node: 14
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.goversion }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([
+        {
+          message: "Context access might be invalid: goversion",
+          range: {
+            end: {
+              character: 41,
+              line: 18
+            },
+            start: {
+              character: 18,
+              line: 18
+            }
+          },
+          severity: DiagnosticSeverity.Warning
+        }
+      ]);
+    });
+
+    it("matrix with include", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: \${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        node: [14, 16]
+        include:
+          - os: macos-latest
+            node: 14
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with only include", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        include:
+          - os: windows-latest
+            node: 14
+    steps:
+      - uses: actions/checkout@v3
+      - run: echo \${{ matrix.os }}
+      - run: echo \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with exclude", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: \${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ubuntu-latest, windows-latest]
+        node: [14, 16]
+        include:
+          - os: macos-latest
+            node: 14
+        exclude:
+          - os: windows-latest
+            node: 14
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with only exclude", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: \${{ matrix.os }}
+    strategy:
+      matrix:
+        exclude:
+          - os: windows-latest
+            node: 14
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.node }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([
+        {
+          message: "Context access might be invalid: os",
+          range: {
+            end: {
+              character: 29,
+              line: 5
+            },
+            start: {
+              character: 13,
+              line: 5
+            }
+          },
+          severity: DiagnosticSeverity.Warning
+        },
+        {
+          message: "Context access might be invalid: node",
+          range: {
+            end: {
+              character: 42,
+              line: 15
+            },
+            start: {
+              character: 24,
+              line: 15
+            }
+          },
+          severity: DiagnosticSeverity.Warning
+        }
+      ]);
+    });
+
+    it("matrix from expression", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix: \${{ fromJSON('{"color":["green","blue"]}') }}
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.ANYVALUE }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with include expression", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        fruit: [apple, pear]
+        animal: [cat, dog]
+        include: \${{ fromJSON('{"color":"green"}') }}
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.ANYVALUE }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with property expression", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        color: \${{ fromJSON('["green","blue"]') }}
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.color }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([]);
+    });
+
+    it("matrix with property expression and invalid property reference", async () => {
+      const input = `
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        color: \${{ fromJSON('["green","blue"]') }}
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: \${{ matrix.shape }}
+`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      expect(result).toEqual([
+        {
+          message: "Context access might be invalid: shape",
+          range: {
+            end: {
+              character: 43,
+              line: 13
+            },
+            start: {
+              character: 24,
+              line: 13
+            }
+          },
+          severity: DiagnosticSeverity.Warning
+        }
+      ]);
+    });
+  });
 });

actions-languageservice/src/validate.ts

@@ -24,6 +24,7 @@ import {getWorkflowContext, WorkflowContext} from "./context/workflow-context";
 import {AccessError, wrapDictionary} from "./expression-validation/error-dictionary";
 import {error} from "./log";
 import {nullTrace} from "./nulltrace";
+import {getAllowedContext} from "./utils/allowed-context";
 import {findToken} from "./utils/find-token";
 import {mapRange} from "./utils/range";
 import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config";
@@ -95,12 +96,14 @@ async function additionalValidations(
     const validationToken = key || parent || token;
     const validationDefinition = validationToken.definition;
 
+    const allowedContext = getAllowedContext(validationToken, parent);
+
     // If this is an expression, validate it
     if (isBasicExpression(token)) {
       await validateExpression(
         diagnostics,
         token,
-        validationDefinition,
+        allowedContext,
         contextProviderConfig,
         getProviderContext(documentUri, template, root, token)
       );
@@ -171,14 +174,13 @@ function getProviderContext(
 async function validateExpression(
   diagnostics: Diagnostic[],
   token: BasicExpressionToken,
-  definition: Definition | undefined,
+  allowedContext: string[],
   contextProviderConfig: ContextProviderConfig | undefined,
   workflowContext: WorkflowContext
 ) {
   // Validate the expression
   for (const expression of token.originalExpressions || [token]) {
-    const allowedContexts = definition?.readerContext || [];
-    const {namedContexts, functions} = splitAllowedContext(allowedContexts);
+    const {namedContexts, functions} = splitAllowedContext(allowedContext);
 
     let expr: Expr | undefined;