Improved field validators (#338)

* Extracted code from mrc_class_method_def functions findLegalMethodName and isMethodNameUsed to make function makeLegalName in validator.ts.

Updated mrc_class_method_def, mrc_component, mrc_event, and mrc_mechanism blocks to use makeLegalName.

* Changed makeLegalName optionally return a valid python identifier.

* Use makeLegalName to make names unique when mechanisms, components, and events are dragged into the holder.

Use makeLegalName to validate parameter names.

* Update comment about case insensitive

Author: lizlooney

src/blocks/mrc_class_method_def.ts

@@ -35,6 +35,7 @@ import * as toolboxItems from '../toolbox/items';
 import { getClassData } from './utils/python';
 import { FunctionData } from './utils/python_json_types';
 import { findConnectedBlocksOfType } from './utils/find_connected_blocks';
+import { makeLegalName } from './utils/validator';
 import { NONCOPYABLE_BLOCK } from './noncopyable_block';
 import { BLOCK_NAME as MRC_GET_PARAMETER_BLOCK_NAME } from './mrc_get_parameter';
 import * as paramContainer from './mrc_param_container'
@@ -293,10 +294,23 @@ const CLASS_METHOD_DEF = {
     // When the user changes the method name on the block, clear the mrcPythonMethodName field.
     this.mrcPythonMethodName = '';
 
-    // Strip leading and trailing whitespace.
-    name = name.trim();
+    if (this.isInFlyout) {
+      // Flyouts can have multiple methods with identical names.
+      return name;
+    }
+
+    const otherNames: string[] = [];
+    this.workspace.getBlocksByType(BLOCK_NAME)
+        .filter(block => block.id !== this.id)
+        .forEach((block) => {
+          otherNames.push(block.getFieldValue(FIELD_METHOD_NAME));
+          const classMethodDefBlock = block as ClassMethodDefBlock;
+          if (classMethodDefBlock.mrcPythonMethodName) {
+            otherNames.push(classMethodDefBlock.mrcPythonMethodName);
+          }
+        });
 
-    const legalName = findLegalMethodName(name, this);
+    const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true);
     const oldName = nameField.getValue();
     if (oldName && oldName !== name && oldName !== legalName) {
       // Rename any callers.
@@ -374,61 +388,6 @@ const CLASS_METHOD_DEF = {
   },
 };
 
-/**
- * Ensure two identically-named methods don't exist.
- * Take the proposed method name, and return a legal name i.e. one that
- * is not empty and doesn't collide with other methods.
- *
- * @param name Proposed method name.
- * @param block Block to disambiguate.
- * @returns Non-colliding name.
- */
-function findLegalMethodName(name: string, block: ClassMethodDefBlock): string {
-  if (block.isInFlyout) {
-    // Flyouts can have multiple methods called 'my_method'.
-    return name;
-  }
-  name = name || 'unnamed';
-  while (isMethodNameUsed(name, block.workspace, block)) {
-    // Collision with another method.
-    const r = name.match(/^(.*?)(\d+)$/);
-    if (!r) {
-      name += '2';
-    } else {
-      name = r[1] + (parseInt(r[2]) + 1);
-    }
-  }
-  return name;
-}
-
-/**
- * Return if the given name is already a method name.
- *
- * @param name The questionable name.
- * @param workspace The workspace to scan for collisions.
- * @param opt_exclude Optional block to exclude from comparisons (one doesn't
- *     want to collide with oneself).
- * @returns True if the name is used, otherwise return false.
- */
-function isMethodNameUsed(
-  name: string, workspace: Blockly.Workspace, opt_exclude?: Blockly.Block): boolean {
-  const nameLowerCase = name.toLowerCase();
-  for (const block of workspace.getBlocksByType(BLOCK_NAME)) {
-    if (block === opt_exclude) {
-      continue;
-    }
-    if (nameLowerCase === block.getFieldValue(FIELD_METHOD_NAME).toLowerCase()) {
-      return true;
-    }
-    const classMethodDefBlock = block as ClassMethodDefBlock;
-    if (classMethodDefBlock.mrcPythonMethodName &&
-        nameLowerCase === classMethodDefBlock.mrcPythonMethodName.toLowerCase()) {
-      return true;
-    }
-  }
-  return false;
-}
-
 export const setup = function () {
   Blockly.Blocks[BLOCK_NAME] = CLASS_METHOD_DEF;
 };

src/blocks/mrc_component.ts

@@ -28,6 +28,7 @@ import { Editor } from '../editor/editor';
 import { ExtendedPythonGenerator } from '../editor/extended_python_generator';
 import { getModuleTypeForWorkspace } from './utils/workspaces';
 import { getAllowedTypesForSetCheck, getClassData, getSubclassNames } from './utils/python';
+import { makeLegalName } from './utils/validator';
 import * as toolboxItems from '../toolbox/items';
 import * as storageModule from '../storage/module';
 import * as storageModuleContent from '../storage/module_content';
@@ -164,10 +165,19 @@ const COMPONENT = {
     }
   },
   mrcNameFieldValidator(this: ComponentBlock, nameField: Blockly.FieldTextInput, name: string): string {
-    // Strip leading and trailing whitespace.
-    name = name.trim();
+    if (this.isInFlyout) {
+      // Flyouts can have multiple methods with identical names.
+      return name;
+    }
+
+    const otherNames: string[] = [];
+    this.workspace.getBlocksByType(BLOCK_NAME)
+        .filter(block => block.id !== this.id)
+        .forEach((block) => {
+          otherNames.push(block.getFieldValue(FIELD_NAME));
+        });
 
-    const legalName = name;
+    const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true);
     const oldName = nameField.getValue();
     if (oldName && oldName !== name && oldName !== legalName) {
       // Rename any callers.

src/blocks/mrc_event.ts

@@ -23,6 +23,7 @@ import * as Blockly from 'blockly';
 
 import { MRC_STYLE_EVENTS } from '../themes/styles'
 import { createFieldNonEditableText } from '../fields/FieldNonEditableText';
+import { makeLegalName } from './utils/validator';
 import { Parameter } from './mrc_class_method_def';
 import { Editor } from '../editor/editor';
 import { ExtendedPythonGenerator } from '../editor/extended_python_generator';
@@ -187,10 +188,19 @@ const EVENT = {
     });
   },
   mrcNameFieldValidator(this: EventBlock, nameField: Blockly.FieldTextInput, name: string): string {
-    // Strip leading and trailing whitespace.
-    name = name.trim();
+    if (this.isInFlyout) {
+      // Flyouts can have multiple methods with identical names.
+      return name;
+    }
+
+    const otherNames: string[] = [];
+    this.workspace.getBlocksByType(BLOCK_NAME)
+        .filter(block => block.id !== this.id)
+        .forEach((block) => {
+          otherNames.push(block.getFieldValue(FIELD_EVENT_NAME));
+        });
 
-    const legalName = name;
+    const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ false);
     const oldName = nameField.getValue();
     if (oldName && oldName !== name && oldName !== legalName) {
       // Rename any callers.

src/blocks/mrc_mechanism.ts

@@ -27,6 +27,7 @@ import { createFieldNonEditableText } from '../fields/FieldNonEditableText';
 import { Editor } from '../editor/editor';
 import { ExtendedPythonGenerator } from '../editor/extended_python_generator';
 import { getAllowedTypesForSetCheck } from './utils/python';
+import { makeLegalName } from './utils/validator';
 import * as toolboxItems from '../toolbox/items';
 import * as storageModule from '../storage/module';
 import * as storageModuleContent from '../storage/module_content';
@@ -167,7 +168,19 @@ const MECHANISM = {
     // Strip leading and trailing whitespace.
     name = name.trim();
 
-    const legalName = name;
+    if (this.isInFlyout) {
+      // Flyouts can have multiple methods with identical names.
+      return name;
+    }
+
+    const otherNames: string[] = [];
+    this.workspace.getBlocksByType(BLOCK_NAME)
+        .filter(block => block.id !== this.id)
+        .forEach((block) => {
+          otherNames.push(block.getFieldValue(FIELD_NAME));
+        });
+
+    const legalName = makeLegalName(name, otherNames, /* mustBeValidPythonIdentifier */ true);
     const oldName = nameField.getValue();
     if (oldName && oldName !== name && oldName !== legalName) {
       // Rename any callers.

src/blocks/mrc_mechanism_component_holder.ts

@@ -22,7 +22,7 @@
 import * as Blockly from 'blockly';
 
 import { MRC_STYLE_MECHANISMS } from '../themes/styles';
-import { getLegalName } from './utils/python';
+import { makeLegalName } from './utils/validator';
 import { Editor } from '../editor/editor';
 import { ExtendedPythonGenerator } from '../editor/extended_python_generator';
 import * as storageModule from '../storage/module';
@@ -220,14 +220,16 @@ const MECHANISM_COMPONENT_HOLDER = {
    */
   setNameOfChildBlock(this: MechanismComponentHolderBlock, child: Blockly.Block): void {
     const otherNames: string[] = []
-    const descendants = this.getDescendants(true);
-    descendants
+    this.getDescendants(true)
         .filter(descendant => descendant.id !== child.id)
         .forEach(descendant => {
           otherNames.push(descendant.getFieldValue('NAME'));
         });
     const currentName = child.getFieldValue('NAME');
-    const legalName = getLegalName(currentName, otherNames);
+    // mechanism and component names must be valid python identifiers.
+    // event names do not need to be valid python identifiers.
+    const mustBeValidPythonIdentifier = (child.type === MRC_MECHANISM_NAME || child.type === MRC_COMPONENT_NAME);
+    const legalName = makeLegalName(currentName, otherNames, mustBeValidPythonIdentifier);
     if (legalName !== currentName) {
       child.setFieldValue(legalName, 'NAME');
     }

src/blocks/mrc_param_container.ts

@@ -21,7 +21,7 @@
  */
 import * as Blockly from 'blockly';
 import { MRC_STYLE_CLASS_BLOCKS } from '../themes/styles';
-import { getLegalName } from './utils/python';
+import { makeLegalName } from './utils/validator';
 
 export const PARAM_CONTAINER_BLOCK_NAME = 'mrc_param_container';
 const PARAM_ITEM_BLOCK_NAME = 'mrc_param_item';
@@ -85,13 +85,16 @@ const PARAM_ITEM = {
     const rootBlock: Blockly.Block | null = this.getRootBlock();
     if (rootBlock) {
       const otherNames: string[] = []
-      rootBlock!.getDescendants(true)?.forEach(itemBlock => {
-        if (itemBlock != this) {
-          otherNames.push(itemBlock.getFieldValue(FIELD_NAME));
-        }
-      });
+      rootBlock.getDescendants(true)
+          .filter(descendant => descendant.type === PARAM_ITEM_BLOCK_NAME && descendant !== this)
+          .forEach(itemBlock => {
+            if (itemBlock != this) {
+              otherNames.push(itemBlock.getFieldValue(FIELD_NAME));
+            }
+          });
       const currentName = this.getFieldValue(FIELD_NAME);
-      this.setFieldValue(getLegalName(currentName, otherNames), FIELD_NAME);
+      const legalName = makeLegalName(currentName, otherNames, /* mustBeValidPythonIdentifier */ true);
+      this.setFieldValue(legalName, FIELD_NAME);
       updateMutatorFlyout(this.workspace);
     }
   },

src/blocks/utils/python.ts

@@ -281,30 +281,6 @@ export function getOutputCheck(type: string): string {
   return type;
 }
 
-// Function to return a legal name based off of proposed names and making sure it doesn't conflict
-// This is a legal name for python methods and variables. 
-export function getLegalName(proposedName: string, existingNames: string[]){
-  let newName = proposedName.trim().replace(' ', '_');
-
-  // TODO: Allow the user to put numbers in the name.
-  
-  if (!/^[A-Za-z_]/.test(newName)){
-      newName = "_" + newName;
-  }
-      
-  while (existingNames.includes(newName)){
-      const match = /(.*?)(\d+)$/.exec(newName)
-
-      if (match) {
-          let lastNumber  = +match[2]
-          newName = match[1] + (lastNumber + 1)
-      } else {
-          newName += "2"
-      }
-  }
-  return newName;
-}
-
 export function isExistingPythonModule(moduleName: string): boolean {
   for (const pythonData of allPythonData) {
     // Process modules.

src/blocks/utils/validator.ts

@@ -0,0 +1,72 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @author [email protected] (Liz Looney)
+ */
+
+/**
+ * Take the proposed name, and return a legal name.
+ * A legal name is:
+ * 1. not empty.
+ * 2. doesn't collide with other names (case insensitive).
+ * 3. Optional: is a valid python identifier.
+ */
+export function makeLegalName(proposedName: string, otherNames: string[], mustBeValidPythonIdentifier: boolean): string {
+  const otherNamesLowerCase = otherNames.map(n => n.toLowerCase());
+
+  // Strip leading and trailing whitespace.
+  let name = proposedName.trim();
+
+  // Make the name non-empty.
+  name = name || 'unnamed';
+
+  if (mustBeValidPythonIdentifier) {
+    if (!name.match(/^[a-zA-Z_][a-zA-Z0-9_]*$/)) {
+      let identifier = '';
+      const firstChar = name[0];
+      if (/[a-zA-Z_]/.test(firstChar)) {
+        identifier += firstChar;
+      } else if (/[a-zA-Z0-9_]/.test(firstChar)) {
+        // If the first character would be valid as the second charactor, insert an underscore.
+        identifier += '_' + firstChar;
+      } else {
+        identifier += '_';
+      }
+      for (let i = 1; i < name.length; i++) {
+        const char = name[i];
+        if (/[a-zA-Z0-9_]/.test(char)) {
+          identifier += char;
+        } else {
+          identifier += '_';
+        }
+      }
+      name = identifier;
+    }
+  }
+
+  while (otherNamesLowerCase.includes(name.toLowerCase())) {
+    // Collision with another name.
+    const r = name.match(/^(.*?)(\d+)$/);
+    if (!r) {
+      name += '2';
+    } else {
+      name = r[1] + (parseInt(r[2]) + 1);
+    }
+  }
+  return name;
+}