Skip to content
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

Initial implemention for code completion #38

Closed
wants to merge 6 commits into from

Conversation

mniewrzal
Copy link
Contributor

This is initial draft for keywords and local variables completion. Provides:

  • keywords from predefined list
  • all variables from file
  • simple mechanism to add new completion items

In future list of variables and keywords should be filtered according to completion context (global/class/method) but it shouldn't be a big problem. For now this PR is to show conception. If it will be approved then I will prepare version with more phpDocs/tests/etc. Few things in code also should be reorganized.

@codecov-io
Copy link

codecov-io commented Sep 29, 2016

Current coverage is 72.21% (diff: 10.67%)

Merging #38 into master will decrease coverage by 18.89%

@@             master        #38   diff @@
==========================================
  Files            34         39     +5   
  Lines           765        943   +178   
  Methods         104        124    +20   
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            697        681    -16   
- Misses           68        262   +194   
  Partials          0          0          
Diff Coverage File Path
0% new src/Completion/Strategies/VariablesStrategy.php
0% new src/Completion/Strategies/ClassMembersStrategy.php
0% new src/Completion/Strategies/GlobalElementsStrategy.php
0% src/Server/TextDocument.php
0% new src/Completion/Strategies/KeywordsStrategy.php
6% new src/Server/CompletionItemResolver.php
7% new src/Completion/CompletionContext.php
•• 20% new src/Completion/CompletionReporter.php
•• 25% src/PhpDocument.php
•••••••••• 100% src/Protocol/CompletionItemKind.php

Review all 11 files changed

Powered by Codecov. Last update 9e65cd4...9b2dcdb

Copy link
Owner

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Took a look at the changes and left some comments.
Will need some rebase work with the new Project and PhpDocument classes.

Additionally, please try to follow these coding styles:

  • use triple equals everywhere
  • end files with a final newline
  • use single quotes (except for template strings)
  • start files with declare(strict_types = 1)

return $this->start->compare($position) <= 0 && $this->end->compare($position) >= 0;
}

public function toString()
Copy link
Owner

Choose a reason for hiding this comment

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

Should be __toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will update PR with this and other suggestions.

new KeywordData("var"),
new KeywordData("while"),
new KeywordData("xor"),
new KeywordData("yield")
Copy link
Owner

Choose a reason for hiding this comment

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

Kind of dislike this class. Seems to me like all these are just constants, so they should be constants imo.
Also, where do these come from? The perfect solution would be to parse them somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more like example of usage then final version. I agree that it can be transformed in [][]. List of keywords can be found here: http://php.net/manual/en/reserved.keywords.php. It would be nice to have parsable keywords list, for now hardcoded list will be good enough.

*/
private $tokens;

public function __construct(array $tokens)
Copy link
Owner

@felixfbecker felixfbecker Sep 30, 2016

Choose a reason for hiding this comment

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

Is it needed to work with the raw PHP tokens? Could you comment on the limitations of PHPParser in that regard? The error recovery should be pretty good by now: nikic/PHP-Parser#262
I would like to work with the AST as much as possible (and if it is not capable, consider contributions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simple example will return null instead of ast:

<?php
function abc(){
}
function test(){
    $

From my personal experience I'm concerned that there are too much cases to cover to have very good parser error recovery. I will explain more about tokens in next comment.

Copy link
Owner

Choose a reason for hiding this comment

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

What about

function abc(){
}
function test(){
    $
}

?
VS Code automatically inserts closing brackets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your version will give AST + error, but we cannot assume that this will be 99% of cases and my version just 1% :) List of tokens will always reflect full file structure, file with or without errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another use case is typing just:

<?php
abstract

In this case the PHP parser returns an empty AST tree.

The only way to find all situations when AST tree is not enough seems to be by trial and error. So, it looks reasonable to rely on tokens than on AST.

public function completion(TextDocumentIdentifier $textDocument, Position $position)
{
$completionReporter = new CompletionReporter();
$tokens = token_get_all($this->documents[$textDocument->uri]);
Copy link
Owner

@felixfbecker felixfbecker Sep 30, 2016

Choose a reason for hiding this comment

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

See above comment, this will essentially re-parse the whole document, even though PHPParser already did this. If PHPParser is really not capable of parsing nodes up until the cursor, we should take as many nodes as it can provide and then only use token_get_all() for the substring between the last node and the cursor, if token_get_all will even work for this. Might even have to use regexps for those last chars in front of the cursor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part was written just to show results, I knew I will need to rebase this PR after #31 merge.

List of tokens is always available after Parser::parse(), Lexer::getTokens() contains such array and it can be used without executing token_get_all() once again. Even without that dividing into tokens is very fast :) Main problem with pure tokens array is that its a little bit inconsistent and it needs to be converted into something more useful for line:column processing. In this case conversion changes all arrays elements into the same object type with document range.

Mixing ast with token_get_all is in theory possible because token_get_all can process part of code ('<?php' tag needs to be added) but I'm not sure if its best solution. It can be tricky in my opinion.

Token approach looks right now to be good solution in my opinion but its just my version and maybe someone will came with better implementation :)

@mniewrzal mniewrzal changed the title Initial implemention for keywords and variables completion Initial implemention for code completion Oct 21, 2016
@mniewrzal
Copy link
Contributor Author

mniewrzal commented Oct 21, 2016

I changed a little bit previous implementation. Now it doesn't use tokens just regexp in some cases. With this patch CA provides:

  • local variables
  • keywords
  • class members (no inheritance)
  • global elements (functions, types)

Implementation is using completionItem/resolve method to provide proposals description.

@felixfbecker
Copy link
Owner

@mniewrzal Really sorry this PR has been stale for so long. It includes a lot of changes for one of the major and most complicated features, so I would like to think about architecture design before merging anything, and I'm currently focused on #54, which completion will also benefit from. You are introducing a lot of new classes like strategies, resolvers and I need to tinker a bit myself to see if that added complexity makes sense.

But I would be interested in your experience with providing the completion: My idea was that we use the AST and get the last nodes before the cursor. I haven't played with it a lot, but did you try this? Because then we can let generic functions handle resolving a property fetch, an array dimension fetch or a method/function call recursively to a type. We then would only need a regexp or tokens for the characters between the last node and the cursor.

@mniewrzal
Copy link
Contributor Author

No problem, I opened this PR as a place for discussion. My idea was to start with something relatively simple and after discussion choose general direction for CA. I don't want to force my implementation, I prefer solution that will came out after discussion :) I also understand that with VS Code code completion is not so urgent because there is build-in support for that in editor.

I was playing with different things to find elements around cursor position (tokens, regexp, ast). I believe that tokens and regexp can be used to do the job (e.g. finding insertion range, context around cursor). I also tried to use AST to do similar things but unfortunately at the moment parser too often is returning null or empty array for code with errors. And this is for very normal and common use cases.

Take your time and think about your solution. Then we can start discussion :)

@felixfbecker
Copy link
Owner

Completion support landed in v4.0.0, thanks for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants