-
Notifications
You must be signed in to change notification settings - Fork 185
Signature help #438
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
Signature help #438
Changes from all commits
95a82dc
5c827d2
b2056c1
1c714f9
f84b6a4
7b54ecd
7d64f06
b5bfaa9
2f9db83
970962d
cc28b47
10efca9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<?php | ||
|
||
function helpFunc1(int $count = 0) | ||
{ | ||
} | ||
|
||
helpFunc1() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<?php | ||
|
||
function helpFunc2(int $count = 0) | ||
{ | ||
} | ||
|
||
helpFunc2( |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
|
||
class HelpClass5 | ||
{ | ||
public function method(string $param = "", int $count = 0, bool $test = null) | ||
{ | ||
} | ||
public function test() | ||
{ | ||
$this->method(); | ||
} | ||
} | ||
|
||
$a = new HelpClass5; | ||
$a->method("asdf", 123, true); |
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(); |
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(), |
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() |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
<?php | ||
declare(strict_types = 1); | ||
|
||
namespace LanguageServer; | ||
|
||
use Microsoft\PhpParser\Node\DelimitedList\ArgumentExpressionList; | ||
use Microsoft\PhpParser\Node\Expression\CallExpression; | ||
use Microsoft\PhpParser\Node\Expression\ArgumentExpression; | ||
use LanguageServer\Index\ReadableIndex; | ||
use LanguageServer\Protocol\{ | ||
Range, | ||
Position, | ||
SignatureHelp, | ||
SignatureInformation, | ||
ParameterInformation | ||
}; | ||
|
||
class SignatureHelpProvider | ||
{ | ||
/** | ||
* @var DefinitionResolver | ||
*/ | ||
private $definitionResolver; | ||
|
||
/** | ||
* @var ReadableIndex | ||
*/ | ||
private $index; | ||
|
||
/** | ||
* @param DefinitionResolver $definitionResolver | ||
* @param ReadableIndex $index | ||
*/ | ||
public function __construct(DefinitionResolver $definitionResolver, ReadableIndex $index) | ||
{ | ||
$this->definitionResolver = $definitionResolver; | ||
$this->index = $index; | ||
} | ||
|
||
/** | ||
* Get the short declaration for a callable (class modifiers, function keyword, etc are stripped) | ||
* | ||
* @param string $declaration | ||
* @return string | ||
*/ | ||
protected function getShortDeclaration(string $declaration): string | ||
{ | ||
$parts = explode('(', $declaration, 2); | ||
$name = array_reverse(explode(' ', trim($parts[0])))[0]; | ||
return $name . '(' . $parts[1]; | ||
} | ||
|
||
/** | ||
* Returns signature help for a specific cursor position in a document | ||
* | ||
* @param PhpDocument $doc The opened document | ||
* @param Position $pos The cursor position | ||
* @return SignatureHelp | ||
*/ | ||
public function provideSignature(PhpDocument $doc, Position $pos) : SignatureHelp | ||
{ | ||
$node = $doc->getNodeAtPosition($pos); | ||
$arge = null; | ||
while ($node && | ||
!($node instanceof ArgumentExpressionList) && | ||
!($node instanceof CallExpression) && | ||
$node->parent | ||
) { | ||
if ($node instanceof ArgumentExpression) { | ||
$arge = $node; | ||
} | ||
$node = $node->parent; | ||
} | ||
if (!($node instanceof ArgumentExpressionList) && | ||
!($node instanceof CallExpression) | ||
) { | ||
return new SignatureHelp; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A signature without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixfbecker please provide further information on how I can help improve this? I am still not sure what the issue is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I confused it with the SignatureInformation class |
||
} | ||
$count = null; | ||
if ($node instanceof ArgumentExpressionList) { | ||
$count = 0; | ||
foreach ($node->getElements() as $param) { | ||
if ($param === $arge) { | ||
break; | ||
} | ||
$count ++; | ||
} | ||
while ($node && !($node instanceof CallExpression) && $node->parent) { | ||
$node = $node->parent; | ||
} | ||
if (!($node instanceof CallExpression)) { | ||
return new SignatureHelp; | ||
} | ||
} | ||
$def = $this->definitionResolver->resolveReferenceNodeToDefinition($node->callableExpression); | ||
if (!$def) { | ||
return new SignatureHelp; | ||
} | ||
return new SignatureHelp( | ||
[ | ||
new SignatureInformation( | ||
$this->getShortDeclaration($def->declarationLine), | ||
$def->documentation, | ||
$def->parameters | ||
) | ||
], | ||
0, | ||
$count !== null && $def->parameters !== null && $count < count($def->parameters) ? $count : null | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method looks like it would ignore many edge cases, for example
(
or spaces inside a default string value.Why attempt to explode a declaration string when we have a parser available? I would add the wanted output to the
Definition
classThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am absolutely certain that this code does not ignore any edge cases - the first
(
in a declaration is always after the function name. The function name is always the last token after all modifiers.I am not sure how to change the current implementation in a robust way - all I need is to remove the modifiers and leave the function name, but keep all parameters (along with their type and default values). Doing this using the parser would require quite a huge block of code (from what I gather) especially since there is no way to pretty print the result.
Basically I would be reconstructing the already available declaration using string concatenation by inspecting the underlying AST just to remove the public / private / abstract / static / function keywords that precede the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, maybe I'm was just unable to follow this code.
(
(function name and keywords) and the rest(
and the restI am pretty sure
end()
is faster to get the last array element than reversing the whole array and taking the first elementThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end
requires a reference, which would mean creating a middle step and allocating a separate variable for the array. I am not very familiar with PHP internals, but I believe this means allocating storage in the heap, instead of the stack, so as far as optimization goes - I am not sure which will be faster and more memory efficient. Anyway - this really is a micro-optimization. I will create a few tests and change it if necessary.EDIT: For what it is worth - I created a very crude test and at least for my configuration using a temporary array and
end
is on average about 3% slower.