Refactor input dispatch and handling layers

Author: istathar

src/engraving/checks/ledger.rs

@@ -285,3 +285,55 @@ fn revoking_an_unknown_serial_changes_nothing() {
             .is_some()
     );
 }
+
+#[test]
+fn an_orphaned_scope_does_not_adopt_what_opens_after_it() {
+    // A run stopped inside step 1 leaves its `Begin` unpaired. Step 2 opens
+    // after it and belongs to the procedure, not to the step the walk happened
+    // to die in — the outcome of the enclosing scope closes whatever it still
+    // held open.
+    let ledger = fold(vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Begin(Vec::new())),
+        record(3, "/task:/1/helper:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Done(None)),
+        record(4, "/task:/2", State::Begin(Vec::new())),
+    ]);
+
+    let entry = ledger
+        .look(Serial(1), "/task:/2")
+        .expect("step 2 keyed under its procedure");
+    assert_eq!(entry.serial, Serial(4));
+}
+
+#[test]
+fn a_resumed_scope_keeps_the_serial_it_was_entered_at() {
+    // A scope opened after an orphan is found again on the next walk, so
+    // resuming does not write a second `Begin` for it. It did, and each resume
+    // then mis-parented the next, ratcheting a duplicate spine onto the trail
+    // one pair of records at a time.
+    let ledger = fold(vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Begin(Vec::new())),
+        record(3, "/task:/1/helper:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Done(None)),
+        record(4, "/task:/2", State::Begin(Vec::new())),
+        record(0, "/", State::Stop),
+        // The second walk re-enters the procedure and finishes the step it
+        // finds standing, rather than opening a fresh one beside it.
+        record(0, "/", State::Resume),
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(4, "/task:/2", State::Done(None)),
+    ]);
+
+    assert_eq!(ledger.serial_for(Serial(1), "/task:/2"), Serial(4));
+    let entry = ledger
+        .look(Serial(1), "/task:/2")
+        .expect("step 2 still keyed under its procedure after the resume");
+    assert!(
+        entry
+            .outcome
+            .is_some(),
+        "the second walk's outcome lands on the entry the first walk opened"
+    );
+}

src/engraving/checks/navigation.rs

@@ -109,3 +109,133 @@ fn a_session_boundary_is_not_a_position() {
         Some(Position::Live)
     );
 }
+
+// The same call, dispatched again by a resumed session that was stopped at the
+// argument prompt before it reached the callee.
+fn redispatched() -> Vec<Record> {
+    vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Begin(Vec::new())),
+        record(
+            2,
+            "/task:/1",
+            State::Invoke(InvokeTarget::Procedure("check:".to_string())),
+        ),
+        record(0, "/", State::Stop),
+        record(0, "/", State::Resume),
+        record(
+            2,
+            "/task:/1",
+            State::Invoke(InvokeTarget::Procedure("check:".to_string())),
+        ),
+        record(
+            2,
+            "/task:/1",
+            State::Invoke(InvokeTarget::Procedure("verify:".to_string())),
+        ),
+    ]
+}
+
+#[test]
+fn a_redispatched_call_is_one_place_to_stand() {
+    // Two sessions reaching the same call wrote the dispatch line twice, and
+    // pressing Up walked both showing the same thing each time. The trail keeps
+    // them; the cursor stops on the last. A second call in the same step writes
+    // a different line at the same address, and stands on its own.
+    let records = redispatched();
+    let trail = Trail::new(&records);
+
+    let at = trail
+        .last()
+        .expect("a position to open on");
+    assert_eq!(at, Position::At(6));
+    assert_eq!(trail.step(at, Motion::Up), Some(Position::At(5)));
+    assert_eq!(
+        trail.step(Position::At(5), Motion::Up),
+        Some(Position::At(1))
+    );
+}
+
+#[test]
+fn two_invocations_of_one_procedure_both_stand() {
+    // A procedure invoked twice records both at its own path, so the path is
+    // not what tells two executions apart — the serial is. Collapsing by path
+    // would have swallowed the first call's whole subtree.
+    let records = vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Begin(Vec::new())),
+        record(3, "/task:/check:", State::Begin(Vec::new())),
+        record(3, "/task:/check:", State::Done(None)),
+        record(2, "/task:/1", State::Done(None)),
+        record(4, "/task:/2", State::Begin(Vec::new())),
+        record(5, "/task:/check:", State::Begin(Vec::new())),
+        record(5, "/task:/check:", State::Done(None)),
+        record(4, "/task:/2", State::Done(None)),
+    ];
+    let trail = Trail::new(&records);
+
+    let mut at = Position::At(8);
+    for expected in [7, 6, 5, 4, 3, 2, 1, 0] {
+        at = trail
+            .step(at, Motion::Up)
+            .expect("every record stands as its own position");
+        assert_eq!(at, Position::At(expected));
+    }
+}
+
+#[test]
+fn an_amended_answer_is_the_only_one_review_reaches() {
+    // Withdrawing an answer and giving a different one writes a second outcome
+    // at the same path under a fresh serial. Navigating back shows the value
+    // the position carries now; the one it replaced, the `Revoke` that took it
+    // away, and the entry line above it are all recorded and none of them are
+    // places to go.
+    let records = vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Begin(Vec::new())),
+        record(2, "/task:/1", State::Skip),
+        record(2, "/task:/1", State::Revoke),
+        record(3, "/task:/1", State::Begin(Vec::new())),
+        record(3, "/task:/1", State::Done(None)),
+    ];
+    let trail = Trail::new(&records);
+
+    let at = trail
+        .last()
+        .expect("a position to open on");
+    assert_eq!(at, Position::At(5));
+    assert_eq!(trail.step(at, Motion::Up), Some(Position::At(4)));
+    assert_eq!(
+        trail.step(Position::At(4), Motion::Up),
+        Some(Position::At(0))
+    );
+}
+
+#[test]
+fn a_revoked_scope_takes_what_it_held_with_it() {
+    // Revoking a call withdraws the whole subtree beneath it: the replay redoes
+    // that work under fresh serials, so the records the first pass left are no
+    // more current than the call that held them.
+    let records = vec![
+        record(1, "/task:", State::Begin(Vec::new())),
+        record(2, "/task:/check:", State::Begin(Vec::new())),
+        record(3, "/task:/check:/1", State::Begin(Vec::new())),
+        record(3, "/task:/check:/1", State::Done(None)),
+        record(2, "/task:/check:", State::Done(None)),
+        record(2, "/task:/check:", State::Revoke),
+        record(4, "/task:/check:", State::Begin(Vec::new())),
+        record(5, "/task:/check:/1", State::Begin(Vec::new())),
+        record(5, "/task:/check:/1", State::Done(None)),
+        record(4, "/task:/check:", State::Done(None)),
+    ];
+    let trail = Trail::new(&records);
+
+    let mut at = Position::At(9);
+    for expected in [8, 7, 6, 0] {
+        at = trail
+            .step(at, Motion::Up)
+            .expect("the standing execution walks back to the root");
+        assert_eq!(at, Position::At(expected));
+    }
+    assert_eq!(trail.step(at, Motion::Up), None);
+}

src/engraving/ledger.rs

@@ -16,10 +16,8 @@ pub struct Entry {
     /// Results bound to variables coming out of scope.
     pub bound: Vec<Supplied>,
     pub outcome: Option<State>,
-    /// This is set by a `Revoke` naming this entry, to be cleared by the next
-    /// `Begin` at this address. Only the target of a revocation is marked,
-    /// never an ancestor, as this is what stops `walk_invoke` otherwise
-    /// thinking it has to restore the very argument being amended.
+    /// Only the target of a revocation is marked, never an ancestor, which is
+    /// what stops `walk_invoke` restoring the very argument being amended.
     pub revoked: bool,
 }
 
@@ -103,13 +101,15 @@ impl Ledger {
                         );
                     }
                 }
-                if self
+                // The stack pops back past this scope, dropping what it still
+                // held open.
+                if let Some(at) = self
                     .open
-                    .last()
-                    == Some(&record.serial)
+                    .iter()
+                    .position(|s| *s == record.serial)
                 {
                     self.open
-                        .pop();
+                        .truncate(at);
                 }
             }
             State::Revoke => self.revoke(record.serial),
@@ -123,9 +123,8 @@ impl Ledger {
         }
     }
 
-    // Ancestors have their outcome cleared too: a Section short-circuits
-    // before descending, so its empty `Begin` would leave it standing.
-    // Descendants are left alone, the input guard reaching them.
+    // A Section short-circuits before descending, so an ancestor left standing
+    // on its empty `Begin` would be skipped on the replay.
     fn revoke(&mut self, serial: Serial) {
         if let Some(key) = self.key_of(serial) {
             if let Some(entry) = self
@@ -136,9 +135,7 @@ impl Ledger {
                 entry.revoked = true;
             }
         }
-        // Retaining each entry, its serial and its `Begin` is what keeps the
-        // descendants reachable and lets a revoked iteration still be claimed
-        // by the item it recorded.
+        // Descendants are left standing, their entries keeping them reachable.
         let mut at = serial;
         while let Some(parent) = self
             .scopes
@@ -160,13 +157,10 @@ impl Ledger {
         }
     }
 
-    // Re-entering a scope implicitly closes whatever was opened inside it, so
-    // the stack pops back past it. Without this a resume after a mid-step Quit
-    // parents the second walk's records under the step that was in flight.
     fn open_scope(&mut self, record: &Record, supplied: Vec<Supplied>) {
-        // Re-entry keeps what the scope already recorded; only a scope being
-        // opened afresh takes a new entry.
         let standing = self.standing(record.serial, &record.path, &supplied);
+        // Without this, a resume after a mid-step Quit parents the second
+        // walk's records under the step that was in flight.
         if let Some(at) = self
             .open
             .iter()
@@ -175,11 +169,8 @@ impl Ledger {
             self.open
                 .truncate(at);
         }
-        // A serial is allocated per (parent, edge) pair, so its parent is
-        // fixed by its route and re-entering the scope does not move it. The
-        // open stack answers only for a serial being seen for the first time —
-        // which is what lets a scope replayed silently, writing no `Begin` of
-        // its own, still have a stale descendant record beneath it correctly.
+        // A serial is allocated per (parent, edge) pair, so re-entry does not
+        // move it; the open stack answers only for one seen for the first time.
         let parent = match self
             .scopes
             .get(&record.serial)
@@ -347,12 +338,8 @@ impl Ledger {
         found
     }
 
-    /// The serial to enter this position at: the one a prior walk used if it
-    /// reached here, otherwise a fresh number. Lookup-before-allocate is what
-    /// keeps a resumed scope addressing its own recorded descendants.
     /// The serial to record work at this address under: the one it already
-    /// wears while it still stands, a fresh one once revoked, redone work
-    /// being new work.
+    /// wears while it still stands, a fresh one once revoked.
     pub fn serial_for(&self, parent: Serial, path: &str) -> Serial {
         match self.look(parent, path) {
             Some(entry) if !entry.revoked => entry.serial,