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

Fix location lookup for code actions, fixes #1047. #1050

Merged
merged 2 commits into from Apr 28, 2022

Conversation

fredrikekre
Copy link
Member

No description provided.

@davidanthoff davidanthoff added this to the Next Patch milestone Apr 8, 2022
@@ -7,7 +7,7 @@ end
function textDocument_codeAction_request(params::CodeActionParams, server::LanguageServerInstance, conn)
commands = Command[]
doc = getdocument(server, URI2(params.textDocument.uri))
offset = get_offset(doc, params.range.start) # Should usef get_offset2?
offset = get_offset2(doc, params.range.start)
x = get_expr(getcst(doc), offset)
arguments = Any[params.textDocument.uri, offset] # use the same arguments for all commands
Copy link
Member

Choose a reason for hiding this comment

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

So, I never understood what exactly get_offset returns, but I always thought that it was the thing that things like get_expr expect...

get_offset2 returns a valid index into the underlying string that holds the document, and I primarily introduced it to fix the text sync implementation. But I'm not sure we can just swap out getoffset2 for getoffset in situations where we then pass the return value onto functionality that was built assuming the get_offset semantics?

But take this just as a heads-up about the history, this fix might also just be correct, I don't really know anything about get_expr or how it works :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I didn't look into the details, but while debugging I noticed the comment and just tried it out. It seems to fix all the issues I had at least.

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Seems correct to me.

@pfitzseb pfitzseb merged commit 8a95a67 into julia-vscode:master Apr 28, 2022
@fredrikekre fredrikekre deleted the fe/code-action-location branch April 28, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants