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

implemented TypedPropertyFromStrictConstructorReadonlyClassRector #4552

Merged
merged 17 commits into from
Jul 23, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 20, 2023

In TypedPropertyFromStrictConstructorRector before this PR we skipped over public properties.

after this PR, we infer native types in @immutable phpdoc typed classes.

the assumption is, that no external code should exist which writes into the public properties

Feature description https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-readonly-and-readonly
PHPStan impl phpstan/phpstan-src#1295

private function skipPublicProperty(Node\Stmt\Property $property, ClassReflection $classReflection, Scope $scope): bool
{
if ($property->isPublic()) {
$isReadOnlyByPhpdoc = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only care about readonly phpdoc, because native readonly classes cannot have non-typed-properties by definition


namespace Rector\Tests\TypeDeclaration\Rector\Property\TypedPropertyFromStrictConstructorRector\Fixture;

class ReadonlyPublicProperty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test-case is not super realistic, but since the readonly flag is only enforced via static analysis its a case which can happen

@staabm staabm marked this pull request as ready for review July 20, 2023 12:52
@staabm staabm requested a review from TomasVotruba as a code owner July 20, 2023 12:52
@samsonasik
Copy link
Member

I think its worth new rule instead to ease improve/skip the rule.

More and more tweak on docblock is harder to patch when found a bug.

@staabm staabm marked this pull request as draft July 23, 2023 11:17
@staabm staabm changed the title Support immutable classes in TypedPropertyFromStrictConstructorRector implemented TypedPropertyFromStrictConstructorReadonlyClassRector Jul 23, 2023
@staabm staabm marked this pull request as ready for review July 23, 2023 11:49
@staabm
Copy link
Contributor Author

staabm commented Jul 23, 2023

separated the immutable handling in a new rule

@TomasVotruba
Copy link
Member

This is very nice, thank you 👏

@TomasVotruba TomasVotruba merged commit a8ae2d8 into rectorphp:main Jul 23, 2023
@staabm staabm deleted the immutable branch July 23, 2023 16:18
@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 19, 2023

I'm looking into more complex rules spotted in #5025. It seems this one is going back to the doblock-based logic we try to avoid from.

The Rector upgrade path is based on PHP native features, so the motivation is to rise the PHP version, instead of "backporting" features using markers. Saying that, I'll deprecate this rule and it should be made private, if needed, not to promote docblock based logic.

My mistake this got through the review :) 🙏

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