[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15984575#comment-15984575
]
Dawid Weiss edited comment on LUCENE-7796 at 4/26/17 11:00 AM:
---
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places where rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think, especially [~hossman] since you provided an
alternative view on how this issue could be solved (which I don't disagree with
-- I just think this patch results in better code for now and the future).
was (Author: dweiss):
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places where rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think, especially [~hossman_luc...@fucit.org] since you
provided an alternative view on how this issue could be solved (which I don't
disagree with -- I just think this patch results in better code for now and the
future).
> 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: dev-unsubscr...@lucene.apache.org
For
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15984575#comment-15984575
]
Dawid Weiss edited comment on LUCENE-7796 at 4/26/17 10:59 AM:
---
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places where rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think, especially [~hossman_luc...@fucit.org] since you
provided an alternative view on how this issue could be solved (which I don't
disagree with -- I just think this patch results in better code for now and the
future).
was (Author: dweiss):
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think.
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980332#comment-15980332
]
Uwe Schindler edited comment on LUCENE-7796 at 4/23/17 9:12 AM:
Did you know that the {{Class#newInstance()}} also throws checked Exceptions
without declaring them! So the old code before using MethodHandles was wrong,
too! (Java 9 deprecated {{Class#newInstance()}} in favor of using Constructor
which wrapps). If you search for {{Class#newInstance()}} in LuSolr's source
code you see many more.
[http://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#newInstance--]
As said before I'd check the ctor on the initial reflection that it does not
throw checked Exceptions. The problem is that
{{MethodHandles#findConstructor()}} and the returned MethodHandle's MethodType
don't show the Exceptions. But with reflective {{jl.reflect.Constructor}}
instances this can be checked.
In general, with MethodHandle's you are leaving the type system anyways, so it
is always possible that types don't match at runtime. So I don't see this as a
problem. If you want to catch all Exceptions in some source code, catch
Throwable and handle based on type + rethrow. That is what all programs out
there do. The main reason why it is recommended is also the problem with
{{newInstance}} above.
was (Author: thetaphi):
Did you know that the Class.newInstance() also throws checked Exceptions
without declaring them! So the old code before using MethodHandles was wrong,
too! (Java 9 deprecated Class#newInstance() in favor of using Constructor which
wrapps). If you search for Class#newInstance in LuSolr's source code you see
many more.
[http://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#newInstance--]
As said before I'd check the ctor on the initial reflection that it does not
throw checked Exceptions. The problem is that MethodHandles.findConstructor()
and the returned MethodHandle's MethodType don't show the Exceptions. But with
reflective jl.reflect.Constructor instances this can be checked.
In general, with MethodHandle's you are leaving the type system anyways, so it
is always possible that types don't match at runtime. So I don't see this as a
problem. If you want to catch all Exceptions in some source code, catch
Throwable and handle based on type + rethrow. That is what all programs out
there do.
> 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)
>
>
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980332#comment-15980332
]
Uwe Schindler edited comment on LUCENE-7796 at 4/23/17 9:06 AM:
Did you know that the Class.newInstance() also throws checked Exceptions
without declaring them! So the old code before using MethodHandles was wrong,
too! (Java 9 deprecated Class#newInstance() in favor of using Constructor which
wrapps). If you search for Class#newInstance in LuSolr's source code you see
many more.
[http://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#newInstance--]
As said before I'd check the ctor on the initial reflection that it does not
throw checked Exceptions. The problem is that MethodHandles.findConstructor()
and the returned MethodHandle's MethodType don't show the Exceptions. But with
reflective jl.reflect.Constructor instances this can be checked.
In general, with MethodHandle's you are leaving the type system anyways, so it
is always possible that types don't match at runtime. So I don't see this as a
problem. If you want to catch all Exceptions in some source code, catch
Throwable and handle based on type + rethrow. That is what all programs out
there do.
was (Author: thetaphi):
Did you know that the Class.newInstance() also throws checked Exceptions
without declaring them! So the old code before using MethodHandles was wrong,
too! (Java 9 deprecated Class#newInstance() in favor of using Constructor which
wrapps). If you search for Class#newInstance in LuSolr's source code you see
many more.
As said before I'd check the ctor on the initial reflection that it does not
throw checked Exceptions. The problem is that MethodHandles.findConstructor()
and the returned MethodHandle's MethodType don't show the Exceptions. But with
reflective jl.reflect.Constructor instances this can be checked.
In general, with MethodHandle's you are leaving the type system anyways, so it
is always possible that types don't match at runtime. So I don't see this as a
problem. If you want to catch all Exceptions in some source code, catch
Throwable and handle based on type + rethrow. That is what all programs out
there do.
> 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)
>
>
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980147#comment-15980147
]
Uwe Schindler edited comment on LUCENE-7796 at 4/22/17 10:40 PM:
-
bq. It is based on JVM spec knowledge, not Java spec and it can lead to
surprising code paths.
That's not true. It is misuse, but behavior is still expected. The same happens
if you change a method to suddenly throw a checked Exception. Code compiled
against old signature leads to no runtime or linkage errors. But it can still
be surprising, because the code may throw a checked exception unexpected.
BTW: We have more rethrows in the expressions module. And there - please don't
remove, it would break the expressions compiler. The reason why its there is:
The ANTLR API needs to implement an interface that does not allow checked
exceptions. But the code inside this interface needs to throw ParseExceptions,
which are checked. So we need the rethrow hack... But the code is also safe, as
the outer method declares the ParseException, so code calling the public
compiler API will only see declared Exceptions:
https://github.com/apache/lucene-solr/blob/master/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java#L298-L301,
but code that calls the visitor declares the ParseException:
https://github.com/apache/lucene-solr/blob/master/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java#L231
was (Author: thetaphi):
bq. It is based on JVM spec knowledge, not Java spec and it can lead to
surprising code paths.
That's not true. It is misuse, but behavior is still expected. The same happens
if you change a method to suddenly throw a checked Exception. Code compiled
against old signature leads to no runtime or linkage errors. But it can still
be surprising, because the code may throw a checked exception unexpected.
BTW: We have more rethrows in the expressions module. And there - please don't
remove, it would break the expressions compiler.
> 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)
>
>
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980152#comment-15980152
]
Uwe Schindler edited comment on LUCENE-7796 at 4/22/17 10:27 PM:
-
BTW: I have a better rethrow idiom that does not even need try..catch. Very
clear code and it clearly shows what happens when used in code. Source:
{code:java}
@FunctionalInterface
public interface Unchecked {
R call() throws T;
@SuppressWarnings("unchecked")
static R exec(Unchecked lambda) {
return ((Unchecked) lambda).call();
}
}
{code}
Usage:
{code:java}
Something ret = Unchecked.exec(() -> {
return doCrazyStuff();
});
{code}
This is looking like a try...catch, but it uses a Lambda trick with the same
sneaky throw trick, although it is very compact and easy to read. Code looks
very clean and you don't need any compiler hacks like proposed in this issue.
Only backside: Don't use it in inner loops or when you want to call
MethodHandles very fast, as it breaks inlining of MethodHandles. So it's a bad
idea for AttributeFactory or Snowball.
was (Author: thetaphi):
BTW: I have a better rethrow idiom that does not even need try..catch. Very
clear code and it clearly shows what happens:
{code:java}
@FunctionalInterface
public interface Unchecked {
R call() throws T;
@SuppressWarnings("unchecked")
static R exec(Unchecked lambda) {
return ((Unchecked) lambda).call();
}
}
{code}
Usage:
{code:java}
Something ret = Unchecked.exec(() -> {
return doCrazyStuff();
});
{code}
This is looking like a try...catch, but it uses a Lambda trick with the same
sneaky throw trick, although it is very compact and easy to read. Code looks
very clean and you don't need any compiler hacks like proposed in this issue.
Only backside: Don't use it in inner loops or when you want to call
MethodHandles very fast, as it breaks inlining of MethodHandles. So it's a bad
idea for AttributeFactory or Snowball.
> 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)
>
>
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (LUCENE-7796) Make reThrow idiom declare RuntimeException return type so callers may use it in a way that compiler knows subsequent code is unreachable
[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980147#comment-15980147
]
Uwe Schindler edited comment on LUCENE-7796 at 4/22/17 10:16 PM:
-
bq. It is based on JVM spec knowledge, not Java spec and it can lead to
surprising code paths.
That's not true. It is misuse, but behavior is still expected. The same happens
if you change a method to suddenly throw a checked Exception. Code compiled
against old signature leads to no runtime or linkage errors. But it can still
be surprising, because the code may throw a checked exception unexpected.
BTW: We have more rethrows in the expressions module. And there - please don't
remove, it would break the expressions compiler.
was (Author: thetaphi):
bq. It is based on JVM spec knowledge, not Java spec and it can lead to
surprising code paths.
That's not true. It is misuse, but behavior is still expected. The same happens
if you change a method to suddenly throw a checked Exception. Code compiled
against old signature leads to now runtime errors. But it can still be
surprising. BTW. We have more rethrows in the expressions module. And there
please don't remove, it would break the compiler.
> 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)
>
>
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
