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

Support @readonly phpdoc #1295

Merged
merged 10 commits into from
May 18, 2022
Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 7, 2022

Same as psalm does it, see https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-readonly-and-readonly

I guess this would be very useful for pre PHP 8.1 and/or to safely enable native readonly. But I think this is also the first step in the direction of a @phpstan-immutable phpdoc on class level, but that's a different thing.

If I understood things right this should not interfere with some of the other rules that are related to native readonly because the phpdoc is not used in https://github.com/phpstan/phpstan-src/blob/1.6.7/src/Node/ClassPropertyNode.php#L77. I added tests to check that where it made sense to me. irrelevant, because the phpdoc rules were separated later on :)

Refs: phpstan/phpstan#4082

@staabm
Copy link
Contributor

staabm commented May 7, 2022

I had a discussion in the past about implementing it.

These are the main directions, which ondrey gave me:

My thinking is that there are separate rules: for uninitialized properties that are opt-in with a config option
for native readonly properties checked on PHP 8.1+
and a different (but similar) rule would have to be added for
@readonly PHPDoc properties.
but it's less relevant over time as people upgrade their PHP version and can start using the native feature.
if you look at UninitializedPropertyRule + ReadOnlyPropertyAssignRule + MissingReadOnlyPropertyAssignRule you can see the differences and the similarilties

I don't know whether this information is still valid as is... just to let you know.

@herndlm herndlm marked this pull request as ready for review May 7, 2022 19:48
@herndlm herndlm marked this pull request as draft May 7, 2022 19:53
@herndlm herndlm force-pushed the support-readonly-phpdoc branch 2 times, most recently from 8372aaa to 60d6be2 Compare May 7, 2022 20:08
@herndlm herndlm marked this pull request as ready for review May 7, 2022 20:14
@herndlm herndlm changed the base branch from 1.6.x to 1.7.x May 8, 2022 17:19
@herndlm herndlm force-pushed the support-readonly-phpdoc branch 2 times, most recently from f070817 to 1812e67 Compare May 8, 2022 17:20
@ondrejmirtes
Copy link
Member

I think for this to work, we need a new method PhpPropertyReflection::isReadOnlyByPhpDoc() and PHPStan\Node\ClassPropertyNode::isReadOnlyByPhpDoc() and a new set of rules (or modified implementation of the current ones with a new set of tests):

  • UninitializedPropertyRule
  • MissingReadOnlyPropertyAssignRule
  • ReadOnlyPropertyAssignRefRule
  • ReadOnlyPropertyAssignRule

I really want to separate the rules for "this is native readonly keyword" and "this is done by PHPDoc" as they can diverge over time. I don't want the current isReadOnly() methods to also report true for PHPDocs.

@herndlm herndlm marked this pull request as draft May 9, 2022 09:33
@herndlm
Copy link
Contributor Author

herndlm commented May 14, 2022

I separated native and phpdoc readonly, as requested, and for simplicity I chose to adapt the current existing rules. I also added new test sets that are basically duplicates of the current ones minus PHP 7.4+ features (native property types, native readonly, enums).

Good idea with the separation for sure, see #1295 (comment) where I decided to not add phpdoc readonly support for MissingReadOnlyPropertyAssignRule and UninitializedPropertyRule because things start to get mixed up with native readonly. I could add support back without that weird change, it would only work combined with native type hints if I'm not mistaken. Maybe that's fine after all. I can't decide :)

What do you think, is this going in the right direction?

@herndlm herndlm marked this pull request as ready for review May 14, 2022 21:35
@herndlm herndlm force-pushed the support-readonly-phpdoc branch 3 times, most recently from 553d42b to 1db1656 Compare May 16, 2022 15:20
@herndlm
Copy link
Contributor Author

herndlm commented May 16, 2022

Sorry for my impatience, I hate that xD
But since I see the 1.7 release nicely finishing up - do you think this can still be squeezed in? I'll prioritize any requested changes

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 17, 2022

do you think this can still be squeezed in? I'll prioritize any requested changes

Let me assure you that you can keep calm because it mostly doesn't matter :)

  • No one says that PHPStan 1.8.0 cannot be released like 5 days after 1.7.0 :)
  • No one says that this PR cannot be released in 1.7.1 for example :)

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

where I decided to not add phpdoc readonly support for MissingReadOnlyPropertyAssignRule and UninitializedPropertyRule

With my proposal of separate rules, this should probably be done, right?

src/Analyser/NodeScopeResolver.php Outdated Show resolved Hide resolved
src/Rules/Properties/ReadOnlyPropertyAssignRefRule.php Outdated Show resolved Hide resolved
src/Rules/Properties/ReadOnlyPropertyAssignRule.php Outdated Show resolved Hide resolved
…rules to bleeding edge, use consistent naming for methods and props
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Really nice! Almost there :)

conf/config.level0.neon Outdated Show resolved Hide resolved
src/Node/ClassPropertyNode.php Outdated Show resolved Hide resolved
src/PhpDoc/PhpDocNodeResolver.php Outdated Show resolved Hide resolved
@ondrejmirtes ondrejmirtes merged commit 9eef412 into phpstan:1.7.x May 18, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@herndlm herndlm deleted the support-readonly-phpdoc branch May 18, 2022 10:40
@ondrejmirtes
Copy link
Member

Can you please send an update to this page @herndlm? https://phpstan.org/writing-php-code/phpdocs-basics

Thank you!

@herndlm
Copy link
Contributor Author

herndlm commented May 18, 2022

I'll update the docs tomorrow, have fun in Verona!

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