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

Provide return type of ReflectionParameter::getType() #5333

Closed
wants to merge 5 commits into from

Conversation

dbrekelmans
Copy link

Fixes #5258

Thanks to @orklah for giving me some hints 👍

@weirdan
Copy link
Collaborator

weirdan commented Mar 6, 2021

@dbrekelmans mind fixing CS (https://circleci.com/gh/vimeo/psalm/22383)?

Tests failures are unrelated, fixed in #5335

dbrekelmans and others added 2 commits March 6, 2021 20:38
Co-authored-by: Bruce Weirdan <weirdan@gmail.com>
@muglug
Copy link
Collaborator

muglug commented Mar 6, 2021

First, thanks for this!

I think I prefer the option you presented in the original issue, following this recent dictum.

Given that ReflectionParameter is an immutable class the assertion would be entirely safe — though it requires a bugfix for Psalm to allow this specific syntax, which I'm working on now.

<?php

/**
 * @immutable
 */
class ReflectionParameter {
  /**
   * @psalm-assert-if-true $this->getType() ReflectionType
   * @psalm-assert-if-false $this->getType() null
   */
  public function hasType() : bool {
      return true;
  }
    
  public function getType() : ?ReflectionType {
   	return null;   
  }
}

The PR is not for nought, as I'll re-use your tests.

@muglug muglug closed this Mar 6, 2021
@orklah
Copy link
Collaborator

orklah commented Mar 6, 2021

@muglug I'm kinda lost
The else part seems to correctly infer that the type is null: https://psalm.dev/r/82732b730e

However, there is no assert-if-false in the stub like in your example above:
https://github.com/vimeo/psalm/blob/master/stubs/Reflection.phpstub#L108

How does that works then?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/82732b730e
<?php
    $method = new ReflectionMethod(stdClass::class);
    $parameters = $method->getParameters();
    foreach ($parameters as $parameter) {
        if ($parameter->hasType()) {
            $parameter->getType()->__toString();
        }
        else{
            $parameter->getType()->__toString();
        }
    }
Psalm output (using commit 65f0fb0):

ERROR: NullReference - 9:36 - Cannot call method __toString on null value

@weirdan
Copy link
Collaborator

weirdan commented Mar 6, 2021

How does that works then?

I mentioned it here: #5248 (comment)
Looks like a bug to me.

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.

ReflectionParameter::hasType() should assert return type of ReflectionParameter::getType()
5 participants