Illegal program state occurs due to belated enforcement by `onchange` methods

[cpcallen]

Invalid program state

One of Blockly's principles is that any connected group of blocks is a valid program. Not necessarily a correct program, but one which could compiled to syntactically valid code in a supported language and then run. This is leveraged by a number of Blockly projects, which generate and run code on every workspace update.

In the process of investigating #7950, it came to light that when a procedures_ifreturn block is inserted into a procedures_defnoreturn this principle is momentarily violated, because the ifreturn block's VALUE input is only removed afterwards, by its onchange handler. A quick investigation of other onchange handlers suggested that many of them also contained code smells for invalid program state.

In addition to potential code-generation-correctness issues, the invalid state prevents undo from working correctly—in this case because undo attempts to reattach a return value to the ifreturn block before it removes the ifreturn block from the defnoreturn block.

Proposal 1: Reaffirm that Blockly does not permit invalid program state

We should reaffirm the principle that Blockly does not allow invalid program states, and clarify that this means even momentarily:

• It should not be possible to connect blocks if doing so would result in an invalid program.
• If there are situations where it is not reasonably practical to prevent a program from being temporarily invalid (perhaps due to changes-at-a-distance, e.g. modifying procedure definitions), Blockly should ensure that the program is made valid again no later than the end of the current microtask (and ideally before the method call that initiated the change returns)—before any events are fired that might cause theinvalid state to be observed.
• The program can be made valid by disabling, disconnecting or deleting blocks as needed (but ideally with due regard to positive user experience).

Proposal 2: Provide a synchronous mechanism for ensuring program validity invariants are maintained

This would replace the use of onchange handlers with some other, more specific and synchronous mechanism, akin to field validators. Ideally this mechanism should be:

• Limited in scope, so that only "affected" blocks are validated/notified of changes.
  • Or, where a wide scope is needed, efficient, so that the validator is only called once per change, not once per change * block.
• Only be called for changes that affect the program structure (i.e. connection/disconnection and possibly creation/mutation/deletion, but not click/drag/viewport change etc.).
• Able to:
  • Allow validator to reject a proposed connection (preventing shadow from appearing).
    • This could subsume the existing type-checking system.
  • Allow affected block(s) to update themselves (and other blocks) when a connection does occur, and later undo those changes when disconnected.
• Flexible enough to handle at least all the validation checks/fixes performed by existing onchange handlers.

(For LambdaMOO programmers: the check vs. update distinction is akin to :acceptable vs. :accept in LambdaCore: either can allow or reject a move, but only the latter should have any side effects.)

In the case of the procedures_ifreturn block, this would mean that the ifreturn block could remove its VALUE (return value) input (and thereby detach any child blocks from it) before its pending connected to a procedures_defnoreturn block is effected.

Scope

While it is desirable that this machinery call the minimum number of validator functions when considering / making a connection, it is not sufficient for only the immediately-involved blocks to be validated.

• Calling a validator on the two blocks being directly connected lets one implement type checking, or have binary operator blocks update the types of their other inputs/outputs.
• Calling a validator on all the blocks in the subtree being attached/detached lets blocks like controls_flow_statements (break/continue) enable or disable themselves depending on whether they are enclosed in a loop block or not, or procedures_ifreturn to add/remove its VALUE input.
• There may be use-cases for blocks to be able to "validate" (or at least synchronously listen to) all changes, as the onchange handler currently allows but in a more timely way—or even all proposed changes.

Proposal 3: Deprecate block onchange methods

We can and probably should continue to support the onchange mechanism indefinitely for backwards compatibility with developer's custom blocks, we should stop using onchange methods in the library blocks and we should discourage their use by developers.

• Almost all the existing onchange methods are used to enforce program validity invariants (see survey in separate comment below), but they can only ever do so belatedly. It seems very likely that any onchange methods not used to enforce program validity could also be converted to use a proposed synchronous validation mechanism instead (just as we currently use field validators for block mutation).
• Block onchange handlers are very powerful, because they can react to any change on the workspace, but also very inefficient, because a separate change listener is registered for every such block on the workspace; this means that a change listener is called for every such block on the workspace for every change, regardless of whether the change affects the block in question, and a workspace with N copies of a particular block will call the block definition's onchange method N times for every event, with only this differing between calls. If fully-general event listening is useful will in many cases be better for developers to register a single change listener at the workspace level that will handle updates for all relevant blocks.

See also:

• Logic Comparison block doesn't immediately update when its children change outputs #757
• Mutated procedures_ifreturn detaches child block, but undoing doesn't reattach it. #7950

[AbhinavKRN]

@cpcallen can you explain the issue briefly?

[cpcallen]

Survey of existing onchange methods

Here are the onchange methods in library blocks, noting which enforce validity invariants:

LOGIC_COMPARE_ONCHANGE_MIXIN

(logic_compare extension / logic_compare block)

• The onchange method is used to enforce validity by ensuring that the compared values are of the same type.
• This was originally introduced in Update logic block so it is reactive to the input attached to it. #53 (using an explicit addChangeListener() call instead of an onchange: method) using type checks to prevent an incompatible second block from being attached at all; this would have prevented the program from ever being in an invalid state.
• @NeilFraser proposed changing it to instead bump out the earlier-connected block when a second incompatible one was connected. This would have entailed allowing it to be temporarily in an invalid state.
• This was not done at the time, but after a bug was discovered involving shadow blocks and a discussion about possible solutions @AnmAtAnm rewrote the onchange method to bump out the later-connected block when a type-mismatch occurred, allowing an invalid configuration to exist temporarily.
• The current version of also contains code that will bump pre-existing invalid configurations (e.g. due to load/paste), drawing attention to the fact that a user connecting blocks one-at-a-time is not the only way that an invalid configuration might occur.

LOGIC_TERNARY_ONCHANGE_MIXIN

(logic_ternary extension / logic_ternary block)

The onchange method enforces validity by checking types of connected 'then' and 'else' blocks and bumping any which are invalid. Git history contains no indication that this was ever done by preemptively setting the types of the inputs.

CONTROL_FLOW_IN_LOOP_CHECK_MIXIN

(controls_flow_in_loop_check extension / controls_flow_statements block)

• The onchange method enforces validity by disabling the block (and thus preventing generator from emitting an illegal break or continue) if the block is not contained in a loop block.

PROCEDURE_CALL_COMMON mixin

(procedures_callreturn + procedures_callnoreturn blocks)

• The onchange method enforces validity by ensuring that there is always a procedure definition block for any procedure call blocks:
  • It will create a procedure definition block if none exists for a newly-created procedure call block (e.g. paste).
  • It will delete any orphaned procedure call blocks when the procedure definition is deleted.
  • It will disable procedure call blocks if the definition is disabled.

PROCEDURES_IFRETURN

(procedures_ifreturn block)

• The onchange method enforces validity by removing the VALUE input from the ifreturn block if it is enclosed in a procedures_defnoreturn, and restoring it if it enclosed in a procedures_defreturn.

CUSTOM_CONTEXT_MENU_VARIABLE_GETTER_SETTER_MIXIN

(contextMenu_variableDynamicSetterGetter extension / variables_get_dynamic + contextMenu_variableDynamicSetterGetter blocks)

• The onchange method enforces validity by setting the type check for the block's output connection or VALUE input (as appropriate) based on the variable model.
  • Not immediately clear how to replace this with a validation; might be more appropriate to have block subscribe to changes to the variable model.