rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.59k stars 680 forks source link

Incorrect behavior of NarrowUnionTypeDocRector, TypedPropertyRector, UnionTypesRector, AddArrayParamDocTypeRector, AddArrayReturnDocTypeRector, ReturnTypeDeclarationRector, PropertyTypeDeclarationRector #6717

Closed tacman closed 2 years ago

tacman commented 2 years ago

Bug Report

Subject Details
Rector version last dev-master
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.org/demo/1ec22611-9811-6b06-afcb-5d27e21096cc

<?php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Gedmo\Mapping\Annotation as Gedmo;

/**
 * @ORM\Entity(repositoryClass="App\Repository\LocationRepository")
 */
class Loc
{

    /**
     * @return mixed
     */
    public function getChildren()
    {
        return $this->children;
    }

    /**
     * @param mixed $children
     * @return Location
     */
    public function setChildren($children)
    {
        $this->children = $children;
        return $this;
    }

    /**
     * @ORM\ManyToOne(targetEntity=Loc::class, inversedBy="children", cascade={"persist"}, fetch="LAZY")
     * @ORM\JoinColumn(name="parent_id", referencedColumnName="id", onDelete="CASCADE")
     */
    private $parent;

    /**
     * @ORM\OneToMany(targetEntity=Loc::class, mappedBy="parent", cascade={"persist", "remove"}, fetch="LAZY")
     */
    private $children;

}

Responsible rules

Expected Behavior

     /**
      * @ORM\OneToMany(targetEntity=Loc::class, mappedBy="parent", cascade={"persist", "remove"}, fetch="LAZY")
+     * @var \App\Entity\Loc[]|Collection|mixed|null
      */
-    private $children;
+    private Collection $children = null; 

}

should be

+    private ?Collection $children = null; 
OR
+    private Collection $children;

but + private Collection $children = null; isn't possible, should never be generated.

Curiously, when I remove the getChildren() and setChildren methods, the newly generated code does not include the null.

TomasVotruba commented 2 years ago

Thank you for your report and demo link!

Could you send a failing test case in a pull-request, so we have it covered in Rector? Here is step by step tutorial how to add it: https://github.com/rectorphp/rector/blob/master/docs/how_to_add_test_for_rector_rule.md

samsonasik commented 2 years ago

For multiple rule usage, you can create failing test case under https://github.com/rectorphp/rector-src/tree/main/tests/Issues

rajyan commented 2 years ago

I had looked into this issue and seems only TypedPropertyRector::class is the cause? https://getrector.org/demo/1ec2a32c-e189-6fe8-9555-cd790c8cae29

rajyan commented 2 years ago

simplified further https://getrector.org/demo/1ec2a341-4ac4-62ec-aa78-d77e99f15e33

tacman commented 2 years ago

https://getrector.org/demo/1ec2a341-4ac4-62ec-aa78-d77e99f15e33

Is still incorrect.

should be

private Collection $children;

OR

private ?Collection $children = null;

On Mon, Oct 11, 2021 at 2:31 AM Tomas Votruba @.***> wrote:

Closed #6717 https://github.com/rectorphp/rector/issues/6717 via rectorphp/rector-src#986 https://github.com/rectorphp/rector-src/pull/986.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rectorphp/rector/issues/6717#event-5441016138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEXIQNVPT4RR5N2YYV5QNLUGKADFANCNFSM5FDVRB2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

samsonasik commented 2 years ago

demo page is using tagged version, you can check locally with "rector/rector": "dev-main"

tacman commented 2 years ago

Yep, it works now! Thanks!!

On Mon, Oct 11, 2021 at 8:02 AM Abdul Malik Ikhsan @.***> wrote:

demo page is using tagged version, you can check locally with "rector/rector": "dev-main"

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rectorphp/rector/issues/6717#issuecomment-939964888, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEXIQNT242DGOUS36VQF6LUGLG4FANCNFSM5FDVRB2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.