Teacher Tool: Variable Usage Validator (#9994)

This adds a new validator which can check if variables are both defined and used. The key difference between this and "block-exist" checks for variable_set and variable_get is that we can ensure the names match, and we can ensure it's checking for multiple variables (so variable_get isn't just called X number of times on the same variable, even if variable_set defines X unique variables, and so on...).

You can optionally filter by name, but we don't expose this currently in any criteria. You can also used nested validation to ensure a variable is set to something specific (like pick random), but again, we don't have any criteria that currently use this.

common-docs/teachertool/catalog-shared.json

@@ -20,22 +20,6 @@
                 }
             ]
         },
-        {
-            "id": "D21D76A2-D9FD-4F9B-B0AC-973CB870EA78",
-            "use": "variable_set",
-            "template": "At least one custom variable is set",
-            "docPath": "/teachertool",
-            "description": "At least one user-defined variable is set to a value.",
-            "maxCount": 1
-        },
-        {
-            "id": "0173898D-8A48-4266-AAB9-CE934471A734",
-            "use": "variable_accessed",
-            "template": "At least one variable is accessed",
-            "docPath": "/teachertool",
-            "description": "At least one variable's value is read.",
-            "maxCount": 1
-        },
         {
             "id": "7AE7EA2A-3AC8-42DC-89DB-65E3AE157156",
             "use": "block_comment_used",
@@ -83,6 +67,22 @@
                     "default": 1
                 }
             ]
+        },
+        {
+            "id": "0DFA44C8-3CA5-4C77-946E-AF09F6C03879",
+            "use": "variable_usage",
+            "template": "Uses at least ${count} variables",
+            "docPath": "/teachertool",
+            "description": "The program creates and uses at least this many user-defined variables.",
+            "maxCount": 1,
+            "params": [
+                {
+                    "name": "count",
+                    "type": "number",
+                    "paths": ["checks[0].count"],
+                    "default": 1
+                }
+            ]
         }
     ]
 }

common-docs/teachertool/validator-plans-shared.json

@@ -86,38 +86,6 @@
                 }
             ]
         },
-        {
-            ".desc": "A variable's value is set",
-            "name": "variable_set",
-            "threshold": 1,
-            "checks": [
-                {
-                    "validator": "blocksExist",
-                    "blockCounts": [
-                        {
-                            "blockId": "variables_set",
-                            "count": 1
-                        }
-                    ]
-                }
-            ]
-        },
-        {
-            ".desc": "A variable's value is used",
-            "name": "variable_accessed",
-            "threshold": 1,
-            "checks": [
-                {
-                    "validator": "blocksExist",
-                    "blockCounts": [
-                        {
-                            "blockId": "variables_get",
-                            "count": 1
-                        }
-                    ]
-                }
-            ]
-        },
         {
             ".desc": "A parameter's value is used",
             "name": "parameter_variable_accessed",
@@ -301,6 +269,18 @@
                     "count": 0
                 }
             ]
+        },
+        {
+            ".desc": "Variable creation & usage validation, with optional name filtering",
+            "name": "variable_usage",
+            "threshold": 1,
+            "checks": [
+                {
+                    "validator": "variableUsage",
+                    "count": 0,
+                    "name": ""
+                }
+            ]
         }
     ]
 }

localtypings/validatorPlan.d.ts

@@ -40,6 +40,12 @@ declare namespace pxt.blocks {
         count: number;
     }
 
+    export interface VariableUsageValidatorCheck extends ValidatorCheckBase {
+        validator: "variableUsage";
+        count: number;
+        name?: string;
+    }
+
     export interface EvaluationResult {
         result?: boolean;
         notes?: string;

pxteditor/code-validation/runValidatorPlan.ts

@@ -8,6 +8,7 @@ import { validateBlocksInSetExist } from "./validateBlocksInSetExist";
 import { validateBlockCommentsExist } from "./validateCommentsExist";
 import { validateSpecificBlockCommentsExist } from "./validateSpecificBlockCommentsExist";
 import { getNestedChildBlocks } from "./getNestedChildBlocks";
+import { validateVariableUsage } from "./validateVariableUsage";
 
 export function runValidatorPlan(usedBlocks: Blockly.Block[], plan: pxt.blocks.ValidatorPlan, planLib: pxt.blocks.ValidatorPlan[]): boolean {
     const startTime = Date.now();
@@ -32,6 +33,9 @@ export function runValidatorPlan(usedBlocks: Blockly.Block[], plan: pxt.blocks.V
             case "blockFieldValueExists":
                 [successfulBlocks, checkPassed] = [...runBlockFieldValueExistsValidation(usedBlocks, check as pxt.blocks.BlockFieldValueExistsCheck)];
                 break;
+            case "variableUsage":
+                [successfulBlocks, checkPassed] = [...runVariableUsageValidation(usedBlocks, check as pxt.blocks.VariableUsageValidatorCheck)];
+                break;
             default:
                 pxt.debug(`Unrecognized validator: ${check.validator}`);
                 checkPassed = false;
@@ -104,3 +108,19 @@ function runBlockFieldValueExistsValidation(usedBlocks: Blockly.Block[], inputs:
     });
     return  [blockResults.successfulBlocks, blockResults.passed];
 }
+
+function runVariableUsageValidation(usedBlocks: Blockly.Block[], inputs: pxt.blocks.VariableUsageValidatorCheck): [Blockly.Block[], boolean] {
+    const blockResults = validateVariableUsage({
+        usedBlocks,
+        count: inputs.count,
+        name: inputs.name
+    });
+
+    // Flatten the map of passing variable definition blocks
+    const passingVarDefinitions: Blockly.Block[] = [];
+    for (const blocks of blockResults.passingVarDefinitions.values()) {
+        passingVarDefinitions.push(...blocks);
+    }
+
+    return [passingVarDefinitions, blockResults.passed];
+}

pxteditor/code-validation/validateVariableUsage.ts

@@ -0,0 +1,55 @@
+/// <reference path="../../localtypings/pxtpackage.d.ts" />
+import * as Blockly from "blockly";
+
+// Validates that variables are created and used within the workspace.
+// Name is optional. If undefined or empty, all variable names are permitted.
+// Returns the definition blocks for variables that passed the check.
+export function validateVariableUsage({
+    usedBlocks,
+    count,
+    name,
+}: {
+    usedBlocks: Blockly.Block[];
+    count: number;
+    name?: String;
+}): {
+    passingVarDefinitions: Map<string, Blockly.Block[]>;
+    passed: boolean;
+} {
+    const varDefinitionBlocks: Map<string, Blockly.Block[]> = new Map();
+    const usedVars: Set<string> = new Set();
+
+    for (const block of usedBlocks) {
+        if (!block.isEnabled()) {
+            continue;
+        }
+
+        const varsUsed = block.getVarModels();
+        for (const varModel of varsUsed ?? []) {
+            const varName = varModel.name;
+            if (!name || varName === name) {
+                if (block.type === "variables_set" || block.type === "variables_change") {
+                    // Variable created
+                    if (!varDefinitionBlocks.has(varName)) {
+                        varDefinitionBlocks.set(varName, []);
+                    }
+                    varDefinitionBlocks.get(varName).push(block);
+                } else {
+                    // Variable used
+                    usedVars.add(varName);
+                }
+            }
+        }
+    }
+
+    // Var passes check if it is both used and defined.
+    // We return the definition blocks to allow for recursively checking how the var was set.
+    const passingVarDefinitions = new Map<string, Blockly.Block[]>();
+    for (const [varName, definitionBlocks] of varDefinitionBlocks) {
+        if (usedVars.has(varName)) {
+            passingVarDefinitions.set(varName, definitionBlocks);
+        }
+    }
+
+    return { passingVarDefinitions, passed: passingVarDefinitions.size >= count };
+}