Add factory methods for common exceptions with formatted messages

[gissuebot]

Original issue created by electrum on 2013-04-23 at 10:31 PM

---

It's common to be in a situation where precondition is known to be violated, but the condition can't be checked for directly by a Precondition method. For example:

if (...) {
} else if (...) {
} else {
  // illegal argument
}

When this occurs, we want to throw an exception with a formatted message, so we have two options:

1. Use checkArgument() or checkState() with "false" as the condition
2. Throw the exception and use String.format()

The first option has the advantages of being shorter and providing safety in the case of a malformed format string. But it still looks a bit ugly and seems less than ideal.

It would be nice to have factories that take Precondition-like format strings for at least the following common exceptions:

• IllegalArgumentException
• IllegalStateException
• AssertionError
• RuntimeException
• UnsupportedOperationException

[gissuebot]

Original comment posted by electrum on 2013-04-23 at 10:40 PM

---

Another benefit of this over using checkArgument(false) is that the compiler knows it always throws, allowing use in more situations than checkArgument(false).

[gissuebot]

Original comment posted by wasserman.louis on 2013-04-23 at 10:44 PM

---

Additional handy fact: the Preconditions.format implementation is GWT-compatible; String.format is not.

If we do decide to do this, it might also be worth supporting the case where you're wrapping another exception with a formatted error message -- e.g. just earlier today I saw some code

try {
  ...
} catch (IllegalArgumentException e) {
  throw new IllegalStateException(
      "Invalid authority '" + authority + "' found in URI '" + uri + "'", e);
}

I kinda like this request, but like all features that make a single-line operation a shorter single line, it'd need method names that are both good and short to be convincingly worthwhile, I think? Do you have method name suggestions?

(FWIW, I would never use checkArgument(false); I'd basically always prefer to either hardcode string concatenation, or use String.format.)

[gissuebot]

Original comment posted by jysjys1486 on 2013-04-23 at 11:23 PM

---

IllegalArgumentException
-- illegalArgument(...)

IllegalStateException
-- illegalState(...)

NullPointerException
-- nullPointer(...)
-- illegalNull(...)

IndexOutOfBoundsException
-- indexOutOfBounds(...)
-- outOfBounds(...)

AssertionError
-- assertionFailed(...)
-- failed(...)

UnsupportedOperationException
-- unsupportedOperation(...)
-- unsupported(...)

//---------------------------------------------------------------------------
// Classes that have the basic constructors [(), (String), (Throwable), etc]
//---------------------------------------------------------------------------
<X extends Exception, T> T raiseException(Class<? extends X>, ...) throws X
<X extends Error, T> T raiseError(Class<? extends X>, ...) throws X

[gissuebot]

Original comment posted by electrum on 2013-04-24 at 12:36 AM

---

The names could be the same as the exception:

throw illegalArgumentException(e, "Invalid authority '%s' found in URI '%s', authority, uri);

[gissuebot]

Original comment posted by electrum on 2013-04-24 at 12:37 AM

---

Related, having a public version of the Preconditions.format() method would be handy.

[gissuebot]

Original comment posted by wasserman.louis on 2013-04-24 at 01:04 AM

---

Related, having a public version of the Preconditions.format() method would be handy.

How would that be different from String.format, other than GWT compatibility?

throw illegalArgumentException(e, "Invalid authority '%s' found in URI '%s', authority, uri);

If you're not using static imports, this seems to be only a few characters shorter than than the alternatives, depending on how you name the class:

throw Exceptions.illegalArgumentException(e, "Invalid authority '%s' found in URI '%s', authority, uri);

versus

throw new IllegalArgumentException(e, String.format("Invalid authority '%s' found in URI '%s', authority, uri));

[gissuebot]

Original comment posted by electrum on 2013-04-24 at 02:38 AM

---

The big difference is that it's safe if you mess up the format string, which is likely for unexpected conditions that aren't covered by unit tests.

[gissuebot]

Original comment posted by ogregoire on 2013-04-24 at 09:27 AM

---

Well, it adds readability. Because this is the real code that we use:

throw new IllegalArgumentException(String.format("Invalid authority '%s' found in URI '%s', authority, uri), e);

with the cause being the last parameter (where you wrote that the cause was the first parameter, Louis). It's barely acknowledgeable that we have a cause here as it can easily be mistaken with a parameter of the exception. If it went missing for some reason, a reviewer wouldn't notice much. And it might be only at runtime that the error is seen. On the other hand, if the cause is the first parameter, people notice it. I know it's bad to create factory methods just to switch the order of the parameters, but I believe that in this case, the JDK is wrong, and I've thought that for years, even before Java 5 and the varargs syntax.

Also, to be honest, with the great work you have done with Guava over the past years, people use static imports a lot; if one library has really popularized those imports, it's Guava. So most of the time, it's not a few characters less: it's nearly half a line less verbose for the same outcome.

I would rather use names like "illegalArgument" than "illegalArgumentException", because with the "throw" before it, the context is already huge and we know it's an exception. So yes, we remove a lot of characters to focus on the essential: that we throw, that it's an IllegalArgumentException and that we provide several parameters to the message.

Finally, this discussion[1] has some strong arguments against String.format(). Of course, here the context is the logging, which is much more sensitive than the exceptions, but I think that some arguments are solid in this case too.

[1] http://bugzilla.slf4j.org/show_bug.cgi?id=116

[gissuebot]

Original comment posted by phwendler on 2013-04-26 at 08:48 AM

---

I like the whole idea, and specifically I think removing the "Exception" from the method name is nice and keeps the line more readable. People who want to see the "Exception" part would not use a static import and have "throw Exceptions.illegalArgument" instead of "throw new IllegalArgumentException".

throw illegalArgument(e, "Invalid authority '%s' found in URI '%s', authority, uri);

is really more readable than

throw new IllegalArgumentException(String.format("Invalid authority '%s' found in URI '%s', authority, uri), e);

The amount of saved characters might not be that much in total, but it removes all the irrelevant stuff from the beginning of the line.

Another nice use case can be

public void add(T obj) {
  unsupportedOperation("Object '%s' added to immutable list", obj);
}

where you don't even need the "throw" if the factory method throws itself and the code becomes almost declarative.

[gissuebot]

Original comment posted by ogregoire on 2013-04-26 at 09:02 AM

---

No phwend, the factory method shouldn't throw the exception. Transform your method add to make it return a value and you'll see why.

[gissuebot]

Original comment posted by phwendler on 2013-04-26 at 09:07 AM

---

ogregoire, why shouldn't it throw the exception (and still keep the exception as the return type)?

I know that in cases where you need to tell the compiler that this will always throw, you need to put the "throw" in front of the method call, but if the factory method throws, you don't need to do this everywhere. Even more important, it is now not a problem if you forget the "throw".

The same principle is used by the methods in the Throwables class.

[gissuebot]

Original comment posted by ogregoire on 2013-04-26 at 09:31 AM

---

It shouldn't throw the exception because this is a factory method. We might still want to do things with the exception prior to throwing it.

The pain is in creating the exception because

1. we don't have a simple and elegant way to add variables in the message
2. the cause isn't the first parameter

Throwing it isn't really a problem.

This is not the same case as Throwables. Throwables solves a problem linked to the types of exception, where we propagate and stuff like that. Here it's a request to make the creation of the most common exceptions easier.

[gissuebot]

Original comment posted by cgdecker on 2013-04-26 at 06:33 PM

---

So, the main reason for Preconditions taking a format string and args is so that it can avoid building a string when no exception needs to be thrown (the majority of the time). In this case, you're always going to be creating and throwing the exception so it's not that valuable. Certainly not worth adding new APIs for, in my opinion.

---

Labels: Type-Addition, Package-Base

[gissuebot]

Original comment posted by jamie.spencer.86 on 2013-04-26 at 07:11 PM

---

Is it also practical to include methods that can create any Throwable so long as the Throwable subclass provides the standard constructors?

class [Exceptions|Throwables] {

   public static <X extends [Exception|Throwable]> X [exception|throwable](         Class<? extends X> clas,
         Object errorMessage
   ) throws X {
      // returns new X(String.valueof(errorMessage));
      // throws IllegalArgument for classes that do not provide X(String)
   }

}

This would seem to be a bit more flexible than Supplier<? extends Exception> or specific static factory methods.

[gissuebot]

Original comment posted by jamie.spencer.86 on 2013-04-27 at 06:53 PM

---

This is an example:

// clas must provide a (String, Throwable) constructor
static <X extends Throwable> X throwable(
        Class<? extends X> clas,
        Throwable cause,
        String format,
        Object... args
) {

MethodType methodType = MethodType.methodType(
        void.class, String.class, Throwable.class);

MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodHandle handle;

try {
    handle = lookup.findConstructor(clas, methodType);
} catch (NoSuchMethodException | IllegalAccessException e) {
    // TODO
    throw illegalArgument(e);
}

try {
    return (X) handle.invoke(String.format(format, args), cause);
} catch (Throwable e) {
    // TODO
    throw illegalArgument(e);
}

}

[gissuebot]

Original comment posted by [email protected] on 2013-05-03 at 09:21 PM

---

Issue #1164 has been merged into this issue.

[dimo414]

Actual exception factory methods is still something being considered, but formatted messages are now possible as of 7fe1702. CC @cpovirk.

[cpovirk]

Another thing that could live here is a factory method for IncomparableValueException (#1913).

[cpovirk]

If we were to do this, we'd want to consider whether to explode out the overloads to avoid boxing and varargs allocation, as we do in Preconditions. But maybe code that uses this is already likely to allocate, as in the following chain...

return some().optional().pipeline().orElseThrow(illegalArgument("foo bar: %s", foo));

(which allocates some Optional instances and a new Supplier -- and there's no getting around the Supplier allocation even if you write () -> throw new IllegalArgumentException(...(..., foo)))

...or it's likely to run only in the case in which we're actually throwing the exception, as in:

if (somethingNotFound) {
  throw illegalArgument("foo bar: %s", foo));
}

[edit: Oops, I am conflating 2 possible APIs: one that returns an IllegalArgumentException and one that returns a Supplier<IllegalArgumentException>. I'll need to make sure I get this straight at some point.]

[cpovirk]

from #5927 (comment):

throw illegalState doesn't save much vs. throw new IllegalStateException, and the "%s" support isn't important when it's not a conditional throw anymore.

Both true. Additionally, illegalState probably even increases the danger that a user would forget to write throw, perhaps assuming that illegalState throws the exception rather than just creating it. (Of course, we'd hope that people use @CheckReturnValue enforcement, which would catch that bug. But not everyone will.)

I do still somewhat like this idea, both for the Optional.orElseThrow case and for the fact that it can do Strings.lenientFormat-style formatting (as opposed to String.format, which is what some people will reflexively reach for). I do think that %s formatting is sometimes nicer to read than + -- but also sometimes worse! -- so I still see value even when we aren't concerned about the cost of string concatenation.

[cpovirk]

(But there is also the general issue of whether such a small improvement justifies all the code-review comments about whether to use that API, even if some tools add review comments to suggest the API automatically.)

[cpovirk]

This came up twice in the past 24 hours.

In the first discussion, our team was chatting about a hypothetical T checkPresent(Optional<T> optional, String template, Object... args) method as a shorter alternative to orElseThrow(() -> new IllegalArgumentException(template.formatted(args))). (checkPresent, notably, would presumably be restricted to throwing IllegalArgumentException, while we could more easily offer a variety of factories alongside illegalArgument. And orElseThrow can be more fluent in long chains. But checkPresent is shorter. Anyway.)

That discussion served as a reminder of a point that I belatedly made above: There are at least two possible signatures for our factory methods:

var inputValue = optional.orElseThrow(() -> illegalArgument(...));

var inputValue = optional.orElseThrow(illegalArgument(...));

Then today, I was reminded that there's actually a third variant that could be useful. That variant would throw the exception, letting us change from...

map.merge(
    key,
    value,
    (first, second) -> {
      throw new IllegalArgumentException("multiple entries: " + first + ", " + second);
    });

...to...

map.merge(
    key,
    value,
    (first, second) -> throwIllegalArgument("multiple entries: %s, %s", first, second));

One nice part about it is that it eliminates the need for a block. But one bad part is that Java doesn't understand that it throws an exception, so it thinks that code afterward would be reachable. Thus, this variant would be unwise to use outside narrow cases like merge, and so it may be an attractive nuisance.