Fix up variable references after deserializing a .sprite2 or .sprite3 to avoid conflicts with pre-existing variables.

src/engine/blocks.js

@@ -6,6 +6,8 @@ const Clone = require('../util/clone');
 const {Map} = require('immutable');
 const BlocksExecuteCache = require('./blocks-execute-cache');
 const log = require('../util/log');
+const Variable = require('./variable');
+const StringUtil = require('../util/string-util');
 
 /**
  * @fileoverview
@@ -670,6 +672,68 @@ class Blocks {
         this.resetCache();
     }
 
+    /**
+     * Fixes up variable references in this blocks container avoiding conflicts with
+     * pre-existing variables in the same scope.
+     * This is used when uploading a new sprite (the given target)
+     * into an existing project, where the new sprite may contain references
+     * to variable names that already exist as global variables in the project
+     * (and thus are in scope for variable references in the given sprite).
+     *
+     * If the given target has a block that references an existing global variable and that
+     * variable *does not* exist in the target itself (e.g. it was a global variable in the
+     * project the sprite was originally exported from), fix the variable references in this sprite
+     * to reference the id of the pre-existing global variable.
+     * If the given target has a block that references an existing global variable and that
+     * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded),
+     * then the variable is renamed to distinguish itself from the pre-existing variable.
+     * All blocks that reference the local variable will be updated to use the new name.
+     * @param {Target} target The new target being uploaded, with potential variable conflicts
+     * @param {Runtime} runtime The runtime context with any pre-existing variables
+     */
+    fixUpVariableReferences (target, runtime) {
+        const blocks = this._blocks;
+        for (const blockId in blocks) {
+            let varOrListField = null;
+            let varType = null;
+            if (blocks[blockId].fields.VARIABLE) {
+                varOrListField = blocks[blockId].fields.VARIABLE;
+                varType = Variable.SCALAR_TYPE;
+            } else if (blocks[blockId].fields.LIST) {
+                varOrListField = blocks[blockId].fields.LIST;
+                varType = Variable.LIST_TYPE;
+            }
+            if (varOrListField) {
+                const currVarId = varOrListField.id;
+                const currVarName = varOrListField.value;
+                if (target.lookupVariableById(currVarId)) {
+                    // Found a variable with the id in either the target or the stage,
+                    // figure out which one.
+                    if (target.variables.hasOwnProperty(currVarId)) {
+                        // 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 = runtime.getTargetForStage();
+                        if (stage && stage.variables &&
+                            stage.lookupVariableByNameAndType(currVarName, varType)) {
+                            // TODO what should the new name be?
+                            const newName = StringUtil.unusedName(
+                                `${target.getName()}_${currVarName}`,
+                                target.getAllVariableNamesInScopeByType(varType));
+                            target.renameVariable(currVarId, newName);
+                            this.updateBlocksAfterVarRename(currVarId, newName);
+                        }
+                    }
+                } else {
+                    const existingVar = target.lookupVariableByNameAndType(currVarName, varType);
+                    if (existingVar) {
+                        varOrListField.id = existingVar.id;
+                    }
+                }
+            }
+        }
+    }
+
     /**
      * Keep blocks up to date after a variable gets renamed.
      * @param {string} varId The id of the variable that was renamed

src/engine/target.js

@@ -80,14 +80,40 @@ class Target extends EventEmitter {
     }
 
     /**
-     * Look up a variable object, and create it if one doesn't exist.
+     * Get the names of all the variables of the given type that are in scope for this target.
+     * For targets that are not the stage, this includes any target-specific
+     * variables as well as any stage variables.
+     * For the stage, this is all stage variables.
+     * @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE
+     * @return {Array<string>} A list of variable names
+     */
+    getAllVariableNamesInScopeByType (type) {
+        if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
+        const targetVariables = Object.values(this.variables)
+            .filter(v => v.type === type)
+            .map(variable => variable.name);
+        if (this.isStage || !this.runtime) {
+            return targetVariables;
+        }
+        const stage = this.runtime.getTargetForStage();
+        const stageVariables = stage.getAllVariableNamesInScopeByType(type);
+        return targetVariables.concat(stageVariables);
+    }
+
+    /**
+     * Look up a variable object, first by id, and then by name if the id is not found.
+     * Create a new variable if both lookups fail.
      * @param {string} id Id of the variable.
      * @param {string} name Name of the variable.
      * @return {!Variable} Variable object.
      */
     lookupOrCreateVariable (id, name) {
-        const variable = this.lookupVariableById(id);
+        let variable = this.lookupVariableById(id);
         if (variable) return variable;
+
+        variable = this.lookupVariableByNameAndType(name, Variable.SCALAR_TYPE);
+        if (variable) return variable;
+
         // No variable with this name exists - create it locally.
         const newVariable = new Variable(id, name, Variable.SCALAR_TYPE, false);
         this.variables[id] = newVariable;
@@ -161,6 +187,40 @@ class Target extends EventEmitter {
         }
     }
 
+    /**
+     * Look up a variable object by its name and variable type.
+     * Search begins with local variables; then global variables if a local one
+     * was not found.
+     * @param {string} name Name of the variable.
+     * @param {string} type Type of the variable. Defaults to Variable.SCALAR_TYPE.
+     * @return {?Variable} Variable object if found, or null if not.
+     */
+    lookupVariableByNameAndType (name, type) {
+        if (typeof name !== 'string') return;
+        if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
+
+        for (const varId in this.variables) {
+            const currVar = this.variables[varId];
+            if (currVar.name === name && currVar.type === type) {
+                return currVar;
+            }
+        }
+
+        if (this.runtime && !this.isStage) {
+            const stage = this.runtime.getTargetForStage();
+            if (stage) {
+                for (const varId in stage.variables) {
+                    const currVar = stage.variables[varId];
+                    if (currVar.name === name && currVar.type === type) {
+                        return currVar;
+                    }
+                }
+            }
+        }
+
+        return null;
+    }
+
     /**
     * Look up a list object for this target, and create it if one doesn't exist.
     * Search begins for local lists; then look for globals.
@@ -169,8 +229,12 @@ class Target extends EventEmitter {
     * @return {!Varible} Variable object representing the found/created list.
      */
     lookupOrCreateList (id, name) {
-        const list = this.lookupVariableById(id);
+        let list = this.lookupVariableById(id);
         if (list) return list;
+
+        list = this.lookupVariableByNameAndType(name, Variable.LIST_TYPE);
+        if (list) return list;
+
         // No variable with this name exists - create it locally.
         const newList = new Variable(id, name, Variable.LIST_TYPE, false);
         this.variables[id] = newList;
@@ -240,11 +304,14 @@ class Target extends EventEmitter {
                         name: 'VARIABLE',
                         value: id
                     }, this.runtime);
+                    const monitorBlock = blocks.getBlock(variable.id);
+                    if (monitorBlock) {
                         this.runtime.requestUpdateMonitor(Map({
                             id: id,
-                        params: blocks._getBlockParams(blocks.getBlock(variable.id))
+                            params: blocks._getBlockParams(monitorBlock)
                         }));
                     }
+                }
 
             }
         }

src/serialization/sb2.js

@@ -189,19 +189,14 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId,
  * Create a callback for assigning fixed IDs to imported variables
  * Generator stores the global variable mapping in a closure
  * @param {!string} targetId the id of the target to scope the variable to
- * @param {object=} initialVars Initial vars in scope if parsing a sprite
  * @return {string} variable ID
  */
 const generateVariableIdGetter = (function () {
     let globalVariableNameMap = {};
     const namer = (targetId, name) => `${targetId}-${name}`;
-    return function (targetId, topLevel, initialVars) {
+    return function (targetId, topLevel) {
         // Reset the global variable map if topLevel
-        if (topLevel) {
-            globalVariableNameMap = {};
-        } else if (initialVars) {
-            globalVariableNameMap = initialVars;
-        }
+        if (topLevel) globalVariableNameMap = {};
         return function (name) {
             if (topLevel) { // Store the name/id pair in the globalVariableNameMap
                 globalVariableNameMap[name] = namer(targetId, name);
@@ -339,10 +334,9 @@ const parseMonitorObject = (object, runtime, targets, extensions) => {
  * @param {ImportedExtensionsInfo} extensions - (in/out) parsed extension information will be stored here.
  * @param {boolean} topLevel - Whether this is the top-level object (stage).
  * @param {?object} zip - Optional zipped assets for local file import
- * @param {object=} initialVarScope - Initial set of variables in scope if parsing a single sprite
  * @return {!Promise.<Array.<Target>>} Promise for the loaded targets when ready, or null for unsupported objects.
  */
-const parseScratchObject = function (object, runtime, extensions, topLevel, zip, initialVarScope) {
+const parseScratchObject = function (object, runtime, extensions, topLevel, zip) {
     if (!object.hasOwnProperty('objName')) {
         if (object.hasOwnProperty('listName')) {
             // Shim these objects so they can be processed as monitors
@@ -433,7 +427,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip,
     // Create the first clone, and load its run-state from JSON.
     const target = sprite.createClone(topLevel ? StageLayering.BACKGROUND_LAYER : StageLayering.SPRITE_LAYER);
 
-    const getVariableId = generateVariableIdGetter(target.id, topLevel, initialVarScope);
+    const getVariableId = generateVariableIdGetter(target.id, topLevel);
 
     const globalBroadcastMsgObj = globalBroadcastMsgStateGenerator(topLevel);
     const addBroadcastMsg = globalBroadcastMsgObj.broadcastMsgMapUpdater;
@@ -673,18 +667,14 @@ const sb2import = function (json, runtime, optForceSprite, zip) {
         extensionURLs: new Map()
     };
 
-    const initialGlobalVars = {};
-    if (optForceSprite && runtime.targets && runtime.getTargetForStage()) {
-        const stageVars = runtime.getTargetForStage().variables;
-        for (const varId in stageVars) {
-            const currVar = stageVars[varId];
-            initialGlobalVars[currVar.name] = varId;
+    return parseScratchObject(json, runtime, extensions, !optForceSprite, zip)
+        .then(targets => {
+            if (optForceSprite && targets.length === 1) {
+                const target = targets[0];
+                target.blocks.fixUpVariableReferences(target, runtime);
             }
-    }
-
-    // If parsing a sprite, use the current global vars as parent scope for the sprite
-    return parseScratchObject(json, runtime, extensions, !optForceSprite, zip,
-        (optForceSprite ? initialGlobalVars : null))
+            return targets;
+        })
         .then(targets => ({
             targets,
             extensions

src/serialization/sb3.js

@@ -936,7 +936,15 @@ const deserialize = function (json, runtime, zip, isSingleSprite) {
     return Promise.all(
         ((isSingleSprite ? [json] : json.targets) || []).map(target =>
             parseScratchObject(target, runtime, extensions, zip))
-    ).then(targets => ({
+    )
+        .then(targets => {
+            if (isSingleSprite && targets.length === 1) {
+                const target = targets[0];
+                target.blocks.fixUpVariableReferences(target, runtime);
+            }
+            return targets;
+        })
+        .then(targets => ({
             targets,
             extensions
         }));