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

Adding `@var` Should not be needed when the property is set by the constructor #1562

Closed
lyrixx opened this issue Oct 31, 2018 · 27 comments

Comments

Projects
None yet
6 participants
@lyrixx
Copy link

commented Oct 31, 2018

Hello

Summary of a problem or a feature request

With this code:

class Foobar
{
    private $httpClient;

    public function __construct(HttpClient $httpClient)
    {
        $this->httpClient = $httpClient;
    }

    public function fetchOrientationToken()
    {
        $this->httpClient->methodDoesNotExist();
    }
}

There are no errors

With this code:

class Foobar
{
    /** @var HttpClient */ // <----------- the difference is here 
    private $httpClient;

    public function __construct(HttpClient $httpClient)
    {
        $this->httpClient = $httpClient;
    }

    public function fetchOrientationToken()
    {
        $this->httpClient->methodDoesNotExist();
    }
}

A violation is raised

Code snippet that reproduces the problem

https://phpstan.org/r/a6aeb2711d867c2bda6e388ee515e9c4

Expected output

A violation should be raised in both situation

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

That's expected behavior. Solving this would require analyzing each class twice.

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

That's too bad. I really don't want to add @var XXX on each properties. This adds nothing for other developers, nor IDE, nor PHP.

Is fixing this something you want to consider ?

@iluuu1994

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@lyrixx It does. The type of the property can change at any time, it is not fixed after the constructor. Adding a @var pins the type and prevents you from setting it to something it shouldn't be.

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

It's just PHP Doc :)

You made an assumption here: It's not because @var is added that it could not change... So we could make the same assumption with the constructor.

@iluuu1994

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

It's just PHP Doc :)

No, if you're using PHPStan it's not just PHPDoc ;)
PHPStan will complain.

@iluuu1994

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Yes, It tested it too :)

See the first case : https://phpstan.org/r/1955eb69ba057ce8dfdb7e814368f6ff

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Could you consider adding a configuration option to "virtually" add @var XXX on top of each properties that is a constructor arguments?

It would be an opt-in option of course.

@iluuu1994

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

By the way, this has been proposed before:

#541
#615

@honzatrtik Did you ever open-source the extension mentioned here?

@dunglas

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Symfony’s coding standards state to not add the PHPDoc to a property if it can be inferred from the constructor, it is also the default configuration of PHP CS Fixer and it is considered a best practice by some.

It would be nice to have this option at least for interoperablility with other tools in the ecosystem.

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

I think that you should add @vars to your properties, it's future-proofing for PHP 7.4 where you will be able to type your properties natively. I'm quite sure that there will be a fixable sniff in slevomat/coding-standard that will move your typehints from phpDoc to native once 7.4 is out :)

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

I understand you don't want to add this option. I will live with it but I really wanted this feature.
More over as @dunglas said, we remove all theses PHP Doc in Symfony, and it's the same for the SF community.

Do you know if we can build a plugin to do that?

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

You might try to do something like I suggested in #615 (comment) but it's gonna be a big hack :)

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@lyrixx Would you expect the following code to report error?
https://phpstan.org/r/4315bb3d9a7f14278be47f2ffb775344

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

I would say yes

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Sure it's not easy. that's why I think the option should be opt-in and stupid. It's juste a 1-1 mapping between the property name, and the property given through the constructor

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Here we go: https://gist.github.com/lyrixx/72d0fddae44b07bd32b31b87a27afa6c

Thanks for the help and BTW your sofware is really flexible. Very easy to hook into 👍 Good job

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

The extension could be written in a much better way. For example you don't provide inference for scalar parameters. Also I'd shy away from using reflection and setAccessible.

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

I'm quite new to php stan internal ! That's why I would like to have this feature in the core :)

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

It's never gonna be in the core - the direction of PHP itself is different - typehint your properties properly :)

@lyrixx

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Sure, I really want to use PHP 7.4, and I will use typed properties for sure :)
But I have to wait a bit more than one year !

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@adrienbrault

This comment has been minimized.

Copy link

commented Nov 5, 2018

@ondrejmirtes Is there a way to issue some kind of warning/error if PHPStan doesn't know the type of something ?

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Good news! PHPStan 0.11.10 includes support for inferring private property type from constructor! https://github.com/phpstan/phpstan/releases/tag/0.11.10

@lyrixx

This comment has been minimized.

Copy link
Author

commented Jul 7, 2019

Thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.