Conversation
by ensuring that the offset is inside (not on the edge of) the EXPR's span
src/requests/features.jl
Outdated
| offset = get_offset2(doc, params.position.line, params.position.character, true) | ||
| x = get_expr(getcst(doc), offset) | ||
| offset = get_offset2(doc, params.position.line, params.position.character) | ||
| x = get_expr_or_parent(getcst(doc), offset - 1) |
There was a problem hiding this comment.
I'm pretty sure there's an off-by-one error somewhere between the CST and what get_offset returns, which might very well cause issues in other cases...
There was a problem hiding this comment.
Do you have a repo case? I think it would be super valuable for us to fix these kind of problems for good...
There was a problem hiding this comment.
It's a systematic error/difference. When you have the cursor at position (0,0) then get_offset returns 1, but we usually start indexing the position in the CST from 0. Neither is incorrect, but afaict we're inconsistent about it.
There was a problem hiding this comment.
So get_offset2 is all around 1-based, I think. And then the idea is that it designates the position to the left of the indexed byte. So generally, the lowest offset that we ever need/get is 1, because that designates the point to the left of the first character in any string. But we can get offset positions that are larger than the length of a string, because when we designate a range say for the entire content of a string, then the stop bye position will be lastbyte+1.
I think that all follows the conventions on the VS Code side of things and the LSP, with the exception that our offsets internally are 1 rather than 0 based.
I have to admit I don't really understand why CST would ever return an offset of 0 :) If that is a 1-based offset, then that would amount to a -1 offset in 0-based land, which I don't understand why we would ever want that?
There was a problem hiding this comment.
No, I think we just usually iterate over the text with a zero-based offset (because that's much more natural for an offset as opposed to an index). See e.g. the definition of get_expr, which includes a pos=0 default argument. I've added a comment and made the code a bit clearer though.
There was a problem hiding this comment.
So I think that means that get_offset2 is probably misnamed, and it should really be named get_string_index or something like that, because it most definitely returns an index and not an offset in the way you defined it (which makes sense to me).
There was a problem hiding this comment.
Yup. Let's tackle that in a follow up though.
src/requests/features.jl
Outdated
| offset = get_offset2(doc, params.position.line, params.position.character, true) | ||
| x = get_expr(getcst(doc), offset) | ||
| offset = get_offset2(doc, params.position.line, params.position.character) | ||
| x = get_expr_or_parent(getcst(doc), offset - 1) |
There was a problem hiding this comment.
Do you have a repo case? I think it would be super valuable for us to fix these kind of problems for good...
2 participants
by ensuring that the offset is inside (not on the edge of) the EXPR's span.
Fixes julia-vscode/julia-vscode#1600 and similar issues (at least when the cursor actually is outside of the module definition).
For every PR, please check the following: