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

Add ReflectionProperty::getType() and hasType() #5584

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Apr 6, 2021

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/b00d652573
<?php

$method = new ReflectionClass(stdClass::class);
$properties = $method->getProperties();
foreach ($properties as $property) {
	if ($property->hasType()) {
		$property->getType()->__toString();
	}
}
Psalm output (using commit 6851b5c):

ERROR: UndefinedMethod - 6:17 - Method ReflectionProperty::hasType does not exist

ERROR: PossiblyNullReference - 7:25 - Cannot call method __toString on possibly null value

@franmomu
Copy link
Contributor Author

I forgot I started this one, so tests fail and the only thing I can think of is that ReflectionParameter is immutable, but ReflectionProperty is not. Any help? 🤔

@caugner
Copy link
Contributor

caugner commented Jul 9, 2021

@franmomu Unfortunately The logs for this run have expired and are no longer available.. Would you be able to amend & force-push to re-trigger the checks?

@franmomu franmomu force-pushed the add_reflection_property branch from 21cbc21 to 9a0f9f9 Compare July 9, 2021 21:14
@franmomu
Copy link
Contributor Author

franmomu commented Jul 9, 2021

@franmomu Unfortunately The logs for this run have expired and are no longer available.. Would you be able to amend & force-push to re-trigger the checks?

Sure, done! Feel free to try it, the problem I think it was immutability, ReflectionProperty has a setValue method so it cannot be immutable, don't know if there is other way.

@caugner
Copy link
Contributor

caugner commented Jul 9, 2021

Sure, done!

Thanks.

There was 1 error:
  
1) Psalm\Tests\AssertAnnotationTest::testValidCode with data set "implicitReflectionPropertyAssertion" ('<?php\n                    $c...     }')
   Psalm\Exception\CodeException: MixedMethodCall - src/somefile.php:6:51 - Cannot determine the type of the object on the left hand side of this expression

And this is line 6:

$property->getType()->allowsNull();

Alright, now I see what you mean. Since $property cannot be declared immutable, Psalm doesn't use assert-if-true from $property->hasType() to determine the type of $property->getType(). 😦

I wonder if it would help to mark hasType() and getType() with @psalm-mutation-free. But still, even if $property is not immutable, Psalm should be able to determine that the assertion by hasType() is true at least immediately for the next statement.

@weirdan
Copy link
Collaborator

weirdan commented Jul 9, 2021

MixedMethod is caused by Psalm thinking you're targeting a PHP version below 7.4. You need to mark the test case as targeting 7.4.

@caugner
Copy link
Contributor

caugner commented Jul 9, 2021

You need to mark the test case as targeting 7.4.

Here's an example (I think):

'implementExtendedInterfaceWithMethodOwnTemplateParams' => [
'<?php
/**
* @template T1
*/
interface IFoo {
/**
* @template T2
* @psalm-param T2 $f
* @psalm-return self<T2>
*/
public static function doFoo($f): self;
}
/**
* @template T3
* @extends IFoo<T3>
*/
interface IFooChild extends IFoo {}
/**
* @template T5
* @implements IFooChild<T5>
*/
class ConcreteFooChild implements IFooChild {
/** @var T5 */
private $baz;
/** @param T5 $baz */
public function __construct($baz) {
$this->baz = $baz;
}
/**
* @template T6
* @psalm-param T6 $f
* @psalm-return ConcreteFooChild<T6>
*/
public static function doFoo($f): self {
$r = new self($f);
return $r;
}
}',
[],
[],
'7.4'
],

@franmomu franmomu force-pushed the add_reflection_property branch from 9a0f9f9 to 55ac52f Compare July 10, 2021 11:02
@franmomu
Copy link
Contributor Author

MixedMethod is caused by Psalm thinking you're targeting a PHP version below 7.4. You need to mark the test case as targeting 7.4.

Here's an example (I think):

'implementExtendedInterfaceWithMethodOwnTemplateParams' => [
'<?php
/**
* @template T1
*/
interface IFoo {
/**
* @template T2
* @psalm-param T2 $f
* @psalm-return self<T2>
*/
public static function doFoo($f): self;
}
/**
* @template T3
* @extends IFoo<T3>
*/
interface IFooChild extends IFoo {}
/**
* @template T5
* @implements IFooChild<T5>
*/
class ConcreteFooChild implements IFooChild {
/** @var T5 */
private $baz;
/** @param T5 $baz */
public function __construct($baz) {
$this->baz = $baz;
}
/**
* @template T6
* @psalm-param T6 $f
* @psalm-return ConcreteFooChild<T6>
*/
public static function doFoo($f): self {
$r = new self($f);
return $r;
}
}',
[],
[],
'7.4'
],

Thanks! done

@franmomu franmomu force-pushed the add_reflection_property branch from 55ac52f to decb397 Compare July 10, 2021 11:08

/**
* @since 7.4
* @psalm-mutation-free
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caugner I think it works with the @psalm-mutation-free you said 👍 (apart from adding the 7.4 check)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check: Did you need to add mutation-free to make the test pass? So marking the test with 7.4 was not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, glad to hear it works! This reminds me that I should use that annotation in my own projects as well. 🙂

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

Thanks, looking good!

@weirdan weirdan merged commit ad3d46c into vimeo:master Jul 10, 2021
@franmomu franmomu deleted the add_reflection_property branch July 11, 2021 00:06
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.

3 participants