Extension functions: IllegalArgumentException vs. IllegalStateException

Given:

class C(val s: String?)

I you consider an extension function just syntactic sugar for a function that takes an object as a parameter, you would do this:

fun C.foo() {
    if (s == null)
        throw IllegalArgumentException("C is illegal because it does not have s.")
}

// Or:

fun C.foo() {
    requireNotNull(s) { "C is illegal because it does not have s." }
}

But when you view it as a method on an object, you would do this:

fun C.foo() {
    if (s == null)
        throw IllegalStateException("You cannot call foo() when s is null.")
}

// Or:

fun C.foo() {
    checkNotNull(s) { "You cannot call foo() when s is null." }
}

So, which one should you use?

2 Likes

I don’t think there is a right and wrong here as long as you stay consistent. I would probably use an IllegalStateException because I think of extension functions as a kind of “member” function. That said your argument to treat the receiver as an argument is also valid.

2 Likes

I think the way that the receiver is called this within the function, as well as the dot notation for calling it, are strong invitations to think of extension functions in a similar way to member functions (i.e. methods).

Yes, they’re implemented as static methods in the bytecode, but I think that’s more a reflection of JVM limitations, and for interoperability, than any statement of intent. It’s similar in Java: in my experience, most Java static methods would be written as extensions if that was an option.

So I think the arguments for treating them like methods are much stronger, and for me an IllegalStateException would feel more natural.

1 Like

This is an interesting point that could be taken for an argument for IllegalArgumentException as well. If you write a library that is used from java (and let’s face it, java is still much more used than kotlin sadly) then throwing an IllegalArgumentException might be the way to go, to make the interop with java even smoother.
And even with me bringing up this argument I personally think IllegalStateException is the way to go.

I think it varies depending on the function is and use-case. For me, this is all about communicating to the user as effectively as possible.

I usually land on illegal state exceptions for extension functions.

Although extension functions are statically resolved, they still feel like a property of instances of their reciever’s type–similar to how member functions are exactly that, but in addition to that member functions are dynamically dispatched.

Just to round out the idea: you could just as easily imagine member functions to be functions with their reciever as a parameter (with dynamical dispatch)–then you’d be asking this question about member functions.

2 Likes

When I look at both exceptions I would see IllegalArgumentException as a usage error, but IllegalStateException as also a design error (sometimes not even a usage error). In principle, you would aim to design classes such that they only have a single (behaviour relevant) state. Even with such a design it can be possible that the state is inconsistent/corrupt, but that would be an indication of an implementation bug (somewhere). Of course there are cases where there must be state, but often subclasses/generics/interfaces can be used to present a typesafe interface that encapsulates this state in a way that the compiler can check.

An IllegalArgumentException indicates to me that the caller is violating the function contract. So in the case that you have an extension function on a list and it doesn’t work on an empty list (by design) that is not a state issue, it is a value issue. The argument doesn’t fit the expected domain. At the same time I can see where you want parity between regular and extension functions and be consistent.

2 Likes

Well said @pdvrieze :+1: I hadn’t looked at it quite that way.

So if one chooses via those guidelines, what is chosen for a class with initialization (no extension functions)? Failing to call MyClass.initialize() before MyClass.doSomething() is a usage error. But I would consider this example to be one for an IllegalStateException.
Do you tend to stick with your guidelines for choosing or do you flex on this case?

A part of me wishes we had an IllegalUsageException instead of IllegalArgumentException.

@arocnies Beyond that you would attempt to avoid classes with initializer functions (one way is to have two types (possibly sharing private state), one is the creator, the other the initialized object - they could even share an interface where appropriate). However, in the case that it is unavoidable (like a fixed ABI) I would go with IllegalStateException as the expectation is that you only use initialized objects.

Btw. the builder pattern in general is a good way to avoid the issue with initialization, and in Kotlin you can even have an inline factory function that does the builder transparently (with lots of bits left out):

class TypeWithComplexInit private constructor(builder:Builder) {
    // code that uses the builder to initialize properties etc.

    class Builder(var propA:Int=0, var propB:Boolean=false)

    companion object {

        inline operator fun invoke(configurator: Builder.() -> Unit):TypeWithComplexInit {
            val b = Builder()
            b.configurator()
            return TypeWithComplexInit(b)
        }

    }
}
1 Like

I think it depends on the kind of contract, is it a verification of required arguments before computing on them, then you got an IllegalArgumentException in case there was a validation error.
If, however, the kind of computation was wrong, i.e. the states after it are not satisfying, then an IllegalStateException would be more appropriate.

It is somewhat true that illegal state exceptions shouldn’t exist as those can be, in theory, replaced by IllegalArgumentExceptions which check all possibilities for emerging illegal states.
But as it already sounds, this is practically unfortunate to impossible to hold on in productive software development even with fancy type systems.

1 Like

IMO, there’s not much difference between the two. When I think about exceptions, I first think about how they should be handled. For example, if an exception signals an unexpected external error, such as running out of disk space, pretty much the only thing you can do is to show an error message (perhaps not even log it, since there’s no free space). There are also expected exceptions that shouldn’t even exist in a well-designed programs, but sometimes you just have to do it. For example, a 3rd party library may throw an exception in a situation that is well expected in your software. In such cases, it makes sense to catch that exception as soon as possible and convert it into something more useful for flow control, such as a result-like object.

But when it comes down to things like IllegalStateException and IllegalArgumentException, they usually indicate a bug in software. It doesn’t really matter what kind of bug it is, as long as there are enough details in the error message. There are few exceptions to this rule, such as NumberFormatException, but that’s just bad library design.

Here’s an example that blurs the line between them even further. I designed a result-like class Outcome to avoid abusing exceptions and to have a neat way to organize code around expected errors. Sometimes, however, you know in advance that a certain call can’t possibly produce an exception (for example, Integer.parse(s) would never throw if you pass in a string that was previously matched against \d{3}). So I eventually came up with a test like this:

val result: Outcome<MyClass> = MyClass.parse(s)
val value: MyClass = resut.get()

Now, parse() can return either a failure or a success. And get() must throw an exception if called on a failure. What exception? Well, it’s an indication that the state of the result is wrong, so it must be an IllegalStateException, right? Seems so. But here’s another example:

fun tryParse(s: String): Outcome<MyClass> = // ...
fun parse(s: String): MyClass = tryParse(s).get()

Now I have two functions: one should be used when I don’t know in advance whether the argument is valid, the other one is to be called I do know that. But with an implementation like this, parse(s) will still throw an IllegalStateException on an invalid argument! This doesn’t make any sense because the Outcome instance is now purely an implementation detail of parse(). So, should I catch the exception in parse() and convert (or wrap) it into an IllegalArgumentException? One would think so, but it would just lead to unnecessary boilerplate while providing little to no value to the API user. Even if I were to encapsulate that boilerplate into something like getOrThrowIllegalArgumentException() and getOrThrowIllegalStateException() that would still be rather ugly.

Eventually I just decided to let IllegalStateException to propagate, and just never write catch (IllegalStateException) or catch (IllegalArgumentException).

2 Likes

+1

Also note, by catching ISE, you can easily make your coroutine/thread uncancellable, because CancellationException extends IllegalStateException

Since catching ISE/IAE is bad practice anyway, it doesn’t really matter which exception class you use.

If your function has an expected outcome, declare it as a separate exception type or use the enum / sealed class as the result.

2 Likes

Thanks for all the thoughtful answers! It seems that most of you favor the IllegalStateException. And it seems that it may have been a better design decision to not make a distinction between these two exception types in the first place.

1 Like