Skip to content

Infer private properties in constructor, even if typehinted#374

Merged
ondrejmirtes merged 2 commits intophpstan:masterfrom
Baldinof:infer-phpdoc-on-typed-props
Dec 9, 2020
Merged

Infer private properties in constructor, even if typehinted#374
ondrejmirtes merged 2 commits intophpstan:masterfrom
Baldinof:infer-phpdoc-on-typed-props

Conversation

@Baldinof
Copy link
Copy Markdown
Contributor

@Baldinof Baldinof commented Nov 15, 2020

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
Copy Markdown
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 :)

ondrejmirtes
ondrejmirtes requested changes Dec 8, 2020
View reviewed changes
tests/PHPStan/Analyser/NodeScopeResolverTest.php Outdated
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Dec 8, 2020

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.

tests/PHPStan/Analyser/data/infer-private-typed-property-type-from-constructor.php Outdated
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Dec 8, 2020

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
Copy Markdown
Contributor Author

@Baldinof Baldinof Dec 8, 2020

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
Copy Markdown
Member

ondrejmirtes commented Dec 9, 2020

Thank you!

@Baldinof
Copy link
Copy Markdown
Contributor Author

Baldinof commented Dec 9, 2020

Thank you for building and maintaining this awesome project :)

@ondrejmirtes ondrejmirtes mentioned this pull request Dec 14, 2020
Closed
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Dec 15, 2020

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
Copy Markdown
Contributor Author

Baldinof commented Dec 15, 2020

No problem, sorry for the troubles :(

@snapshotpl
Copy link
Copy Markdown
Contributor

snapshotpl commented Dec 15, 2020

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

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Dec 15, 2020

@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

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Baldinof @ondrejmirtes @snapshotpl