Skip to content

better getModuleAt #905

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.

Sign up for GitHub

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

Merged
davidanthoff merged 3 commits into from
Mar 3, 2021
Merged

better getModuleAt #905

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fb58c6
better getModuleAt
pfitzseb Mar 3, 2021
7a30d07
re-enable get_offset2 nothrow
pfitzseb Mar 3, 2021
2c6b6ec
clearer code
pfitzseb Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/requests/features.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ function julia_getModuleAt_request(params::VersionedTextDocumentPositionParams,
doc = getdocument(server, uri)
if doc._version == params.version
offset = get_offset2(doc, params.position.line, params.position.character, true)
x = get_expr(getcst(doc), offset)
x = get_expr_or_parent(getcst(doc), offset - 1)
Copy link
Copy Markdown
Member Author

@pfitzseb pfitzseb Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Member

@davidanthoff davidanthoff Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a repo case? I think it would be super valuable for us to fix these kind of problems for good...

Copy link
Copy Markdown
Member Author

@pfitzseb pfitzseb Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@davidanthoff davidanthoff Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@pfitzseb pfitzseb Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@davidanthoff davidanthoff Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

@pfitzseb pfitzseb Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Let's tackle that in a follow up though.

if x isa EXPR
scope = StaticLint.retrieve_scope(x)
if scope !== nothing
Expand Down
26 changes: 26 additions & 0 deletions src/utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,32 @@ function get_expr(x, offset, pos=0, ignorewhitespace=false)
end
end

# like get_expr, but only returns a expr if offset is not on the edge of it's span
function get_expr_or_parent(x, offset, pos=0)
if pos > offset
return nothing
end
if length(x) > 0 && headof(x) !== :NONSTDIDENTIFIER
for a in x
if pos < offset <= (pos + a.fullspan)
if pos < offset < (pos + a.span)
return get_expr_or_parent(a, offset, pos)
else
return x
end
end
pos += a.fullspan
end
elseif pos == 0
return x
elseif (pos < offset <= (pos + x.fullspan))
if pos + x.span < offset
return x.parent
end
return x
end
end

function get_expr(x, offset::UnitRange{Int}, pos=0, ignorewhitespace=false)
if all(pos .> offset)
return nothing
Expand Down