fix: creating duplicate params via renaming vars (#6885)

* feat: add IVariableHolder

* chore: reorganize variable prompts to early return

* fix: add retriggering prompt for conflicting params

* chore: add unit tests

* chore: fix build

* chore: reorganize checking for param conflicts

* fix: visibility

* chore: rename variable holder interface

* chore: fix typo

* chore: fix lint

.eslintrc.js

@@ -176,6 +176,7 @@ function buildTSOverride({files, tsconfig}) {
       'jsdoc/check-param-names': ['off', {'checkDestructured': false}],
       // Allow any text in the license tag. Other checks are not relevant.
       'jsdoc/check-values': ['off'],
+      'jsdoc/newline-after-description': ['error'],
     },
   };
 }

core/blockly.ts

@@ -106,6 +106,7 @@ import {ISelectableToolboxItem} from './interfaces/i_selectable_toolbox_item.js'
 import {IStyleable} from './interfaces/i_styleable.js';
 import {IToolbox} from './interfaces/i_toolbox.js';
 import {IToolboxItem} from './interfaces/i_toolbox_item.js';
+import {IVariableBackedParameterModel, isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js';
 import * as internalConstants from './internal_constants.js';
 import {ASTNode} from './keyboard_nav/ast_node.js';
 import {BasicCursor} from './keyboard_nav/basic_cursor.js';
@@ -701,6 +702,7 @@ export {ISelectableToolboxItem};
 export {IStyleable};
 export {IToolbox};
 export {IToolboxItem};
+export {IVariableBackedParameterModel, isVariableBackedParameterModel};
 export {Marker};
 export {MarkerManager};
 export {Menu};

core/interfaces/i_legacy_procedure_blocks.ts

@@ -0,0 +1,47 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+
+/**
+ * Legacy means of representing a procedure signature. The elements are
+ * respectively: name, parameter names, and whether it has a return value.
+ */
+export type ProcedureTuple = [string, string[], boolean];
+
+/**
+ * Procedure block type.
+ *
+ * @internal
+ */
+export interface ProcedureBlock {
+  getProcedureCall: () => string;
+  renameProcedure: (p1: string, p2: string) => void;
+  getProcedureDef: () => ProcedureTuple;
+}
+
+/** @internal */
+export interface LegacyProcedureDefBlock {
+  getProcedureDef: () => ProcedureTuple
+}
+
+/** @internal */
+export function isLegacyProcedureDefBlock(block: Object):
+    block is LegacyProcedureDefBlock {
+  return (block as any).getProcedureDef !== undefined;
+}
+
+/** @internal */
+export interface LegacyProcedureCallBlock {
+  getProcedureCall: () => string;
+  renameProcedure: (p1: string, p2: string) => void;
+}
+
+/** @internal */
+export function isLegacyProcedureCallBlock(block: Object):
+    block is LegacyProcedureCallBlock {
+  return (block as any).getProcedureCall !== undefined &&
+      (block as any).renameProcedure !== undefined;
+}

core/interfaces/i_variable_backed_parameter_model.ts

@@ -0,0 +1,23 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import type {VariableModel} from '../variable_model.js';
+import {IParameterModel} from './i_parameter_model.js';
+
+
+/** Interface for a parameter model that holds a variable model. */
+export interface IVariableBackedParameterModel extends IParameterModel {
+  /** Returns the variable model held by this type. */
+  getVariableModel(): VariableModel;
+}
+
+/**
+ * Returns whether the given object is a variable holder or not.
+ */
+export function isVariableBackedParameterModel(param: IParameterModel):
+    param is IVariableBackedParameterModel {
+  return (param as any).getVariableModel !== undefined;
+}

core/procedures.ts

@@ -29,6 +29,7 @@ import {IParameterModel} from './interfaces/i_parameter_model.js';
 import {IProcedureMap} from './interfaces/i_procedure_map.js';
 import {IProcedureModel} from './interfaces/i_procedure_model.js';
 import {IProcedureBlock, isProcedureBlock} from './interfaces/i_procedure_block.js';
+import {isLegacyProcedureCallBlock, isLegacyProcedureDefBlock, ProcedureBlock, ProcedureTuple} from './interfaces/i_legacy_procedure_blocks.js';
 import {ObservableProcedureMap} from './observable_procedure_map.js';
 import * as utilsXml from './utils/xml.js';
 import * as Variables from './variables.js';
@@ -50,37 +51,6 @@ export const CATEGORY_NAME = 'PROCEDURE';
  */
 export const DEFAULT_ARG = 'x';
 
-export type ProcedureTuple = [string, string[], boolean];
-
-/**
- * Procedure block type.
- */
-export interface ProcedureBlock {
-  getProcedureCall: () => string;
-  renameProcedure: (p1: string, p2: string) => void;
-  getProcedureDef: () => ProcedureTuple;
-}
-
-interface LegacyProcedureDefBlock {
-  getProcedureDef: () => ProcedureTuple
-}
-
-function isLegacyProcedureDefBlock(block: Object):
-    block is LegacyProcedureDefBlock {
-  return (block as any).getProcedureDef !== undefined;
-}
-
-interface LegacyProcedureCallBlock {
-  getProcedureCall: () => string;
-  renameProcedure: (p1: string, p2: string) => void;
-}
-
-function isLegacyProcedureCallBlock(block: Object):
-    block is LegacyProcedureCallBlock {
-  return (block as any).getProcedureCall !== undefined &&
-      (block as any).renameProcedure !== undefined;
-}
-
 /**
  * Find all user-created procedure definitions in a workspace.
  *
@@ -492,4 +462,5 @@ export {
   isProcedureBlock,
   IProcedureMap,
   IProcedureModel,
+  ProcedureTuple,
 };

core/variables.ts

@@ -14,7 +14,9 @@ goog.declareModuleId('Blockly.Variables');
 
 import {Blocks} from './blocks.js';
 import * as dialog from './dialog.js';
+import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js';
 import {Msg} from './msg.js';
+import {isLegacyProcedureDefBlock} from './interfaces/i_legacy_procedure_blocks.js';
 import * as utilsXml from './utils/xml.js';
 import {VariableModel} from './variable_model.js';
 import type {Workspace} from './workspace.js';
@@ -249,32 +251,28 @@ export function createVariableButtonHandler(
   // This function needs to be named so it can be called recursively.
   function promptAndCheckWithAlert(defaultName: string) {
     promptName(Msg['NEW_VARIABLE_TITLE'], defaultName, function(text) {
-      if (text) {
-        const existing = nameUsedWithAnyType(text, workspace);
-        if (existing) {
-          let msg;
-          if (existing.type === type) {
-            msg = Msg['VARIABLE_ALREADY_EXISTS'].replace('%1', existing.name);
-          } else {
-            msg = Msg['VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE'];
-            msg = msg.replace('%1', existing.name).replace('%2', existing.type);
-          }
-          dialog.alert(msg, function() {
-            promptAndCheckWithAlert(text);
-          });
-        } else {
-          // No conflict
-          workspace.createVariable(text, type);
-          if (opt_callback) {
-            opt_callback(text);
-          }
-        }
-      } else {
-        // User canceled prompt.
-        if (opt_callback) {
-          opt_callback(null);
-        }
+      if (!text) {  // User canceled prompt.
+        if (opt_callback) opt_callback(null);
+        return;
       }
+
+      const existing = nameUsedWithAnyType(text, workspace);
+      if (!existing) {  // No conflict
+        workspace.createVariable(text, type);
+        if (opt_callback) opt_callback(text);
+        return;
+      }
+
+      let msg;
+      if (existing.type === type) {
+        msg = Msg['VARIABLE_ALREADY_EXISTS'].replace('%1', existing.name);
+      } else {
+        msg = Msg['VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE'];
+        msg = msg.replace('%1', existing.name).replace('%2', existing.type);
+      }
+      dialog.alert(msg, function() {
+        promptAndCheckWithAlert(text);
+      });
     });
   }
   promptAndCheckWithAlert('');
@@ -299,28 +297,33 @@ export function renameVariable(
     const promptText =
         Msg['RENAME_VARIABLE_TITLE'].replace('%1', variable.name);
     promptName(promptText, defaultName, function(newName) {
-      if (newName) {
-        const existing =
-            nameUsedWithOtherType(newName, variable.type, workspace);
-        if (existing) {
-          const msg = Msg['VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE']
-                          .replace('%1', existing.name)
-                          .replace('%2', existing.type);
-          dialog.alert(msg, function() {
-            promptAndCheckWithAlert(newName);
-          });
-        } else {
-          workspace.renameVariableById(variable.getId(), newName);
-          if (opt_callback) {
-            opt_callback(newName);
-          }
-        }
-      } else {
-        // User canceled prompt.
-        if (opt_callback) {
-          opt_callback(null);
-        }
+      if (!newName) {  // User canceled prompt.
+        if (opt_callback) opt_callback(null);
+        return;
       }
+
+      const existing = nameUsedWithOtherType(newName, variable.type, workspace);
+      const procedure =
+          nameUsedWithConflictingParam(variable.name, newName, workspace);
+      if (!existing && !procedure) {  // No conflict.
+        workspace.renameVariableById(variable.getId(), newName);
+        if (opt_callback) opt_callback(newName);
+        return;
+      }
+
+      let msg = '';
+      if (existing) {
+        msg = Msg['VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE']
+                  .replace('%1', existing.name)
+                  .replace('%2', existing.type);
+      } else if (procedure) {
+        msg = Msg['VARIABLE_ALREADY_EXISTS_FOR_A_PARAMETER']
+                  .replace('%1', newName)
+                  .replace('%2', procedure);
+      }
+      dialog.alert(msg, function() {
+        promptAndCheckWithAlert(newName);
+      });
     });
   }
   promptAndCheckWithAlert('');
@@ -393,6 +396,68 @@ export function nameUsedWithAnyType(
   return null;
 }
 
+/**
+ * Returns the name of the procedure with a conflicting parameter name, or null
+ * if one does not exist.
+ *
+ * This checks the procedure map if it contains models, and the legacy procedure
+ * blocks otherwise.
+ *
+ * @param oldName The old name of the variable.
+ * @param newName The proposed name of the variable.
+ * @param workspace The workspace to search for conflicting parameters.
+ * @internal
+ */
+export function nameUsedWithConflictingParam(
+    oldName: string, newName: string, workspace: Workspace): string|null {
+  return workspace.getProcedureMap().getProcedures().length ?
+      checkForConflictingParamWithProcedureModels(oldName, newName, workspace) :
+      checkForConflictingParamWithLegacyProcedures(oldName, newName, workspace);
+}
+
+/**
+ * Returns the name of the procedure model with a conflicting param name, or
+ * null if one does not exist.
+ */
+function checkForConflictingParamWithProcedureModels(
+    oldName: string, newName: string, workspace: Workspace): string|null {
+  oldName = oldName.toLowerCase();
+  newName = newName.toLowerCase();
+
+  const procedures = workspace.getProcedureMap().getProcedures();
+  for (const procedure of procedures) {
+    const params = procedure.getParameters()
+                       .filter(isVariableBackedParameterModel)
+                       .map((param) => param.getVariableModel().name);
+    if (!params) continue;
+    const procHasOld = params.some((param) => param.toLowerCase() === oldName);
+    const procHasNew = params.some((param) => param.toLowerCase() === newName);
+    if (procHasOld && procHasNew) return procedure.getName();
+  }
+  return null;
+}
+
+/**
+ * Returns the name of the procedure block with a conflicting param name, or
+ * null if one does not exist.
+ */
+function checkForConflictingParamWithLegacyProcedures(
+    oldName: string, newName: string, workspace: Workspace): string|null {
+  oldName = oldName.toLowerCase();
+  newName = newName.toLowerCase();
+
+  const blocks = workspace.getAllBlocks(false);
+  for (const block of blocks) {
+    if (!isLegacyProcedureDefBlock(block)) continue;
+    const def = block.getProcedureDef();
+    const params = def[1];
+    const blockHasOld = params.some((param) => param.toLowerCase() === oldName);
+    const blockHasNew = params.some((param) => param.toLowerCase() === newName);
+    if (blockHasOld && blockHasNew) return def[0];
+  }
+  return null;
+}
+
 /**
  * Generate DOM objects representing a variable field.
  *

msg/messages.js

@@ -166,6 +166,9 @@ Blockly.Msg.VARIABLE_ALREADY_EXISTS = 'A variable named "%1" already exists.';
 /** @type {string} */
 /// alert - Tells the user that the name they entered is already in use for another type.
 Blockly.Msg.VARIABLE_ALREADY_EXISTS_FOR_ANOTHER_TYPE = 'A variable named "%1" already exists for another type: "%2".';
+/** @type {string} */
+/// alert - Tells the user that the name they entered is already in use as a parameter to a procedure, that the variable they are renaming also exists on. Renaming would create two parameters with the same name, which is not allowed.
+Blockly.Msg.VARIABLE_ALREADY_EXISTS_FOR_A_PARAMETER = 'A variable named "%1" already exists as a parameter in the procedure "%2".';
 
 // Variable deletion.
 /** @type {string} */

tests/mocha/blocks/variables_test.js

@@ -7,6 +7,8 @@
 goog.declareModuleId('Blockly.test.variables');
 
 import {sharedTestSetup, sharedTestTeardown} from '../test_helpers/setup_teardown.js';
+import {nameUsedWithConflictingParam} from '../../../build/src/core/variables.js';
+import {MockParameterModelWithVar, MockProcedureModel} from '../test_helpers/procedures.js';
 
 
 suite('Variables', function() {
@@ -139,4 +141,100 @@ suite('Variables', function() {
       chai.assert.equal(var3, result3);
     });
   });
+
+  suite('renaming variables creating conflicts', function() {
+    suite('renaming variables creating parameter conflicts', function() {
+      test(
+          'conflicts within legacy procedure blocks return the procedure name',
+          function() {
+            Blockly.serialization.blocks.append({
+              'type': 'procedures_defnoreturn',
+              'extraState': {
+                'params': [
+                  {
+                    'name': 'x',
+                    'id': '6l3P%Y!9EgA(Nh{E`Tl,',
+                  },
+                  {
+                    'name': 'y',
+                    'id': 'l1EtlJe%z_M[O-@uPAQ8',
+                  },
+                ],
+              },
+              'fields': {
+                'NAME': 'test name',
+              },
+            }, this.workspace);
+
+            chai.assert.equal(
+                'test name',
+                nameUsedWithConflictingParam('x', 'y', this.workspace),
+                'Expected the name of the procedure with the conflicting ' +
+                'param to be returned');
+          });
+
+      test(
+          'if no legacy block has the old var name, no procedure ' +
+          'name is returned',
+          function() {
+            Blockly.serialization.blocks.append({
+              'type': 'procedures_defnoreturn',
+              'extraState': {
+                'params': [
+                  {
+                    'name': 'definitely not x',
+                    'id': '6l3P%Y!9EgA(Nh{E`Tl,',
+                  },
+                  {
+                    'name': 'y',
+                    'id': 'l1EtlJe%z_M[O-@uPAQ8',
+                  },
+                ],
+              },
+              'fields': {
+                'NAME': 'test name',
+              },
+            }, this.workspace);
+
+            chai.assert.isNull(
+                nameUsedWithConflictingParam('x', 'y', this.workspace),
+                'Expected there to be no conflict');
+          });
+
+      test(
+          'conflicts within procedure models return the procedure name',
+          function() {
+            this.workspace.getProcedureMap().add(
+              new MockProcedureModel('test name')
+                .insertParameter(
+                    new MockParameterModelWithVar('x', this.workspace), 0)
+                .insertParameter(
+                    new MockParameterModelWithVar('y', this.workspace), 0));
+
+            chai.assert.equal(
+                'test name',
+                nameUsedWithConflictingParam('x', 'y', this.workspace),
+                'Expected the name of the procedure with the conflicting ' +
+                'param to be returned');
+          });
+
+      test(
+          'if no procedure model has the old var, no procedure ' +
+          'name is returned',
+          function() {
+            this.workspace.getProcedureMap().add(
+              new MockProcedureModel('test name')
+                .insertParameter(
+                    new MockParameterModelWithVar(
+                        'definitely not x', this.workspace),
+                    0)
+                .insertParameter(
+                    new MockParameterModelWithVar('y', this.workspace), 0));
+
+            chai.assert.isNull(
+                nameUsedWithConflictingParam('x', 'y', this.workspace),
+                'Expected there to be no conflict');
+          });
+    });
+  });
 });

tests/mocha/test_helpers/procedures.js

@@ -6,6 +6,7 @@
 goog.declareModuleId('Blockly.test.helpers.procedures');
 
 import {ConnectionType} from '../../../build/src/core/connection_type.js';
+import {VariableModel} from '../../../build/src/core/variable_model.js';
 
 
 /**
@@ -150,9 +151,9 @@ export function createProcCallBlock(
 }
 
 export class MockProcedureModel {
-  constructor() {
+  constructor(name = '') {
     this.id = Blockly.utils.idGenerator.genUid();
-    this.name = '';
+    this.name = name;
     this.parameters = [];
     this.returnTypes = null;
     this.enabled = true;
@@ -241,3 +242,14 @@ export class MockParameterModel {
     return this.id;
   }
 }
+
+export class MockParameterModelWithVar extends MockParameterModel {
+  constructor(name, workspace) {
+    super(name);
+    this.variable = new VariableModel(workspace, name);
+  }
+
+  getVariableModel() {
+    return this.variable;
+  }
+}