Skip to content

Signature help #250

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

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions fixtures/signatureHelp/funcClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

function helpFunc1(int $count = 0)
{
}

helpFunc1()
7 changes: 7 additions & 0 deletions fixtures/signatureHelp/funcNotClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

function helpFunc2(int $count = 0)
{
}

helpFunc2(
15 changes: 15 additions & 0 deletions fixtures/signatureHelp/methodClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

class HelpClass1
{
public function method(string $param = "")
{
}
public function test()
{
$this->method();
}
}

$a = new HelpClass1;
$a->method();
17 changes: 17 additions & 0 deletions fixtures/signatureHelp/methodNotClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php

class HelpClass2
{
protected function method(string $param = "")
{
}
public function test()
{
$this->method(1,1);
}
}
$a = new HelpClass2;
$a
->method(
1,
array(),
10 changes: 10 additions & 0 deletions fixtures/signatureHelp/staticClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

class HelpClass3
{
public static function method(string $param = "")
{
}
}

HelpClass3::method()
10 changes: 10 additions & 0 deletions fixtures/signatureHelp/staticNotClosed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

class HelpClass4
{
public static function method(string $param = "")
{
}
}

HelpClass4::method(1
7 changes: 7 additions & 0 deletions src/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,11 @@ class Definition
* @var string
*/
public $documentation;

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

Choose a reason for hiding this comment

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

This should be ParameterInformation[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@felixfbecker felixfbecker Jan 21, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker the impact was not significant

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker anything I can help with here?

*/
public $parameters;
}
13 changes: 13 additions & 0 deletions src/DefinitionResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use PhpParser\PrettyPrinter\Standard as PrettyPrinter;
use phpDocumentor\Reflection\{Types, Type, Fqsen, TypeResolver};
use LanguageServer\Protocol\SymbolInformation;
use LanguageServer\Protocol\ParameterInformation;
use LanguageServer\Index\ReadableIndex;

class DefinitionResolver
Expand Down Expand Up @@ -131,6 +132,18 @@ public function createDefinitionFromNode(Node $node, string $fqn = null): Defini
$def->type = $this->getTypeFromNode($node);
$def->declarationLine = $this->getDeclarationLineFromNode($node);
$def->documentation = $this->getDocumentationFromNode($node);
$def->parameters = [];
if ($node instanceof Node\FunctionLike) {
foreach ($node->getParams() as $param) {
if (!$param->getAttribute('parentNode')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Owner

Choose a reason for hiding this comment

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

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

$param->setAttribute('parentNode', $node);
}
$def->parameters[] = new ParameterInformation(
$this->prettyPrinter->prettyPrint([$param]),
$this->getDocumentationFromNode($param)
);
}
}
return $def;
}

Expand Down
6 changes: 5 additions & 1 deletion src/LanguageServer.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
InitializeResult,
SymbolInformation,
TextDocumentIdentifier,
CompletionOptions
CompletionOptions,
SignatureHelpOptions
};
use LanguageServer\FilesFinder\{FilesFinder, ClientFilesFinder, FileSystemFilesFinder};
use LanguageServer\ContentRetriever\{ContentRetriever, ClientContentRetriever, FileSystemContentRetriever};
Expand Down Expand Up @@ -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 = ['(',','];
// Support global references
$serverCapabilities->xworkspaceReferencesProvider = true;
$serverCapabilities->xdefinitionProvider = true;
Expand Down
10 changes: 10 additions & 0 deletions src/Protocol/ParameterInformation.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,14 @@ class ParameterInformation
* @var string|null
*/
public $documentation;

/**
* @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.
*/
public function __construct(string $label = null, string $documentation = null)
{
$this->label = $label;
$this->documentation = $documentation;
}
}
12 changes: 12 additions & 0 deletions src/Protocol/SignatureHelp.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,16 @@ class SignatureHelp
* @var int|null
*/
public $activeParameter;

/**
* @param SignatureInformation[] $signatures The signatures.
* @param int|null $activeSignature The active signature.
* @param int|null $activeParameter The active parameter of the active signature.
*/
public function __construct(array $signatures = [], int $activeSignature = null, int $activeParameter = null)
{
$this->signatures = $signatures;
$this->activeSignature = $activeSignature;
$this->activeParameter = $activeParameter;
}
}
12 changes: 12 additions & 0 deletions src/Protocol/SignatureInformation.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,16 @@ class SignatureInformation
* @var ParameterInformation[]|null
*/
public $parameters;

/**
* @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 = [])
Copy link
Owner

Choose a reason for hiding this comment

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

Use null as default for $parameters

Copy link
Owner

Choose a reason for hiding this comment

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

default value for $parameters must be null

{
$this->label = $label;
$this->documentation = $documentation;
$this->parameters = $parameters;
}
}
16 changes: 15 additions & 1 deletion src/Server/TextDocument.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

use PhpParser\PrettyPrinter\Standard as PrettyPrinter;
use PhpParser\{Node, NodeTraverser};
use LanguageServer\{LanguageClient, PhpDocumentLoader, PhpDocument, DefinitionResolver, CompletionProvider};
use LanguageServer\{LanguageClient, PhpDocumentLoader, PhpDocument, DefinitionResolver, CompletionProvider, SignatureHelpProvider};
use LanguageServer\NodeVisitor\VariableReferencesCollector;
use LanguageServer\Protocol\{
SymbolLocationInformation,
Expand Down Expand Up @@ -63,6 +63,11 @@ class TextDocument
*/
protected $completionProvider;

/**
* @var SignatureHelpProvider
*/
protected $signatureHelpProvider;

/**
* @var ReadableIndex
*/
Expand Down Expand Up @@ -99,6 +104,7 @@ public function __construct(
$this->prettyPrinter = new PrettyPrinter();
$this->definitionResolver = $definitionResolver;
$this->completionProvider = new CompletionProvider($this->definitionResolver, $index);
$this->signatureHelpProvider = new SignatureHelpProvider($this->definitionResolver, $index);
$this->index = $index;
$this->composerJson = $composerJson;
$this->composerLock = $composerLock;
Expand Down Expand Up @@ -343,6 +349,14 @@ public function completion(TextDocumentIdentifier $textDocument, Position $posit
});
}

public function signatureHelp(TextDocumentIdentifier $textDocument, Position $position): Promise
{
return coroutine(function () use ($textDocument, $position) {
$document = yield $this->documentLoader->getOrLoad($textDocument->uri);
return $this->signatureHelpProvider->provideSignature($document, $position);
});
}

/**
* This method is the same as textDocument/definition, except that
*
Expand Down
Loading