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

[WIP] Variable collector #224

Closed
wants to merge 9 commits into from
Closed

Conversation

dantleech
Copy link
Contributor

@dantleech dantleech commented Nov 27, 2016

This is an initial attempt at a variable collector (#222), which can be used by the ReflectionMethod to determine the variables that are used. The bulk of this code is concerned with resolving types so that:

/**
 * @var Foobar
 */
public function something()
{
     $bar = $this->foobar->getSomeClass()->getFoo();
}

If the getSomeClass method has the return type SomeClass class and SomeClass a method getFoo that returns type int, then $bar will be resolved to int.

$reflection = $reflector->reflect(SomeClass::class);
$vars = $reflection->getMethod('something')->getVariables();

echo $vars[0]->getName(); // bar
echo $vars[0]->getType(); // int
echo $vars[0]->getDeclaredAt(); // 6

The variable collector visitor can return locally assigned and parameter variables available in the AST it is traversing (regardless of scope). In the case of getting the variables in a method it would suffice to traverse only the method AST.

This is really rough WIP atm and contains an unknown number of bugs, so before reviewing do you agree with the general approach?

TODO:

  • Types from static methods/properties.
  • ...


/**
* @param string $name
* @param int $declaredAt
Copy link
Member

Choose a reason for hiding this comment

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

Unsure about just having the line number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, ideally there would be the range of (characters?) that the variable is available for, but the PHP parser just gives us line numbers ..

/**
* @param string $name
* @param int $declaredAt
* @param string $type
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't say much - need some sort of more restrictive hint here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its actually a ReflectionType ...

}

/**
* Get a PhpDocumentor type object for this type
Copy link
Member

Choose a reason for hiding this comment

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

What if the type mutated during execution? Two variables with same name, but different type and declaration line, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We return all declarations, so you could get two with the same name, but covering different parts of the code.

* @see https://github.com/php/php-src/blob/master/ext/reflection/php_reflection.c#L2993
* @return string
*/
public function __toString()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't add this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found it useful for testing, but mainly I added it to be consistent with the other Reflection* classes.

Copy link
Member

Choose a reason for hiding this comment

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

Unless it actually is required API, I wouldn't add it. __toString() is often abused, so let's not give users more rope to hang themselves :)

Copy link
Contributor Author

@dantleech dantleech Dec 4, 2016

Choose a reason for hiding this comment

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

ok, will remove it. I can only guess that the __toString functionality in the other classes is useful for dumping a representation of a class as a whole, and that Variables would not be part of the representation /cc @asgrim

}
}

private function processAssignation(Expr\Assign $node)
Copy link
Member

Choose a reason for hiding this comment

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

Just processAssign

@Ocramius
Copy link
Member

Approach looking sound to me. Loads of testing and refactoring to be done though

@@ -1,7 +1,7 @@
language: php

php:
- 5.6
# - 5.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have switched to using PHP7 at least for development - guessing that you are going to switch to PHP7 in the near-future anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - that's the plan :) #221

// actually have a return type??
if ($expr instanceof Expr\FuncCall) {
$func = (string) $expr->name;
$reflection = new \ReflectionFunction($func);
Copy link
Contributor Author

@dantleech dantleech Nov 28, 2016

Choose a reason for hiding this comment

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

This is pretty hacky - wonder if we should simply not support functions I guess we should at least try and use BR\ReflecitonFunction.

Copy link
Member

Choose a reason for hiding this comment

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

You can stop at the return type declared on the function btw - no need to introspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, this is a function_call(), how else would I determine the return type? But anyway, not tested yet, so I have no idea what would happen here.


private function createUnknownReflectionType()
{
return $this->createReflectionTypeFromString('mixed', false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we do when the type cannot be determined? Currently returning mixed - should we return the type as null?

Copy link
Member

Choose a reason for hiding this comment

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

Should be null, IMO. Adding mixed is a very specific thing, plus nothing prevents a class mixed {} to exist.

@asgrim asgrim added the WIP label Nov 30, 2016
@dantleech
Copy link
Contributor Author

dantleech commented Dec 4, 2016

At the moment this PR scope is about collecting "available" variables (#222), currently including the parameters given in a method call. If taken further this means we should also return $this as a variable, and further still, globals, which we cannot do.

For my specifc use case, I need the "available" variables (albeit without the globals). And I am already using this PR as a base for further introspection.

Do you think this functionality be limited to variables assigned within a scope? Or should we return the func/method param variables also?

@MarkRedeman
Copy link
Contributor

@dantleech I think there are still a couple of variable definitions that you're missing in this PR.

@dantleech
Copy link
Contributor Author

I think there are still a couple of variable definitions that you're missing in this PR.

indeed, it is far from being complete.

Variable definition through a foreach statement.

How? PHPStorm would rely on an annotation above the foreach loop here I think (?)

@MarkRedeman
Copy link
Contributor

indeed, it is far from being complete.

I understand, I mostly wanted to make you aware of those use cases :).

How? PHPStorm would rely on an annotation above the foreach loop here I think (?)

That's probably going to be a tricky one.. You might be able to check whether the type of the iterable. So that if you're iterating over an Iterator then you can check the return type of current.
If the iterable is an IteratorAggregate then this would require some recursive checks.
For arrays I guess the best use case would be to guess their types from the docblocks.

Btw the use cases I mentioned were mostly about finding the definitions of a variable, not necessarily including their types (though it would be very useful).

@asgrim
Copy link
Member

asgrim commented Dec 24, 2016

@dantleech note: the core namespace is now \Roave\BetterReflection so this PR will need updating. No need to provide BC shim class entries for this PR as it's a new feature anyway :)

@TomasVotruba
Copy link
Contributor

@dantleech Hey, thanks for this awesome PR. I need exactly this feature, just without reflection and based on nikic\php-parser, so I might take a lot from it.

Do you expect to finish this now on BR 2.0? I could help if you need

@dantleech
Copy link
Contributor Author

dantleech commented Oct 24, 2017

@TomasVotruba I don't anticipate working on this, I already started another project which does this. The problem is that it is a huge amount of work with lots of edge cases. My implementation works "OK" for the purpose (auto completion) / reference finding.

@TomasVotruba
Copy link
Contributor

@dantleech Oh, I found out your package WorseReflection where you've probably implemented this.

Where could I find there this feature? I looked to ReflectionMethod but could not find it.

@dantleech
Copy link
Contributor Author

dantleech commented Oct 24, 2017

The entry point is Reflector::reflectOffset(): ReflectionOffset ($offset->frame() returns the frame).

Most of the code is here and close by (but I think ReflectionMethod also has a ->frame() method which returns all the variables).

@TomasVotruba
Copy link
Contributor

@dantleech Not sure why, but I didn't see your comments on mobile, that explains my comments :).
Neither I got notified about your last comment. Thanks a lot for that, I'll look on it.

How are you satisfied with the "microsoft/tolerant-php-parser"?

Any useful code parts that you think might be useful for me?

@dantleech
Copy link
Contributor Author

dantleech commented Oct 26, 2017

In general I am happy with tolerant parser. It's designed for IDE usage, the API is richer than nikic/php-parser. For example $node->getFirstAncestor(FunctionLike::class, ClassLike::class), $node->getDescendantAtOffset(1234). It also will resolve the fully qualified names of things, so $node->getFirstAncestor(ClassLike::class)->getFullyQualifiedName() etc.

The tree is read-only however, so you can't build code (but it offers a TextEdit class which can apply changes to the raw source code).

It's not stable, and obviously not ubiquitous as nikic/php-parser, so some potential risk there I think

@TomasVotruba
Copy link
Contributor

@dantleech Thanks, I'll look on these features.
I managed to implement most of them via custom NodeVisitors already.

Just off-topic: It looks like you have found another solution, so maybe this PR can be closed. I thouth it's WIP and expected it might be finished, but now I see there is complete another way you took. What do you think?

@dantleech
Copy link
Contributor Author

The approach is basically the same - the only reason I created WorseReflection was because it's the core domain of Phpactor and I had lots of requirements. I think the WR implementation could be used as a reference for implementing this feature in the future (I also think the PHP Language Server would be worth a look in this respect).

@dantleech dantleech closed this Oct 26, 2017
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 27, 2017

In case anyone would be looking for getFirstAncestor for nikic/php-parser, here it is.

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

Successfully merging this pull request may close these issues.

5 participants