PSR2.ControlStructures.SwitchDeclaration.TerminatingComment does not handle try/finally blocks

[umherirrender]

Describe the bug
Using the sniff PSR2.ControlStructures.SwitchDeclaration brings in an edge case for the error code TerminatingComment when having try/finally in the switch case body

Code sample

<?php

$statusValue = mt_rand();
$resource = SomeClass::getWorker();

switch ( $statusValue ) {
	case 0:
		// fall-through
	case 2:
		try {
			return $resource->doWork();
		} finally {
			$resource->release();
		}
	case 3:
		$other = $code;
		break;
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
  <rule ref="PSR2.ControlStructures.SwitchDeclaration" />
</ruleset>

To reproduce
Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs test.php ...
3. See error message displayed

 9 | ERROR | There must be a comment when fall-through is intentional
   |       | in a non-empty case body
   |       | (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)

Expected behavior
The try/finally always returns, that means there is no fall-through which does not need a comment.
That also the same when using finally + catch or catch alone

Versions (please complete the following information):

• OS: Windows 10
• PHP: 8.0
• PHPCS: 3.6.0
• Standard: -

Additional context

[jrfnl]

The try/finally always returns, that means there is no fall-through which does not need a comment.

Sorry, but I think you may be misunderstanding how finally works. finally only returns when it contains a return statement.
Yes, it will always be executed, but no, it does not end the switch execution.

See this simple demonstration: https://3v4l.org/t2pt6

In other words, the fact that the sniff demands a "fall through" comment is 100% correct and in my opinion, this issue can be closed.

[umherirrender]

In my example the try block always returns and in that case there is no fall-through, because the switch execution is ended by the return inside the try block.

If the try block does not always returns than the comment is needed, that is correct, but not the issue here.

https://3v4l.org/a4nlI

[jrfnl]

@umherirrender Sorry, help me out - the try is there to catch a possible Exception, so the return in the try may not be reached and the code does not catch whatever Exception you are expecting and the finally - which will always be executed - does not contain a return....

So the only reason the code does not fall through to case 3 is that the try was successful and happened not to encounter an Exception and if it would have encountered an Exception, the only reason the code doesn't fall through to case 3, is that the program stops with an uncaught Exception.

[umherirrender]

If the exception (explicit thrown in the try block or implicit within its called function) is uncatched, the switch is also ended. No catch block would throw the exception to the caller of the function with the switch (after the finally block is executed).
https://3v4l.org/hYTqu

No catch block does not mean the exception is ignored, that needs an catch block without terminate statement (return or throw)
https://3v4l.org/D4KMS

[jrfnl]

@umherirrender Okay, so if we want to address this, what would be rules need to be ?

• If there is a finally statement and the body of that contains a "terminating statement" (return, break etc), we don't need to check the body of the try or any of the catch statements as, no matter what, all execution branches are then covered by that one terminating statement.
• If there is no finally or if there is, but it does not contain a terminating statement, then all try and catch structures should each contain a terminating statement in the body.

Does that sound about right ? Am I still overlooking something ?

[jrfnl]

@umherirrender I've added a fix for this to PR #3354 which was open with another fix for the same sniff. Testing appreciated.