Signature help #250

vakata · 2017-01-20T15:53:34Z

Closes #18
Parameter counting is "hackish" and working with php://temp maybe undesired? I also had to store parsed parameters along with the definition (similar to declarationLine) to avoid parsing again.

codecov-io · 2017-01-20T18:38:56Z

Current coverage is 87.37% (diff: 75.00%)

Merging #250 into master will decrease coverage by 1.22%

@@             master       #250   diff @@
==========================================
  Files            51         55     +4   
  Lines          1412       1552   +140   
  Methods         152        158     +6   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1251       1356   +105   
- Misses          161        196    +35   
  Partials          0          0

Diff Coverage	File Path
••••••• 70%	new src/SignatureHelpProvider.php
•••••••••• 100%	src/DefinitionResolver.php
•••••••••• 100%	src/Protocol/ParameterInformation.php
•••••••••• 100%	src/Server/TextDocument.php
•••••••••• 100%	src/Protocol/SignatureHelp.php
•••••••••• 100%	src/Protocol/SignatureInformation.php

Powered by Codecov. Last update 47b5b67...d4d0c47

felixfbecker

Let me know if I can help you with anything

felixfbecker · 2017-01-21T17:36:27Z

src/Definition.php

+    /**
+     * Parameters array (for methods and functions), for use in textDocument/signatureHelp
+     *
+     * @var string[]


This should be ParameterInformation[]

You want me to build the objects here? Won't that use more memory than plain strings, which are converted to ParameterInformation when needed?

If I understand your code correctly this string array only contains the pretty-printed parameter node. That means you cannot construct the documentation property from it, which you need to read from the @param tag. Of course as a new feature it is expected to use more memory.

Yes, I currently populate only the label (using this string). I will add the documentation and construct ParameterInformation objects - of course that would mean an object for every parameter of every method / function in the project. I will test and let you know how it impacts memory usage.

@felixfbecker the impact was not significant

FYI we already keep Hover and SymbolInformation in memory. I thought about changing Definition so that it contains just the data necessary to construct those and then build them from this, but I would prefer to first implement the features and then improve perf.

If memory usage is too high the implementation would not save anything in Definition and load the document on demand.

You can leave out the docs first but lets use objects from the start on.

It is already done, and the docs are in place as well. I added a comment for you though - Param nodes do not have a parentNode attribute - I could not fix it - the attribute was added in ReferenceAdder but then was not available at a later point.

@felixfbecker anything I can help with here?

felixfbecker · 2017-01-21T17:36:57Z

src/DefinitionResolver.php

+            // Resolve $this
+            if ($node->name === 'this' && $fqn = $this->getContainingClassFqn($node)) {
+                return $this->index->getDefinition($fqn, false);
+            }


Please keep this out of this PR

It is actually needed in order to provide signatureHelp when invoking a method on $this, and those lines are not part of the $this autocomplete PR, as it was not needed for autocomplete - different parts of the code are involved. I could move it to a separate PR?

Okay. If you do it in a separate PR I think I would merge it straight away, change looks good.

felixfbecker · 2017-01-21T17:37:11Z

src/LanguageServer.php

@@ -247,6 +248,9 @@ public function initialize(ClientCapabilities $capabilities, string $rootPath =
            $serverCapabilities->completionProvider = new CompletionOptions;
            $serverCapabilities->completionProvider->resolveProvider = false;
            $serverCapabilities->completionProvider->triggerCharacters = ['$', '>'];
+            // Support "Signature Help"
+            $serverCapabilities->signatureHelpProvider = new SignatureHelpOptions;
+            $serverCapabilities->signatureHelpProvider->triggerCharacters = ['('];


Add comma as a trigger character

felixfbecker · 2017-01-21T17:39:17Z

src/SignatureHelpProvider.php

+    {
+        $this->definitionResolver = $definitionResolver;
+        $this->index = $index;
+        $this->parser = new Parser;


Why do we need a new parser instance here?

Sorry - that was leftover - I eventually used Lexer directly - will remove.

felixfbecker · 2017-01-21T17:40:18Z

src/SignatureHelpProvider.php

+    public function provideSignature(PhpDocument $doc, Position $pos) : SignatureHelp
+    {
+        $help = new SignatureHelp;
+        $help->signatures = [];


Change the constructor of SignatureHelp to initialize this

felixfbecker · 2017-01-21T17:41:40Z

src/SignatureHelpProvider.php

+
+        $handle = fopen('php://temp', 'r+');
+        fwrite($handle, $doc->getContent());
+        fseek($handle, 0);


We have the whole content as a string, so opening a stream for this provides no benefit at all.
Did you try solving this just with the PhpParser node offsets you get from getNodeAtPosition()?

Yes, but unfortunately most of the time when dealing with an unclosed brace the nearest node was very far from the actual position. As for the buffer - I will rewrite and use string operations, but I doubt I will have the time to do it tonight.

Could you give an example?

I do not have the time to prepare an example at the moment. The problem was that the parser errors before reaching the cursor position, which means there is no node at that position - it should be easy to reproduce.

In that case lets open issues for better error recovery at these places. A lot of improvements were already made in that area.

I just checked - the methodNotClosed test requires at least a couple of symbols of backtracking before getNodeAtPosition returns a node.

Maybe this should remain open until I rewrite it to use getContent instead of a buffer? Or would you rather wait for improvements to PhpParser and then remove backtracking completely?

It is weird to use the parser for completion, but the lexer/content for signature help.
If error recovery works for completion for methods, it should work for the parameters too, right?
What is the emitted AST at the point where the parser "fails" for the signature help use case?

The node at the requested position is null, after backtracking 31 symbols (each of those positions is still null) we get to an Error node, whose parent is a PropertyFetch node. The $var property of the fetch is populated, but the $name property is not (it is the Error node). This is why I used string operations to get the method being called, and then - the Lexer with some custom logic on top to get the parameter count.
As I said in the Signature Help issue - it is a messy solution, but it is the only one I could come up with. I am sure someone with superior skills can take it a few steps further to something elegant and more robust :)

felixfbecker · 2017-01-21T17:44:01Z

tests/Server/TextDocument/SignatureHelpTest.php

+
+        $help = new SignatureHelp;
+        $help->signatures = [];
+        $info = new SignatureInformation;


Add constructors to the classes that allow to pass all properties as arguments so we can define the result inline without saving it to a variable

vakata · 2017-01-21T20:51:08Z

src/DefinitionResolver.php

@@ -134,7 +135,13 @@ public function createDefinitionFromNode(Node $node, string $fqn = null): Defini
        $def->parameters = [];
        if ($node instanceof Node\FunctionLike) {
            foreach ($node->getParams() as $param) {
-                $def->parameters[] = $this->prettyPrinter->prettyPrint([$param]);
+                if (!$param->getAttribute('parentNode')) {


@felixfbecker I could not figure out why those instances do not have a parentNode attribute - this is a temporary patch. Could you have a look?

Every node except the root node should have a parentNode
Otherwise there must be a bug in NodeVisitor or ReferencesAdder

felixfbecker · 2017-01-21T21:04:21Z

src/Protocol/SignatureHelp.php

+     * @param bool                   $activeSignature The active signature.
+     * @param bool                   $activeParameter The active parameter of the active signature.
+     */
+    public function __construct(array $signatures = [], int $activeSignature = 0, int $activeParameter = 0)


Use null as default value for the last two params because they are optional on the object.

felixfbecker · 2017-01-21T21:04:42Z

src/Protocol/SignatureHelp.php

+    /**
+     * @param SignatureInformation[] $signatures      The signatures.
+     * @param bool                   $activeSignature The active signature.
+     * @param bool                   $activeParameter The active parameter of the active signature.


felixfbecker · 2017-01-21T21:05:13Z

src/Protocol/SignatureInformation.php

+     * @param string|null            $documentation The human-readable doc-comment of this signature.
+     * @param ParameterInformation[] $parameters    The parameters of this signature.
+     */
+    public function __construct(string $label = null, string $documentation = null, array $parameters = [])


Use null as default for $parameters

felixfbecker · 2017-01-21T21:05:25Z

src/Protocol/SignatureInformation.php

+    /**
+     * @param string                 $label         The label of this signature. Will be shown in the UI.
+     * @param string|null            $documentation The human-readable doc-comment of this signature.
+     * @param ParameterInformation[] $parameters    The parameters of this signature.


adapt type for $parameters

felixfbecker · 2017-01-26T15:32:12Z

src/Protocol/SignatureInformation.php

-     * @param ParameterInformation[] $parameters    The parameters of this signature.
+     * @param string                      $label         The label of this signature. Will be shown in the UI.
+     * @param string|null                 $documentation The human-readable doc-comment of this signature.
+     * @param ParameterInformation[]|null $parameters    The parameters of this signature.
     */
    public function __construct(string $label = null, string $documentation = null, array $parameters = [])


default value for $parameters must be null

vakata · 2017-06-17T21:01:00Z

I am closing this as this solution will now longer work as it depends on the Lexer from nikic's PHP parser.

vakata and others added 15 commits January 19, 2017 16:35

handle hovering $this

ddf8274

added signature help

f9240cd

restored fixture

6bca8df

added missing class

67be71a

added tests

1e109a3

fixed file count

fdda97c

fixed signature helper reference

16c7560

added param count

3b4e0c3

fixed param counting

a7c094e

Improved param counting

8995611

Updated tests

8fbbf3d

Now works with unsaved files

d433ae4

Properly hiding when done with params

75e5e24

Updated tests

6980fb5

Fixed phpcs error

b0f3952

Fixed hiding the tooltip too early, fixed tests

1b68c73

felixfbecker requested changes Jan 21, 2017

View reviewed changes

vakata added 4 commits January 21, 2017 20:55

Added comma as a trigger character

96948c4

Removed parser instance

9e822fb

Added constructors

df0c9d4

Fixed original node reference

412a884

vakata commented Jan 21, 2017

View reviewed changes

felixfbecker requested changes Jan 21, 2017

View reviewed changes

vakata and others added 2 commits January 21, 2017 23:16

Fixed doc blocks

549645b

Merge branch 'master' into signatureHelp

1f20605

felixfbecker reviewed Jan 26, 2017

View reviewed changes

vakata and others added 2 commits January 26, 2017 18:56

Fixed default value for parameters in SignatureInformation

32b9984

Merge branch 'master' into signatureHelp

d4d0c47

vakata closed this Jun 17, 2017

vakata deleted the signatureHelp branch June 17, 2017 21:01

vakata restored the signatureHelp branch June 17, 2017 21:01

Signature help #250

Signature help #250

Conversation

vakata commented Jan 20, 2017

codecov-io commented Jan 20, 2017 • edited Loading

Current coverage is 87.37% (diff: 75.00%)

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vakata Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vakata Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vakata commented Jun 17, 2017

codecov-io commented Jan 20, 2017 •

edited

Loading

felixfbecker Jan 21, 2017 •

edited

Loading

vakata Jan 21, 2017 •

edited

Loading

vakata Jan 21, 2017 •

edited

Loading