Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7796

Make reThrow idiom declare Error return type so callers may use it in a way that compiler knows subsequent code is unreachable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.0, 6.6
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      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.

      1. LUCENE-7796.patch
        32 kB
        Dawid Weiss
      2. LUCENE-7796.patch
        30 kB
        Dawid Weiss

        Issue Links

          Activity

          Hide
          jpountz Adrien Grand added a comment -

          Maybe we'd need to rename it as well to make clear that it returns an exception rather than throwing an exception. Otherwise I'm afraid it would be error-prone in void methods?

          Show
          jpountz Adrien Grand added a comment - Maybe we'd need to rename it as well to make clear that it returns an exception rather than throwing an exception. Otherwise I'm afraid it would be error-prone in void methods?
          Hide
          dweiss Dawid Weiss added a comment -

          Sorry, I wasn't clear – this method would still unconditionally always throw an exception. So even if you forgot to do a throw, the code wouldn't get past this method. The return unchecked exception is only useful for cases like switch statements or (theoretically) reachable code but practically we know reThrow never returns. See Mike's code in LUCENE-7792 and you'll see what I mean.

          Show
          dweiss Dawid Weiss added a comment - Sorry, I wasn't clear – this method would still unconditionally always throw an exception. So even if you forgot to do a throw , the code wouldn't get past this method. The return unchecked exception is only useful for cases like switch statements or (theoretically) reachable code but practically we know reThrow never returns. See Mike's code in LUCENE-7792 and you'll see what I mean.
          Hide
          jpountz Adrien Grand added a comment -

          Ohhhhhh I get it now! Thanks for the explanation, +1 on doing chis change.

          Show
          jpountz Adrien Grand added a comment - Ohhhhhh I get it now! Thanks for the explanation, +1 on doing chis change.
          Hide
          dweiss Dawid Weiss added a comment -

          No problem, my description wasn't clear and was meant as a placeholder until I find some time to work on it.

          Show
          dweiss Dawid Weiss added a comment - No problem, my description wasn't clear and was meant as a placeholder until I find some time to work on it.
          Hide
          hossman Hoss Man added a comment -

          clarified summary & description

          Show
          hossman Hoss Man added a comment - clarified summary & description
          Hide
          dweiss Dawid Weiss added a comment -

          That has to be an unchecked exception, Hoss, not Throwable (or the type of argument). The whole point of the rethrow hack is to rethrow a checked exception from a method that doesn't declare checked exceptions.

          I scanned the code quickly and the rethrow idiom is actually in a few places (AttributeFactory, Rethrow, SnowballProgram, IOUtils – although slightly different here). It'd be a good chance to clean it up. IOUtils has a slightly different flavor, but all the others use the same idiom.

          Show
          dweiss Dawid Weiss added a comment - That has to be an unchecked exception, Hoss, not Throwable (or the type of argument). The whole point of the rethrow hack is to rethrow a checked exception from a method that doesn't declare checked exceptions. I scanned the code quickly and the rethrow idiom is actually in a few places (AttributeFactory, Rethrow, SnowballProgram, IOUtils – although slightly different here). It'd be a good chance to clean it up. IOUtils has a slightly different flavor, but all the others use the same idiom.
          Hide
          dweiss Dawid Weiss added a comment -

          I looked at the code now. The test-framework's Rethrow is what I originally had in mind, but I was mistaken about Mike's code – this uses IOUtils.reThrow and there is a problem with it – "If the argument is null then this method does nothing."... so in fact a call to IOUtils.reThrow can be a no-op, as is evident when one looks at how it's used:

                // NOTE: does nothing if firstThrowable is null
                IOUtils.reThrowUnchecked(firstThrowable);
          

          or

          if (success) {
                // Does nothing if firstExc is null:
                IOUtils.reThrow(firstExc);
          }
          

          we can make it return IOException (or something unchecked), but if the argument can be null it absolutely cannot be used with throw... Will this make code clearer or more confusing? For me, it was surprising the argument can be null (and ignored), so based on least surprise principle I'd make IOUtils.reThrow(exception) require a non-null argument and always throw internally. Then the return type can be used without side effects (either with throw or as a simple statement).

          A cleanup of other "rethrow-hack" places would require moving Rethrow from test utils back to the core and I think (but didn't look further) we actually moved it the other way around so as to prevent people from using it... Although, on the other hand – AttributeFactory:

            // Hack to rethrow unknown Exceptions from {@link MethodHandle#invoke}:
            // TODO: remove the impl in test-framework, this one is more elegant :-)
            static void rethrow(Throwable t) {
          ...
          

          So I'm a bit lost as to which direction I should go with this patch.

          Show
          dweiss Dawid Weiss added a comment - I looked at the code now. The test-framework's Rethrow is what I originally had in mind, but I was mistaken about Mike's code – this uses IOUtils.reThrow and there is a problem with it – "If the argument is null then this method does nothing."... so in fact a call to IOUtils.reThrow can be a no-op, as is evident when one looks at how it's used: // NOTE: does nothing if firstThrowable is null IOUtils.reThrowUnchecked(firstThrowable); or if (success) { // Does nothing if firstExc is null : IOUtils.reThrow(firstExc); } we can make it return IOException (or something unchecked), but if the argument can be null it absolutely cannot be used with throw... Will this make code clearer or more confusing? For me, it was surprising the argument can be null (and ignored), so based on least surprise principle I'd make IOUtils.reThrow(exception) require a non-null argument and always throw internally. Then the return type can be used without side effects (either with throw or as a simple statement). A cleanup of other "rethrow-hack" places would require moving Rethrow from test utils back to the core and I think (but didn't look further) we actually moved it the other way around so as to prevent people from using it... Although, on the other hand – AttributeFactory : // Hack to rethrow unknown Exceptions from {@link MethodHandle#invoke}: // TODO: remove the impl in test-framework, this one is more elegant :-) static void rethrow(Throwable t) { ... So I'm a bit lost as to which direction I should go with this patch.
          Hide
          dweiss Dawid Weiss added a comment -

          This is what I think IOUtils.reThrow* methods should look like:

            /**
             * An utility method that takes a previously caught
             * {@code Throwable} and rethrows either {@code
             * IOException} if it the argument was an {@code IOException}
             * wraps the argument in a {@code RuntimeException}.
             * 
             * @param th The throwable to rethrow, must not be null.
             * @return This method never returns normally, it always throws an exception. The return
             *   value of this method can be used for an idiom that informs the compiler that the code
             *   below the invocation of this method is unreachable:
             *   {@code throw reThrow(argument)}.  
             */
            public static RuntimeException reThrow(Throwable th) throws IOException {
              if (th == null) {
                throw new AssertionError("reThrow doesn't accept null arguments.");
              }
              if (th instanceof IOException) {
                throw (IOException) th;
              }
              throw reThrowUnchecked(th);
            }
          
            /**
             * An utility method that takes a previously caught
             * {@code Throwable} and rethrows it if it's an 
             * unchecked exception or wraps it in a {@code RuntimeException} otherwise.
             * 
             * @param th The throwable to rethrow, must not be null.
             * @return This method never returns normally, it always throws an exception. The return
             *   value of this method can be used for an idiom that informs the compiler that the code
             *   below the invocation of this method is unreachable:
             *   {@code throw reThrowUnchecked(argument)}.  
             */
            public static RuntimeException reThrowUnchecked(Throwable th) {
              if (th == null) {
                throw new AssertionError("reThrowUnchecked doesn't accept null arguments.");
              }
              if (th instanceof RuntimeException) {
                throw (RuntimeException) th;
              }
              if (th instanceof Error) {
                throw (Error) th;
              }
              throw new RuntimeException(th);
            }
          
          Show
          dweiss Dawid Weiss added a comment - This is what I think IOUtils.reThrow* methods should look like: /** * An utility method that takes a previously caught * {@code Throwable} and rethrows either {@code * IOException} if it the argument was an {@code IOException} * wraps the argument in a {@code RuntimeException}. * * @param th The throwable to rethrow, must not be null . * @ return This method never returns normally, it always throws an exception. The return * value of this method can be used for an idiom that informs the compiler that the code * below the invocation of this method is unreachable: * {@code throw reThrow(argument)}. */ public static RuntimeException reThrow(Throwable th) throws IOException { if (th == null ) { throw new AssertionError( "reThrow doesn't accept null arguments." ); } if (th instanceof IOException) { throw (IOException) th; } throw reThrowUnchecked(th); } /** * An utility method that takes a previously caught * {@code Throwable} and rethrows it if it's an * unchecked exception or wraps it in a {@code RuntimeException} otherwise. * * @param th The throwable to rethrow, must not be null . * @ return This method never returns normally, it always throws an exception. The return * value of this method can be used for an idiom that informs the compiler that the code * below the invocation of this method is unreachable: * {@code throw reThrowUnchecked(argument)}. */ public static RuntimeException reThrowUnchecked(Throwable th) { if (th == null ) { throw new AssertionError( "reThrowUnchecked doesn't accept null arguments." ); } if (th instanceof RuntimeException) { throw (RuntimeException) th; } if (th instanceof Error) { throw (Error) th; } throw new RuntimeException(th); }
          Hide
          hossman Hoss Man added a comment -

          Strawman:

          • add Error rethrowAsUnchecked(...) to work the way this issue suggests it should
            • or RuntimeException if you prefer – no opinion, just going based on the cast currently done in Rethrow
            • personally i think the "Sneaky" code looks sketchy as hell and would feel a lot more comfortable using the more obvious and easy to understand throw new RuntimeException(t)
          • rename existing void rethrow(...) methods to void rethrowIfNonNull(...) (they can delegate to rethrowAsUnchecked)
          • some existing type specific impls (like the IOException version in IOUtils can/should probably be renamed to be clear what they're for and have javadoc (links back to the "main" versions) justifying their differences

          A cleanup of other "rethrow-hack" places would require moving Rethrow from test utils back to the core and I think (but didn't look further) we actually moved it the other way around so as to prevent people from using it... So I'm a bit lost as to which direction I should go with this patch

          If the idiom/code is in use (outside of tests), then the code is in use – we could consider those usages "bugs" and "fix" the code to not need a reThrow(...) type function, but as long as that code is going to exist, having one copy of it (in core) seems better then having 2,3,5,100 copies of it.

          // TODO: remove the impl in test-framework, this one is more elegant

          wait ... am i missing something? aren't those 2 impls identical?????

          Show
          hossman Hoss Man added a comment - Strawman: add Error rethrowAsUnchecked(...) to work the way this issue suggests it should or RuntimeException if you prefer – no opinion, just going based on the cast currently done in Rethrow personally i think the "Sneaky" code looks sketchy as hell and would feel a lot more comfortable using the more obvious and easy to understand throw new RuntimeException(t) rename existing void rethrow(...) methods to void rethrowIfNonNull(...) (they can delegate to rethrowAsUnchecked ) some existing type specific impls (like the IOException version in IOUtils can/should probably be renamed to be clear what they're for and have javadoc (links back to the "main" versions) justifying their differences A cleanup of other "rethrow-hack" places would require moving Rethrow from test utils back to the core and I think (but didn't look further) we actually moved it the other way around so as to prevent people from using it... So I'm a bit lost as to which direction I should go with this patch If the idiom/code is in use (outside of tests), then the code is in use – we could consider those usages "bugs" and "fix" the code to not need a reThrow(...) type function, but as long as that code is going to exist, having one copy of it (in core) seems better then having 2,3,5,100 copies of it. // TODO: remove the impl in test-framework, this one is more elegant wait ... am i missing something? aren't those 2 impls identical?????
          Hide
          dweiss Dawid Weiss added a comment -

          I agree: the rethrow-checked hack is essentially an abuse of the type erasure system. It is based on JVM spec knowledge, not Java spec and it can lead to surprising code paths.

          I'll file a separate issue to remove sneaky throws from the core codebase.

          wait ... am i missing something? aren't those 2 impls identical?????

          They are (now); I think it's a remnant from times when they weren't.

          Show
          dweiss Dawid Weiss added a comment - I agree: the rethrow-checked hack is essentially an abuse of the type erasure system. It is based on JVM spec knowledge, not Java spec and it can lead to surprising code paths. I'll file a separate issue to remove sneaky throws from the core codebase. wait ... am i missing something? aren't those 2 impls identical????? They are (now); I think it's a remnant from times when they weren't.
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          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

          Show
          thetaphi Uwe Schindler added a comment - - edited 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
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          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:

          @FunctionalInterface
          public interface Unchecked<R,T extends Throwable> {
            
            R call() throws T;
            
            @SuppressWarnings("unchecked")
            static <R> R exec(Unchecked<R,?> lambda) {
              return ((Unchecked<R,RuntimeException>) lambda).call();
            }
            
          }
          

          Usage:

          Something ret = Unchecked.exec(() -> {
            return doCrazyStuff();
          });
          

          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.

          Show
          thetaphi Uwe Schindler added a comment - - edited 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: @FunctionalInterface public interface Unchecked<R,T extends Throwable> { R call() throws T; @SuppressWarnings( "unchecked" ) static <R> R exec(Unchecked<R,?> lambda) { return ((Unchecked<R,RuntimeException>) lambda).call(); } } Usage: Something ret = Unchecked.exec(() -> { return doCrazyStuff(); }); 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.
          Hide
          dweiss Dawid Weiss added a comment -

          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.

          I don't think this is a good example, Uwe. What you're talking about is JVM validation (which indeed treats all exceptions equal). What I'm talking about is Java specification. From java specification viewpoint it is a compilation error if your depending class changes and suddently throws an unchecked exception. The fact that you can substitute a new class with different method signatures and re-run binaries is a weakness in java "linking" system and is, to me, unrelated.

          What I'm talking about can be showcased fairly easily. Consider this code:

          public class TestCustomAttr extends LuceneTestCase {
            public static interface CustomAttr extends Attribute {
            }
           
            public static final class CustomAttrImpl extends AttributeImpl {
              public CustomAttrImpl() throws Exception {
                throw new Exception("bah");
              }
              
              @Override
              public void clear() {}
          
              @Override
              public void reflectWith(AttributeReflector reflector) {}
          
              @Override
              public void copyTo(AttributeImpl target) {}
            }
            
            private class CustomFilter extends TokenFilter {
              protected CustomFilter(TokenStream input) {
                super(input);
          
                addAttribute(CustomAttr.class);
              }
          
              @Override
              public boolean incrementToken() throws IOException {
                return false;
              }
            }
            
            public void testUncheckedExceptionInAttrImpl() {
              Analyzer a = new Analyzer() {
                @Override
                protected TokenStreamComponents createComponents(String fieldName) {
                  Tokenizer source = new WhitespaceTokenizer();
                  TokenStream filters = source;
                  filters = new CustomFilter(source);
          
                  return new TokenStreamComponents(source, filters);
                }
              };
          
              a.tokenStream("",  "foo");
            }
          }
          

          As a Java programmer I have every reason to believe a.tokenStream never throws a checked exception, yet it surely does. I fail to see how this is verified by Java MethodHandles lookup (in fact, as the example above shows, it isn't).

          I'm really fine in leaving it as is – I understand how it works internally. But I do have a strong feeling sneaky throws aren't the "right" way in terms of how the Java language was designed (not talking about the JVM here). If you need a motivational example – this is exactly the same situation as with method handles (invokeExact, etc.) or the old reflection API (Method) – they declare Throwable, ExecutionException or other exception type wrapper as a java-language-compatible way of propagating unknown (checked or unchecked) exception from the (unknown) callee. Straight and simple, no surprises.

          Show
          dweiss Dawid Weiss added a comment - 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. I don't think this is a good example, Uwe. What you're talking about is JVM validation (which indeed treats all exceptions equal). What I'm talking about is Java specification. From java specification viewpoint it is a compilation error if your depending class changes and suddently throws an unchecked exception. The fact that you can substitute a new class with different method signatures and re-run binaries is a weakness in java "linking" system and is, to me, unrelated. What I'm talking about can be showcased fairly easily. Consider this code: public class TestCustomAttr extends LuceneTestCase { public static interface CustomAttr extends Attribute { } public static final class CustomAttrImpl extends AttributeImpl { public CustomAttrImpl() throws Exception { throw new Exception( "bah" ); } @Override public void clear() {} @Override public void reflectWith(AttributeReflector reflector) {} @Override public void copyTo(AttributeImpl target) {} } private class CustomFilter extends TokenFilter { protected CustomFilter(TokenStream input) { super (input); addAttribute(CustomAttr.class); } @Override public boolean incrementToken() throws IOException { return false ; } } public void testUncheckedExceptionInAttrImpl() { Analyzer a = new Analyzer() { @Override protected TokenStreamComponents createComponents( String fieldName) { Tokenizer source = new WhitespaceTokenizer(); TokenStream filters = source; filters = new CustomFilter(source); return new TokenStreamComponents(source, filters); } }; a.tokenStream( "", " foo"); } } As a Java programmer I have every reason to believe a.tokenStream never throws a checked exception, yet it surely does. I fail to see how this is verified by Java MethodHandles lookup (in fact, as the example above shows, it isn't). I'm really fine in leaving it as is – I understand how it works internally. But I do have a strong feeling sneaky throws aren't the "right" way in terms of how the Java language was designed (not talking about the JVM here). If you need a motivational example – this is exactly the same situation as with method handles ( invokeExact , etc.) or the old reflection API ( Method ) – they declare Throwable, ExecutionException or other exception type wrapper as a java-language-compatible way of propagating unknown (checked or unchecked) exception from the (unknown) callee. Straight and simple, no surprises.
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          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.

          Show
          thetaphi Uwe Schindler added a comment - - edited 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.
          Hide
          dweiss Dawid Weiss added a comment -

          Did you know that the Class#newInstance() also throws checked Exceptions without declaring them!

          Yes, I know that. And there is a reason they deprecated it, isn't there? And while we could use reflection to inspect for unchecked thrown exceptions I really see little point in doing so – this is a runtime contract enforcement that cannot be verified by the compiler. On the other hand, we could simply declare that attribute instantiation can throw something like AttributeInstantiationException which would have a cause initialized to the underlying throwable and have a clean and portable specification on what happens if the underlying code throws something unchecked.

          If you want to catch all Exceptions in some source code, catch Throwable and handle based on type + rethrow.

          You don't see this as a problem because you're aware of the implementation details (be it method handles or whatever). I see this as an API inconsistency. Look again at the code snippet I posted – how is the (much less experienced than you) programmer to know that he or she should handle all Throwables, not just Error|RuntimeException?

              Analyzer a = new Analyzer() {
                @Override
                protected TokenStreamComponents createComponents(String fieldName) {
                  Tokenizer source = new WhitespaceTokenizer();
                  TokenStream filters = source;
                  filters = new CustomFilter(source);
          
                  return new TokenStreamComponents(source, filters);
                }
              };
          
              a.tokenStream("",  "foo");
          

          This is a problem in particular if you don't have control over CustomFilter and don't see that it can throw some kind of unchecked exception. I think some kind of runtime exception (could be something Lucene-specific – like the AttributeInstantiationException I mentioned) would be a much better fit here. tokenStream could even declare it as thrown (even though it's unchecked) to increase awareness of what can happen at runtime.

          What do you think?

          Show
          dweiss Dawid Weiss added a comment - Did you know that the Class#newInstance() also throws checked Exceptions without declaring them! Yes, I know that. And there is a reason they deprecated it, isn't there? And while we could use reflection to inspect for unchecked thrown exceptions I really see little point in doing so – this is a runtime contract enforcement that cannot be verified by the compiler. On the other hand, we could simply declare that attribute instantiation can throw something like AttributeInstantiationException which would have a cause initialized to the underlying throwable and have a clean and portable specification on what happens if the underlying code throws something unchecked. If you want to catch all Exceptions in some source code, catch Throwable and handle based on type + rethrow. You don't see this as a problem because you're aware of the implementation details (be it method handles or whatever). I see this as an API inconsistency. Look again at the code snippet I posted – how is the (much less experienced than you) programmer to know that he or she should handle all Throwables, not just Error|RuntimeException ? Analyzer a = new Analyzer() { @Override protected TokenStreamComponents createComponents( String fieldName) { Tokenizer source = new WhitespaceTokenizer(); TokenStream filters = source; filters = new CustomFilter(source); return new TokenStreamComponents(source, filters); } }; a.tokenStream( "", " foo"); This is a problem in particular if you don't have control over CustomFilter and don't see that it can throw some kind of unchecked exception. I think some kind of runtime exception (could be something Lucene-specific – like the AttributeInstantiationException I mentioned) would be a much better fit here. tokenStream could even declare it as thrown (even though it's unchecked) to increase awareness of what can happen at runtime. What do you think?
          Hide
          dweiss Dawid Weiss added a comment -

          Hi Uwe. Let me know what you think about the above before this slips into oblivion. If you're convinced everything should remain as it is it's also an answer (although I don't buy your arguments ).

          Show
          dweiss Dawid Weiss added a comment - Hi Uwe. Let me know what you think about the above before this slips into oblivion. If you're convinced everything should remain as it is it's also an answer (although I don't buy your arguments ).
          Hide
          dweiss Dawid Weiss added a comment - - edited

          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:

               if (success) {
          -      // Does nothing if firstExc is null:
          -      IOUtils.reThrow(firstExc);
          +      if (firstExc != null) {
          +        throw IOUtils.rethrowAlways(firstExc);
          +      }
               }
          

          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:

               } catch (Throwable t) {
          -      verifyChecksum(t, source.writer);
          +      throw verifyChecksum(t, source.writer);
               } 
          

          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 Hoss Man 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).

          Show
          dweiss Dawid Weiss added a comment - - edited 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: if (success) { - // Does nothing if firstExc is null : - IOUtils.reThrow(firstExc); + if (firstExc != null ) { + throw IOUtils.rethrowAlways(firstExc); + } } 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: } catch (Throwable t) { - verifyChecksum(t, source.writer); + throw verifyChecksum(t, source.writer); } 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 Hoss Man 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).
          Hide
          rcmuir Robert Muir added a comment -

          I can revert it to be named identical as before if somebody really insists.

          I insist that the name change if we do this.

          By the way, I think this is incredibly risky for the benefit. Are you sure its really the right tradeoff? If so, I really think it would be better to add the new method and migrate code over at a smaller pace.

          Show
          rcmuir Robert Muir added a comment - I can revert it to be named identical as before if somebody really insists. I insist that the name change if we do this. By the way, I think this is incredibly risky for the benefit. Are you sure its really the right tradeoff? If so, I really think it would be better to add the new method and migrate code over at a smaller pace.
          Hide
          dweiss Dawid Weiss added a comment -

          I agree we should rename the method.

          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:

                   } 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);
          

          The above could fall through to result = null. Even if you didn't know about it and wrote:

          } catch (ExecutionException ee) {
           throw IOUtils.rethrowAlways(ee.getCause());
          

          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).

          Show
          dweiss Dawid Weiss added a comment - I agree we should rename the method. 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: } 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); The above could fall through to result = null. Even if you didn't know about it and wrote: } catch (ExecutionException ee) { throw IOUtils.rethrowAlways(ee.getCause()); 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).
          Hide
          rcmuir Robert Muir added a comment -

          I think we both agree to the end goal, my problem is how you get there.

          Are you really sure exceptions are never null in all these places? That is my problem. Sure in some places its totally obvious because it has a comment like that, maybe those places should be fixed first, but I am worried about bugs.

          Why not add the new method, and deprecate the old one? Then the old lenient method is discouraged for any new code, and old code can be migrated more carefully.

          Is there a strong technical reason to remove the deprecated method from master or is it just "but that doesnt seem right to have one there"? Its a million times more important to not introduce bugs in exception handling.

          Show
          rcmuir Robert Muir added a comment - I think we both agree to the end goal, my problem is how you get there. Are you really sure exceptions are never null in all these places? That is my problem. Sure in some places its totally obvious because it has a comment like that, maybe those places should be fixed first, but I am worried about bugs. Why not add the new method, and deprecate the old one? Then the old lenient method is discouraged for any new code, and old code can be migrated more carefully. Is there a strong technical reason to remove the deprecated method from master or is it just "but that doesnt seem right to have one there"? Its a million times more important to not introduce bugs in exception handling.
          Hide
          dweiss Dawid Weiss added a comment -

          Are you really sure exceptions are never null in all these places?

          I have reviewed it manually (and will again, if there's an agreement on this). The tests pass – I ran it a few times now.

          Why not add the new method, and deprecate the old one?

          Yes, that's my "less radical" approach – I planned this for 6x. But we can as well do this on master, no problem there.

          Is there a strong technical reason to remove the deprecated method from master or is it just "but that doesnt seem right to have one there"? Its a million times more important to not introduce bugs in exception handling.

          I removed it because I wanted to make sure it's not used in the code. And I did review those existing usages on a case-by-case basis (there were about ~50 of them). I'm also concerned about correctness and it's true that this patch touches code paths that are very likely not frequently (or at all) touched in tests. Hence manual review.

          Also note that just about the only difference that can happen is passing null to that method (which no longer accepts it), which I tried to guard against in all places I wasn't 100% sure about non-null argument. Even if you don't use the return type (i.e., you don't use the throw), the call to this new method is not going to be ignored (it'll always throw an exception on non-null argument, just like previously).

          So the chances of screwing up are there, sure, but they seem minimal.

          Show
          dweiss Dawid Weiss added a comment - Are you really sure exceptions are never null in all these places? I have reviewed it manually (and will again, if there's an agreement on this). The tests pass – I ran it a few times now. Why not add the new method, and deprecate the old one? Yes, that's my "less radical" approach – I planned this for 6x. But we can as well do this on master, no problem there. Is there a strong technical reason to remove the deprecated method from master or is it just "but that doesnt seem right to have one there"? Its a million times more important to not introduce bugs in exception handling. I removed it because I wanted to make sure it's not used in the code. And I did review those existing usages on a case-by-case basis (there were about ~50 of them). I'm also concerned about correctness and it's true that this patch touches code paths that are very likely not frequently (or at all) touched in tests. Hence manual review. Also note that just about the only difference that can happen is passing null to that method (which no longer accepts it), which I tried to guard against in all places I wasn't 100% sure about non-null argument. Even if you don't use the return type (i.e., you don't use the throw ), the call to this new method is not going to be ignored (it'll always throw an exception on non-null argument, just like previously). So the chances of screwing up are there, sure, but they seem minimal.
          Hide
          rcmuir Robert Muir added a comment -

          Yeah, this is my concern, since you say "not 100% sure" and so on.

          Honestly if your comment had said "I think we should backport all this to 6x too, because the arg cant not be null in any of these places and we need more compile-time safety and simplicity around our exception handling", I would never have made any comment here at all.

          But you don't seem confident that your changes are correct

          Show
          rcmuir Robert Muir added a comment - Yeah, this is my concern, since you say "not 100% sure" and so on. Honestly if your comment had said "I think we should backport all this to 6x too, because the arg cant not be null in any of these places and we need more compile-time safety and simplicity around our exception handling", I would never have made any comment here at all. But you don't seem confident that your changes are correct
          Hide
          dweiss Dawid Weiss added a comment -

          I am very confident in my changes. I mostly changed places like this one:

                   } catch (Throwable t) {
                     if (doSave) {
          -            IOUtils.reThrow(t);
          +            throw IOUtils.rethrowAlways(t);
          

          Which are guaranteed not to ever be null. Other places already had null checks, but seemed like they could fall through. Now they're clear.

                 if (this.tragedy != null) {
                   // Another thread is already dealing / has dealt with the tragedy:
          -        IOUtils.reThrow(tragedy);
          +        throw IOUtils.rethrowAlways(tragedy);
                 }
          

          or this gem here:

          -    assert th != null; // extra safety - if we get here, it means the callable failed
          -    IOUtils.reThrow(th);
          -    return null; // silly, if we're here, IOUtils.reThrow always throws an exception
          +    throw IOUtils.rethrowAlways(th);
          

          Many other places did have the possibility of a null argument though (and legitiamately fall through); then I explicitly emulated the behavior before with an explicit if:

                 if (containsDestructive(options)) {
                   sop("newAsynchronousFileChannel" + options + ": " + path(path), exception);
                 } else {
          -        IOUtils.reThrow(exception);
          +        if (exception != null) {
          +          throw IOUtils.rethrowAlways(exception);
          +        }
                 }
               }
               throw new AssertionError();
          

          So if you need a stronger assertion then yes – I'm 100% sure. Of course the note to this should read: "beware of bugs in the above code; I have only proved it correct, not tried it.".

          Show
          dweiss Dawid Weiss added a comment - I am very confident in my changes. I mostly changed places like this one: } catch (Throwable t) { if (doSave) { - IOUtils.reThrow(t); + throw IOUtils.rethrowAlways(t); Which are guaranteed not to ever be null. Other places already had null checks, but seemed like they could fall through. Now they're clear. if ( this .tragedy != null ) { // Another thread is already dealing / has dealt with the tragedy: - IOUtils.reThrow(tragedy); + throw IOUtils.rethrowAlways(tragedy); } or this gem here: - assert th != null ; // extra safety - if we get here, it means the callable failed - IOUtils.reThrow(th); - return null ; // silly, if we're here, IOUtils.reThrow always throws an exception + throw IOUtils.rethrowAlways(th); Many other places did have the possibility of a null argument though (and legitiamately fall through); then I explicitly emulated the behavior before with an explicit if: if (containsDestructive(options)) { sop( "newAsynchronousFileChannel" + options + ": " + path(path), exception); } else { - IOUtils.reThrow(exception); + if (exception != null ) { + throw IOUtils.rethrowAlways(exception); + } } } throw new AssertionError(); So if you need a stronger assertion then yes – I'm 100% sure. Of course the note to this should read: "beware of bugs in the above code; I have only proved it correct, not tried it.".
          Hide
          rcmuir Robert Muir added a comment -

          Yes, the last case is my major concern, thanks for making clear how you tackled it. I agree with this approach.

          Show
          rcmuir Robert Muir added a comment - Yes, the last case is my major concern, thanks for making clear how you tackled it. I agree with this approach.
          Hide
          dweiss Dawid Weiss added a comment -

          I'll let this bake on my test machine for a bit and see I can catch anything.

          Show
          dweiss Dawid Weiss added a comment - I'll let this bake on my test machine for a bit and see I can catch anything.
          Hide
          hossman Hoss Man added a comment -

          Let me know what you think, especially Hoss Man since you provided an alternative view ...

          I don't have strong opinions on this at all, i was just putting out some strawmen to address the two diff calling models you were trying to fix w/minimal changes – unifying those models to force the caller to do the null check is completely fine with me.

          Show
          hossman Hoss Man added a comment - Let me know what you think, especially Hoss Man since you provided an alternative view ... I don't have strong opinions on this at all, i was just putting out some strawmen to address the two diff calling models you were trying to fix w/minimal changes – unifying those models to force the caller to do the null check is completely fine with me.
          Hide
          dweiss Dawid Weiss added a comment -

          I ran several dozen runs of the full core (Lucene) test suite. I detected a problem related to another issue (OfflineSorter, already fixed by Mike), but nothing else popped up.

          I added deprecated versions of methods that were previously in IOUtils, along with a verbose javadoc comment and example stating how the new method can be used.

          I'll attach the final patch in a second and commit tomorrow if there are no other comments.

          Show
          dweiss Dawid Weiss added a comment - I ran several dozen runs of the full core (Lucene) test suite. I detected a problem related to another issue (OfflineSorter, already fixed by Mike), but nothing else popped up. I added deprecated versions of methods that were previously in IOUtils, along with a verbose javadoc comment and example stating how the new method can be used. I'll attach the final patch in a second and commit tomorrow if there are no other comments.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 3866c72fe9f6753b491099c961200377567a0b08 in lucene-solr's branch refs/heads/branch_6x from Dawid Weiss
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3866c72 ]

          LUCENE-7796: Make IOUtils.reThrow idiom declare Error return type so
          callers may use it in a way that compiler knows subsequent code is
          unreachable. reThrow is now deprecated in favor of IOUtils.rethrowAlways.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 3866c72fe9f6753b491099c961200377567a0b08 in lucene-solr's branch refs/heads/branch_6x from Dawid Weiss [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3866c72 ] LUCENE-7796 : Make IOUtils.reThrow idiom declare Error return type so callers may use it in a way that compiler knows subsequent code is unreachable. reThrow is now deprecated in favor of IOUtils.rethrowAlways.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e52d8609949369aebae0297c855936138141c557 in lucene-solr's branch refs/heads/master from Dawid Weiss
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e52d860 ]

          LUCENE-7796: Make IOUtils.reThrow idiom declare Error return type so
          callers may use it in a way that compiler knows subsequent code is
          unreachable. reThrow is now deprecated in favor of IOUtils.rethrowAlways.

          Show
          jira-bot ASF subversion and git services added a comment - Commit e52d8609949369aebae0297c855936138141c557 in lucene-solr's branch refs/heads/master from Dawid Weiss [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e52d860 ] LUCENE-7796 : Make IOUtils.reThrow idiom declare Error return type so callers may use it in a way that compiler knows subsequent code is unreachable. reThrow is now deprecated in favor of IOUtils.rethrowAlways.

            People

            • Assignee:
              dweiss Dawid Weiss
              Reporter:
              dweiss Dawid Weiss
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development