Skip to content

Infer private properties in constructor, even if typehinted #374

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

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

Baldinof
Copy link
Contributor

Hi!

When using the flag inferPrivatePropertyTypeFromConstructor: true, the inference is disabled if the property is natively typehinted.

However the inference could provide a more specific type (eg: generic).

A usecase is ORM:

class UserRepository
{
    private DatabaseClient $db;

    public function __construct(DatabaseClientProvider $dbClientProvider)
    {
        $this->db = $dbClientProvider->for(User::class);
    }

    public function get(string $id): User
    {
         return $this->db->query('...');
    }
}

This PR allows PHPStan to infer DatabaseClient<User> instead of just DatabaseClient for the $db property.

@Baldinof Baldinof force-pushed the infer-phpdoc-on-typed-props branch 2 times, most recently from a11f084 to 446647a Compare November 16, 2020 09:20
@Baldinof Baldinof marked this pull request as draft November 16, 2020 09:21
@Baldinof Baldinof force-pushed the infer-phpdoc-on-typed-props branch 2 times, most recently from 5855971 to 29d4260 Compare November 17, 2020 08:31
@Baldinof Baldinof marked this pull request as ready for review November 17, 2020 08:56
@Baldinof
Copy link
Contributor Author

Baldinof commented Dec 8, 2020

Hi @ondrejmirtes, as this PR started as a draft, I don't know if you received a notification when I marked it as ready for review, so I am just raising a hand in case :)

@@ -10615,6 +10615,41 @@ public function testInferPrivatePropertyTypeFromConstructor(
);
}

public function dataInferPrivateTypedPropertyTypeFromConstructor(): array
Copy link
Member

Choose a reason for hiding this comment

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

The more modern way of testing such stuff is done in NodeScopeResolverTest::testFileAsserts(), please rework your tests based on that.

public function __construct(string ...$typedArray)
{
$this->genericFoo = $this->newGenericFoo(\stdClass::class);
$this->typedArray = $typedArray;
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing a test scenario here - the property has native type int, the constructor assigns type string. I'm interested in what type the final property has - should be int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I added some cases, it seems TypehintHelper is doing a very good job.

Do you see other scenario where it could misbehave?

@Baldinof Baldinof force-pushed the infer-phpdoc-on-typed-props branch from 29d4260 to 565d41a Compare December 8, 2020 22:45
@ondrejmirtes ondrejmirtes merged commit 31a17c3 into phpstan:master Dec 9, 2020
@ondrejmirtes
Copy link
Member

Thank you!

@Baldinof
Copy link
Contributor Author

Baldinof commented Dec 9, 2020

Thank you for building and maintaining this awesome project :)

@ondrejmirtes
Copy link
Member

I'm sorry, I had to revert this. It led to some undesired behaviour that's hard to avoid (while keeping the rest of the functionality): phpstan/phpstan#4234

@Baldinof
Copy link
Contributor Author

No problem, sorry for the troubles :(

@snapshotpl
Copy link
Contributor

Damn, that was great feature to reduce phpdocs. @ondrejmirtes do you have a plan to bring it back with fixes?

@ondrejmirtes
Copy link
Member

@snapshotpl It still works when the property doesn't have a native typehint. I don't plan to bring it back when the property has a native typehint, the need for PHPDocs is reduced greatly in that case, and the feature led to some false positives...

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