Skip to content

Fix sequence diff alignment#493

Merged
younes-io merged 2 commits intomasterfrom
fix/sequence-diff-lcs
Feb 9, 2026
Merged

Fix sequence diff alignment#493
younes-io merged 2 commits intomasterfrom
fix/sequence-diff-lcs

Conversation

@younes-io
Copy link
Collaborator

  • Summary
  • Root cause
    • Sequence diffs were index-aligned, so head/middle deletions appeared as modifications instead of deletions.
  • Fix
    • Use LCS alignment for sequences when lengths differ, preserving ordered diff for equal lengths.
  • Tests
    • MOCHA_GREP="Sequence diff" npm test
    • MOCHA_GREP="Check Model Test Suite" npm test

@younes-io younes-io marked this pull request as ready for review February 4, 2026 12:29
@lemmy lemmy added the bug Something isn't working label Feb 4, 2026
@younes-io
Copy link
Collaborator Author

younes-io commented Feb 4, 2026

@lemmy here I did not port the Toolbox diff algorithm, as this was something I realized later on in your comment. The change is limited to src/model/check.ts: it adds an LCS-based alignment only for sequences when lengths differ, while keeping the existing ordered diff for equal lengths.

I looked up the Toolbox diff algorithm and it is not LCS. It’s prefix/suffix detection for sequences and falls back to function diff (index-based) otherwise. That means this PR is not a port of the Toolbox logic.

I'm converting this to Draft, for now.

@younes-io younes-io marked this pull request as draft February 4, 2026 15:34
@lemmy
Copy link
Member

lemmy commented Feb 4, 2026

@younes-io I wonder whether the extension should adopt the same behavior as the Toolbox and not display deleted items. The current behavior—showing deleted items at the bottom—is rather confusing, especially given that the extension also displays the indices of elements in the sequence. If deleted items must be shown, it would be clearer to display them at their original positions rather than moving them to the bottom.

image image
----- MODULE SeqRem -----
EXTENDS Sequences

VARIABLES s

Init == s = << 1, 2, 3>>

Next == s # << >> /\ s' = Tail(s)

======
------ CONFIG SeqRem -----
INIT Init
NEXT Next
CHECK_DEADLOCK TRUE 
======

@younes-io
Copy link
Collaborator Author

Thanks for the repro. I’ve switched the sequence diff to match Toolbox (prefix/suffix add/remove; otherwise index diff), and I’ve hidden deleted sequence items so they don’t appear at the bottom. This should address the confusing output in your screenshots.

@younes-io younes-io marked this pull request as ready for review February 4, 2026 18:29
@younes-io younes-io self-assigned this Feb 4, 2026
@younes-io younes-io requested a review from lemmy February 6, 2026 12:40
Copy link
Member

@lemmy lemmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New functionality looks good to me.

@younes-io younes-io merged commit 2413a13 into master Feb 9, 2026
4 checks passed
@younes-io younes-io deleted the fix/sequence-diff-lcs branch February 9, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

2 participants