From ed608ffe6f26a4093ab3b101c21adf91a5cb9cf9 Mon Sep 17 00:00:00 2001
From: Karishma Chadha <kchadha@scratch.mit.edu>
Date: Tue, 3 Jul 2018 23:20:49 -0400
Subject: [PATCH] Refactor to fix issue with global var not checking for name
 conflicts on all sprites.

---
 src/engine/blocks.js  | 28 +++++++++++++++++-----------
 src/engine/runtime.js |  9 +++++++++
 src/engine/target.js  | 14 +++++++++-----
 3 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/src/engine/blocks.js b/src/engine/blocks.js
index 7d052d612..7b184bf34 100644
--- a/src/engine/blocks.js
+++ b/src/engine/blocks.js
@@ -330,19 +330,25 @@ class Blocks {
             this.deleteBlock(e.blockId);
             break;
         case 'var_create':
-            // New variables being created by the user are all global.
-            // Check if this variable exists on the current target or stage.
-            // If not, create it on the appropriate target based on the event's
-            // 'isLocal' flag.
-            if (editingTarget) {
+            // Check if the variable being created is global or local
+            // If local, create a local var on the current editing target, as long
+            // as there are no conflicts, and the current target is actually a sprite
+            // If global or if the editing target is not present or we somehow got
+            // into a state where a local var was requested for the stage,
+            // create a stage (global) var after checking for name conflicts
+            // on all the sprites.
+            if (e.isLocal && editingTarget && !editingTarget.isStage) {
                 if (!editingTarget.lookupVariableById(e.varId)) {
-                    const varTarget = e.isLocal ? editingTarget : stage;
-                    varTarget.createVariable(e.varId, e.varName, e.varType);
+                    editingTarget.createVariable(e.varId, e.varName, e.varType);
+                }
+            } else {
+                // Check for name conflicts in all of the targets
+                const allTargets = optRuntime.targets.filter(t => t.isOriginal);
+                for (const target of allTargets) {
+                    if (target.lookupVariableByNameAndType(e.varName, e.varType, true)) {
+                        return;
+                    }
                 }
-            } else if (!stage.lookupVariableById(e.varId)) {
-                // Since getEditingTarget returned null, we now need to
-                // explicitly check if the stage has the variable, and
-                // create one if not.
                 stage.createVariable(e.varId, e.varName, e.varType);
             }
             break;
diff --git a/src/engine/runtime.js b/src/engine/runtime.js
index f34ccb604..a458fddee 100644
--- a/src/engine/runtime.js
+++ b/src/engine/runtime.js
@@ -1715,6 +1715,15 @@ class Runtime extends EventEmitter {
         return this._editingTarget;
     }
 
+    getAllVarNamesOfType (varType) {
+        let varNames = [];
+        for (const target of this.targets) {
+            const targetVarNames = target.getAllVariableNamesInScopeByType(varType, true);
+            varNames = varNames.concat(targetVarNames);
+        }
+        return varNames;
+    }
+
     /**
      * Tell the runtime to request a redraw.
      * Use after a clone/sprite has completed some visible operation on the stage.
diff --git a/src/engine/target.js b/src/engine/target.js
index 69620c91b..89cb27d06 100644
--- a/src/engine/target.js
+++ b/src/engine/target.js
@@ -83,17 +83,19 @@ class Target extends EventEmitter {
     /**
      * 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.
+     * variables as well as any stage variables unless the skipStage flag is true.
      * For the stage, this is all stage variables.
      * @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE
+     * @param {?bool} skipStage Optional flag to skip the stage.
      * @return {Array<string>} A list of variable names
      */
-    getAllVariableNamesInScopeByType (type) {
+    getAllVariableNamesInScopeByType (type, skipStage) {
         if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
+        skipStage = skipStage || false;
         const targetVariables = Object.values(this.variables)
             .filter(v => v.type === type)
             .map(variable => variable.name);
-        if (this.isStage || !this.runtime) {
+        if (skipStage || this.isStage || !this.runtime) {
             return targetVariables;
         }
         const stage = this.runtime.getTargetForStage();
@@ -194,11 +196,13 @@ class Target extends EventEmitter {
      * was not found.
      * @param {string} name Name of the variable.
      * @param {string} type Type of the variable. Defaults to Variable.SCALAR_TYPE.
+     * @param {?bool} skipStage Optional flag to skip checking the stage
      * @return {?Variable} Variable object if found, or null if not.
      */
-    lookupVariableByNameAndType (name, type) {
+    lookupVariableByNameAndType (name, type, skipStage) {
         if (typeof name !== 'string') return;
         if (typeof type !== 'string') type = Variable.SCALAR_TYPE;
+        skipStage = skipStage || false;
 
         for (const varId in this.variables) {
             const currVar = this.variables[varId];
@@ -207,7 +211,7 @@ class Target extends EventEmitter {
             }
         }
 
-        if (this.runtime && !this.isStage) {
+        if (!skipStage && this.runtime && !this.isStage) {
             const stage = this.runtime.getTargetForStage();
             if (stage) {
                 for (const varId in stage.variables) {