Go to definition support #49

felixfbecker · 2016-10-08T13:13:39Z

Closes #19

Works for

class instantiations
static method calls
class constant access
static property access
parameter type hints
return type hints
method calls, if the variable was assigned to a new object in the same scope
property access, if the variable was assigned to a new object in the same scope
variables
parameters
imported closure variables (use)
use statements that import a class-like
class-like after implements/extends
function calls
constant access

What does not work yet:

$x = new Whatever();
$y = $x;
$y->myMethod(); // cannot get definition for myMethod

$x = new Whatever();
$x->property->myMethod(); // cannot get definition for myMethod
$x->myMethod()->anotherMethod(); // cannot get definition for anotherMethod

$this->myMethod(); // cannot get definition for myMethod, $this is not resolved to the class

trait MyTrait {}
class MyClass {
  use MyTrait; // cannot get definition for MyTrait
}

Classes, interfaces, traits, methods, properties and constant declarations get indexed by their fully qualified name at parse time.
Examples for FQNs:

TestNamespace\testFunction()
TestNamespace\TEST_CONSTANT
TestNamespace\TestClass
TestNamespace\TestClass::TEST_CONSTANT
TestNamespace\TestClass::staticTestProperty
TestNamespace\TestClass::testProperty
TestNamespace\TestClass::staticTestMethod()
TestNamespace\TestClass::testMethod()

At a definition request, the node under the cursor is searched for and the referenced FQN is constructed (except for variables). Then the index is searched for that FQN.

Variables are resolved at request by traversing the AST upwards from the node under the cursor.

Other changes:

ASTs are now kept in memory
All nodes get references to their parent, siblings and document

* constants * static properties * static methods

codecov-io · 2016-10-08T13:15:15Z

Current coverage is 89.67% (diff: 88.88%)

Merging #49 into master will decrease coverage by 2.08%

@@             master        #49   diff @@
==========================================
  Files            19         21     +2   
  Lines           291        455   +164   
  Methods          45         66    +21   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            267        408   +141   
- Misses           24         47    +23   
  Partials          0          0

Diff Coverage	File Path
••••••• 77%	src/Server/TextDocument.php
•••••••• 83%	src/Protocol/Position.php
•••••••• 86%	src/PhpDocument.php
•••••••••• 100%	new src/NodeVisitor/DefinitionCollector.php
•••••••••• 100%	new src/NodeVisitor/NodeAtPositionFinder.php
•••••••••• 100%	src/Server/Workspace.php
•••••••••• 100%	src/NodeVisitor/ColumnCalculator.php
•••••••••• 100%	new src/NodeVisitor/ReferencesAdder.php
•••••••••• 100%	src/Protocol/Location.php
•••••••••• 100%	src/Protocol/Range.php

Review all 12 files changed

Powered by Codecov. Last update 6cb916e...0387f28

felixfbecker · 2016-10-08T14:26:42Z

Anyone have an idea what kind of sorcery this is?

$project = $this->project;

Error: Cannot access private property LanguageServer\PhpDocument::$project
C:\Users\felix\git\OpenSource\php-language-server\src\PhpDocument.php:345

How can this error ever happen when reading from $this in a non-derived class?!

sunverwerth · 2016-10-08T14:54:15Z

src/NodeVisitors/NodeAtPositionFinder.php

+    {
+        if (
+            !isset($this->node)
+            && $node->getAttribute('startLine') <= $this->position->line + 1


This will not work, you are assuming the node spans a rectangular section of the source code.
First and last line have to be handled separately.

Should be fixed now, could you take a look?

looks good to me

sunverwerth · 2016-10-08T18:24:23Z

That's a lot of changes... can you give a summary when you are finished?

felixfbecker · 2016-10-08T19:09:21Z

@madhed All the latest commits were pretty much bug fixes. I tried to make the commits easy to review one-by-one (actually spend quite some time rebasing).

Test refactoring etc. should be self-explanatory.

4786fe1, befb07d: This adds new attributes to each Node that allow you to get the parent node, siblings and the document of the node. Inspired by
https://developer.mozilla.org/en-US/docs/Web/API/Node/parentNode
https://developer.mozilla.org/en-US/docs/Web/API/Node/nextSibling
https://developer.mozilla.org/en-US/docs/Web/API/Node/previousSibling
https://developer.mozilla.org/en-US/docs/Web/API/Node/ownerDocument

Implementation of definition request (5fc35e7):
Upon parsing, a new NodeVisitor DefinitionCollector traverses the AST(s) and collects all declarations (except variables). The declaration nodes are stored in a map by their (unique) FQN. Methods/properties are separated from their classes with ::, methods and functions are appended ().
These definitions are then stored in the PhpDocument object under $definitions.
At the same time, the FQNs are also registered at the Project object to point to this PhpDocument object.

To get the definition, we first have to find the node under the cursor. This is done by the NodeAtPositionFinder (2fd3b58), which traverses the AST depth-first and returns the first node that contains the cursor position. This requires to keep the AST in memory, which is now saved under $stmts on the PhpDocument object.

Once we have the node we want to get the definition for, we build the FQN from it.
This happens in PhpDocument::getDefinitionByNode(). We basically have to go through all possible nodes one could ask the definition for (property fetch, function call, method call, ...) and try to resolve the FQN of the symbol.
This is pretty easy for classes or static method calls for example.
Method calls, property fetches and variables are more complicated. We first have to find an assignment or parameter node that this variable comes from, and then hopefully can infer it from there.
This is done in PhpDocument::getVariableDefinition().
The algorithm is as follows: From the variable node, check if any previous siblings are an assignment node to that variable. If not, go one level up. Repeat this until a function/method node is met, and finally check parameters.
The types are resolved very basic atm, it only works if the expression is a New node (instantiation) or if the parameter has a type hint. It breaks when the variable is assigned to another variable or the return value of a function. DocBlocks are also not taken into account at all. I want to save this for later implementation, will become more important for provideCompletion. The biggest use cases for definition are implemented in this PR.

Finally, I removed the SymbolFinder and instead use the $definitions array to build the SymbolInformation array.

felixfbecker · 2016-10-08T19:17:06Z

Squashed some commits to ease reviewing.

sunverwerth · 2016-10-08T19:20:11Z

Ok let me check this out.

sunverwerth

Overall good, but a fatal error must be fixed. More rigorous testing is advised.

Furthermore I was not able to goto definition of parameter type hints. Is this expected?

Memory consumption went up to 264MiB but this was to be expected.

sunverwerth · 2016-10-08T19:43:19Z

src/NodeVisitors/DefinitionCollector.php

+    {
+        if ($node instanceof Node\Stmt\ClassLike && isset($node->name)) {
+            // Class, interface or trait declaration
+            $this->definitions[(string)$node->namespacedName] = $node;


I think the existance of name implies that namespacedName is also set, but maybe check for isset($node->namespaceName) to avoid confusion?

I read isset($node->name) as does the class have a name? e.g. is the class an anonymous class? which is why I wrote it like this

Fair enough, it just looked like a bug at first because you test for one attribute but access another.

sunverwerth · 2016-10-08T20:05:56Z

src/PhpDocument.php

+                return null;
+            }
+            // Need to resolve variable to a class
+            $varDef = $this->getVariableDefinition($node->var);


This crashes with fatal when trying to goto definition of write in $this->protocolWriter->write.

Fatal error: Uncaught TypeError: Argument 1 passed to LanguageServer\PhpDocument::getVariableDefinition() must be an instance of PhpParser\Node\Expr\Variable, instance of PhpParser\Node\Expr\PropertyFetch given, called in E:\Projekte\php-language-server\src\PhpDocument.php on line 315 and defined in E:\Projekte\php-language-server\src\PhpDocument.php:391

Just fixed this and amended

sunverwerth · 2016-10-08T20:16:20Z

src/PhpDocument.php

+            Node\Const_::class                => SymbolKind::CONSTANT
+        ];
+        $symbols = [];
+        foreach ($this->definitions as $fqn => $node) {


Warning: Invalid argument supplied for foreach when invoking workspace/symbol search

Can you reproduce? Maybe this is only when parsing hasn't finished yet?

Can reproduce, let the indexer run all the way through. Every time(!) I open workspace symbol search the warning appears. The search works, though.

I can reproduce too. But it is weird because parse() sets it to an array and getSymbols() never runs before parse().

I just found it. It comes from invalid_file.php. It's never successfully parsed.

sunverwerth · 2016-10-08T20:32:47Z

Works for the trivial cases, but feels a little brittle all around... what do you think?
We definitely need to do a lot of testing to prevent crashing.

Also some time later we should switch to a custom AST representation like @mniewrzal proposed in #31 (comment) this would probably decrease the memory footprint a lot and also tidy up the code quite a bit. I was a sometimes bit lost while reviewing 😁

Also I just noticed when there are multiple assignment to a variable and you go to its definition, it just jumps up one assignment instead of going to the first one.

felixfbecker · 2016-10-08T20:48:18Z

Works for the trivial cases, but feels a little brittle all around... what do you think?

Yeah well the type resolving is still not very good. That will play a bigger role for provideCompletion. I want to get basic support for the major requests asap

We definitely need to do a lot of testing to prevent crashing.

For sure. DefinitionTest is already the biggest test class

Also some time later we should switch to a custom AST representation like @mniewrzal proposed in #31 (comment) this would probably decrease the memory footprint a lot and also tidy up the code quite a bit. I was a sometimes bit lost while reviewing 😁

Still not 100% sold on this. The AST nodes are very very powerful to work with. Recreating them all seems a bit pointless to me. But what we can do now is not keep the file content in memory anymore, because we only need it at parse time for errors and use the AST afterwards. That alone will reduce memory usage (the AST doesn't hold any content, only structure), but as I said already in other places I'm not concerned at the moment with RAM usage, rather with features and response times.

Also I just noticed when there are multiple assignment to a variable and you go to its definition, it just jumps up one assignment instead of going to the first one.

Yeah, noticed that later on too. The algorithms picks the closest definition. Instead, it should go all the way up to a function node and then down again to find the declaration. But that is much more complicated because you need to make sure you are going the right path.

felixfbecker · 2016-10-08T21:00:29Z

Furthermore I was not able to goto definition of parameter type hints. Is this expected?

Fixed

felixfbecker · 2016-10-08T21:26:07Z

src/PhpDocument.php

@@ -53,7 +53,7 @@ class PhpDocument
     *
     * @var Node[]
     */
-    private $stmts;
+    private $stmts = [];


I don't think this is good - there is a semantic difference between "a document has no statements" and "we couldn't parse the statements"

felixfbecker · 2016-10-08T21:37:29Z

Please avoid merge commits

Keep AST in memory

Move logic to PhpDocument::getDefininedFqn() for reusability Fix DefinitionResolverTest

felixfbecker added 2 commits October 8, 2016 15:01

Move NodeVisitors to own namespace

063c7f9

Add more symbols to symbol test

658a27f

* constants * static properties * static methods

felixfbecker added feature in progress labels Oct 8, 2016

Decorate all nodes with parent, sibling references

4786fe1

felixfbecker force-pushed the definition branch 2 times, most recently from 2959b05 to 9f2d3a4 Compare October 8, 2016 13:19

sunverwerth reviewed Oct 8, 2016

View reviewed changes

felixfbecker force-pushed the definition branch from a0ad0ef to 6309b73 Compare October 8, 2016 19:15

sunverwerth suggested changes Oct 8, 2016

View reviewed changes

felixfbecker force-pushed the definition branch from 6309b73 to 844707b Compare October 8, 2016 20:29

felixfbecker commented Oct 8, 2016

View reviewed changes

sunverwerth approved these changes Oct 8, 2016

View reviewed changes

felixfbecker added 7 commits October 9, 2016 15:51

Add method to find out node at position

48c71e5

Keep AST in memory

Refactor TextDocument tests into separate classes

987308f

Add ownerDocument attribute to nodes

fbdf1aa

Add Position::compare() and Range::includes()

827ab4c

Add textDocument/definition support

d4757e0

Use DefinitionCollector for symbol requests

6be53ad

Split up PhpDocument::getDefinitionByNode()

3a88093

felixfbecker added 2 commits October 9, 2016 15:58

Add fromNode() factories and correct columns

7322a6c

Refactor DefinitionResolver

7f95b76

Move logic to PhpDocument::getDefininedFqn() for reusability Fix DefinitionResolverTest

felixfbecker force-pushed the definition branch from 1fbff27 to 7f95b76 Compare October 9, 2016 14:01

Rename NodeVisitors NS to NodeVisitor

0387f28

felixfbecker merged commit 7032f80 into master Oct 9, 2016

felixfbecker removed the in progress label Oct 9, 2016

felixfbecker deleted the definition branch October 9, 2016 14:26

felixfbecker mentioned this pull request Oct 9, 2016

Resolve types recursively #54

Closed

jakejarrett mentioned this pull request Nov 10, 2017

Jump to Definition thiagojedi/vala-code#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go to definition support #49

Go to definition support #49

felixfbecker commented Oct 8, 2016 •

edited

Loading

codecov-io commented Oct 8, 2016 •

edited

Loading

felixfbecker commented Oct 8, 2016 •

edited

Loading

sunverwerth Oct 8, 2016

felixfbecker Oct 8, 2016

sunverwerth Oct 8, 2016

sunverwerth commented Oct 8, 2016

felixfbecker commented Oct 8, 2016 •

edited

Loading

felixfbecker commented Oct 8, 2016 •

edited

Loading

sunverwerth commented Oct 8, 2016

sunverwerth left a comment

sunverwerth Oct 8, 2016

felixfbecker Oct 8, 2016 •

edited

Loading

sunverwerth Oct 8, 2016

sunverwerth Oct 8, 2016

felixfbecker Oct 8, 2016

sunverwerth Oct 8, 2016

sunverwerth Oct 8, 2016

felixfbecker Oct 8, 2016

sunverwerth Oct 8, 2016

felixfbecker Oct 8, 2016

sunverwerth Oct 8, 2016

sunverwerth Oct 8, 2016

sunverwerth commented Oct 8, 2016

felixfbecker commented Oct 8, 2016

felixfbecker commented Oct 8, 2016

felixfbecker Oct 8, 2016

felixfbecker commented Oct 8, 2016

Go to definition support #49

Go to definition support #49

Conversation

felixfbecker commented Oct 8, 2016 • edited Loading

codecov-io commented Oct 8, 2016 • edited Loading

Current coverage is 89.67% (diff: 88.88%)

felixfbecker commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunverwerth commented Oct 8, 2016

felixfbecker commented Oct 8, 2016 • edited Loading

felixfbecker commented Oct 8, 2016 • edited Loading

sunverwerth commented Oct 8, 2016

sunverwerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Oct 8, 2016 • edited Loading

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

sunverwerth commented Oct 8, 2016

felixfbecker commented Oct 8, 2016

felixfbecker commented Oct 8, 2016

Choose a reason for hiding this comment

felixfbecker commented Oct 8, 2016

felixfbecker commented Oct 8, 2016 •

edited

Loading

codecov-io commented Oct 8, 2016 •

edited

Loading

felixfbecker commented Oct 8, 2016 •

edited

Loading

felixfbecker commented Oct 8, 2016 •

edited

Loading

felixfbecker commented Oct 8, 2016 •

edited

Loading

felixfbecker Oct 8, 2016 •

edited

Loading