[PRE-RFC] Deprecate returning from a finally block

Hi all,

I'd like to gauge interest in deprecating return inside a finally block, before I request karma to propose it as a full RFC.

A return inside a finally silently discards whatever the try/catch was doing...a pending exception or return value just disappears, with no error or notice.

As far as I know, it's the only abrupt exit from a finally PHP leaves silent...
break/continue/goto out of one are already compile errors, and a throw auto-chains the discarded exception as $previous. Only return destroys the in-flight state and says nothing.

Looking into the history of finally I found that the author of the original RFC said he added it only because Java allowed it, even though he thought it made "no sense" https://externals.io/message/61670#61678
But Java itself warns about it (javac -Xlint:finally).

I also found that it was the source of a few bugs:

• https://bugs.php.net/bug.php?id=70228
• https://bugs.php.net/bug.php?id=72213
• https://github.com/php/php-src/issues/11028

To see what a change would actually cost, I implemented the deprecation and ran it over the top ~5000 Composer packages. And I found that it's rare...there were only 12 occurrences across 9 packages out of 4992. Of those, 3 look like genuine latent bugs the deprecation caught, and the rest look deliberate.
Here's the list of the occurrences:

• ibexa/admin-ui (swallows the exceptions its own docblock declares)
• shopware/core (eats thumbnail failures and still reports success)
• amphp/http-server (drops exceptions when the stream is already gone)
• dvdoug/PHPCoord (deliberate...finally is the method's return)
• dvdoug/PHPCoord (again)
• dvdoug/PHPCoord (again)
• dvdoug/PHPCoord (again)
• spatie/ray (delibrate...any failure just returns false)
• cakephp/cakephp (catch consumed the exception...finally just returns array)
• hyperf/http-server (catch consumed the exception...finally emits response)
• silverstripe/framework (catch consumed the mysqli error)
• symfony/flex (try can't actually throw)

My take here is that the language already handles the analogous cases (continue/break/goto/...etc) natively, so leaving return looks like an inconsistency.

I lean toward deprecating it now with an eye to an error in a future major (following the steps of Tim's deprecate-return-from-constructor RFC)...though I guess a plain warning would be fine too. The main thing is that it seems worth doing something about.

Is there appetite for this?

Thanks,
Osama

I would be in favour of this, it should be noted that C# does not permit "exiting" a finally block via control flow statements: [1]

It is a compile-time error for a break, continue, or goto statement to transfer control out of a finally block. When a break, continue, or goto statement occurs in a finally block, the target of the statement shall be within the same finally block, or otherwise a compile-time error occurs.

It is a compile-time error for a return statement to occur in a finally block.

Best regards,
Gina P. Banyard
[1] https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/statements#1311-the-try-statement

Hi all,

I'd like to gauge interest in deprecating return inside a finally
block, before I request karma to propose it as a full RFC.

A return inside a finally silently discards whatever the
try/catch was doing...a pending exception or return value just
disappears, with no error or notice.

As far as I know, it's the only abrupt exit from a finally PHP leaves
silent...
break/continue/goto out of one are already compile errors, and a
throw auto-chains the discarded exception as $previous. Only
return destroys the in-flight state and says nothing.

Looking into the history of finally I found that the author of the
original RFC said he added it only because Java allowed it, even though
he thought it made "no sense" https://externals.io/message/61670#61678
But Java itself warns about it (javac -Xlint:finally).

I also found that it was the source of a few bugs:
• https://bugs.php.net/bug.php?id=70228
• https://bugs.php.net/bug.php?id=72213
• https://github.com/php/php-src/issues/11028
To see what a change would actually cost, I implemented the deprecation
https://github.com/php/php-src/compare/master...aldemeery:php-src:deprecate-return-in-finally
and ran it over the top ~5000 Composer packages. And I found that it's
rare...there were only 12 occurrences across 9 packages out of 4992. Of
those, 3 look like genuine latent bugs the deprecation caught, and the
rest look deliberate.

Here's the list of the occurrences:

1. ibexa/admin-ui (swallows the exceptions its own docblock declares)
   https://github.com/ibexa/admin-ui/blob/2322d54/src/bundle/Controller/ContentTypeController.php#L834
2. shopware/core (eats thumbnail failures and still reports success)
   https://github.com/shopware/core/blob/e8079a0/Content/Media/Thumbnail/ThumbnailService.php#L294
3. amphp/http-server (drops exceptions when the stream is already
   gone)
   https://github.com/amphp/http-server/blob/b306134/src/Driver/Http2Driver.php#L1322
4. dvdoug/PHPCoord (deliberate...finally is the method's return)
   https://github.com/dvdoug/PHPCoord/blob/ced02c4/src/Point/CompoundPoint.php#L131
5. dvdoug/PHPCoord (again)
   https://github.com/dvdoug/PHPCoord/blob/ced02c4/src/Point/GeocentricPoint.php#L148
6. dvdoug/PHPCoord (again)
   https://github.com/dvdoug/PHPCoord/blob/ced02c4/src/Point/GeographicPoint.php#L221
7. dvdoug/PHPCoord (again)
   https://github.com/dvdoug/PHPCoord/blob/ced02c4ad44aa4a558f69d53af4834a9d50ab2aa/src/Point/ProjectedPoint.php#L242-L247
8. spatie/ray (delibrate...any failure just returns false)
   https://github.com/spatie/ray/blob/2da2079/src/Client.php#L71
9. cakephp/cakephp (catch consumed the exception...finally just
   returns array)
   https://github.com/cakephp/cakephp/blob/eef91f28de119bee5536905244d2096f752f2920/src/Database/Query.php#L1748-L1755
10. hyperf/http-server (catch consumed the exception...finally emits
    response)
    https://github.com/hyperf/http-server/blob/80c52d4/src/Server.php#L140
11. silverstripe/framework (catch consumed the mysqli error)
    https://github.com/silverstripe/silverstripe-framework/blob/9c59c42/src/ORM/Connect/MySQLiConnector.php#L213
12. symfony/flex (try can't actually throw)
    https://github.com/symfony/flex/blob/4a6d98e/src/PackageResolver.php#L88
    My take here is that the language already handles the analogous cases
    (continue/break/goto/...etc) natively, so leaving return looks
    like an inconsistency.

I lean toward deprecating it now with an eye to an error in a future
major (following the steps of Tim's deprecate-return-from-constructor
RFC
https://wiki.php.net/rfc/deprecate-return-value-from-construct)...though
I guess a plain warning would be fine too. The main thing is that it
seems worth doing something about.

Is there appetite for this?

Thanks,
Osama

I think this makes sense, but it would definitely need to go through a Deprecation phase. I don't think it needs its own RFC; It can probably go into the omnibus deprecations RFC for this version. (Which should get going very soon.)

--Larry Garfield