Signature help #438

vakata · 2017-07-14T23:26:35Z

Initial idea to start the discussion (still needs work)

vakata · 2017-07-14T23:38:18Z

Currently this PR is not ready for merging - I want to get some opinions on the approach and fix as necessary.

There is one major problem I noticed so far - the currently active parameter is not properly determined (as soon as something is written it shifts one parameter to the right) - I would like some help with that if someone can spare the time and have a look, as I am not very familiar with tolerant parser's objects.
UPDATE: I managed to improve the situation a bit, please review.

One thing I definitely noticed - this implementation is way cleaner than the one with PhpParser.

codecov · 2017-07-14T23:43:52Z

Codecov Report

Merging #438 into master will increase coverage by 0.57%.
The diff coverage is 95.83%.

@@             Coverage Diff              @@
##             master     #438      +/-   ##
============================================
+ Coverage     79.38%   79.96%   +0.57%     
- Complexity      837      867      +30     
============================================
  Files            56       60       +4     
  Lines          1979     2051      +72     
============================================
+ Hits           1571     1640      +69     
- Misses          408      411       +3

Impacted Files	Coverage Δ	Complexity Δ
src/Definition.php	`100% <ø> (ø)`	`6 <0> (ø)`	⬇️
src/Server/TextDocument.php	`75.91% <100%> (+0.91%)`	`56 <1> (+1)`	⬆️
src/Protocol/SignatureInformation.php	`100% <100%> (ø)`	`1 <1> (?)`
src/Protocol/SignatureHelp.php	`100% <100%> (ø)`	`1 <1> (?)`
src/DefinitionResolver.php	`84.31% <100%> (+0.26%)`	`307 <0> (+5)`	⬆️
src/LanguageServer.php	`77.87% <100%> (+0.39%)`	`27 <0> (ø)`	⬇️
src/Protocol/ParameterInformation.php	`100% <100%> (ø)`	`1 <1> (?)`
src/SignatureHelpProvider.php	`92.85% <92.85%> (ø)`	`21 <21> (?)`
... and 2 more

felixfbecker

This looks very good and is what I had in mind for the implementation (no use of lexer/tokenizer).

felixfbecker · 2017-07-15T08:34:47Z

src/DefinitionResolver.php

@@ -234,6 +235,18 @@ public function createDefinitionFromNode(Node $node, string $fqn = null): Defini
            $def->documentation = $this->getDocumentationFromNode($node);
        }

+        $def->parameters = [];
+        if (property_exists($node, 'parameters') && $node->parameters) {


can you do this without reflection? i.e. instanceof

I will need @roblourens help for this - I have no idea what to check for. From what I gather it is a trait.

@roblourens could you add an interface for FunctionHeader or callables so this can be checked for with instanceof?

felixfbecker · 2017-07-15T08:37:49Z

src/SignatureHelpProvider.php

+        if (!($node instanceof ArgumentExpressionList) &&
+            !($node instanceof CallExpression)
+        ) {
+            return new SignatureHelp;


A signature without label is not valid according to the docblock

There is no label property in the SignatureHelp object - maybe I am missin something - please explain? This is supposed to be an empty result (no nested SignatureHelpInformation objects).

@felixfbecker please provide further information on how I can help improve this? I am still not sure what the issue is.

Sorry, I confused it with the SignatureInformation class

felixfbecker · 2017-07-15T08:40:36Z

src/SignatureHelpProvider.php

+        return new SignatureHelp(
+            [
+                new SignatureInformation(
+                    trim(str_replace(['public', 'protected', 'private', 'function', 'static'], '', $def->declarationLine)),


Don't like the string manipulation here, you could have a parameter $function. I would rather extend Definition with another property or method (if possible, without saving new duplicate data to save RAM)

felixfbecker · 2017-07-15T08:41:47Z

src/Definition.php

@@ -97,6 +98,12 @@ class Definition
     * @var string
     */
    public $documentation;
+    /**


add an empty line above this

felixfbecker · 2017-07-15T08:41:59Z

src/DefinitionResolver.php

+                //var_dump($param); die();
+                $def->parameters[] = new ParameterInformation(
+                    $this->getDeclarationLineFromNode($param),
+                    //$param->getName(), // TODO: rebuild this


what does this mean?

I was old code - I am cleaning it up.

felixfbecker · 2017-07-15T08:42:38Z

src/Server/TextDocument.php

@@ -411,4 +416,12 @@ public function xdefinition(TextDocumentIdentifier $textDocument, Position $posi
            return [new SymbolLocationInformation($descriptor, $def->symbolInformation->location)];
        });
    }
+
+    public function signatureHelp(TextDocumentIdentifier $textDocument, Position $position): Promise


Please add a docblock for this method with the description from the protocol spec (makes it easier for newcomers to understand the purpose of the method)

felixfbecker · 2017-07-15T08:44:53Z

Maybe @roblourens can help with the parser specifics, he knows more about it than me. Maybe the parameters array gets a MissingToken so the count gets confused?

vakata · 2017-07-15T15:26:39Z

The count should be OK now, but I will need @roblourens help with the property_exists issue.

vakata · 2017-07-15T19:11:43Z

I am afraid the failing build is not my fault - please advise on how to proceed?
The error is in the parse-stubs script (while parsing ...phpstorm-stubs/SplType/SplType.php):

Class 'phpDocumentor\Reflection\Types\Mixed' not found

I do not get this error locally.

felixfbecker · 2017-07-15T20:47:17Z

weird

roblourens · 2017-07-17T02:23:03Z

I'm not sure what you mean by "the property_exists issue". Your change from the last commit seems good to me - I think it's better to explicitly check the type with instanceof instead of assuming that any node with parameters is the type you want.

felixfbecker · 2017-07-19T11:08:46Z

@roblourens it would be nice if checking for a callable, function-like node only required a single instanceof check for an interface instead of for every possible concrete implementation. PHPParser had a FunctionLike interface for this. Given TolerantParser uses a trait for to introduce the parameters property, it would make sense to have an interface that the classes that use the trait implement

vakata · 2017-07-19T11:46:19Z

I've been using this branch for a few days now. Issues I've noticed so far:

rarely the parameters can not be counted and there is no active parameter highlighted (but I have not been able to reproduce this in a test so far)
the position is sometimes off array_reverse(|explode( gives signature help for explode
sometimes there is a minimal delay when moving the cursor fast

I will look into the second issue - I will create a test and try to fix it.

It would be great if someone else also tried this branch, just so that we can gather opinions (although subjective) about performance - I am using a Surface Pro 4 (i5, 8GB) and experience no issues, but it may be different for someone else.

roblourens · 2017-07-19T17:51:54Z

I do see some relevant helpers in ParserHelpers.php, isFunctionLike does the same check. Maybe that should be in the parser.

felixfbecker · 2017-07-19T18:38:07Z

I think an interface would be more useful for static analysis if we ever implement type narrowing through instanceof like TS does (#168)

felixfbecker · 2017-07-28T12:09:39Z

@vakata do you feel this is stable enough to release?

felixfbecker · 2017-07-28T12:14:51Z

src/SignatureHelpProvider.php

+    {
+        $parts = explode('(', $declaration, 2);
+        $name = array_reverse(explode(' ', trim($parts[0])))[0];
+        return $name . '(' . $parts[1];


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 class

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.

Okay, maybe I'm was just unable to follow this code.

you split the declaration into two parts, the part before the ( (function name and keywords) and the rest

you trim whitespace from the first part (function name and keywords)

you split the first part by (function name and keywords) by spaces

you reverse the whole array

you take the first (initially last) element (function name)

you concatenate the function name, ( and the rest

I am pretty sure end() is faster to get the last array element than reversing the whole array and taking the first element

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.

vakata · 2017-07-28T12:52:34Z

@felixfbecker as I mentioned in my previous comment:

There is a known issue where the position is not calculated correctly. One could argue that any signature help (even with a few edge cases not handled) is better than no signature help. Personally I think the issue should be resolved before merging, but it is ultimately your call. I will have some time to work on this issue next week.
Performance-wise: I use this branch on a daily basis and have not experienced any issues, but I would be a lot more comfortable if someone else also tested and shared their experience.

felixfbecker · 2017-07-28T12:58:59Z

I don't work on any PHP project currently so I can't do any real usage testing. I don't think a lot of people will try it out by cloning this branch, you could build a .vsix though. Otherwise I would just release it and let people try it out.

vakata · 2017-07-28T13:15:55Z

If you feel the issue I described above is minor enough I am OK with publishing. I am not sure many people would download the .vsix anyway. If this PR is merged I will still try to fix the issue next week in a separate PR.

jens1o · 2017-08-07T21:26:42Z

well, if somebody builds me such a file or lets me know how I do that, I'm happy to test it. :)

felixfbecker · 2017-08-22T05:43:51Z

@vakata any update?

felixfbecker · 2017-10-19T21:23:43Z

@vakata are you still working on this?

vakata · 2017-10-20T06:22:42Z

@felixfbecker no, I haven't had the time to do any further work. Actually the only thing that was left was the occasional slip up in position calculation, but I doubt I will have time any time soon to work on this.

jens1o · 2017-11-15T18:53:11Z

Could I do something to help you? @vakata

vakata · 2017-11-20T07:47:11Z

@felixfbecker @jens1o sorry I was not around sooner. Currently the only issue I have with this PR is the occasional position miscalculation, but I doubt I will have time to work on it any time soon. I guess the most helpful thing would be for someone else beside me to test this change and share if all worked fine.

felixfbecker · 2017-12-10T05:15:03Z

Closing in favor of #547

vakata added 3 commits July 15, 2017 02:12

initial signature help commit

95a82dc

fixed crashes

5c827d2

updated tests

b2056c1

vakata added 2 commits July 15, 2017 03:07

improved param counting

1c714f9

optimized param count calculation

f84b6a4

felixfbecker requested changes Jul 15, 2017

View reviewed changes

felixfbecker added the feature label Jul 15, 2017

updated docs and comments

7b54ecd

vakata added 2 commits July 15, 2017 21:44

moved short declaration extraction to a separate function

7d64f06

switched from property_exists to instanceof

b5bfaa9

felixfbecker approved these changes Jul 19, 2017

View reviewed changes

Merge branch 'master' into signature-help

2f9db83

roblourens mentioned this pull request Jul 19, 2017

Implement a FunctionLike interface microsoft/tolerant-php-parser#162

Closed

felixfbecker mentioned this pull request Jul 26, 2017

Poorer intellisense than default? #444

Closed

felixfbecker reviewed Jul 28, 2017

View reviewed changes

Merge branch 'master' into signature-help

970962d

DeltaEvo mentioned this pull request Aug 22, 2017

Unusual completion description autozimu/LanguageClient-neovim#105

Closed

Merge branch 'master' into signature-help

cc28b47

Merge branch 'master' into signature-help

10efca9

phil-nelson mentioned this pull request Dec 4, 2017

Signature help #547

Merged

felixfbecker closed this Dec 10, 2017

Signature help #438

Signature help #438

Conversation

vakata commented Jul 14, 2017

vakata commented Jul 14, 2017 • edited Loading

codecov bot commented Jul 14, 2017 • edited by ghost Loading

Codecov Report

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

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

felixfbecker commented Jul 15, 2017

vakata commented Jul 15, 2017

vakata commented Jul 15, 2017

felixfbecker commented Jul 15, 2017

roblourens commented Jul 17, 2017

felixfbecker commented Jul 19, 2017

vakata commented Jul 19, 2017 • edited Loading

roblourens commented Jul 19, 2017 • edited Loading

felixfbecker commented Jul 19, 2017

felixfbecker commented Jul 28, 2017

Choose a reason for hiding this comment

vakata Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vakata Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

vakata commented Jul 28, 2017 • edited Loading

felixfbecker commented Jul 28, 2017

vakata commented Jul 28, 2017 • edited Loading

jens1o commented Aug 7, 2017

felixfbecker commented Aug 22, 2017

felixfbecker commented Oct 19, 2017

vakata commented Oct 20, 2017

jens1o commented Nov 15, 2017

vakata commented Nov 20, 2017

felixfbecker commented Dec 10, 2017

vakata commented Jul 14, 2017 •

edited

Loading

codecov bot commented Jul 14, 2017 •

edited by ghost

Loading

vakata commented Jul 19, 2017 •

edited

Loading

roblourens commented Jul 19, 2017 •

edited

Loading

vakata Jul 28, 2017 •

edited

Loading

vakata Jul 28, 2017 •

edited

Loading

vakata commented Jul 28, 2017 •

edited

Loading

vakata commented Jul 28, 2017 •

edited

Loading