diff --git a/src/compiler/irgen.js b/src/compiler/irgen.js index ca3d7f310b1..0ef1a696e88 100644 --- a/src/compiler/irgen.js +++ b/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,7 +1240,7 @@ 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; @@ -1253,13 +1248,25 @@ class ScriptTreeGenerator { // 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,7 +1291,12 @@ 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 + }; } } } @@ -1287,6 +1304,9 @@ class ScriptTreeGenerator { // 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) { diff --git a/test/fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3 b/test/fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3 new file mode 100644 index 00000000000..57a7c005e21 Binary files /dev/null and b/test/fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3 differ diff --git a/test/integration/tw_automatic_variable_creation.js b/test/integration/tw_automatic_variable_creation.js new file mode 100644 index 00000000000..e77d575ad53 --- /dev/null +++ b/test/integration/tw_automatic_variable_creation.js @@ -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(); + }); + }); +} diff --git a/test/snapshot/__snapshots__/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot b/test/snapshot/__snapshots__/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot new file mode 100644 index 00000000000..ac6f722391f --- /dev/null +++ b/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; +}; }) diff --git a/test/snapshot/__snapshots__/warp-timer/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot b/test/snapshot/__snapshots__/warp-timer/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot new file mode 100644 index 00000000000..ac6f722391f --- /dev/null +++ b/test/snapshot/__snapshots__/warp-timer/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; +}; })