[otherjoel]

@rfindler or @mflatt ¹ — Fixes #376. Both bugs are in split-backward in color:text-mixin, which decides what to re-lex after an edit.

Below are explanations of what split-backwards is doing to cause each of the 2 bugs, with examples that use the new tests (and the line-backup-lexer used in the tests).

To reiterate from #376, the key thing about the lexer in the tests is that it uses the starting column of every token as its backup value (except for line-ending whitespace which gets backup=0). Which means any token getting re-lexed should start again from the beginning of the line.

Bug 1: backing up moves the kept position too

Each time the loop backs up, it rebinds orig-token-start, orig-token-end, and orig-data to the token it just removed. Only invalid-tree keeps its value from the first split. So the kept position moves back with the restart position, and the colorer thinks the kept tokens start earlier than they really do.

The first new test: the buffer is x\naa bb cc\ndd\n, and we delete the second b.

x \n aa ␠ bb ␠ cc \n dd \n
         ^^ the edit is in "bb" (backup 3)

1. bb has a backup of 3 in this case, so the loop backs up over ␠ and aa, and re-lexing restarts at aa. That part is correct.
2. The kept tokens are ␠ cc \n dd \n, which start after bb. But orig-token-end is now the end of aa.
3. After it re-lexes aa, the colorer thinks it has already passed the first kept token, ␠, and drops it. The modes match, so it stops and appends the rest right after aa:

text:    x   \n  aa  ␠b  ␠   cc  \n  dd\n
tokens:  x   \n  aa  cc  \n  dd  \n  (none)

The old cc token now covers b, every token after it is out of place, and the token list ends 3 characters before the buffer.

Fix: the first split/data call now happens once, before the loop, and its end, invalid, and data are returned unchanged. The loop updates only start and valid, so only the restart position moves.

Bug 2: at a token boundary, the loop checks the wrong token's backup

An edit exactly between two tokens can change both of them, so split/data removes both. But it returns only the later token's data, and the loop decides from that data whether to back up further.

The second new test: the buffer is " 1 2 3\nz\n, and we insert " between 3 and the newline.

"  ␠ 1 ␠ 2 ␠ 3  \n z \n
              ^ insert here

1. split/data removes 3 (backup 6) and \n (backup 0), and returns the data for \n.
2. The loop sees backup 0 (from \n), so it does not back up, and re-lexing restarts at 3.
3. The " at the start of the line is never re-lexed, so it stays an error token.

The docs say that the backup to use is the backup of "the token immediately preceding the change." Here that token is 3. So it's wrong according to the docs.

Fix: before it splits, split-backward finds the token that ends exactly at the edit and saves that token's data as ending-data. The loop now checks earliest-data, which is the data of the earliest token removed so far. It starts as (or ending-data data).

Tests

The PR adds three tests to gui-test/framework/tests/color-text.rkt. All three fail without this change:

• the existing backing-up-lexer on aaabcaaabc, inserting at position 4 (bug 1). The existing test makes the same edit in aaabc, where no tokens follow the edit.
• the two cases above, with a new line-backup-lexer whose tokens back up to the start of their line

I manually ran the full gui-test suite on 9.3 CS/macOS against this change before and after. All framework tests pass. The only failures I had were in tests/gracket and don't involve the colorer: wxme.rkt needs racket/snip@b7aab95, which Racket 9.3 doesn't have, and test.rkt has two frame-position checks (get-y: expected 21; got 1) (which maybe is because I'm running dual monitors?)

The explanations for each bug were technically drafted by the clanker (hence blockquotes), which also coded the fixes to split-backwards. This was unfamiliar territory for me, so I spent a few hours testing things myself in DrRacket to be sure I understood them, and editing the explanations for clarity.

Footnotes

1. I pinged you since it seems that @-mentioning a reviewer is apparently best practice now (see Expand the current pull request template to help draft better pull requests racket#5625, discourse 1, 2) ↩

[github-actions]

Resyntax analyzed 2 files in this pull request and found no issues.

[rfindler]

I can't say I really understand this code, but looking at the diff, I do think that (backup-pos . < . pos) is totally sketchy, given that backup-pos isn't used again and is computed by subtracting something from pos.

I've used this for about a day in my drracket and am not seeing any problems.