コンパイルエラーが出てないからって油断はしちゃだめだよ #Android

経緯

こんなコードがありました。

val hoge: Hoge?
if (savedInstanceState != null) {
    hoge = savedInstanceState.getParcelable("hoge")
} else {
    hoge = arguments?.getParcelable("hoge")
} 

if (hoge == null) {
    throw IllegalArgumentException()
}

私は、良かれと思ってこういうコードに書き換えてコミットしました。

val hoge: Hoge = if (savedInstanceState != null) {
    savedInstanceState.getParcelable("hoge")
} else {
    arguments?.getParcelable("hoge")
} ?: throw IllegalArgumentException()

冗長な処理を短く書いただけ　という認識でしっかりチェックをしませんでした。

さて、問題はあるでしょうか？
答えはYESです。このコードは実行時にクラッシュします。

環境

kotlin 1.7.2

対策

正確にはarguments?.getParcelable("hoge")の部分でjava.lang.ClassCastExceptionの例外でクラッシュします。
Hoge cannot be cast to java.lang.Voidと続きます。

しかしこのコードはAndroid Studio上ではエラーを吐きません。
過去にも事例があるようです。

例えば以下の様に個別に型キャストをするとクラッシュしなくなります。

val hoge = if (savedInstanceState != null) {
    savedInstanceState.getParcelable("hoge") as Hoge?
} else {
    arguments?.getParcelable("hoge") as Hoge?
} ?: throw IllegalArgumentException()

ここで、コンパイルでエラーが出なかったせいだ！というのは簡単ですが、
しかしこのクラッシュを見過ごした原因は明らかにヒューマンエラーです。

教訓１

Kotlinは大変優秀な言語で、大体のバグはコンパイルの過程で未然に防いでくれます。
裏を返せば、コンパイルが通ったから重大なバグは発生しない、という油断にも繋がりかねません。

変更を加えた箇所は必ずテストをすること、またはテストコードがあると尚良かったですね。

教訓２

実は上述のコードはgetParcelableの部分でdeprecatedのハイライトをされます。
deprecatedになっている処理を放っておくのはそもそもよくありません。今回のようなバグの原因調査のノイズにもなります。

その後PRレビューでご指摘いただきまして、例えばこのように定義されたインライン関数を用いて、

util.kt

inline fun <reified T : Parcelable> Bundle.parcelable(key: String): T? = when {
    Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU -> getParcelable(key, T::class.java)
    else -> @Suppress("DEPRECATION") getParcelable(key)
}

getParcelableを置き換える形に最終的に落ち着きました。

val hoge: Hoge = if (savedInstanceState != null) {
    savedInstanceState.parcelable("hoge")
} else {
    arguments?.parcelable("hoge")
} ?: throw IllegalArgumentException()

結論

変更した箇所は動作確認(相当のチェック)をする

あまりにも雑ですが、これに尽きるかもしれない。

余談

ちなみにこの変更は色々なチェックを華麗にスルーし、リリース30分後に気づきました(急いでhotfixを出しました)。肝が冷える。
今ではみんな気が引き締まっていますが、本当に油断は大敵ですね。人間は信用なりません。