Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Characters rendering multiple times #2248

Closed
ashbork opened this issue Apr 24, 2022 · 7 comments · Fixed by #4113
Closed

Characters rendering multiple times #2248

ashbork opened this issue Apr 24, 2022 · 7 comments · Fixed by #4113
Assignees
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@ashbork
Copy link

ashbork commented Apr 24, 2022

Summary

When written code produces warnings/errors and they happen to extend past the end of the document, characters are shadowed in the non-existent lines.

image
Video of the behavior

I used the LSP provided within the documentation (pylsp) with python-lsp-black, pylsp-rope and pylsp-mypy (none of these could cause that sort of behavior as far as I know)

Reproduction Steps

I tried this:

Inserting text

I expected this to happen:

Text to be inserted once

Instead, this happened:

It appears multiple times

Helix log

No response

Platform

Linux

Terminal Emulator

Konsole 21.12.3

Helix Version

helix 0.6.0

@ashbork ashbork added the C-bug Category: This is a bug label Apr 24, 2022
@kirawi
Copy link
Member

kirawi commented Apr 24, 2022

Can you check if this was fixed in 22.03?

@ashbork
Copy link
Author

ashbork commented Apr 24, 2022

Can you check if this was fixed in 22.03?

Tried 22.05-dev, same results.

@ashbork
Copy link
Author

ashbork commented Apr 24, 2022

A fresh install of the dev branch with a fresh pylsp install produces the same results, too.

@kirawi kirawi added A-language-server Area: Language server client A-helix-term Area: Helix term improvements labels Apr 24, 2022
@archseer
Copy link
Member

It's only at the end of the document, right? I've seen that happen before when the diagnostics ranges were pointing past the document end

@ashbork
Copy link
Author

ashbork commented Apr 25, 2022

Oh, that seems to be correct -
image
image
I'll note that within the issue.

@the-mikedavis
Copy link
Member

Naively I think we might be able to handle this here:

doc.diagnostics()
.iter()
.map(|diagnostic| {
let diagnostic_scope = match diagnostic.severity {
Some(Severity::Info) => info,
Some(Severity::Hint) => hint,
Some(Severity::Warning) => warning,
Some(Severity::Error) => error,
_ => r#default,
};
(
diagnostic_scope,
diagnostic.range.start..diagnostic.range.end,
)
})

by doing a min(diagnostic.range.end, document_end). I can give this a proper look in a while unless someone wants to beat me to it 🙂

@the-mikedavis
Copy link
Member

the-mikedavis commented Sep 4, 2022

Digging into this, the diagnostic isn't beyond the document - we do have checks that handle that case in the rendering code. So using the min of the range and document-end isn't the solution (☝️).

The issue is that there are overlapping diagnostics at the end of the document. This falls into a specific hole in the highlight event stream merge Iterator implementation (helix_core::syntax::merge) which allows the diagnostic event to skip past the syntax highlighting events and produce a HighlightEvent::Source at the end of the Iterator which is out-of-order. The rendering loop assumes that the highlight events are in-order and non-overlapping, so we end up with duplicated "ghost text" at the end of the document.

helix_core::syntax::merge produces a non-overlapping in-order event stream assuming that the input Iterator and span Vec are also both fully sorted and non-overlapping. Diagnostics from the Language Server may overlap though. This is usually handled by the merge function by discarding the overlapping diagnostics (this branch) but when the overlapping diagnostics are at the end of the syntax highlight iterator1, we instead fall into this branch which emits the diagnostic span as a HighlightEvent::Source.

The easiest way to fix the exact case of #3617 (and this case too I expect) is to deduplicate the diagnostics Vec in helix_term::ui::editor. Ideally though we should be allowed to have overlapping diagnostics. I have a branch that I will clean up and PR that subslices diagnostic ranges into a correctly sorted and non-overlapping HighlightEvent stream, then rewrites the helix_core::syntax::merge to merge two HighlightEvent Iterators. This will also be useful for the rainbow highlights implementation.

Footnotes

  1. It's a very specific series of events that leads to this happening: two diagnostics overlapping exactly can produce the behavior. The first is correctly subsliced. The iterator assumes that ranges are monotonically increasing though (hence non-overlapping) so the second event appears to start before the next HighlightEvent::Source and so it is not subsliced. When self.iter.next() is None (end of syntax highlight event stream), that trailing duplicated diagnostic range is emitted as a highlight event out-of-order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
4 participants