uschindler edited a comment on pull request #643: URL: https://github.com/apache/lucene/pull/643#issuecomment-1030807558
> This isn't exactly right - it is legal bytecode but it isn't exactly playing well with the language. Hi @dweiss: Let's not discuss this here, the whole thing does not apply to this PR. My recommendation was to add another functional interface to IOUtils (we already created them there). I would never design my APIs in a way like Java streams API was done, so having a separate functional interface for IO actions is a good idea and was applied by Tomoko to this PR. So no need for further work. The above tweet is more about working around problems with Java Streams API. We know for sure that Java Streams API internally handles exception in a correct way (using finally blocks), so the code example you posted does not apply there. To me, checked exceptions are a misconception of Java, but you can start flamewars about that. Another misconception was to let MethodHandle.invoke() throw Throwable. Thats another case where sneaky rethoring *must* be done to have nice and correct behaviour for the caller of the stuff you do internally with MethodHandles. But there it is no problem because you left Java Type System already! So inside methods that call MethodHandle.invoke(), it is recommended and perfectly fine to use rethrow, as long as you make sure the caller form public API gets correct exceptions (means you sneaky rethrow all excepions, but you declare the method to have the checked exceptions you know of). BTW, Java internally also rethrows like this in the lamda bootstraps (I rewrote it for Elasticsearch Painless language). So whenever you have lambdas or MethodReferences in your code, sneaky rethrowing is used in bootstraps extensive. In general: Every cast is a misuse of the Java language, and so is this one. What we do here is just casting the exception type. So it is a misuse of type system (like every cast is), but it is not a misuse of the language. Its perfectly legal to do it. If you try out my above Sneaky class and look (with eclipse) at the types introduced for the generics type arguments, it is funny how intelligent the Java compiler is: If you have a functional interface throwing `E extends Exception` and the code does not throw any checked exception, Javac inserts `RuntimeException` into the type argument. I already talked too much, my problem with Sneaky is that if you use such type casting, the Java Compiler won't allow you to catch checked Exceptions anymore, that's the worst thing that happens - but Sneaky works great if you make sure that the method itsself calling your erased Java streams is redeclaring `throws IOException`. The try...catch posted above is.... bad code anyways. Better use finally blocks to ensure something is executed for sure. One last thing: The above try..catch also leads to surprises, if the method you cann suddenly changes throws in a later version of the API and you do not recompiled. So try...catch blocks like this are a risky and errorprone/PMD warn you about such code. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
