Renaming vars so that you get duplicate procedure vars breaks mutator (sometimes)

[BeksOmega]

Describe the bug

If you rename a variable via a variable block, it can cause duplicate variables in a procedure.

To Reproduce

Steps to reproduce the behavior:

1. Create a mutator with two variables.
2. Grab a variable block for one of the variables.
3. Rename the variable to match the other variable in the mutator.
4. Observe how there are duplicate vars in the procedure. (bad)

The next bit does not always happen, depending on which var you rename:

1. Open the mutator.
2. Observe how you have two vars.
3. Close the mutator and reopen.
4. Observe how you get errors in the console.

Expected behavior

There are two parts:

1. Duplicate vars: No clue how this should be handled.
2. Errors: Errors are bad, definitely do not want errors.

Screenshots

Errors:

No Errors:

Desktop (please complete the following information):

• OS: Ubunto 18.04
• Browser Chrome
• Version 80

Stack Traces

procedures.js:110 Uncaught TypeError: Cannot read property 'name' of null
    at Blockly.BlockSvg.mutationToDom (procedures.js:110)
    at Object.Blockly.Procedures.mutateCallers (procedures.js:374)
    at Blockly.BlockSvg.compose (procedures.js:227)
    at Blockly.Mutator.workspaceChanged_ (mutator.js:377)
    at Blockly.WorkspaceSvg.Blockly.Workspace.fireChangeListener (workspace.js:657)
    at Blockly.Events.fireNow_ (events.js:192)
mutationToDom @ procedures.js:110
Blockly.Procedures.mutateCallers @ procedures.js:374
compose @ procedures.js:227
Blockly.Mutator.workspaceChanged_ @ mutator.js:377
Blockly.Workspace.fireChangeListener @ workspace.js:657
Blockly.Events.fireNow_ @ events.js:192
setTimeout (async)
Blockly.Events.fire @ events.js:174
Blockly.Mutator.setVisible @ mutator.js:274
Blockly.Icon.iconClick_ @ icon.js:124
Blockly.Mutator.iconClick_ @ mutator.js:124
wrapFunc @ blockly.js:462
variables.js:51 Uncaught TypeError: Cannot read property 'getId' of null
    at Object.Blockly.Variables.allUsedVarModels (variables.js:51)
    at Object.Blockly.Xml.workspaceToDom (xml.js:38)
    at exportWorkspace (my_playground.html:438)
    at autoUpdater (my_playground.html:561)
    at Blockly.WorkspaceSvg.Blockly.Workspace.fireChangeListener (workspace.js:657)
    at Blockly.Events.fireNow_ (events.js:192)
Blockly.Variables.allUsedVarModels @ variables.js:51
Blockly.Xml.workspaceToDom @ xml.js:38
exportWorkspace @ my_playground.html:438
autoUpdater @ my_playground.html:561
Blockly.Workspace.fireChangeListener @ workspace.js:657
Blockly.Events.fireNow_ @ events.js:192
setTimeout (async)
Blockly.Events.fire @ events.js:174
Blockly.Mutator.setVisible @ mutator.js:274
Blockly.Icon.iconClick_ @ icon.js:124
Blockly.Mutator.iconClick_ @ mutator.js:124
wrapFunc @ blockly.js:462
procedures.js:110 Uncaught TypeError: Cannot read property 'name' of null
    at Blockly.BlockSvg.mutationToDom (procedures.js:110)
    at Object.Blockly.Procedures.mutateCallers (procedures.js:374)
    at Blockly.BlockSvg.decompose (procedures.js:202)
    at Blockly.Mutator.setVisible (mutator.js:292)
    at Blockly.Mutator.Blockly.Icon.iconClick_ (icon.js:124)
    at Blockly.Mutator.iconClick_ (mutator.js:124)
    at SVGGElement.wrapFunc (blockly.js:462)
mutationToDom @ procedures.js:110
Blockly.Procedures.mutateCallers @ procedures.js:374
decompose @ procedures.js:202
Blockly.Mutator.setVisible @ mutator.js:292
Blockly.Icon.iconClick_ @ icon.js:124
Blockly.Mutator.iconClick_ @ mutator.js:124
wrapFunc @ blockly.js:462
variables.js:51 Uncaught TypeError: Cannot read property 'getId' of null
    at Object.Blockly.Variables.allUsedVarModels (variables.js:51)
    at Object.Blockly.Xml.workspaceToDom (xml.js:38)
    at exportWorkspace (my_playground.html:438)
    at autoUpdater (my_playground.html:561)
    at Blockly.WorkspaceSvg.Blockly.Workspace.fireChangeListener (workspace.js:657)
    at Blockly.Events.fireNow_ (events.js:192)

Additional context

N/A

[seldomU]

Shouldn't the variable renaming be prohibited due to the name collision? See also #3877

[BeksOmega]

Turns out that with the addition of the backing procedure models, fixing this becomes much trickier.

Here are several failed ideas:

• We could look through all of the procedure definition blocks in the workspace, and check that renaming the given variable would not create parameter conflicts.
  This does not work with shareable procedures, because a problematic procedure definition block could exist in another workspace, which we do not have access to.
• We could look through all of the procedure definition blocks in the workspace + check the procedure map.
  This does not work because IParameterModel does not assume that you're using a backing variable. So it's actually impossible to check against the procedure map.
• We could look through all of the procedure definition blocks in the workspace + install our own renaming logic in the shareable-procedures plugin to look at the ObservableParameterModels.
  This does not work because the renaming function that's called is hard-coded into the variable field.

That leaves us with two viable options:

1. We make getVariableModel an optional method on IParameterModel (or define a separate interface that contains getVariableModel) and we go through the procedure map checking that.
2. We add a way to register validation functions to the variable create / rename logic.

Registering validation functions supports more theoretical use cases (e.g. if someone wanted to use variables to represent methods on a class, and they wanted to make sure each method name was unique). But since the use cases are theoretical, we don't have enough information about them to make sure we're implementing the validation functions in a way that's going to best serve them.

I'm currently leaning toward solution adding the getVariableModel interface, since it's relatively low lift, and shouldn't constrain us from implementing additional validation in the future. @maribethb opinions?

[maribethb]

Let's do option 1; as you noted, we can add additional validation if/when people ask for it in the future.

[cpcallen]

Testing procedure blocks for the 2023Q2 release, it appears that the current behaviour is that if you rename a variable in a way that would create a duplicate parameter, the variable name change fails silently, giving no feedback to the user beyond the field briefly turning red, and leaving the workspace in the same state as it was before the attempt. Is that the expected outcome?

With respect to the original bug and other ways that this could have been fixed, my (naïve advanced programmer) expectations would be:

• Renaming a variable using a block outside a procedure would not rename any procedure parameters.
• Renaming a variable attached to a procedure definition and matching the name of one of the parameters would rename the corresponding parameter, unless it created a duplicate name in which case either it would generate an error message or it would be impossible to "finish" the renaming (i.e., the field would stay red and continue to have focus even when I press enter).

…but this is obviously an extension of my expectation that procedure parameters are lexical, to the point that I have a hard time forming a good mental model of what they actually are in Blockly.

(I realise the unable-to-finish-entry-of-invalid-data behaviour is contrary to how most fields work at the present time, so probably not actually appropriate in the context of established user expectations.)

[cpcallen]

This issue is not fixed in develop. In particular, renaming a variable that is also a parameter to conflict with another parameter of the same function fails silently with the following stack trace:

variables.ts:333 Uncaught TypeError: Cannot read properties of undefined (reading 'replace')
    at variables.ts:333:12
    at variables.ts:367:5
    at promptImplementation (dialog.ts:32:3)
    at Module.prompt (dialog.ts:101:3)
    at promptName (variables.ts:357:10)
    at promptAndCheckWithAlert (variables.ts:306:5)
    at Module.renameVariable (variables.ts:341:3)
    at FieldVariable.onItemSelected_ (field_variable.ts:502:19)
    at FieldVariable.handleMenuActionEvent (field_dropdown.ts:368:10)
    at MenuItem.performAction (menuitem.ts:237:12)