Fix off-by-one highlighting error #360
Conversation
Codecov Report
@@ Coverage Diff @@
## master #360 +/- ##
=======================================
Coverage 85.86% 85.86%
=======================================
Files 14 14
Lines 651 651
Branches 174 174
=======================================
Hits 559 559
Misses 92 92
Continue to review full report at Codecov.
|
| @@ -687,7 +687,7 @@ export function createHoverifier<C extends object, D, A>({ | |||
| }, | |||
| end: { | |||
| line: hoverOrError.range.end.line + 1, | |||
| character: hoverOrError.range.end.character + 1, | |||
| character: hoverOrError.range.end.character, | |||
There was a problem hiding this comment.
It definitely feels wrong to remove the + 1 from the character calculation but not from the line. That can't be right, and it will probably make the code more confusing/harder to debug for the next issue.
The comment above indicates that this transformation is done because the input LSP range is 0-indexed, but other parts of the code are 1-indexed.
// The result is 0-indexed; the code view is treated as 1-indexed.The issue here is probably more of a problem with the logic that maps the range to nodes treating the end as inclusive as opposed to exclusive, and the fix would be in that logic, as opposed to here. So something involving getTokenAtPositionOrRange() or getTokenAtPositionOrRange() probably.
BIG DISCLAIMER THAT I'M JUST GUESSING 😄
There was a problem hiding this comment.
@felixfbecker I agree, and I opened this PR to get exactly this type of advice. Who knows this code well enough to be able to formulate an actual solution?
There was a problem hiding this comment.
@tjkandala when he's back from vacation
There was a problem hiding this comment.
He's still out of office today :)
There was a problem hiding this comment.
@tjkandala have a good vacay today cuz ur gettin pinged tmr
There was a problem hiding this comment.
@felixfbecker Looks like you're right (fix in PR #366)
This PR fixes an off-by-one highlighting issue for all LSIF-based that was introduced (and has been present since) #331.
This fixes the behavior for me when run locally, but I'm going to delegate an actual solution to @tjkandala as there may be a better or more general solution to what's here. I'm not 100% I understand my change or a lot of the related code here. There's likely a better solution farther up or farther down the call stack, but this seems to work for a very specific use case during manual testing.
The recent merge of sourcegraph/code-intel-extensions#594 should make this problem (and subsequent fix) more apparent. We were not always returning the range data from the code intel extension. Note that we always return zero-indexed and half-closed intervals for ranges (as stated by the LSP spec).
Note that the highlighting is correct for document highlights, so this bug seems to only exist when displaying the ranges given back by the hover observable. The other observable behavior seem correct.