diff --git a/.eslintrc.js b/.eslintrc.js index e92f3d8c6..1bd12ca8b 100644 --- a/.eslintrc.js +++ b/.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'], }, }; } diff --git a/core/blockly.ts b/core/blockly.ts index 98399c766..d8a030893 100644 --- a/core/blockly.ts +++ b/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}; diff --git a/core/interfaces/i_legacy_procedure_blocks.ts b/core/interfaces/i_legacy_procedure_blocks.ts new file mode 100644 index 000000000..410048e4e --- /dev/null +++ b/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; +} diff --git a/core/interfaces/i_variable_backed_parameter_model.ts b/core/interfaces/i_variable_backed_parameter_model.ts new file mode 100644 index 000000000..39530b81e --- /dev/null +++ b/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; +} diff --git a/core/procedures.ts b/core/procedures.ts index 42c948e21..3dfe87601 100644 --- a/core/procedures.ts +++ b/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, }; diff --git a/core/variables.ts b/core/variables.ts index c62735515..99a2d7793 100644 --- a/core/variables.ts +++ b/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. * diff --git a/msg/messages.js b/msg/messages.js index 37311dfa5..e8ffe4aff 100644 --- a/msg/messages.js +++ b/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} */ diff --git a/tests/mocha/blocks/variables_test.js b/tests/mocha/blocks/variables_test.js index 7de2b7368..f64275509 100644 --- a/tests/mocha/blocks/variables_test.js +++ b/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'); + }); + }); + }); }); diff --git a/tests/mocha/test_helpers/procedures.js b/tests/mocha/test_helpers/procedures.js index dea6dc5fb..8fb40dcde 100644 --- a/tests/mocha/test_helpers/procedures.js +++ b/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; + } +}