Fix wrong semantic highlighting due to out-of-date AST being used. #2709
Conversation
- There are chances that the AST used for semantic highlighting is out-of-date, which leads to wrong highlighting. This fix simply compare the length of the document with the length of the AST to check if the AST is out-of-date. In most of the case, checking the length is enough. - For root cause analysis, see: eclipse-jdt/eclipse.jdt.core#1151 Signed-off-by: Sheng Chen <[email protected]>
|
test this please |
|
We can use this patch as a temporary workaround, once the upstream |
+1, I tried the patch, didn't see highlighting break |
|
I just got notified on redhat-developer/vscode-java#2176 and was reminded of #1918, which I started investigating over a year ago (and then forgot about since I was too busy to spend time on it). This workaround seems fine to me, but I'm not sure if it will solve the token desync in all cases. The Javadoc for The other case where I think token desync will still be able to happen is in the case where the document has been edited, but is still the same length as before. For example, if you replace one part of the document with some new code which is still the same length as the old code. Or if you just move some lines around:
Of course, in this case, the token desync is not quite as noticeable, since it will only affect the changed lines, not the entire document (because the offsets for the outdated tokens will still "work" when the document length is unchanged). Now that I have some more time and was reminded of #1918, I started investigating it again. I was about to submit a PR since I believe I've found a better (and possibly simpler) fix for this issue. I'll take another look at it and will probably submit it later today. |
|
I have now submitted #2714 as an alternative to this PR. See also #1918 (comment) for more details on my investigation and as background for #2714. |
|
@0dinD Another little benefit we can get from this change is that it will dispose the out-of-dated AST and 'correct' it. Then other LSP handlers which need to use AST may have the chance to use the corrected one. Indeed, it will have some impact on the perf. Another thought I'm thinking is that, instead of regenerated a new AST, maybe we can first send a telemetry when the AST length does not match the buffer length. Then we can understand how bad or good the situation is. (Assume that most part of code change will change the content length as well) I think your PR #2714 can be merged separately with this PR. We can discuss whether we need to 'correct' the out-of-dated AST or only track the problem from BI in this PR. |
Right, that is a benefit with this solution. 👍
I agree. |
|
test this please |
There are chances that the AST used for semantic highlighting is out-of-date, which leads to wrong highlighting. This fix simply compares the length of the document with the length of the AST to check if the AST is out-of-date. In most of the case, checking the length is enough, this is guaranteed by the current implementation that document change event is handled in a blocking manner, so the buffer length is always right.
For root cause analysis, see: [Bug] Potential synchronization problem in CoreASTProvider eclipse-jdt/eclipse.jdt.core#1151
fix #1918
fix redhat-developer/vscode-java#2176
fix redhat-developer/vscode-java#3147
How to test the patch
Openning a relatively complex java file, e.g. BaseDocumentLifeCycleHandler. Then keeping pasting/removing multiple lines of code, like this:
semantic-highlighting.mp4
From my observation, the wrong highlighting happens around 1~2 out of ten tries using
1.19.0.