tidy up

fixtures/signature_help/calls.php

@@ -47,7 +47,8 @@ function foo(int $i, bool $b = false)
 
 $t = new Test();
 $t->foo();
-$t->foo(1, );
+$t->foo(1,  
+$t->foo(1,);
 $t->baz();
 
 foo();

src/Server/TextDocument.php

@@ -58,6 +58,9 @@ class TextDocument
      */
     protected $completionProvider;
 
+    /**
+     * @var SignatureHelpProvider
+     */
     protected $signatureHelpProvider;
 
     /**
@@ -254,12 +257,13 @@ class TextDocument
     }
 
     /**
-     * The goto definition request is sent from the client to the server to resolve the definition location of a symbol
+     * The signature help request is sent from the client to the server to request signature information at a given
-     * at a given text document position.
+     * cursor position.
      *
      * @param TextDocumentIdentifier $textDocument The text document
      * @param Position               $position     The position inside the text document
-     * @return Promise <Location|Location[]>
+     *
+     * @return Promise <SignatureHelp>
      */
     public function signatureHelp(TextDocumentIdentifier $textDocument, Position $position): Promise
     {

src/SignatureHelpProvider.php

@@ -21,11 +21,15 @@ class SignatureHelpProvider
     /** @var ReadableIndex */
     private $index;
 
+    /** @var PhpDocumentLoader */
     private $documentLoader;
 
     /**
+     * Constructor
+     *
      * @param DefinitionResolver $definitionResolver
      * @param ReadableIndex      $index
+     * @param PhpDocumentLoader  $documentLoader
      */
     public function __construct(DefinitionResolver $definitionResolver, ReadableIndex $index, PhpDocumentLoader $documentLoader)
     {
@@ -34,81 +38,44 @@ class SignatureHelpProvider
         $this->documentLoader = $documentLoader;
     }
 
+    /**
+     * Finds signature help for a callable position
+     *
+     * @param PhpDocument $doc      The document the position belongs to
+     * @param Position    $position The position to detect a call from
+     *
+     * @return SignatureHelp
+     */
     public function getSignatureHelp(PhpDocument $doc, Position $position): SignatureHelp
     {
         // Find the node under the cursor
         $node = $doc->getNodeAtPosition($position);
 
-        $fqn = null;
+        // Find the definition of the item being called
-
+        list($def, $argumentExpressionList) = $this->getCallingInfo($node);
-        // First find the node that the call belongs to
-        if ($node instanceof Node\DelimitedList\ArgumentExpressionList) {
-            $argumentExpressionList = $node;
-            if ($node->parent instanceof Node\Expression\ObjectCreationExpression) {
-                $node = $node->parent->classTypeDesignator;
-                if (!$node instanceof Node\QualifiedName) {
-                    return new SignatureHelp();
-                }
-                $fqn = $this->definitionResolver->resolveReferenceNodeToFqn($node);
-                $fqn = "{$fqn}->__construct()";
-            } else {
-                $node = $node->parent->getFirstChildNode(
-                    Node\Expression\MemberAccessExpression::class,
-                    Node\Expression\ScopedPropertyAccessExpression::class,
-                    Node\QualifiedName::class
-                );
-            }
-        } elseif ($node instanceof Node\Expression\CallExpression) {
-            $argumentExpressionList = $node->getFirstChildNode(Node\DelimitedList\ArgumentExpressionList::class);
-            $node = $node->getFirstChildNode(
-                Node\Expression\MemberAccessExpression::class,
-                Node\Expression\ScopedPropertyAccessExpression::class,
-                Node\QualifiedName::class
-            );
-        } elseif ($node instanceof Node\Expression\ObjectCreationExpression) {
-            $argumentExpressionList = $node->getFirstChildNode(Node\DelimitedList\ArgumentExpressionList::class);
-            //$node = $node->getFirstChildNode(Node\QualifiedName::class);
-            $node = $node->classTypeDesignator;
-            if (!$node instanceof Node\QualifiedName) {
-                return new SignatureHelp();
-            }
-            $fqn = $this->definitionResolver->resolveReferenceNodeToFqn($node);
-            $fqn = "{$fqn}->__construct()";
-        } else {
-            $node = null;
-        }
-
-        if (!$node) {
-            return new SignatureHelp();
-        }
-
-        // Now find the definition of the call
-        $fqn = $fqn ?: DefinitionResolver::getDefinedFqn($node);
-        if ($fqn) {
-            $def = $this->index->getDefinition($fqn);
-        } else {
-            $def = $this->definitionResolver->resolveReferenceNodeToDefinition($node);
-        }
 
         if (!$def) {
             return new SignatureHelp();
         }
 
+        // Find the active parameter
         $activeParam = $argumentExpressionList
             ? $this->findActiveParameter($argumentExpressionList, $position, $doc)
             : 0;
 
-        $doc = $this->documentLoader->get($def->symbolInformation->location->uri);
+        // Get information from the item being called to build the signature information
-        if (!$doc) {
+        $calledDoc = $this->documentLoader->get($def->symbolInformation->location->uri);
+        if (!$calledDoc) {
             return new SignatureHelp();
         }
-        $node = $doc->getNodeAtPosition($def->symbolInformation->location->range->start);
+        $calledNode = $calledDoc->getNodeAtPosition($def->symbolInformation->location->range->start);
-        $params = $this->getParameters($node, $doc);
+        $params = $this->getParameters($calledNode, $calledDoc);
-        $label = $this->getLabel($node, $params, $doc);
+        $label = $this->getLabel($calledNode, $params, $calledDoc);
+
         $signatureInformation = new SignatureInformation();
         $signatureInformation->label = $label;
         $signatureInformation->parameters = $params;
-        $signatureInformation->documentation = $this->definitionResolver->getDocumentationFromNode($node);
+        $signatureInformation->documentation = $this->definitionResolver->getDocumentationFromNode($calledNode);
         $signatureHelp = new SignatureHelp();
         $signatureHelp->signatures = [$signatureInformation];
         $signatureHelp->activeSignature = 0;
@@ -117,11 +84,84 @@ class SignatureHelpProvider
     }
 
     /**
-     * @param Node\MethodDeclaration|Node\Statement\FunctionDeclaration $node
+     * Given a node that could be a callable, finds the definition of the call and the argument expression list of
+     * the node
+     *
+     * @param Node $node The node to find calling information from
+     *
+     * @return array|null
      */
-    private function getLabel($node, array $params, PhpDocument $doc): string
+    private function getCallingInfo(Node $node)
+    {
+        $fqn = null;
+        $callingNode = null;
+        if ($node instanceof Node\DelimitedList\ArgumentExpressionList) {
+            // Cursor is already inside a (
+            $argumentExpressionList = $node;
+            if ($node->parent instanceof Node\Expression\ObjectCreationExpression) {
+                // Constructing something
+                $callingNode = $node->parent->classTypeDesignator;
+                if (!$callingNode instanceof Node\QualifiedName) {
+                    // We only support constructing from a QualifiedName
+                    return null;
+                }
+                $fqn = $this->definitionResolver->resolveReferenceNodeToFqn($callingNode);
+                $fqn = "{$fqn}->__construct()";
+            } else {
+                $callingNode = $node->parent->getFirstChildNode(
+                    Node\Expression\MemberAccessExpression::class,
+                    Node\Expression\ScopedPropertyAccessExpression::class,
+                    Node\QualifiedName::class
+                );
+            }
+        } elseif ($node instanceof Node\Expression\CallExpression) {
+            $argumentExpressionList = $node->getFirstChildNode(Node\DelimitedList\ArgumentExpressionList::class);
+            $callingNode = $node->getFirstChildNode(
+                Node\Expression\MemberAccessExpression::class,
+                Node\Expression\ScopedPropertyAccessExpression::class,
+                Node\QualifiedName::class
+            );
+        } elseif ($node instanceof Node\Expression\ObjectCreationExpression) {
+            $argumentExpressionList = $node->getFirstChildNode(Node\DelimitedList\ArgumentExpressionList::class);
+            $callingNode = $node->classTypeDesignator;
+            if (!$callingNode instanceof Node\QualifiedName) {
+                // We only support constructing from a QualifiedName
+                return null;
+            }
+            // Manually build the __construct fqn
+            $fqn = $this->definitionResolver->resolveReferenceNodeToFqn($callingNode);
+            $fqn = "{$fqn}->__construct()";
+        }
+
+        if (!$callingNode) {
+            return null;
+        }
+
+        // Now find the definition of the call
+        $fqn = $fqn ?: DefinitionResolver::getDefinedFqn($callingNode);
+        if ($fqn) {
+            $def = $this->index->getDefinition($fqn);
+        } else {
+            $def = $this->definitionResolver->resolveReferenceNodeToDefinition($callingNode);
+        }
+
+        if (!$def) {
+            return null;
+        }
+        return [$def, $argumentExpressionList];
+    }
+
+    /**
+     * Creates a label for SignatureInformation
+     *
+     * @param Node\MethodDeclaration|Node\Statement\FunctionDeclaration $node   The method/function declaration node
+     *                                                                          we are building the label for
+     * @param ParameterInformation[]                                    $params Parameters that belong to the node
+     *
+     * @return string
+     */
+    private function getLabel($node, array $params): string
     {
-        //$label = $node->getName() . '(';
         $label = '(';
         if ($params) {
             foreach ($params as $param) {
@@ -130,21 +170,16 @@ class SignatureHelpProvider
             $label = substr($label, 0, -2);
         }
         $label .= ')';
-        /*
-        if ($node->returnType) {
-            $label .= ': ';
-            if ($node->returnType instanceof QualifiedName) {
-                $label .= $node->returnType->getResolvedName();
-            } else {
-                $label .= $node->returnType->getText($doc->getContent());
-            }
-        }
-        */
         return $label;
     }
 
     /**
-     * @param Node\MethodDeclaration|Node\Statement\FunctionDeclaration $node
+     * Builds ParameterInformation from a node
+     *
+     * @param Node\MethodDeclaration|Node\Statement\FunctionDeclaration $node The node to build parameters from
+     * @param PhpDocument                                               $doc  The document the node belongs to
+     *
+     * @return ParameterInformation[]
      */
     private function getParameters($node, PhpDocument $doc): array
     {
@@ -165,6 +200,15 @@ class SignatureHelpProvider
         return $params;
     }
 
+    /**
+     * Given a position and arguments, finds the "active" argument at the position
+     *
+     * @param Node\DelimitedList\ArgumentExpressionList $argumentExpressionList The argument expression list
+     * @param Position                                  $position               The position to detect the active argument from
+     * @param PhpDocument                               $doc                    The document that contains the expression
+     *
+     * @return int
+     */
     private function findActiveParameter(
         Node\DelimitedList\ArgumentExpressionList $argumentExpressionList,
         Position $position,
@@ -177,7 +221,6 @@ class SignatureHelpProvider
             if ($arg instanceof Node) {
                 $start = $arg->getFullStart();
                 $end = $arg->getEndPosition();
-                ++$i;
             } else {
                 $start = $arg->fullStart;
                 $end = $start + $arg->length;
@@ -187,6 +230,9 @@ class SignatureHelpProvider
                 $found = $i;
                 break;
             }
+            if ($arg instanceof Node) {
+                ++$i;
+            }
         }
         if (is_null($found)) {
             $found = $i;

tests/Server/TextDocument/SignatureHelpTest.php

@@ -80,7 +80,26 @@ class SignatureHelpTest extends TestCase
                 ]),
             ],
             'member call 2nd param active' => [
-                new Position(49, 11),
+                new Position(49, 12),
+                $this->createSignatureHelp([
+                    'label' => '(\\Foo\\SomethingElse $a, int|null $b = null)',
+                    'documentation' => 'Function doc',
+                    'parameters' => [
+                        [
+                            'label' => '\\Foo\\SomethingElse $a',
+                            'documentation' => 'A param with a different doc type',
+                        ],
+                        [
+                            'label' => 'int|null $b = null',
+                            'documentation' => 'Param with default value',
+                        ],
+                    ],
+                    'activeSignature' => 0,
+                    'activeParameter' => 1,
+                ]),
+            ],
+            'member call 2nd param active and closing )' => [
+                new Position(50, 11),
                 $this->createSignatureHelp([
                     'label' => '(\\Foo\\SomethingElse $a, int|null $b = null)',
                     'documentation' => 'Function doc',
@@ -99,7 +118,7 @@ class SignatureHelpTest extends TestCase
                 ]),
             ],
             'method with no params' => [
-                new Position(50, 9),
+                new Position(51, 9),
                 $this->createSignatureHelp([
                     'label' => '()',
                     'documentation' => 'Method with no params',
@@ -133,7 +152,7 @@ class SignatureHelpTest extends TestCase
                 ]),
             ],
             'global function' => [
-                new Position(52, 4),
+                new Position(53, 4),
                 $this->createSignatureHelp([
                     'label' => '(int $i, bool $b = false)',
                     'documentation' => null,
@@ -152,7 +171,7 @@ class SignatureHelpTest extends TestCase
                 ]),
             ],
             'static method' => [
-                new Position(54, 10),
+                new Position(55, 10),
                 $this->createSignatureHelp([
                     'label' => '(mixed $a)',
                     'documentation' => null,