ir: Match vanilla quirks when handling variable or list fields with null ID

Author: GarboMuffin

src/compiler/irgen.js

@@ -19,17 +19,12 @@ const LIST_TYPE = 'list';
  */
 
 /**
- * Create a variable codegen object.
- * @param {'target'|'stage'} scope The scope of this variable -- which object owns it.
- * @param {import('../engine/variable.js')} varObj The Scratch Variable
- * @returns {*} A variable codegen object.
+ * @typedef DescendedVariable
+ * @property {'target'|'stage'} scope
+ * @property {string} id
+ * @property {string} name
+ * @property {boolean} isCloud
  */
-const createVariableData = (scope, varObj) => ({
-    scope,
-    id: varObj.id,
-    name: varObj.name,
-    isCloud: varObj.isCloud
-});
 
 /**
  * @param {string} code
@@ -1245,21 +1240,33 @@ class ScriptTreeGenerator {
      * @param {string} name The name of the variable.
      * @param {''|'list'} type The variable type.
      * @private
-     * @returns {*} A parsed variable object.
+     * @returns {DescendedVariable} A parsed variable object.
      */
     _descendVariable (id, name, type) {
         const target = this.target;
         const stage = this.stage;
 
         // Look for by ID in target...
         if (Object.prototype.hasOwnProperty.call(target.variables, id)) {
-            return createVariableData('target', target.variables[id]);
+            const currVar = target.variables[id];
+            return {
+                scope: 'target',
+                id: currVar.id,
+                name: currVar.name,
+                isCloud: currVar.isCloud
+            };
         }
 
         // Look for by ID in stage...
         if (!target.isStage) {
             if (stage && Object.prototype.hasOwnProperty.call(stage.variables, id)) {
-                return createVariableData('stage', stage.variables[id]);
+                const currVar = stage.variables[id];
+                return {
+                    scope: 'stage',
+                    id: currVar.id,
+                    name: currVar.name,
+                    isCloud: currVar.isCloud
+                };
             }
         }
 
@@ -1268,7 +1275,12 @@ class ScriptTreeGenerator {
             if (Object.prototype.hasOwnProperty.call(target.variables, varId)) {
                 const currVar = target.variables[varId];
                 if (currVar.name === name && currVar.type === type) {
-                    return createVariableData('target', currVar);
+                    return {
+                        scope: 'target',
+                        id: currVar.id,
+                        name: currVar.name,
+                        isCloud: currVar.isCloud
+                    };
                 }
             }
         }
@@ -1279,14 +1291,22 @@ class ScriptTreeGenerator {
                 if (Object.prototype.hasOwnProperty.call(stage.variables, varId)) {
                     const currVar = stage.variables[varId];
                     if (currVar.name === name && currVar.type === type) {
-                        return createVariableData('stage', currVar);
+                        return {
+                            scope: 'stage',
+                            id: currVar.id,
+                            name: currVar.name,
+                            isCloud: currVar.isCloud
+                        };
                     }
                 }
             }
         }
 
         // Create it locally...
         const newVariable = new Variable(id, name, type, false);
+
+        // Intentionally not using newVariable.id so that this matches vanilla Scratch quirks regarding
+        // handling of null variable IDs.
         target.variables[id] = newVariable;
 
         if (target.sprite) {
@@ -1300,7 +1320,14 @@ class ScriptTreeGenerator {
             }
         }
 
-        return createVariableData('target', newVariable);
+        return {
+            scope: 'target',
+            // If the given ID was null, this won't match the .id property of the Variable object.
+            // This is intentional to match vanilla Scratch quirks.
+            id,
+            name: newVariable.name,
+            isCloud: newVariable.isCloud
+        };
     }
 
     descendProcedure (block) {

test/fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3

@@ -0,0 +1,43 @@
+const fs = require('fs');
+const path = require('path');
+const {test} = require('tap');
+const VM = require('../../src/virtual-machine');
+
+for (const compilerEnabled of [false, true]) {
+    const prefix = compilerEnabled ? 'compiler' : 'interpreter';
+    test(`${prefix} - quirks when block field has literal null for variable ID`, t => {
+        const vm = new VM();
+        vm.setCompilerOptions({
+            enabled: compilerEnabled
+        });
+        t.equal(vm.runtime.compilerOptions.enabled, compilerEnabled, 'compiler options sanity check');
+
+        // The execute tests ensure that this fixture compiles and runs fine and the snapshot test ensures
+        // it compiles correctly. This additional test will ensure that the internal variable objects are
+        // being created with the expected properties.
+        const fixturePath = path.join(
+            __dirname,
+            '../fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3'
+        );
+
+        vm.loadProject(fs.readFileSync(fixturePath)).then(() => {
+            vm.greenFlag();
+            vm.runtime._step();
+
+            // Variable does not exist, should get made as local variable in sprite
+            const variables = vm.runtime.targets[1].variables;
+            t.equal(Object.keys(variables).length, 1, 'created 1 new variable');
+
+            // Scratch quirk - the entry in .variables should have key "null"
+            const newVariableKey = Object.keys(variables)[0];
+            t.equal(newVariableKey, 'null', 'key is "null"');
+
+            // Scratch quirk - the actual variable.id should be the random string
+            const newVariable = Object.values(variables)[0];
+            t.notEqual(newVariable.id, 'null', 'variable.id is not "null"');
+            t.type(newVariable.id, 'string', 'variable.id is a string');
+
+            t.end();
+        });
+    });
+}

test/integration/tw_automatic_variable_creation.js

@@ -0,0 +1,13 @@
+// TW Snapshot
+// Input SHA-256: 4764ae15e39b22b1a071c9ac79f8758f24ef41855802db8674e200fd26139ed0
+
+// Sprite1 script
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+const b0 = runtime.getOpcodeFunction("looks_say");
+const b1 = target.variables["null"];
+return function* genXYZ () {
+yield* executeInCompatibilityLayer({"MESSAGE":"plan 0",}, b0, false, false, "a", null);
+b1.value = 5;
+yield* executeInCompatibilityLayer({"MESSAGE":"end",}, b0, false, false, "d", null);
+retire(); return;
+}; })

test/snapshot/__snapshots__/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot

@@ -0,0 +1,13 @@
+// TW Snapshot
+// Input SHA-256: 4764ae15e39b22b1a071c9ac79f8758f24ef41855802db8674e200fd26139ed0
+
+// Sprite1 script
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+const b0 = runtime.getOpcodeFunction("looks_say");
+const b1 = target.variables["null"];
+return function* genXYZ () {
+yield* executeInCompatibilityLayer({"MESSAGE":"plan 0",}, b0, false, false, "a", null);
+b1.value = 5;
+yield* executeInCompatibilityLayer({"MESSAGE":"end",}, b0, false, false, "d", null);
+retire(); return;
+}; })