Incorrect behavior of ReturnTypeDeclarationRector

[genesiscz]

Bug Report

Subject	Details
Rector version	0.8.56, demo ver ?
Installed as	composer dependency

Minimal PHP Code Causing Issue

See https://getrector.org/demo/9cba5a13-fa55-4f06-880a-ccdd8259db78

<?php

class DemoFile
{
    public function run()
    {
        throw new Exception("not implemented");
    }
}

class DemoFile2 extends DemoFile {
    
    public function run() : Baba {
       return new Baba();        
    }
}

class Baba {
}

Rector diff:

 
 class DemoFile2 extends DemoFile {
     
-    public function run() : Baba {
+    public function run() : void {
        return new Baba();        
     }
 }
 
 class Baba {
}

Responsible rules

• ReturnTypeDeclarationRector

Expected Behavior

Should be skipped.

In my own code it has a little bit different behaviour for some reason: it does add : void to the throwing function as a return type, which makes the code screaming about compatibility between the overriden function.

[genesiscz]

Ok I got the first part of the issue narrowed down. I had app/model/XXAuthenticator.php files while having App\Authenticators namespace not properly mapped in PSR-4. Sorry for that. But after I made it right, I am getting the following

1) app/Authenticators/BaseAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -44,7 +44,7 @@
        /**
         * @param string ...$credentials
         */
-       public function login(...$credentials)
+       public function login(...$credentials): void
        {
                throw new Security\AuthenticationException();
        }
    ----------- end diff -----------


Applied rules:

 * Rector\TypeDeclaration\Rector\FunctionLike\ReturnTypeDeclarationRector


2) app/Authenticators/FacebookAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -7,7 +7,7 @@
 class FacebookAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                [$fbid, $access_token] = $credentials;
    ----------- end diff -----------


3) app/Authenticators/PasswordAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -9,7 +9,7 @@
 class PasswordAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                if (count($credentials) !== 2) {
                        throw new AException("Internal server error");
    ----------- end diff -----------


4) app/Authenticators/PinAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -9,7 +9,7 @@
 class PinAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                if (count($credentials) !== 1) {
                        throw new AException("Internal server error");
    ----------- end diff -----------


5) app/Authenticators/QRAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -9,7 +9,7 @@
 class QRAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                if (count($credentials) !== 1) {
                        throw new AException("Internal server error");
    ----------- end diff -----------


6) app/Authenticators/TicketAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -8,7 +8,7 @@
 class TicketAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                if (count($credentials) !== 1) {
                        throw new AException("No ticket data");
    ----------- end diff -----------


7) app/Authenticators/UIDAuthenticator.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -7,7 +7,7 @@
 class UIDAuthenticator extends BaseAuthenticator
 {

-       public function login(...$credentials): Identity
+       public function login(...$credentials): void
        {
                [$uid] = $credentials;
                $row = $this->drivers->get($uid);
    ----------- end diff -----------


                                                                                                                        
 [OK] Rector is done! 7 files would have changed (dry-run).                                                             
                             

Here's an example of UIDAuthenticator:

namespace App\Authenticators;

use Nette\Security\Identity;

class UIDAuthenticator extends BaseAuthenticator
{

	public function login(...$credentials): Identity
	{
		[$uid] = $credentials;
		$row = $this->drivers->get($uid);

		return $this->checkRowAndReturnIdentity($row);
	}

}

and checkRowAndReturnIdentity function:

	public function checkRowAndReturnIdentity(?Driver $row): Identity
	{
		if (!$row) {
			throw new AException('User does not exist.', IAuthenticator::IDENTITY_NOT_FOUND);
		}

		$arr = $row->toArray();

		if ($row->active == 0) {
			throw new AException("You've been deactivated. Please, contact our helpdesk");
		}

		$row->update(["last_active_timestamp" => time()]);
		unset($arr["password"]);
		$arr["uid"] = $arr["id"];
		$identity = new Identity($row->id, $row->user_type, $arr);
		$this->user->login($identity);

		return $identity;
	}

[genesiscz]

The same happens with App\Models\BaseModel, which extends from Illuminate\Database\Eloquent\Model.

Eloquent model has this signature:

    /**
     * Save the model to the database.
     *
     * @param  array  $options
     * @return bool
     */
    public function save(array $options = [])

BaseModel has this one:

    /**
     * Save model, but not when not dirty
     *
     * @param string[] $options
     */
    public function save($options = [])
    {

gets changed to

    ---------- begin diff ----------
--- Original
+++ New
@@ -99,7 +99,7 @@
      *
      * @param string[] $options
      */
-    public function save($options = [])
+    public function save($options = []): bool
     {
    ----------- end diff -----------

I guess this happens because of the inferred return type from vendor's file (or maybe because of @return bool on the parent class), but obviously I can't do this as it does produce a compatibility error.

Maybe a little related:
#2509

[TomasVotruba]

Rector0.8 is not supported anymore. How does 0.9 perform here?

If it still fails, we'll need a failing test fixture for particular rule to isolate the problem.

[genesiscz]

Honestly I am not sure about the multiple namespace declaration - should that be a part of the fixture? I removed them but if anything is wrong, I can change that.