Skip to content

[DX] Remove *AnnotationIncorrectNullableRector rules as works with unreliable docblocks and can have 2 solutions#4719

Merged
TomasVotruba merged 6 commits intomainfrom
tv-less-docblock
Aug 8, 2023
Merged

[DX] Remove *AnnotationIncorrectNullableRector rules as works with unreliable docblocks and can have 2 solutions#4719
TomasVotruba merged 6 commits intomainfrom
tv-less-docblock

Conversation

@TomasVotruba
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba commented Aug 8, 2023

These rules work with unreliable docblocks and can have 2 solutions.

Since Rector 0.15 we declare type stricness and avoid docblock changes, as unreliable and could lead to incorrect assumptions: https://getrector.com/blog/new-in-rector-015-complete-safe-and-known-type-declarations

These are one of few forgotten rules. Instead handle manually and let PHPStan help you decide 👍

@TomasVotruba TomasVotruba changed the title tv less docblock [DX] Remove ParamAnnotationIncorrectNullableRector as works with unreliable docblocks and can have 2 solutions Aug 8, 2023
@TomasVotruba TomasVotruba enabled auto-merge (squash) August 8, 2023 13:12
/**
* @see \Rector\Tests\TypeDeclaration\Rector\ClassMethod\ParamAnnotationIncorrectNullableRector\ParamAnnotationIncorrectNullableRectorTest
*/
final class ParamAnnotationIncorrectNullableRector extends AbstractRector implements MinPhpVersionInterface
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are 3 rules that fix incorrect nullable:

ReturnAnnotationIncorrectNullableRector
ParamAnnotationIncorrectNullableRector
VarAnnotationIncorrectNullableRector

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see, I'll add them to this removal list 👍 Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, I can see these rules are actually good, this example verify based on typed existing param, eg:

     /**
-     * @param \DateTime[] $dateTimes
+     * @param \DateTime[]|null $dateTimes
      */
     public function setDateTimes(?array $dateTimes): self

@TomasVotruba TomasVotruba disabled auto-merge August 8, 2023 13:13
@samsonasik
Copy link
Copy Markdown
Member

/cc @dorrogeray FYI

@TomasVotruba TomasVotruba changed the title [DX] Remove ParamAnnotationIncorrectNullableRector as works with unreliable docblocks and can have 2 solutions [DX] Remove *Param*AnnotationIncorrectNullableRector rules as works with unreliable docblocks and can have 2 solutions Aug 8, 2023
@TomasVotruba TomasVotruba changed the title [DX] Remove *Param*AnnotationIncorrectNullableRector rules as works with unreliable docblocks and can have 2 solutions [DX] Remove *AnnotationIncorrectNullableRector rules as works with unreliable docblocks and can have 2 solutions Aug 8, 2023
@TomasVotruba TomasVotruba merged commit 3de7c69 into main Aug 8, 2023
@TomasVotruba TomasVotruba deleted the tv-less-docblock branch August 8, 2023 13:21
@samsonasik
Copy link
Copy Markdown
Member

samsonasik commented Aug 8, 2023

@TomasVotruba actually, I can see these rules are actually good, this example verify based on typed existing param, eg:

     /**
-     * @param \DateTime[] $dateTimes
+     * @param \DateTime[]|null $dateTimes
      */
     public function setDateTimes(?array $dateTimes): self

probably you can reconsider it?

@TomasVotruba
Copy link
Copy Markdown
Member Author

I was thinking about the usefulness and they can preserve the state like example. The question is, often the nullability is not desired and code should be changed differently:

     /**
     * @param \DateTime[] $dateTimes
      */
-     public function setDateTimes(?array $dateTimes): self
+     public function setDateTimes(array $dateTimes): self

Same applies for var/return cases.

Instead of silently preserving wrong state, it should be handled by developer to fits exact needs. That's why I think it's better let PHPStan report these and people to decide what way should be taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants