Skip to content

Illegal program state occurs due to belated enforcement by onchange methods #8467

Open

Description

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions