From 908dbf32cf2d4efa8661a3857f807ad184132af8 Mon Sep 17 00:00:00 2001
From: Hung Nguyen <hungnv@google.com>
Date: Fri, 13 Jan 2023 16:11:12 +0000
Subject: [PATCH] IC: Clean up closing caches logic in
 IncrementalCompilerRunner.kt

Make the code more readable and address a small issue where we
did not close caches when returning `ICResult.RequiresRebuild`
(it was not exactly a correctness issue because we would recompile
non-incrementally in that case).

^KT-55709 In progress (This is a follow-up on a previous change for this
    bug)
---
 .../incremental/IncrementalCompilerRunner.kt  | 52 +++++++++++--------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt
index d1fa057606b..eafa2cbaa0e 100644
--- a/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt
+++ b/compiler/incremental-compilation-impl/src/org/jetbrains/kotlin/incremental/IncrementalCompilerRunner.kt
@@ -156,27 +156,13 @@ abstract class IncrementalCompilerRunner<
         }
         changedFiles as ChangedFiles.Known?
 
-        // Because `caches` is a Closeable resource, it is important to close them in the event of an exception or before any return
-        // statements.
-        // Note: Historically, closing caches used to throw an exception sometimes, so currently we want to collect those exceptions. In the
-        // future, if closing caches is safe, and we are no longer interested in those exceptions, we can simplify this code by using
-        // Kotlin's `Closable.use` function (see `compileNonIncrementally`).
-        fun closeCaches(caches: CacheManager, activeException: Throwable) {
-            try {
-                caches.close()
-            } catch (e: Throwable) {
-                activeException.addSuppressed(e)
-            }
-        }
-
         val caches = createCacheManager(args, projectDir)
-        val exitCode: ExitCode
-        try {
+
+        fun compile(): ICResult {
             // Step 1: Get changed files
             val knownChangedFiles: ChangedFiles.Known = try {
                 getChangedFiles(changedFiles, allSourceFiles, caches)
             } catch (e: Throwable) {
-                closeCaches(caches, e)
                 return ICResult.Failed(IC_FAILED_TO_GET_CHANGED_FILES, e)
             }
 
@@ -188,7 +174,6 @@ abstract class IncrementalCompilerRunner<
                     calculateSourcesToCompile(caches, knownChangedFiles, args, messageCollector, classpathAbiSnapshot ?: emptyMap())
                 }
             } catch (e: Throwable) {
-                closeCaches(caches, e)
                 return ICResult.Failed(IC_FAILED_TO_COMPUTE_FILES_TO_RECOMPILE, e)
             }
 
@@ -209,23 +194,48 @@ abstract class IncrementalCompilerRunner<
             } else null
 
             // Step 3: Compile incrementally
-            exitCode = try {
+            val exitCode = try {
                 compileImpl(compilationMode as CompilationMode.Incremental, allSourceFiles, args, caches, abiSnapshotData, messageCollector)
             } catch (e: Throwable) {
-                closeCaches(caches, e)
                 return ICResult.Failed(IC_FAILED_TO_COMPILE_INCREMENTALLY, e)
             }
+
+            return ICResult.Completed(exitCode)
+        }
+
+        fun closeCaches(caches: CacheManager, activeException: Throwable) {
+            try {
+                caches.close()
+            } catch (e: Throwable) {
+                activeException.addSuppressed(e)
+            }
+        }
+
+        // Because `caches` is a Closeable resource, it is important to close them in both cases:
+        //    1. in the event of an exception
+        //    2. after a normal execution
+        // Note: Historically, closing caches used to throw exceptions sometimes, so currently we want to collect those exceptions. In the
+        // future, if closing caches is safe, we can simplify the code by using Kotlin's `Closable.use` function (similar to the code in
+        // `compileNonIncrementally`).
+        val icResult = try {
+            compile()
         } catch (e: Throwable) {
+            // Case 1 - Close the caches upon an exception
             closeCaches(caches, e)
             throw e
         }
+
+        // Case 2 - Close the caches after a normal execution
         try {
             caches.close()
         } catch (e: Throwable) {
-            return ICResult.Failed(IC_FAILED_TO_CLOSE_CACHES, e)
+            return ICResult.Failed(
+                IC_FAILED_TO_CLOSE_CACHES,
+                RuntimeException("Failed to close caches, previous ICResult `$icResult` was discarded", e)
+            )
         }
 
-        return ICResult.Completed(exitCode)
+        return icResult
     }
 
     private fun compileNonIncrementally(