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

LSP: Add support for goto def in compiler for VSCode extension #982

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ankitaS11
Copy link
Collaborator

@ankitaS11 ankitaS11 commented Aug 17, 2022

This PR adds support for goto-def option in the compiler, which will be used by VSCode extension. Usage is:

lpython --goto-def <index> <file_name>

In case of errors, or the index not found, it outputs an empty dictionary so that the extension doesn't error out. Otherwise, it outputs a list of valid JSON/dictionary that is parsed in the extension.

PR for the extension depends on this: lcompilers/lpython-vscode-extension#3

cc: @certik

token_name = "\"" + yystype.string.str() + "\"";
}
return token_name;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this function in pickle_token, so that we do not repeat the code?

Comment on lines +583 to +585
if (input_in_scope_identifier(
index,
std::make_pair<int, int>(locations[i].first, locations[i].last))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just do:

Suggested change
if (input_in_scope_identifier(
index,
std::make_pair<int, int>(locations[i].first, locations[i].last))) {
if (locations[i].first <= index && index <= locations[i].last) {
}

And remove input_in_scope_identifier.

}
std::vector<LFortran::LPython::document_symbols> symbol_lists;
LFortran::LPython::document_symbols loc;
for (auto &a : x.result->m_global_scope->get_scope()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only be able to lookup symbols at global scope. Rather, we need to use resolve_symbol to lookup the symbol in the scoped symbol table. For that, we need to know which ASR node we are in.

To do that, we need to implement "index" -> ASR node calculation.

This will have to be one by the "overlapping intervals" problem, as we discussed some time ago.

@@ -859,6 +1005,7 @@ int main(int argc, char *argv[])
// LSP specific options
app.add_flag("--show-errors", show_errors, "Show errors when LSP is running in the background");
app.add_flag("--show-document-symbols", show_document_symbols, "Show symbols in lpython file");
app.add_option("--goto-def", goto_def_index, "Pass index for GoTo Definition");
Copy link
Contributor

Choose a reason for hiding this comment

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

VSCode calls it Go to Definition, so maybe we can call this option --go-to-definition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems the input almost always is a line and column number? If so, that should be the input here as well. So the description would be "Return a definition for the symbol at the given line and column", and if this is too long, we can shorten it.

@gnikit
Copy link

gnikit commented Aug 18, 2022

@ankitaS11 Are you launching an lpython process every time you issue a server request? If so, do you use any persistent caching or do you have to reparse the source files to AST and ASR with every request?

@certik
Copy link
Contributor

certik commented Aug 18, 2022

Currently we don't, but we will probably have to add it.

@gnikit
Copy link

gnikit commented Aug 18, 2022

I would probably guess that you need to leave the process running, since more interactive requests like hover will potentially overwhelm the users computer by "spamming" hover requests.

@czgdp1807
Copy link
Collaborator

Any news on this?

@gnikit
Copy link

gnikit commented Oct 10, 2022

Any news on this?

I would recommend you hold the merge on this (possibly turn it into a draft) until there is an agreement on the server design. If merged it would be harder to undo in the future.

The server should really be a running process with an open communications channel otherwise you are parsing the sources + serving LSP every time you make a request, you should only be doing the latter. For small projects that is probably fine but if this is to be a functional language server it needs to be able to store and query the ASR objects.

@certik certik marked this pull request as draft October 10, 2022 18:51
@certik
Copy link
Contributor

certik commented Oct 10, 2022

More discussion is needed on this, so I converted into a Draft.

I didn't have time to push this yet, but I think the design has to be to have a long running LPython/LFortran server process. We had this design, so it is in the history. I think we should revert to it and start improving it. I'll try to get to this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants