Lombok: WrappedThrows

[Maaartinus]

I dislike @SneakyThrows for the reasons given in its documentation, especially the impossibility to catch them. I also hate checked exception for reasons not fitting this page.

Quite often, I wrap my exceptions manually, which generates quite some clutter and leads to strange looking code. I wonder if automated exception wrapping could be the way to go.

@WrappedThrows
public void run() {
    Files.copy(source, destination);
}

becomes

public void run() {
    try {
        Files.copy(source, destination);
    } catch (IOException) {
        throw new RuntimeException(e);
    }
}

I'm unsure about what it should do in case of multiple statements to be wrapped. When doing this manually, I usually wrap them all in a single block possibly extended so that no variable declarations need to be split and so that the logical structure doesn't suffer. Lombok always wrapping the whole method body could be just fine.

I'm also unsure if this is a good idea at all... but it spooks in my head for quite some time.

[rzwitserloot]

The cool thing about @SneakyThrows is that it just... throws it.

Here we have to wrap, which raises the issue of: Kay, but, WHAT should we wrap? Presumably, there's no sense wrapping something that needn't actually be wrapped.

That'd be:

• RuntimeException
• Error
• anything that is declared in the closest method scope's 'throws' line
• anything that is declared in any catch block associated with any encompassing try.

Therefore, we'd have to generate an instanceof for all of that.

We can do that, of course. It's boilerplate hidden from view, who cares that it might turn into quite the unwieldy list. Also, presumably, if you're using @WrappedThrows, you aren't catching a whole host of explicitly listed exception types.

I like it. Not too high on the todolist though, I think; it's a bit exotic.

[cchudant]

Rethrow RuntimeException is generally a bad coding practice.
What about args like: @WrappedThrows(catch = {IOException.class}, value = {MyCustomNonRuntimeException.class}) or @WrappedThrows(MyCustomNonRuntimeException.class) which will catch any non-runtime exception on the catch block

Also I think @SneakyThrows is a bad coding practice ^^ This idea is better, but not likely to be used: How often would you use this in a real project? Not really often, I think.

[rzwitserloot]

You can say things like 'throwing RuntimeException is a bad coding practice', but that doesn't make it true.

Facts are required. An appeal to authority also, generally, does not apply unless it's one heck of an authority.

Defaulting to RuntimeException is fine until such convincing argumentation is provided.

The notion of being able to specify exactly WHAT the exception should be wrapped into, that's a fine idea.

Given the confusion, I don't think it's a good idea to name either one 'value'. That is, @WrappedThrows(SomeExType.class) simply won't be legal. We'll force you to specify catch= and wrapWith= or something similar. catch= will default to Throwable.class, wrapWith will default to RuntimeException.

[Maaartinus]

Rethrow RuntimeException is generally a bad coding practice.

I actually always used either my SomeSpecificRuntimeException or my GeneralWrappingRuntimeException, which I found much better than plain RuntimeException, but I found myself being unable to get any advantage thereof (except for one or two cases of SomeSpecificRuntimeException).

Also I think @SneakyThrows is a bad coding practice

Many Java programmers hate checked exceptions, but nearly everybody is scared of dropping them. As hardly any other language has them, I strongly doubt they can carry they weight.

I'm afraid, using @SneakyThrows changes the rules quite a bit. An advice like "be specific, never catch Exception" becomes a clear disaster. Probably, code like

try {
    whatever();
} catch (Exception e) {
    if (e instanceof IOException) {
        handleIOException(e, someParams);
    } else {
        handleOtherException(e, someParams);
    }
}

is the correct thing under the changed rules. The if-else cascade doesn't look nice, but it's always pretty short. It's surely better than sometimes seen code repetition in different clauses.

---

What could be a real problem is the interaction with library classes like the ThreadPoolExecutor. When you let your Runnable throw an IOException, it gets wrapped into an Error. Which may be(*) what you get (wrapped again in an ExecutionException) from a Future obtained from an ExecutorService or alike. The bad thing is that nobody thinks about looking into the Error in order to poll out the real cause.

(*) I didn't try, as this surely will happen with some class if not this one.

[Maaartinus]

I guess, I'd like to use configuration for specifying the wrapping exception. It could be something allowing things like e.rethrowIfCausedBy(IOException.class) or whatever. I guess, I'd always use the same exception. Defaulting to RuntimeException sounds good.

Regarding the wrapped exception, I guess, catching everything what has to be caught is a good default and would make me happy >90% of time. So writing just @WrappedThrows without any arguments would be the favorite usage.

[Maaartinus]

@cchudant

Also I think @SneakyThrows is a bad coding practice ^^ This idea is better, but not likely to be used: How often would you use this in a real project? Not really often, I think.

Actually, I wrap pretty every checked exception ASAP as I hate the throws clutter. IMHO this is the second best thing (with dropping checked exception being the best).

---

I'd propose the following

@Retention(RetentionPolicy.CLASS)
@Target(ElementType.METHOD)
public @interface WrappedThrows {
    /** 
     * A list of all caught exceptions classes. All exceptions of any of the specified class get wrapped, unless
     * the exception is an instance of a class listed in the {@link #passing()} list
     * or declared in the {@code throws} class of the annotated method.
     */
    Class<? extends Throwable>[] catching() default Exception.class;

    /**
     * A list of all passed exceptions classes.
     * Instances of the listed classes don't get wrapped, even when they're listed in {@link #catching()} list.
     */
    Class<? extends Throwable>[] passing() default RuntimeException.class;

    /**
     * The class of the wrapping thrown exception. 
     */
    Class<? extends Throwable> throwing() default RuntimeException.class;
}

There should be corresponding config keys. IMHO the only important setting is throwing (using gerund to avoid the keyword) as the defaults cover all common use cases (most of the time, I want to get rid of IOException or SQLException).

---

Here we have to wrap, which raises the issue of: Kay, but, WHAT should we wrap? Presumably, there's no sense wrapping something that needn't actually be wrapped.

Actually, given that the annotation makes sense on methods only, it's pretty simple:

• don't fool by anything but Exception (at least not by default)
• rethrow everything listed in the throws clause
• rethrow everything listed in the passing list
• don't bother with existing catch classes; they don't matter

In a simple case (assuming no configuration) like

@WrappedThrows
void m() throw SQLException, FileNotFoundException {
    .... original method body
}

the resulting code could be just

void m() throw SQLException, FileNotFoundException {
    try {
        .... original method body
    } catch (SQLException | FileNotFoundException | RuntimeException e) {
       throw e;
    } catch (Exception e) {
       throw new RuntimeException(e);
    }
}

The general case would be more complicated, from

@WrappedThrows(catching={C1.class, C2.class), passing={P1.class, P2.class}, throwing=T.class)
void m() throw SQLException, FileNotFoundException {
    .... original method body
}

generating

void m() throw SQLException, FileNotFoundException {
    try {
        .... original method body
    } catch (SQLException | FileNotFoundExceptione) {
       throw e;
    } catch (Throwable e) {
       if (e instanceof P1 || e instanceof P2) throw e;
       if (!(e instanceof C1) && !(e instanceof C2)) throw e;
       throw new T(e);
    }
}

[stickfigure]

Just some feedback from a user here:

I (and my past teams) use @SneakyThrows a lot, in large production codebases. It's great. But I would also prefer wrapping. The problem is that it is fairly common in transaction-processing libraries to see the heuristic: "runtime exceptions rollback, checked exceptions commit".

For example, here's guice-persist:

@Transactional
public void doSomeWork() {
    // ...ten stack frames deeper, someone sneakythrows a checked exception
}

This ends up with a commit even though you expect a rollback!

The current answer is that every declaration of transactional needs to be @Transactional(rollbackOn = Exception.class), and anyone that forgets this risks introducing really hard-to-track-down data corruption issues.

@SneakyThrows is so useful that it's worth the risk (with periodic grep-audits for mistakes) but I'd love to use @WrapThrows instead. I'm perfectly content with RuntimeException as the wrapper.

[Maaartinus]

@SneakyThrows is so useful that it's worth the risk (with periodic grep-audits for mistakes)

I'd suggest to write a test loading all your classes and scanning the methods for the annotation. I do this for similar reasons and it works perfectly and fast (initially, I used ClassPath from Guava, but that's too slow as there's much more stuff on the class path than my code; so I scan a directory or a JAR file instead).

but I'd love to use @WrapThrows instead.

I was too afraid to use @SneakyThrows as the undeclared exception could sneak though some third party code meant to catch everything. Lacking @WrappedThrows, I wrap everything manually and that's rather boring.

I'm perfectly content with RuntimeException as the wrapper.

Me too.

[rzwitserloot]

It is fairly common for transaction-processing libs to commit on checked exceptions??? That sounds _INSANE_ to me, I must be missing something.

[Maaartinus]

Insane but true:

In its default configuration, the Spring Framework’s transaction infrastructure code only marks a transaction for rollback in the case of runtime, unchecked exceptions; that is, when the thrown exception is an instance or subclass of RuntimeException. ( Errors will also - by default - result in a rollback). Checked exceptions that are thrown from a transactional method do not result in rollback in the default configuration.

It seems to be configurable (in a wonderful XML), however, it may not work if Spring relies on the method signature specifying no checked exceptions.

[rzwitserloot]

Okay. My head asplodes. It's a cliche, but, what were they smoking?

I'm having a hard time catering to clear boneheadedness like this. Surely there is a sane explanation, but I may have to insist I get it before we introduce any lombok features that attempt to cater to this (so far, to me anyway) crazy default.

Fortunately, I do see the point in wrapping exceptions, but not to cater to crazy spring defaults, just, for general purposes, and in passing it would help out the spring db users a bit.

Still struggling with the ideas of syntax and how we need to avoid attempting to catch anything for wrapping when you catch it yourself; the feature sounds quite a bit more complicated than @SneakyThrows, weirdly.

[Maaartinus]

Okay. My head asplodes. It's a cliche, but, what were they smoking?

Not sure, but I guess I should try it, too.

Still struggling with the ideas of syntax

See my above comment. Basically three optional arguments:

• catching() default {Exception.class} - what's not there won't get touched
• passing() default {RuntimeException.class} - what's there will be rethrown
• throwing() default RuntimeException.class - the wrapping exception class

The real default should be configurable. It doesn't support the possibility of throwing different exceptions based on what gets caught, but that's IMHO not necessary.

and how we need to avoid attempting to catch anything for wrapping when you catch it yourself

I see, you wrote "anything that is declared in any catch block associated with any encompassing try.", but I can't see why. When the user catches something, then it's the same as if they didn't throw it, so it doesn't matter for the resulting code, does it? I don't think we have to ever look at the wrapped code at all.

The most basic usage would just

• catch and rethrow all declared exceptions plus RuntimeException in the first clause
• catch and wrap Exception in the second clause
• do not touch Error.

For me and for the poor Spring users, it'd be enough. It could happen that it doesn't compile, but only if the original method doesn't compile either (or it declares throws Exception, but then the wrapping is pretty pointless).

For the general usage, see the above comment.

the feature sounds quite a bit more complicated than @SneakyThrows, weirdly.

I must be missing something.

[rzwitserloot]

Good point. And if someone catches, say, IOException, in the catch block rethrows it, and they have annotated the method with `@WrapExceptions, then, presumably, they still wanna wrap that.

You're not missing something – you're right, there is no need to introspect the body, but I thought we did, and that's why I said it seemed complex. Rejoice! This shouldn't be too difficult :)

[Rawi01]

@Maaartinus
I tried to implement this feature and noticed some problems:

• Should it also wrap @NonNull checks or not?
• If throwing is a checked exception we need to add it to the throw part of the original method. As we do not know that without resolution we should simply add it all the time or only support RuntimeException.
• The provided example result does not compile:

public void test() {
    try {
        throw new Exception("");
    } catch (final Throwable $ex) {
        if ($ex instanceof RuntimeException) throw $ex;
        if (!($ex instanceof Exception)) throw $ex;
        throw new RuntimeException($ex);
    }
}

ERROR unreported exception java.lang.Exception; must be caught or declared to be thrown

It seems like javac can not figure out that we always wrap Exception. Eclipse complains about an Unhandled exception type Throwable.

I tried to find some code that works and ended up with this (Java 7+) code:

void m() throw SQLException, FileNotFoundException {
    try {
        .... original method body
    } catch (SQLException | FileNotFoundException | P1 | P2 e) {
       throw e;
    } catch (C1 | C2 e) {
       throw new T(e);
    }
}

[stickfigure]

Sorry about missing out on all the commentary since January, not sure why Github only notified me now.

As a historical note the "runtime exceptions rollback, checked exceptions commit" rule dates back at least to the EJB spec of the late 90s (checked exceptions were considered "application exceptions" - an intentional part of control flow). It's weird but seems like just one more bit of downstream inanity caused by checked exceptions.

IMHO (emphasis on the H, since someone else is doing the work):

I'd make the first pass at this wrapper as simple as possible, no configuration options to start. Always wrap with RuntimeException, always wrap every checked exception that isn't declared. The reasoning is:

• Anyone who wants alternative behavior can always use a try/catch block instead.
• You can always add other options later after this feature gets some real-world use.
• This is the easiest thing to implement.

Cover the most likely use case, get some real world experience, then build configuration options for the pain points (if they exist). My WAG is that the only real configuration anyone will want is a global config flag for "use this wrapper exception instead" but even that might not be necessary.

[Rawi01]

Keeping it simple is always a good idea but I'm not sure if we would restrict this feature to much by doing so. Adding some parameters and create some dynamic code is not that hard.

Right now I still try to figure out how the generated code should look like. The example I added in my last comment will also show an error if we try to catch an exception that cannot be thrown or was catched earlier...

[stickfigure]

This feels like one of those "perfect is the enemy of the good" situations? This is all I need, and my guess will suffice for 90% of users, and will always compile:

try {
        .... original method body
    } catch (RuntimeException e) {
       throw e;
    } catch (Exception e) {
       throw new RuntimeException(e);
    }

Get it out there, see some real-world use patterns, add more parameters later if necessary? It's much easier to add params than to take them away.