fixUpVariableReferences should handle conflicting local variables that don't have blocks referencing them. Add unit tests for the function.

src/engine/target.js

@@ -354,8 +354,29 @@ class Target extends EventEmitter {
     fixUpVariableReferences () {
         if (!this.runtime) return; // There's no runtime context to conflict with
         if (this.isStage) return; // Stage can't have variable conflicts with itself (and also can't be uploaded)
+        const stage = this.runtime.getTargetForStage();
+        if (!stage || !stage.variables) return;
+
+        const renameConflictingLocalVar = (id, name, type) => {
+            const conflict = stage.lookupVariableByNameAndType(name, type);
+            if (conflict) {
+                const newName = StringUtil.unusedName(
+                    `${this.getName()}: ${name}`,
+                    this.getAllVariableNamesInScopeByType(type));
+                this.renameVariable(id, newName);
+                return newName;
+            }
+            return null;
+        };
 
         const allReferences = this.blocks.getAllVariableAndListReferences();
+        const unreferencedLocalVarIds = [];
+        if (Object.keys(this.variables).length > 0) {
+            for (const localVarId in this.variables) {
+                if (!this.variables.hasOwnProperty(localVarId)) continue;
+                if (!allReferences[localVarId]) unreferencedLocalVarIds.push(localVarId);
+            }
+        }
         const conflictIdsToReplace = Object.create(null);
         for (const varId in allReferences) {
             // We don't care about which var ref we get, they should all have the same var info
@@ -369,15 +390,16 @@ class Target extends EventEmitter {
                     // If the target has the variable, then check whether the stage
                     // has one with the same name and type. If it does, then rename
                     // this target specific variable so that there is a distinction.
-                    const stage = this.runtime.getTargetForStage();
-                    if (stage && stage.variables &&
-                        stage.lookupVariableByNameAndType(varName, varType)) {
-                        // TODO what should the new name be?
-                        const newName = StringUtil.unusedName(
-                            `${this.getName()}_${varName}`,
-                            this.getAllVariableNamesInScopeByType(varType));
-                        this.renameVariable(varId, newName);
-                        this.blocks.updateBlocksAfterVarRename(varId, newName);
+                    const newVarName = renameConflictingLocalVar(varId, varName, varType);
+
+                    if (newVarName) {
+                        // We are not calling this.blocks.updateBlocksAfterVarRename
+                        // here because it will search through all the blocks. We already
+                        // have access to all the references for this var id.
+                        allReferences[varId].map(ref => {
+                            ref.referencingField.value = newVarName;
+                            return ref;
+                        });
                     }
                 }
             } else {
@@ -387,6 +409,17 @@ class Target extends EventEmitter {
                 }
             }
         }
+        // Rename any local variables that were missed above because they aren't
+        // referenced by any blocks
+        for (const id in unreferencedLocalVarIds) {
+            const varId = unreferencedLocalVarIds[id];
+            const name = this.variables[varId].name;
+            const type = this.variables[varId].type;
+            renameConflictingLocalVar(varId, name, type);
+        }
+        // Finally, handle global var conflicts (e.g. a sprite is uploaded, and has
+        // blocks referencing some variable that the sprite does not own, and this
+        // variable conflicts with a global var)
         for (const conflictId in conflictIdsToReplace) {
             const existingId = conflictIdsToReplace[conflictId];
             allReferences[conflictId].map(varRef => {

test/unit/engine_target.js

@@ -1,6 +1,9 @@
 const test = require('tap').test;
 const Target = require('../../src/engine/target');
 const Variable = require('../../src/engine/variable');
+const adapter = require('../../src/engine/adapter');
+const Runtime = require('../../src/engine/runtime');
+const events = require('../fixtures/events.json');
 
 test('spec', t => {
     const target = new Target();
@@ -145,7 +148,7 @@ test('deleteVariable2', t => {
     t.end();
 });
 
-test('lookupOrCreateList creates a list if var with given id does not exist', t => {
+test('lookupOrCreateList creates a list if var with given id or var with given name does not exist', t => {
     const target = new Target();
     const variables = target.variables;
 
@@ -174,6 +177,22 @@ test('lookupOrCreateList returns list if one with given id exists', t => {
     t.end();
 });
 
+test('lookupOrCreateList succeeds in finding list if id is incorrect but name matches', t => {
+    const target = new Target();
+    const variables = target.variables;
+
+    t.equal(Object.keys(variables).length, 0);
+    target.createVariable('foo', 'bar', Variable.LIST_TYPE);
+    t.equal(Object.keys(variables).length, 1);
+
+    const listVar = target.lookupOrCreateList('not foo', 'bar');
+    t.equal(Object.keys(variables).length, 1);
+    t.equal(listVar.id, 'foo');
+    t.equal(listVar.name, 'bar');
+
+    t.end();
+});
+
 test('lookupBroadcastMsg returns the var with given id if exists', t => {
     const target = new Target();
     const variables = target.variables;
@@ -263,3 +282,147 @@ test('creating a comment with a blockId also updates the comment property on the
 
     t.end();
 });
+
+test('fixUpVariableReferences fixes sprite global var conflicting with project global var', t => {
+    const runtime = new Runtime();
+
+    const stage = new Target(runtime);
+    stage.isStage = true;
+
+    const target = new Target(runtime);
+    target.isStage = false;
+
+    runtime.targets = [stage, target];
+
+    // Create a global variable
+    stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE);
+
+    target.blocks.createBlock(adapter(events.mockVariableBlock)[0]);
+
+    t.equal(Object.keys(target.variables).length, 0);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+
+    target.fixUpVariableReferences(target, runtime);
+
+    t.equal(Object.keys(target.variables).length, 0);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'pre-existing global var id');
+
+    t.end();
+});
+
+test('fixUpVariableReferences fixes sprite local var conflicting with project global var', t => {
+    const runtime = new Runtime();
+
+    const stage = new Target(runtime);
+    stage.isStage = true;
+
+    const target = new Target(runtime);
+    target.isStage = false;
+    target.getName = () => 'Target';
+
+    runtime.targets = [stage, target];
+
+    // Create a global variable
+    stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE);
+    target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE);
+
+    target.blocks.createBlock(adapter(events.mockVariableBlock)[0]);
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+    t.equal(target.variables['mock var id'].name, 'a mock variable');
+
+    target.fixUpVariableReferences(target, runtime);
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+    t.equal(target.variables['mock var id'].name, 'Target: a mock variable');
+
+    t.end();
+});
+
+test('fixUpVariableReferences fixes conflicting sprite local var without blocks referencing var', t => {
+    const runtime = new Runtime();
+
+    const stage = new Target(runtime);
+    stage.isStage = true;
+
+    const target = new Target(runtime);
+    target.isStage = false;
+    target.getName = () => 'Target';
+
+    runtime.targets = [stage, target];
+
+    // Create a global variable
+    stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE);
+    target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE);
+
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.equal(target.variables['mock var id'].name, 'a mock variable');
+
+    target.fixUpVariableReferences(target, runtime);
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.equal(target.variables['mock var id'].name, 'Target: a mock variable');
+
+    t.end();
+});
+
+test('fixUpVariableReferences does not change variable name if there is no variable conflict', t => {
+    const runtime = new Runtime();
+
+    const stage = new Target(runtime);
+    stage.isStage = true;
+
+    const target = new Target(runtime);
+    target.isStage = false;
+    target.getName = () => 'Target';
+
+    runtime.targets = [stage, target];
+
+    // Create a global variable
+    stage.createVariable('pre-existing global var id', 'a variable', Variable.SCALAR_TYPE);
+    stage.createVariable('pre-existing global list id', 'a mock variable', Variable.LIST_TYPE);
+    target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE);
+
+    target.blocks.createBlock(adapter(events.mockVariableBlock)[0]);
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 2);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+    t.equal(target.variables['mock var id'].name, 'a mock variable');
+
+    target.fixUpVariableReferences(target, runtime);
+
+    t.equal(Object.keys(target.variables).length, 1);
+    t.equal(Object.keys(stage.variables).length, 2);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+    t.equal(target.variables['mock var id'].name, 'a mock variable');
+
+    t.end();
+});