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: Signature Help #1841

Closed
iluuu1994 opened this issue Jun 24, 2019 · 7 comments
Closed

LSP: Signature Help #1841

iluuu1994 opened this issue Jun 24, 2019 · 7 comments

Comments

@iluuu1994
Copy link
Contributor

I've been investigating on how to implement the SignatureHelp API. Can you give me a few pointers? It's pretty similar to Hover. The main challenge is that Codebase::getReferenceAtPosition can't be used because it only records the position of the identifier (excluding the parentheses). Furthermore we need to not only know which method is being called but also at what parameter the cursor is at.

@muglug
Copy link
Collaborator

muglug commented Jun 24, 2019

What do you want exactly? Something like Codebase::getFunctionArgumentAtPosition that returns the method id and arg offset? Or do you also need the arg name?

@iluuu1994
Copy link
Contributor Author

iluuu1994 commented Jun 24, 2019

Yeah something like that. So I should probably extend Analyzer to store the parameters in a $parameter_map, right?

Or do you also need the arg name?

Just the arg index is fine.

@muglug
Copy link
Collaborator

muglug commented Jun 24, 2019

Yeah, the general idea being that all these maps are cached, and only updated/invalidated when individual methods are changed. Which is to say I think this would only work for methods, not functions (at the moment at least)

@iluuu1994
Copy link
Contributor Author

Cool, I'll give that a shot.

@iluuu1994
Copy link
Contributor Author

iluuu1994 commented Jun 25, 2019

This is not quite as easy to implement as I thought. The main issue is that the whitespace and , argument separator tokens are just thrown away by the parser. Currently I'm looking for the next , after the arguments getEndFilePos. This can be quite easily fooled with a comment:

Screen Shot 2019-06-25 at 23 41 27

We could use token_get_all here but I wanted to avoid tokenizing again.

There are other issues too. The parser is not as fault tolerant as I'd like. For example, more than one trailing comma results in the entire method call missing from the AST:

$this->foo($foo,,); // Poof!

Thus the argument positions won't be recorded and you'll get no signature help.

Kind of a bummer, those are likely not easily fixable.

@muglug
Copy link
Collaborator

muglug commented Jun 25, 2019

This can be quite easily fooled with a comment

Yeah, it's not the best, but it wouldn't be too hard to add some functionality that detects comments - I have this code block that could probably be extracted and reused.

For example, more than one trailing comma results in the entire method call missing from the AST

I've ticketed that here: nikic/PHP-Parser#616, but I'm not sure it's a blocker - I think the usability benefit of this massively outweighs the edge-case annoyance.

@iluuu1994
Copy link
Contributor Author

In that case I think I'll just use token_get_all after all. The only thing that needs to be tokenized is the source code from the getEndFilePos of the previous argument to the getStartFilePos of the next argument.

I've ticketed that here

Thanks!

iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 28, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 28, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 28, 2019
muglug pushed a commit to iluuu1994/psalm that referenced this issue Jun 30, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 30, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 30, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 30, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jun 30, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jul 1, 2019
iluuu1994 added a commit to iluuu1994/psalm that referenced this issue Jul 1, 2019
@muglug muglug closed this as completed in 67c3726 Jul 1, 2019
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

No branches or pull requests

2 participants