Clearing the name of a procedure parameter (even temporarily) breaks everything

[rachel-fenichel]

Problem statement

Trying to rename x to another name by deleting x and then starting to type causes major problems.

This is because there's an intermediate state where it tries to create a variable with the empty name. Then calls to allUsedVariables and fail, as does mutateCallers.

If the variable name is empty we should not attempt to create a variable with that name. (Do we just keep the old name around instead, and revert to it if they end editing there?)

Stack Traces

procedures.js:133 Uncaught TypeError: Cannot read property 'name' of null
    at Blockly.BlockSvg.mutationToDom (procedures.js:133)
    at Object.Blockly.Procedures.mutateCallers (procedures.js:281)
    at Blockly.BlockSvg.compose (procedures.js:225)
    at Blockly.Mutator.workspaceChanged_ (mutator.js:339)
    at Blockly.WorkspaceSvg.Blockly.Workspace.fireChangeListener (workspace.js:436)
    at Blockly.Events.fireNow_ (events.js:160)```

[picklesrus]

Turns out this is likely what happened here. #1815
Glad we have repro steps now!

[rachel-fenichel]

Andrew and I dug into this for a while today. Part of the problem is that the validator is returning null when the variable name is empty, but setValue is still being called on the field. That counts a workspace change, and that fires compose, and so on.

Updating FieldTextInput's setValue function to actually respect the null result from the validator does fix this problem, but it might break other stuff. We need to dig more before applying this patch.

The fix (that may cause other problems) is to replace the current setValue function with:

/**
 * Set the value of this field.
 * @param {?string} newValue New value.
 * @override
 */
Blockly.FieldTextInput.prototype.setValue = function(newValue) {
  if (newValue !== null) { // No change if null.
    if (this.sourceBlock_) {
      var validated = this.callValidator(newValue);
      // If the new value is invalid, validation returns null.
      if (validated === null) {
        return;
      }
      // If the validator returned undefined, it didn't have an
      // opinion.  If it cared, use the new value.
      if (typeof validated != 'undefined') {
        newValue = validated;
      }
    }
    Blockly.Field.prototype.setValue.call(this, newValue);
  }
};