Incorrect behavior of AddVoidReturnTypeWhereNoReturnRector - throw prevents refactor

[krossekrabbe]

Bug Report

As soon as a method throws an exception it prevents AddVoidReturnTypeWhereNoReturnRector from adding a void return type to its signature.

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

Minimal PHP Code Causing Issue

See https://getrector.org/demo/6e6aaeee-2e84-41ef-8570-cbc2d75dfe3a

<?php

class DemoFile
{
    private function run()
    {
    }
    
    private function runWithException()
    {
        if (true) {
            return;
        }
        
        throw new \Exception();
    }
}

Responsible rules

• AddVoidReturnTypeWhereNoReturnRector

Expected Behavior

 class DemoFile
 {
-    private function run()
+    private function run(): void
     {
     }
-    private function runWithException()
+    private function runWithException(): void
     {

[samsonasik]

throw Exception should return never type, there is ReturnNeverTypeRector for that, ref https://getrector.org/demo/e9980211-c436-408e-8c5e-d7a18452364c

[krossekrabbe]

@samsonasik Sorry I was just in the process of updating my example because I've noticed it probably should better be never in that case. Can you check my updated example please? Should it still not add void in that case?

[samsonasik]

AddVoidReturnTypeWhereNoReturnRector is using SilentVoidResolver service, which check against never type as well, which if found, it just skipped

https://github.com/rectorphp/rector-src/blob/5ec4cff203f68d9578719b54ce3a145efb32bc79/rules/TypeDeclaration/TypeInferer/SilentVoidResolver.php#L37

I think skip is valid to avoid cross rules apply invalid result.

[krossekrabbe]

Hmm, well I am not deep enough in the topic to assess if it is necessary to avoid rule conflicts, but unfortunately this makes a whole lot of methods skipped in our code base :-D

Only to clarify again, that hasNeverType in SilentVoidResolver makes any function containing a throw at any point skipped, even if it only conditionally throws an exception. Here's another real life example:

https://getrector.org/demo/93036d67-5a0b-463c-b9b1-8a184d3df8a1

    // not refactored to :void
    public function touchFile(string $path)
    {
        if (!touch($path)) {
            throw new \Exception('The path could not be touched');
        }
    }

[krossekrabbe]

But thanks for pointing out that SilentVoidResolver to me @samsonasik, I'll probably try to adjust it to ignore the never type stuff so I can finish the refactoring. I guess there are almost no actual : never functions in our codebase anyway.

[TomasVotruba]

This case would be hard to detect, as both void and never would be false positives here. That's why Rector skips it :) I'm closing this to keep current default functionality.

If you fancy, you make this rule optionally configurable to skip the exception detection and always go for void return type. That we could accept :wink: