[CodeMirror] Mark decorations may not be empty in lint

[microbit-matt-hillsdon]

Bug Description

We're seeing crashes in lint.ts. There are several variations on this.

It seems they may have been introduced in 3.0.7 which upgraded CM, though we've fixed lots of CM extension crashes since then so perhaps they've just changed form slightly and were lost in the noise before.

RangeError: Mark decorations may not be empty
  at range(../node_modules/@codemirror/view/dist/index.js:1305:19)
  at ? (editor/codemirror/lint/lint.ts:147:12)
  at Array.map(<anonymous>)
  at init(editor/codemirror/lint/lint.ts:131:51)
  at updateF(editor/codemirror/lint/lint.ts:214:27)
  at update(../node_modules/@codemirror/state/dist/index.js:1795:34)
  at computeSlot(../node_modules/@codemirror/state/dist/index.js:2627:94)
  at ensureAddr(../node_modules/@codemirror/state/dist/index.js:2031:25)
  at new e(../node_modules/@codemirror/state/dist/index.js:2564:13)
  at applyTransaction(../node_modules/@codemirror/state/dist/index.js:2627:9)
  at state(../node_modules/@codemirror/state/dist/index.js:2278:29)
  at update(../node_modules/@codemirror/view/dist/index.js:6231:24)
  at _dispatch(../node_modules/@codemirror/view/dist/index.js:6140:59)
  at dispatch(../node_modules/@codemirror/view/dist/index.js:6212:14)
  at wrappedArguments(editor/codemirror/language-server/view.ts:46:12)
  at i(../../../../browser/src/helpers.ts:73:23)

How To Reproduce

TBD

Expected behavior

No crash!

Environment

Windows/Chrome 107 but likely a logic issue.

[microbit-matt-hillsdon]

I think CM should be using widget decorations for empty/point diagnostics, so this is interesting.

The lint module is our edited version of the CM code to add logic for treating the currently-being-edited line differently, so this could be a CM bug or something we broke.

Diagnostics can potentially be out of date wrt the document so perhaps that can somehow be the cause? Though I think that should just result in slightly incorrect positions (or discarding, if out of document).

[microbit-matt-hillsdon]

We're handling a setDiagnosticsEffect so completely replacing the diagnostics.

The actual error is:

if (from >= to) throw new RangeError("Mark decorations may not be empty")

Which is interesting, as d.from == d.to is checked to create a widget instead of a decoration. So we're in the wilder case of from > to.

Somehow Pyright (and our mapping to the doc) is generating a diagnostic that we map to a range with from > to.

[microbit-matt-hillsdon]

So the first step is to review our diagnostics mapping code, to see if it could introduce this error where it wasn't previously present in Pyright.

positionToOffset does have a some what suspicious if (offset > document.length) return; which probably has an off by one. I can't see how that could lead to this as such though. It could be triggered by out of date diagnostics for a longer document being mapped to a shorter one. I'll write a test for it and fix that anyway.

We bail if we fail to map either of start or end. I can't really see an issue here. It's probably impractical to find a bug like this in Pyright but I'll give it some more thought. We could perhaps log when we encounter this.

[microbit-matt-hillsdon]

OK, I think the problem could be:

const offset = document.line(position.line + 1).from + position.character;

We should probably check that this doesn't end up on a different line in the document we're trying to map to and bail (pending some future set of diagnostics) if it does.

Imagine scenario:

from: line 0 column 99 to: line 1 column 0

but imagine line 0 has been truncated by the time we get the diagnostic

so we get from = 0 + 99 and to = 1 + 0 and they're the wrong way around.

[microbit-matt-hillsdon]

I've fixed the position mapping logic in https://github.com/microbit-foundation/python-editor-v3/pull/1064 and added a specific check for to/from issues just in case it's a Pyright internal issue (seems very unlikely to me but it can't hurt).

[microbit-matt-hillsdon]

The check has been in CM forever so there's no particular reason to think this was new with the CM upgrade.