Feature request: An exception-safe undo API

[ExpHP]

It feels like the begin_undo_actions/commit_undo_actions API is very difficult to use correctly, which is troubling because messing with Undo history is honestly already pretty scary business! As a typical example of how things can go wrong: Suppose a function calls begin_undo_actions(), then raises an Exception before it calls commit_undo_actions(). Then anything done after this could end up recorded as part of the same Undo action.

I think the ideal API for something like this would be a context manager. Ideally, it would also have a way to automatically cancel changes made if the with block is exited via an uncaught exception. Something like:

with bv.begin_undo_actions(auto_cancel=True) as transaction:
    # Modify the BinaryView in here.
    # commit_undo_actions() occurs automatically on leaving the 'with' block.
    # One can call `transaction.cancel()` to undo all changes in the 'with' block.

    # auto_cancel=True means changes inside the 'with' block are automatically
    #  canceled if it is exited through an uncaught exception.  When False, all
    #  changes prior to the exception will simply be committed as a single
    #  undoable action.

(Bikeshed: if auto_cancel=True sounds like a reasonable default, maybe the option should instead be keep_on_error which defaults to False?)

I tried to write one myself as a wrapper around the existing API, and it was pretty tricky. I had to simulate "canceling" by committing and then undoing, but this is not safe to do if no changes have been made yet (it would commit nothing and undo the previous change instead!). It ended up looking like this:

import contextlib

_RECORDING_UNDO = False

@contextlib.contextmanager
def recording_undo(bv):
    """ Context manager for ``bv.begin_undo_actions()``.  The contents of the ``with`` block
    will become a single undo-able action. (assuming at least one change was made inside)

    Changes made inside the ``with`` block can be rolled back on uncaught exceptions. However,
    for this to occur, you must call ``.enable_auto_rollback()`` on the returned object at least
    once after performing at least one successful modification to the ``BinaryView``.

    This context manager is not reentrant.  Do not use it recursively, or from multiple threads.
    (obviously, you also should not use BinaryNinja's own undo API while using it!)

    >>> from binaryninja import Symbol, SymbolType
    >>> def rename_type_in_funcs(bv, old, new):
    ...     old_prefix = f'{old}::'
    ...     new_prefix = f'{new}::'
    ...     with recording_undo(bv) as rec:
    ...         for func in bv.functions:
    ...             if func.name.startswith(old_prefix):
    ...                 suffix = func.name[len(old_prefix):]
    ...                 new_name = new_prefix + suffix
    ...                 bv.define_user_symbol(Symbol(SymbolType.FunctionSymbol, func.start, new_name))
    ...                 rec.enable_auto_rollback()
    """
    global _RECORDING_UNDO

    if _RECORDING_UNDO:
        raise RuntimeError(f'Attempted to use `recording_undo` recursively!')

    class UndoRecorder:
        def __init__(self): self.active = False
        def enable_auto_rollback(self): self.active = True

    rec = UndoRecorder()

    _RECORDING_UNDO = True
    bv.begin_undo_actions()
    try:
        # execute the 'with' block
        yield rec
    except:
        # If at least one action was performed, 'cancel' it by committing and undoing.
        # Even if no actions were performed, make BN stop recording by committing.
        bv.commit_undo_actions()
        if rec.active:
            bv.undo()
        raise
    finally:
        _RECORDING_UNDO = False

    bv.commit_undo_actions()

[CouleeApps]

Updates to the undo api are being planned that will make this possible to implement. Specifically:

• bv.begin_undo_actions() will return an identifier for a newly created undo state which...
• bv.commit_undo_actions() will now take the undo state identifier and commit all actions since the identifier was created
• bv.revert_undo_actions() will be created taking an undo state identifier to revert all actions since the identifier was created
• Undo states can be nested
• Undo states can be reverted without affecting actions done prior to calling bv.begin_undo_actions()

With these changes, context manager support should be as simple as

@contextlib.contextmanager
def recording_undo(bv):
    state = bv.begin_undo_actions()
    try:
        yield
        bv.commit_undo_actions(state)
    except:
        bv.revert_undo_actions(state)
        raise

[CouleeApps]

This has been implemented as of version 3.5.4321-dev. See: BinaryView.undoable_transaction()