[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15984605#comment-15984605
]
Dawid Weiss commented on LUCENE-7796:
-------------------------------------
I agree we should rename the method.
bq. By the way, I think this is incredibly risky for the benefit.
I considered a few options and ended up with this one. Look at the patch -- any
use case where reThrow was a fall through on null seems weird to me. Sure,
shorter by a bit, but potentially confusing to those looking at it (and the
compiler). Maybe it's just method naming (rethrowIfNotNull) in a few places,
but in many others (where it's never a guaranteed no-fall-through) it just
helps to understand what's going on so much better -- you just see that this
method never returns (and have an assertion for non-null argument).
I really wish there was a different compiler trick/ hint to do this, but I
don't know of any. Again -- looking at the patch and oddball comments ("does
nothing if null, can never reach here, javac disagrees") I think it's more
beneficial if the behavior is stated in code rather than those comments.
In fact, when you look at the current code it's not even always correct with
this method, as it was here:
{code}
} catch (ExecutionException ee) {
- IOUtils.reThrow(ee.getCause());
-
- // dead code but javac disagrees:
- result = null;
+ // Theoretically cause can be null; guard against that.
+ Throwable cause = ee.getCause();
+ throw IOUtils.rethrowAlways(cause != null ? cause : ee);
{code}
The above could fall through to result = null. Even if you didn't know about it
and wrote:
{code}
} catch (ExecutionException ee) {
throw IOUtils.rethrowAlways(ee.getCause());
{code}
it'd fail with an assertion error, hopefully eagerly somewhere during tests.
Sure, we have to test it and let it bake (on master). Both this issue and
LUCENE-7800 cover the "unlikely" scenarios, but I think they improve code
aesthetics and legibility. Whether they're worth committing is probably a
subjective decision because both versions "work" unless somebody screws up. To
me, the patched version is harder to screw up (and easier to understand
execution-flow wise).
> Make reThrow idiom declare RuntimeException return type so callers may use it
> in a way that compiler knows subsequent code is unreachable
> -----------------------------------------------------------------------------------------------------------------------------------------
>
> Key: LUCENE-7796
> URL: https://issues.apache.org/jira/browse/LUCENE-7796
> Project: Lucene - Core
> Issue Type: Improvement
> Reporter: Dawid Weiss
> Assignee: Dawid Weiss
> Priority: Trivial
> Fix For: 6.x, master (7.0)
>
> Attachments: LUCENE-7796.patch
>
>
> A spinoff from LUCENE-7792: reThrow can be declared to return an unchecked
> exception so that callers can choose to use {{throw reThrow(...)}} as an
> idiom to let the compiler know any subsequent code will be unreachable.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
