requireNotNull vs sure operator !! in Kotlin

Question

We have a Kotlin based application and recently we added third party code quality tools (Detekt in Codacy). However, we started facing UnsafeCallOnNullableType errors. We found that approach viable to use is to add requireNotNull checks on all parameter which maybe null. Currently, we are using sure operator (!!)

Do we have any specific reason or convention to chose one over the other. As far as I see, both will throw an Exception and will block the execution flow, except one will throw IllegalArgumentException while the other will throw NullPointerException.

Zel

Found answer from kotlin team. discuss.kotlinlang.org/t/requirenotnull-or/1562/2 – Doloroso

Answer 1

requireNotNull, assuming you're referring to Objects#requireNonNull, is a Java method that is equivalent to !!, but with a different exception.

You didn't add any code, so it's slightly hard to help you debug. You mentioned third party code quality tools, but not which. I randomly stumbled over this GH issue that matches the error you're having. It's also the only thing I can find that would at any point use that exact error. I might have missed some, but it's the one that covers the top Google hits, so I'm going to go off that.

If you are using Detekt, this is a reported bug. Using !! is even suggested by IntelliJ.

However, you can do it another way.

Yes, using Objects#requireNonNull is one option. There is a second one though, and that's using the null-safe operator, as m0skit0 mentioned.

The reason this works is because if anything called is null, the final result is null. I.e. this:

instance.nonNullType.nullable?.nullableChild?.someOtherNullableChild

If any of the nullable ones are null, the final result is null, and none of the others are called.

Now, considering this is likely a bug in Detect, this seems like the easiest workaround for now:

whatever.calls.you?.make?.to?.the?.database ?: throw NullPointerException("Something is null");

It also keeps the variable non-null, which means you don't need null-safe calls later. The elvis operator checks if anything is null, then throws an exception. Alternatively, you can just use Objects#requireNotNull:

Objects.requireNonNull(whatever.calls.you.make.to.the.database)

If you really need to validate every single step, you'll just need to keep null checks everywhere

TL;DR:

!! and requireNotNull are effectively identical in how they work, except requireNotNull is a method call and !! compiles to an if-statement:

if(whatever == null) {
    Intrinsics.throwNpe();
}

The reason !! triggers UnsafeCallOnNullableType is because of a (probable) bug in Detekt. Both of the options are syntactic sugar for the same thing, though: if the variable is null, throw an NPE.

Kingpin

Codacy is the tool we have integrated with GitHub repository. Its not code specific but triggers error wherever we used !!. Though, our code runs successfully, if we ignore Codacy. Yeah, we indeed use Detekt. Didn't realize, its a bug. Definitely, the tool wanted to change all ours !! to requireNotNull :) Thanks!! – Zel

Answer 2

As you mentioned, requireNotNull() throws IllegalArgumentException and !! throws NullPointerException. That could be helpful if you want to differentiate between defensive code added by your devs versus undefended code (by using !! is isn't immediately obvious).

However, the bigger benefit of using requireNotNull() is using the function that takes the lazyMessage argument. This way your devs can add more meaningful messages to the exception, which could help in debugging

Underpay

Answer 3

In my experience the best practice when a value is nullable is to use ? and ?: operators and provide an alternative (if available) when the value is null, for example:

settings?.getValue("some-setting") ?: defaultValue

Note that this expression will return defaultValue when either settings or getValue return null.

It is best to try to avoid using !! operator at all since this basically defeats any protection against nulls. When this is not possible, I would throw a more comprehensive exception instead of relying on generic exceptions like IAE or NPE, for example:

settings?.getValue("some-setting") ?: throw SettingNotFound("Descriptive message")

Felafel

In my case, its more like object.property!! This is so because property in my requirements can be null. We know the cases where this can be null and therefore when pulling value from store or DB, we add !! to validate this should never be null during those cases. Using elvis operator, will not ensure if its null and we still have to manually add null checks and throw exception if value is not there. – Zel

Then the property should be declared as non-nullable and add the null checking when pulling this property from store/DB and act accordingly, i.e. throwing a more comprehensive exception than NPE (it is never a good idea to catch a RuntimeException). – Felafel