Completion #165

Merged
merged 27 commits into from Nov 30, 2016

Conversation

4 participants
@felixfbecker
Owner

felixfbecker commented Nov 19, 2016

Closes #9 馃挭
Big ups to @nikic for making this possible with the improvements in error recovery in v3!

animation

Works for properties and methods with all of the recursive type goodness of #54

Todo:

  • variables
  • keywords
  • classes
  • namespaces
  • constants
  • differentiate between static and instance properties/methods

Testing

Two things are preventing a nice testing experience:

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 19, 2016

Current coverage is 95.02% (diff: 96.00%)

Merging #165 into master will increase coverage by 0.31%

@@             master       #165   diff @@
==========================================
  Files            31         34     +3   
  Lines           416        583   +167   
  Methods          67         75     +8   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            394        554   +160   
- Misses           22         29     +7   
  Partials          0          0          
Diff Coverage File Path
鈥⑩⑩⑩⑩⑩⑩⑩⑩ 94% new src/CompletionProvider.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/NodeVisitor/DefinitionCollector.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/CompletionList.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/SymbolInformation.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/Position.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/CompletionItem.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/utils.php

Powered by Codecov. Last update 5077b1a...cc8365d

codecov-io commented Nov 19, 2016

Current coverage is 95.02% (diff: 96.00%)

Merging #165 into master will increase coverage by 0.31%

@@             master       #165   diff @@
==========================================
  Files            31         34     +3   
  Lines           416        583   +167   
  Methods          67         75     +8   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            394        554   +160   
- Misses           22         29     +7   
  Partials          0          0          
Diff Coverage File Path
鈥⑩⑩⑩⑩⑩⑩⑩⑩ 94% new src/CompletionProvider.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/NodeVisitor/DefinitionCollector.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/CompletionList.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/SymbolInformation.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/Position.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/Protocol/CompletionItem.php
鈥⑩⑩⑩⑩⑩⑩⑩⑩⑩ 100% src/utils.php

Powered by Codecov. Last update 5077b1a...cc8365d

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 24, 2016

Contributor

Hi, I'm trying to play with this PR but I cannot make it working. I'm using code from gif. Is there anything specific I need to do to test it?

Contributor

mniewrzal commented Nov 24, 2016

Hi, I'm trying to play with this PR but I cannot make it working. I'm using code from gif. Is there anything specific I need to do to test it?

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 24, 2016

Owner

Are trying with VS Code? You need to manually patch your VS Code installation to fix the bugs in VS Code
Besides, this is WIP, maybe latest commit is broken. Are you getting any stack trace?

Owner

felixfbecker commented Nov 24, 2016

Are trying with VS Code? You need to manually patch your VS Code installation to fix the bugs in VS Code
Besides, this is WIP, maybe latest commit is broken. Are you getting any stack trace?

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 24, 2016

Contributor

Yes, I'm using vscode 1.7.2, but without patching. Only stack trace is for $a->':

shell.ts:440 Cannot read property 'range' of null: TypeError: Cannot read property 'range' of null
    at asTextEdit (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:100:46)
    at /home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:95:67
    at set (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:83:13)
    at asCompletionItem (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:95:9)
    at Array.map (native)
    at asCompletionResult (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:76:26)
    at process._tickCallback (internal/process/next_tick.js:103:7)

If you feel its to early to test it let me know, I can wait.

Contributor

mniewrzal commented Nov 24, 2016

Yes, I'm using vscode 1.7.2, but without patching. Only stack trace is for $a->':

shell.ts:440 Cannot read property 'range' of null: TypeError: Cannot read property 'range' of null
    at asTextEdit (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:100:46)
    at /home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:95:67
    at set (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:83:13)
    at asCompletionItem (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:95:9)
    at Array.map (native)
    at asCompletionResult (/home/wywrzal/git/vscode-php-intellisense/node_modules/vscode-languageclient/lib/protocolConverter.js:76:26)
    at process._tickCallback (internal/process/next_tick.js:103:7)

If you feel its to early to test it let me know, I can wait.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 24, 2016

Owner

That is the described bug in VS Code. I have proposed a fix here: https://github.com/Microsoft/vscode-languageserver-node/pull/128/files

You need to go into ~/.vscode/extensions/felixfbecker.vscode-php-intellisense-x.x.x/node_modules/vscode-languageclient/out/protocolConverter.js and patch it there, then it works fine.

Owner

felixfbecker commented Nov 24, 2016

That is the described bug in VS Code. I have proposed a fix here: https://github.com/Microsoft/vscode-languageserver-node/pull/128/files

You need to go into ~/.vscode/extensions/felixfbecker.vscode-php-intellisense-x.x.x/node_modules/vscode-languageclient/out/protocolConverter.js and patch it there, then it works fine.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 24, 2016

Contributor

Ok, I will give it a try.

Contributor

mniewrzal commented Nov 24, 2016

Ok, I will give it a try.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 24, 2016

Contributor

With patch completion works as described :) Thanks!

Contributor

mniewrzal commented Nov 24, 2016

With patch completion works as described :) Thanks!

felixfbecker added some commits Nov 24, 2016

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 25, 2016

Owner

I think this is ready. Feedback? :)

Owner

felixfbecker commented Nov 25, 2016

I think this is ready. Feedback? :)

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 26, 2016

Contributor
Contributor

kaloyan-raev commented Nov 26, 2016

- 'completionProvider' => null,
+ 'completionProvider' => (object)[
+ 'resolveProvider' => false,
+ 'triggerCharacters' => ['$', '>']

This comment has been minimized.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

'>' should be '->', isn't it? I think we also need to add '::' here.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

'>' should be '->', isn't it? I think we also need to add '::' here.

This comment has been minimized.

@felixfbecker

felixfbecker Nov 28, 2016

Owner

Only characters are allowed in the array unfortunately. Using -> will trigger the completion on - instead, as VS Code only looks at the first char. We should open an issue at LSP to support strings, this is a pretty big limitation for PHP and C++. : should be added

@felixfbecker

felixfbecker Nov 28, 2016

Owner

Only characters are allowed in the array unfortunately. Using -> will trigger the completion on - instead, as VS Code only looks at the first char. We should open an issue at LSP to support strings, this is a pretty big limitation for PHP and C++. : should be added

This comment has been minimized.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

I don't see such limitation in the protocol. The type of triggerCharacters is string[], which implicitly means "0 or more characters" for each array item. So, it should be a bug in the VSCode if it looks only at the first char.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

I don't see such limitation in the protocol. The type of triggerCharacters is string[], which implicitly means "0 or more characters" for each array item. So, it should be a bug in the VSCode if it looks only at the first char.

This comment has been minimized.

@felixfbecker

felixfbecker Nov 28, 2016

Owner

That is because JSON does not have a "character" type, only a string type. But the property name clearly says trigger Characters, so if trigger sequences are supported, it should be clarified in the protocol documentation.

@felixfbecker

felixfbecker Nov 28, 2016

Owner

That is because JSON does not have a "character" type, only a string type. But the property name clearly says trigger Characters, so if trigger sequences are supported, it should be clarified in the protocol documentation.

This comment has been minimized.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Obviously it can be read in different ways. So a clarification in the protocol would be helpful.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Obviously it can be read in different ways. So a clarification in the protocol would be helpful.

src/CompletionProvider.php
+ $items[] = $item;
+ }
+
+ return $items;

This comment has been minimized.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Please return CompletionList instead of CompletionItem[]. Due to Microsoft/language-server-protocol#39 the Java binding supports only CompletionList.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Please return CompletionList instead of CompletionItem[]. Due to Microsoft/language-server-protocol#39 the Java binding supports only CompletionList.

This comment has been minimized.

@felixfbecker

felixfbecker Nov 28, 2016

Owner

Okay, no problem. Is this the case for other places in the protocol too? How do you handle MarkedString | string? Pretty big limitation of the Java bindings if you ask me... Couldn't this be solved with polymorphism/generics?

@felixfbecker

felixfbecker Nov 28, 2016

Owner

Okay, no problem. Is this the case for other places in the protocol too? How do you handle MarkedString | string? Pretty big limitation of the Java bindings if you ask me... Couldn't this be solved with polymorphism/generics?

This comment has been minimized.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

I haven't noticed any issue with the hovering feature.

You can see the arguments of the people behind the Java binding in the protocol's issue. I haven't dug into it. It seems there is some soft of agreement that the protocol will avoid mixed return types in future.

@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

I haven't noticed any issue with the hovering feature.

You can see the arguments of the people behind the Java binding in the protocol's issue. I haven't dug into it. It seems there is some soft of agreement that the protocol will avoid mixed return types in future.

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

After fixing locally the issue with the CompletionList return type, I am able to test it with Eclipse Che, but I don't get the expected completion result on lots of places. Some examples:

Contributor

kaloyan-raev commented Nov 28, 2016

After fixing locally the issue with the CompletionList return type, I am able to test it with Eclipse Che, but I don't get the expected completion result on lots of places. Some examples:

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

Please see the tests for what is supported & tested: https://github.com/felixfbecker/php-language-server/tree/completion/fixtures/completion

I get nothing here: https://github.com/felixfbecker/php-language-server/blob/master/bin/php-language-server.php#L7. In VSCode I see some results, but they don't come from the language server

Might be because it's in global scope, there is only a test for function scope.

Here: https://github.com/felixfbecker/php-language-server/blob/master/src/ClientHandler.php#L28, after "this->" I get only one proposal: "protocolReader". I should see also all properties and methods. I have no results if I just have "this->" on the line alone...

If you request completion at the PropertyFetch node, it will be filtered by the property name that is already typed. So if protocolReader is already there, it will be the only suggestion.

I have no results if I just have "this->" on the line alone...

That might be the same issue as #171... The type is resolved to Types\This, but that of course needs to be resolved to the containing class. Maybe that's not done.

I get the available variables only if I have typed "$". They should be included in the result even if I havent typed "$" yet.

Yes, variable suggestions are only provided when the node under the cursor is a Variable node.

Please note that a lot of features are not related to the completion support, but rather to the definition resolver (which means it also affects go-to-def and find-all-references).
I would be interested in whether the completion scenarios that are unit-tested (see fixtures/completion) work in Eclipse Che?

Owner

felixfbecker commented Nov 28, 2016

Please see the tests for what is supported & tested: https://github.com/felixfbecker/php-language-server/tree/completion/fixtures/completion

I get nothing here: https://github.com/felixfbecker/php-language-server/blob/master/bin/php-language-server.php#L7. In VSCode I see some results, but they don't come from the language server

Might be because it's in global scope, there is only a test for function scope.

Here: https://github.com/felixfbecker/php-language-server/blob/master/src/ClientHandler.php#L28, after "this->" I get only one proposal: "protocolReader". I should see also all properties and methods. I have no results if I just have "this->" on the line alone...

If you request completion at the PropertyFetch node, it will be filtered by the property name that is already typed. So if protocolReader is already there, it will be the only suggestion.

I have no results if I just have "this->" on the line alone...

That might be the same issue as #171... The type is resolved to Types\This, but that of course needs to be resolved to the containing class. Maybe that's not done.

I get the available variables only if I have typed "$". They should be included in the result even if I havent typed "$" yet.

Yes, variable suggestions are only provided when the node under the cursor is a Variable node.

Please note that a lot of features are not related to the completion support, but rather to the definition resolver (which means it also affects go-to-def and find-all-references).
I would be interested in whether the completion scenarios that are unit-tested (see fixtures/completion) work in Eclipse Che?

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Another issue is that most of the time I don't see the insertText or the textEdit calculated properly.

Example 1. Type "ec" in new PHP file with only <?php tag. If I invoke code completion, I get only one proposal for "echo" with insertText = "echo ". The correct insertText in this case should be "ho ".

Example 2. Open https://github.com/felixfbecker/php-language-server/blob/master/bin/php-language-server.php#L6, place cursor right after "$op" and invoke code completion. I get a single result "$options" with

textEdit: { 
  "newText": "tions", 
  "range": {
    "start": {
      "line": 5,
      "character": 3,
    },
    "end": {
      "line": 5,
      "character": 3,
    }
  }
}

Leaving aside that "$options" should not be proposed here, the newText is correct, while the end of the range should not be equal to the start of the range. It should be correctly calculated to replace the current variable.

Note that the above examples have no issue with VSCode, because VSCode has a very sophisticated logic on the client side for applying completion proposals. Such logic is not described in LSP and my guess is that VSCode has it before the era of LSP. However, we should not expect other clients to have similiar logic implemented.

We should strive to have a textEdit with correct newText and range implemented for all CompletionItems returns by the language service. This would guarantee the best integration for all clients.

Contributor

kaloyan-raev commented Nov 28, 2016

Another issue is that most of the time I don't see the insertText or the textEdit calculated properly.

Example 1. Type "ec" in new PHP file with only <?php tag. If I invoke code completion, I get only one proposal for "echo" with insertText = "echo ". The correct insertText in this case should be "ho ".

Example 2. Open https://github.com/felixfbecker/php-language-server/blob/master/bin/php-language-server.php#L6, place cursor right after "$op" and invoke code completion. I get a single result "$options" with

textEdit: { 
  "newText": "tions", 
  "range": {
    "start": {
      "line": 5,
      "character": 3,
    },
    "end": {
      "line": 5,
      "character": 3,
    }
  }
}

Leaving aside that "$options" should not be proposed here, the newText is correct, while the end of the range should not be equal to the start of the range. It should be correctly calculated to replace the current variable.

Note that the above examples have no issue with VSCode, because VSCode has a very sophisticated logic on the client side for applying completion proposals. Such logic is not described in LSP and my guess is that VSCode has it before the era of LSP. However, we should not expect other clients to have similiar logic implemented.

We should strive to have a textEdit with correct newText and range implemented for all CompletionItems returns by the language service. This would guarantee the best integration for all clients.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

Yes, that would be optimal. I started with just labels and moved some stuff already to using textEdit, with looking at the text before the cursor. Actually that logic should use the nodes more though and less the text content. I do not yet look at the text after the cursor, because that is not the common use case (TypeScript doesn't do this either).

Owner

felixfbecker commented Nov 28, 2016

Yes, that would be optimal. I started with just labels and moved some stuff already to using textEdit, with looking at the text before the cursor. Actually that logic should use the nodes more though and less the text content. I do not yet look at the text after the cursor, because that is not the common use case (TypeScript doesn't do this either).

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

I would be interested in whether the completion scenarios that are unit-tested (see fixtures/completion) work in Eclipse Che?

Sure. I will go over the unit tests and try them in Che. Would you accept a PR to this "completion" branch with corrections to the existing tests?

Contributor

kaloyan-raev commented Nov 28, 2016

I would be interested in whether the completion scenarios that are unit-tested (see fixtures/completion) work in Eclipse Che?

Sure. I will go over the unit tests and try them in Che. Would you accept a PR to this "completion" branch with corrections to the existing tests?

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

Sure. I will go over the unit tests and try them in Che. Would you accept a PR to this "completion" branch with correction to the existing tests?

Of course. I don't know when I will have the time to do the other improvements so otherwise I was thinking about merging as-is and doing improvements later. But I can resolve the small things like returning CompletionList first.

Owner

felixfbecker commented Nov 28, 2016

Sure. I will go over the unit tests and try them in Che. Would you accept a PR to this "completion" branch with correction to the existing tests?

Of course. I don't know when I will have the time to do the other improvements so otherwise I was thinking about merging as-is and doing improvements later. But I can resolve the small things like returning CompletionList first.

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

OK. Then please fix the CompletionList and merge to master. We will invest time on our side to prepare PRs with the fixes for the issues I mentioned.

Contributor

kaloyan-raev commented Nov 28, 2016

OK. Then please fix the CompletionList and merge to master. We will invest time on our side to prepare PRs with the fixes for the issues I mentioned.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

The issue I see with CompletionList is that returning CompletionItem[] lets the editor decide if it should re-request the completion list after further typing. But when returning CompletionList, you either tell the client that it should definitely re-request, or definitely not.

Owner

felixfbecker commented Nov 28, 2016

The issue I see with CompletionList is that returning CompletionItem[] lets the editor decide if it should re-request the completion list after further typing. But when returning CompletionList, you either tell the client that it should definitely re-request, or definitely not.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

In VS Code, if isIncomplete is false, the editor will filter the results client-side

Owner

felixfbecker commented Nov 28, 2016

In VS Code, if isIncomplete is false, the editor will filter the results client-side

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

As far as I studied VSCode some time ago, it always filters the results client-side. The isIncomplete flag determines whether to send a Completion Request on further typing.

Contributor

kaloyan-raev commented Nov 28, 2016

As far as I studied VSCode some time ago, it always filters the results client-side. The isIncomplete flag determines whether to send a Completion Request on further typing.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

Yeah, so if the user types $ and we send back

  • $var
  • $param

when the user types an additional v, with isIncomplete: true it would send another request, while with false it would just use client side filtering on the previous result. The output would be the same, so we could save that request, right?

Owner

felixfbecker commented Nov 28, 2016

Yeah, so if the user types $ and we send back

  • $var
  • $param

when the user types an additional v, with isIncomplete: true it would send another request, while with false it would just use client side filtering on the previous result. The output would be the same, so we could save that request, right?

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

Exactly!

Contributor

kaloyan-raev commented Nov 28, 2016

Exactly!

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 28, 2016

Owner

So why do you think we should set it to true?

Owner

felixfbecker commented Nov 28, 2016

So why do you think we should set it to true?

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 28, 2016

Contributor

In the above example, if you invoke code assist on an empty line, it will return a list without the two variables (this is one of the issues I reported). Then, if you type $ and we have isIncomplete: false, the editor won't send another request to get them in the result list.

There are other issues like this. So, for now it's better to set isIncomplete: true to avoid unnecessary confusion. I also think it will be easier to find issues this way.

Contributor

kaloyan-raev commented Nov 28, 2016

In the above example, if you invoke code assist on an empty line, it will return a list without the two variables (this is one of the issues I reported). Then, if you type $ and we have isIncomplete: false, the editor won't send another request to get them in the result list.

There are other issues like this. So, for now it's better to set isIncomplete: true to avoid unnecessary confusion. I also think it will be easier to find issues this way.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 29, 2016

Owner

@mniewrzal Had to update CodeSniffer to latest master because of a bug, but now the formatter tests fail. Looks like the Config class has been removed?

Owner

felixfbecker commented Nov 29, 2016

@mniewrzal Had to update CodeSniffer to latest master because of a bug, but now the formatter tests fail. Looks like the Config class has been removed?

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 29, 2016

Contributor

Ok, I will take a look at this.

Contributor

mniewrzal commented Nov 29, 2016

Ok, I will take a look at this.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 29, 2016

Owner

Never mind, fixed it. I referenced the wrong branch in composer.json. Sorry for the ping.

Owner

felixfbecker commented Nov 29, 2016

Never mind, fixed it. I referenced the wrong branch in composer.json. Sorry for the ping.

felixfbecker added some commits Nov 29, 2016

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

I found one issue, such code:

<?php

class Test
{
    public function getAAA()
    {
    }
    public function getBBB()
    {
    }
}
$a = new Test();
$a->|getAAA();

If you will invoke completion in place of | you will see only getAAA but also getBBB should be proposed and should replace getAAA if applied.

Contributor

mniewrzal commented Nov 30, 2016

I found one issue, such code:

<?php

class Test
{
    public function getAAA()
    {
    }
    public function getBBB()
    {
    }
}
$a = new Test();
$a->|getAAA();

If you will invoke completion in place of | you will see only getAAA but also getBBB should be proposed and should replace getAAA if applied.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

Another issue:

<?php
$a->
$a->|getAAA();

Invoke completion in place of | and you should see:

[Error - 10:52:03 AM] Request textDocument/completion failed.
  Message: strlen() expects parameter 1 to be string, null given
  Code: -32603 
TypeError: strlen() expects parameter 1 to be string, null given in /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/CompletionProvider.php:164
Stack trace:
#0 /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/CompletionProvider.php(164): strlen(NULL)
#1 /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/Server/TextDocument.php(241): LanguageServer\CompletionProvider->provideCompletion(Object(LanguageServer\PhpDocument), Object(LanguageServer\Protocol\Position))
#2 [internal function]: LanguageServer\Server\TextDocument->LanguageServer\Server\{closure}()
#3 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/coroutine.php(70): Generator->send(Object(LanguageServer\PhpDocument))
#4 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/Promise.php(242): Sabre\Event\{closure}(Object(LanguageServer\PhpDocument))
#5 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/Loop/Loop.php(26
Contributor

mniewrzal commented Nov 30, 2016

Another issue:

<?php
$a->
$a->|getAAA();

Invoke completion in place of | and you should see:

[Error - 10:52:03 AM] Request textDocument/completion failed.
  Message: strlen() expects parameter 1 to be string, null given
  Code: -32603 
TypeError: strlen() expects parameter 1 to be string, null given in /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/CompletionProvider.php:164
Stack trace:
#0 /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/CompletionProvider.php(164): strlen(NULL)
#1 /home/wywrzal/git/vscode-php-intellisense/vendor/felixfbecker/language-server/src/Server/TextDocument.php(241): LanguageServer\CompletionProvider->provideCompletion(Object(LanguageServer\PhpDocument), Object(LanguageServer\Protocol\Position))
#2 [internal function]: LanguageServer\Server\TextDocument->LanguageServer\Server\{closure}()
#3 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/coroutine.php(70): Generator->send(Object(LanguageServer\PhpDocument))
#4 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/Promise.php(242): Sabre\Event\{closure}(Object(LanguageServer\PhpDocument))
#5 /home/wywrzal/git/vscode-php-intellisense/vendor/sabre/event/lib/Loop/Loop.php(26
@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

@mniewrzal can you get the full stack trace? You need to change this line to (string)$e

@kaloyan-raev I set isIncomplete to true.

Owner

felixfbecker commented Nov 30, 2016

@mniewrzal can you get the full stack trace? You need to change this line to (string)$e

@kaloyan-raev I set isIncomplete to true.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

@felixfbecker I updated issue description.

Contributor

mniewrzal commented Nov 30, 2016

@felixfbecker I updated issue description.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

Fixed, thanks!

Owner

felixfbecker commented Nov 30, 2016

Fixed, thanks!

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

Another thing that is missing (and its important imho) is completion for types. Now its hard to get completion in many places. Simple example:

<?php
interface Example {
}
class Test implements Ex| {
}

From code I see that completion is limited to closed list of nodes. I think it would be great that if node isn't from handled list (or there is problem with parser) then result list should be as big as possible (types and keywords). It will be filtered by vscode after first letters.

Contributor

mniewrzal commented Nov 30, 2016

Another thing that is missing (and its important imho) is completion for types. Now its hard to get completion in many places. Simple example:

<?php
interface Example {
}
class Test implements Ex| {
}

From code I see that completion is limited to closed list of nodes. I think it would be great that if node isn't from handled list (or there is problem with parser) then result list should be as big as possible (types and keywords). It will be filtered by vscode after first letters.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

extends / implements is a scenario that should be supported & tested. Should be quite easy with a check for Node\Name and parent for Node\Stmt\Class where currently only Node\Expr\New_ and Node\Expr\ConstFetch are accepted. We can do these improvements in follow-up PRs though.

Owner

felixfbecker commented Nov 30, 2016

extends / implements is a scenario that should be supported & tested. Should be quite easy with a check for Node\Name and parent for Node\Stmt\Class where currently only Node\Expr\New_ and Node\Expr\ConstFetch are accepted. We can do these improvements in follow-up PRs though.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

I understand that its doable but until new cases will be supported maybe it would be good approach to return 'everything' that workspace contains. It will also help with cases were parser will be not error tolerant. Of course it can be done later :)

Contributor

mniewrzal commented Nov 30, 2016

I understand that its doable but until new cases will be supported maybe it would be good approach to return 'everything' that workspace contains. It will also help with cases were parser will be not error tolerant. Of course it can be done later :)

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

Another thing that should be considered is completion filtering in server code. This PR is using substring but vscode supports filtering on client side and its 'fuzzy' filtering (e.g. in CSS bc will give border-color) which is more useful in my opinion.

In general vscode built-in language servers like CSS doesn't do filtering on server side, results list is depends on code context not on prefix. This part was left for client (at least for now).

If you feel that this is too much for this PR just merge it and I will open issues for things I mentioned in previous comments :)

Contributor

mniewrzal commented Nov 30, 2016

Another thing that should be considered is completion filtering in server code. This PR is using substring but vscode supports filtering on client side and its 'fuzzy' filtering (e.g. in CSS bc will give border-color) which is more useful in my opinion.

In general vscode built-in language servers like CSS doesn't do filtering on server side, results list is depends on code context not on prefix. This part was left for client (at least for now).

If you feel that this is too much for this PR just merge it and I will open issues for things I mentioned in previous comments :)

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

Ok, one more thing that isn't enhancement but a bug ;) Example:

<?php
class Test
{
    public function def(){}
    public function abc()
    {
        $this->|
    }
}

No completion after ->. I need add one letter (a or d) to have proposals.

Contributor

mniewrzal commented Nov 30, 2016

Ok, one more thing that isn't enhancement but a bug ;) Example:

<?php
class Test
{
    public function def(){}
    public function abc()
    {
        $this->|
    }
}

No completion after ->. I need add one letter (a or d) to have proposals.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

The thing is that fuzzy filtering has much worse performance. Remember that we might have to iterate over 100k definitions. Checking the key for a prefix is fast. Doing strpos is slower already. Fuzzy search would be even slower. We also don't do fuzzy search in symbol search.

Is your last bug related to $this? There are tests for property access without prefix.

Owner

felixfbecker commented Nov 30, 2016

The thing is that fuzzy filtering has much worse performance. Remember that we might have to iterate over 100k definitions. Checking the key for a prefix is fast. Doing strpos is slower already. Fuzzy search would be even slower. We also don't do fuzzy search in symbol search.

Is your last bug related to $this? There are tests for property access without prefix.

@mniewrzal

This comment has been minimized.

Show comment
Hide comment
@mniewrzal

mniewrzal Nov 30, 2016

Contributor

The thing is that fuzzy filtering has much worse performance. Remember that we might have to iterate over 100k definitions.

I understand, what I wanted to say is that there are many approaches:

  • leave strpos as it is
  • remove strpos filtering completely (client will filter response)
  • apply strpos only to places where performance is endangered

Is your last bug related to $this? There are tests for property access without prefix.

Yes, looks like that. I checked this:

<?php
class Test
{
    public function adef(){
        $a = new Test();
        $a->
    }
}

and works ok.

Contributor

mniewrzal commented Nov 30, 2016

The thing is that fuzzy filtering has much worse performance. Remember that we might have to iterate over 100k definitions.

I understand, what I wanted to say is that there are many approaches:

  • leave strpos as it is
  • remove strpos filtering completely (client will filter response)
  • apply strpos only to places where performance is endangered

Is your last bug related to $this? There are tests for property access without prefix.

Yes, looks like that. I checked this:

<?php
class Test
{
    public function adef(){
        $a = new Test();
        $a->
    }
}

and works ok.

@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

Yes, but sending 100k suggestions has a performance impact too. I think the current solution is the best compromise for now.

This might be #171 then

Owner

felixfbecker commented Nov 30, 2016

Yes, but sending 100k suggestions has a performance impact too. I think the current solution is the best compromise for now.

This might be #171 then

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 30, 2016

Contributor

@felixfbecker I suggest that we follow the pattern already used for the VSCode language servers implemented by Microsoft, which is:

  • Do context-based filtering, i.e. avoid returning completion item kinds that are not suited for the current cursor position.
  • Don't do input-based filtering and leave this to the client. By "input-based filtering" I mean filtering based on the current word entered by the user.
  • Return complete result lists, i.e. with isIncomplete: false

The pros of the above approach is:

  • Client can decide on the input-based filtering algorithm. The fuzzy filtering is a great usability feature and we should give the opportunity to clients to implement it.
  • Any additional typing after the initial completion request is very fast and does not involve further client-server communication.

Cons:

  • The initial completion request may take some time due to the big completion result, e.g. tens of thousands of items. This is partially mitigated by the context-based filtering and especially by the faster updates on the additional typing.

If we decide on a stricter server-based filtering to reduce the size of the completion list then:

  • We give up the fuzzy filtering, which is a pitty
  • We give up on returning complete result lists, forcing the client to send completion requests on every keystroke on further typing after the initial completion request. This has its own performance implications.
Contributor

kaloyan-raev commented Nov 30, 2016

@felixfbecker I suggest that we follow the pattern already used for the VSCode language servers implemented by Microsoft, which is:

  • Do context-based filtering, i.e. avoid returning completion item kinds that are not suited for the current cursor position.
  • Don't do input-based filtering and leave this to the client. By "input-based filtering" I mean filtering based on the current word entered by the user.
  • Return complete result lists, i.e. with isIncomplete: false

The pros of the above approach is:

  • Client can decide on the input-based filtering algorithm. The fuzzy filtering is a great usability feature and we should give the opportunity to clients to implement it.
  • Any additional typing after the initial completion request is very fast and does not involve further client-server communication.

Cons:

  • The initial completion request may take some time due to the big completion result, e.g. tens of thousands of items. This is partially mitigated by the context-based filtering and especially by the faster updates on the additional typing.

If we decide on a stricter server-based filtering to reduce the size of the completion list then:

  • We give up the fuzzy filtering, which is a pitty
  • We give up on returning complete result lists, forcing the client to send completion requests on every keystroke on further typing after the initial completion request. This has its own performance implications.
@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

What does "context-based" filtering mean? When giving completions for a method for example, we have to filter by an FQN prefix to filter by the class - so why not include the part of the typed property name too?

Owner

felixfbecker commented Nov 30, 2016

What does "context-based" filtering mean? When giving completions for a method for example, we have to filter by an FQN prefix to filter by the class - so why not include the part of the typed property name too?

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 30, 2016

Contributor

Example for "context-based" filtering. If you have $this->meth|, the completion list should return all visible methods and members of the current class (and its hierarchy), without filtering it by the "meth" input (the input-based filtering). Perhaps, variables should be included too to make something like $this->$my_local_var possible.

But the completion list should not include:

  • non-visible methods and members, e.g. private methods of a superclass
  • keywords
  • global functions
  • Class/Interface/Traits/Namespaces names
  • etc.
Contributor

kaloyan-raev commented Nov 30, 2016

Example for "context-based" filtering. If you have $this->meth|, the completion list should return all visible methods and members of the current class (and its hierarchy), without filtering it by the "meth" input (the input-based filtering). Perhaps, variables should be included too to make something like $this->$my_local_var possible.

But the completion list should not include:

  • non-visible methods and members, e.g. private methods of a superclass
  • keywords
  • global functions
  • Class/Interface/Traits/Namespaces names
  • etc.
@felixfbecker

This comment has been minimized.

Show comment
Hide comment
@felixfbecker

felixfbecker Nov 30, 2016

Owner

yeah, gotcha. Just saying that the class-filtering is implemented atm by doing a FQN prefix search. So I don't really see the disadvantage of also excluding anything that doesn't match what is already typed? Besides fuzzy searching of course. It saves a few items being sent to the client that don't match anyway.

Owner

felixfbecker commented Nov 30, 2016

yeah, gotcha. Just saying that the class-filtering is implemented atm by doing a FQN prefix search. So I don't really see the disadvantage of also excluding anything that doesn't match what is already typed? Besides fuzzy searching of course. It saves a few items being sent to the client that don't match anyway.

@kaloyan-raev

This comment has been minimized.

Show comment
Hide comment
@kaloyan-raev

kaloyan-raev Nov 30, 2016

Contributor

I wouldn't worry about the size of the completion list at that place. I don't believe a class can have thousands of methods and members.

The largest list is produced when you invoke code completion on an empty line.

Contributor

kaloyan-raev commented Nov 30, 2016

I wouldn't worry about the size of the completion list at that place. I don't believe a class can have thousands of methods and members.

The largest list is produced when you invoke code completion on an empty line.

@felixfbecker felixfbecker merged commit 10fb3c9 into master Nov 30, 2016

4 checks passed

codecov/patch 96.00% of diff hit (target 94.77%)
Details
codecov/project 95.06% (+0.29%) compared to f56b144
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@felixfbecker felixfbecker deleted the completion branch Nov 30, 2016

@Ciantic Ciantic referenced this pull request in Microsoft/vscode Dec 8, 2016

Closed

[php] Ability to turn off PHP autocompletion by VSCode #9003

@felixfbecker felixfbecker referenced this pull request Dec 27, 2016

Closed

Is it working? #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment