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

Generic functions reflection and integration #2219

Merged
merged 6 commits into from
Jun 20, 2019

Conversation

arnaud-lb
Copy link
Contributor

@arnaud-lb arnaud-lb commented Jun 17, 2019

This adds reflection support for @template (integrates the phpdoc parser's result with the rest), and adds integration tests.

NameScope now takes a TemplateTypeMap, such that PhpDocNodeResolver / TypeNodeResolver can properly resolve references to template types (for example, @return array<T> is resolved as ArrayType(MixedType, TemplateType(T))).

In integration tests, I felt the need to assert the static type of some expressions, so I added a assertType() function and a AssertTypeRule rule (in tests only).

With this PR, we can actually start to use generic functions \o/ There are some caveats, however:

  • There are no Rules, so no safe net
  • Template types can only be unbound, or bound to a class/interface name (no scalars, etc)
  • class-string not support yet
  • There are probably undiscovered / unimplemented edge cases

@@ -232,10 +233,12 @@ function (\PhpParser\Node $node) use ($fileName, $lookForTrait, &$phpDocMap, &$c
$uses[strtolower($use->getAlias()->name)] = sprintf('%s\\%s', $prefix, (string) $use->name);
}
return null;
} elseif ($node instanceof Node\Stmt\ClassMethod) {
$functionName = ltrim(sprintf('%s\\%s', $namespace, $node->name->name), '\\');
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. Let's say we have class Bar in namespace Foo with method baz. This will produce Foo\baz...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks !

foreach ($phpDocNode->getTemplateTagValues() as $tagValue) {
$resolved[$tagValue->name] = new TemplateTag(
$tagValue->name,
$this->typeNodeResolver->resolve($tagValue->bound, $nameScope)
Copy link
Member

Choose a reason for hiding this comment

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

What does the $tagValue->bound contain if we have @template T without any bound in the phpDoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It always contains a TypeNode. @JanTvrdik suggested that it should be null in this case; I will make this change later.

@@ -5,12 +5,10 @@
/**
* @param array<int, int, int> $arrayWithTooManyArgs
* @param iterable<int, int, int> $iterableWithTooManyArgs
* @param \Foo<int> $genericFoo
Copy link
Member

Choose a reason for hiding this comment

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

I think this should still fail in this PR, there's no GenericObjectType implementation yet...

@@ -15,13 +15,9 @@ protected function getRule(): Rule
public function testRule(): void
{
$this->analyse([__DIR__ . '/data/incompatible-property-phpdoc.php'], [
[
'PHPDoc tag @var for property InvalidPhpDoc\FooWithProperty::$foo contains unresolvable type.',
Copy link
Member

Choose a reason for hiding this comment

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

ditto, this should still fail

$typeString = $valueType->describe(VerbosityLevel::precise());
if ($constant->getValue() !== $typeString) {
return [sprintf(
'Expected type %s, got %s',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this rule is a good fit inside this PR. There's a similar proposal here (#2102) but I unfortunately haven't had time to look at it. Everything around asserting types should be done in NodeScopeResolverTest until we have a better complex approach in master. For example look at dataBinary/testBinary methods in NodeScopeResolverTest to get an idea how it works. Please add any type assertions in new dataGenerics/testGenerics pair there, thank you.

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 either have a single assertion point at die; or you can specify your own assertion points like it's in testForeachLoopVariables.

@ondrejmirtes ondrejmirtes merged commit 1e58d40 into phpstan:master Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants