From 387ee3a0959a0b211e78ca773b8cfee8ce6c1018 Mon Sep 17 00:00:00 2001 From: Thomas Sparks <69657545+thsparks@users.noreply.github.com> Date: Fri, 3 May 2024 15:25:16 -0700 Subject: [PATCH] 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 | 32 +++++------ .../teachertool/validator-plans-shared.json | 44 ++++----------- localtypings/validatorPlan.d.ts | 6 ++ pxteditor/code-validation/runValidatorPlan.ts | 20 +++++++ .../code-validation/validateVariableUsage.ts | 55 +++++++++++++++++++ 5 files changed, 109 insertions(+), 48 deletions(-) create mode 100644 pxteditor/code-validation/validateVariableUsage.ts diff --git a/common-docs/teachertool/catalog-shared.json b/common-docs/teachertool/catalog-shared.json index 922e2a0558a8..883162e77ca0 100644 --- a/common-docs/teachertool/catalog-shared.json +++ b/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 + } + ] } ] } diff --git a/common-docs/teachertool/validator-plans-shared.json b/common-docs/teachertool/validator-plans-shared.json index ecfb58877648..09fa4551c12d 100644 --- a/common-docs/teachertool/validator-plans-shared.json +++ b/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": "" + } + ] } ] } diff --git a/localtypings/validatorPlan.d.ts b/localtypings/validatorPlan.d.ts index 439e94fef223..13ec08c14972 100644 --- a/localtypings/validatorPlan.d.ts +++ b/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; diff --git a/pxteditor/code-validation/runValidatorPlan.ts b/pxteditor/code-validation/runValidatorPlan.ts index 3a96867a1de7..1022341fd0c9 100644 --- a/pxteditor/code-validation/runValidatorPlan.ts +++ b/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]; +} diff --git a/pxteditor/code-validation/validateVariableUsage.ts b/pxteditor/code-validation/validateVariableUsage.ts new file mode 100644 index 000000000000..97c023f41597 --- /dev/null +++ b/pxteditor/code-validation/validateVariableUsage.ts @@ -0,0 +1,55 @@ +/// +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; + passed: boolean; +} { + const varDefinitionBlocks: Map = new Map(); + const usedVars: Set = 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(); + for (const [varName, definitionBlocks] of varDefinitionBlocks) { + if (usedVars.has(varName)) { + passingVarDefinitions.set(varName, definitionBlocks); + } + } + + return { passingVarDefinitions, passed: passingVarDefinitions.size >= count }; +}