Detect catching java.lang.Error (#15)

* Add an error catch detector * Cleanup messages * Improve error messages * Fix style
Faithlife · Nov 15, 2022 · 5d3e729 · 5d3e729
1 parent 93c55e7
commit 5d3e729
Show file tree

Hide file tree

Showing 4 changed files with 231 additions and 0 deletions.
diff --git a/changelog.md b/changelog.md
@@ -12,6 +12,7 @@
   - This detector works best when applied to an app project with `lint.checkDependencies = true` in the app module AGP DSL.
 - `ForEachFunctionDetector` reports `forEach` and `forEachIndexed` use and encourages a language for loop replacement
 - `SkippedClassLocalOverrideDetector` warns when an explicit super method is called outside of the corresponding override.
+- `ErrorCatchDetector` reports an error when a catch block might catch a `java.lang.Error` type.
 
 ### Changed
 - Updated build tooling

diff --git a/checks/src/main/kotlin/com/faithlife/lint/ErrorCatchDetector.kt b/checks/src/main/kotlin/com/faithlife/lint/ErrorCatchDetector.kt
@@ -0,0 +1,82 @@
+package com.faithlife.lint
+
+import com.android.tools.lint.client.api.UElementHandler
+import com.android.tools.lint.detector.api.Category
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Implementation
+import com.android.tools.lint.detector.api.Incident
+import com.android.tools.lint.detector.api.Issue
+import com.android.tools.lint.detector.api.JavaContext
+import com.android.tools.lint.detector.api.Scope
+import com.android.tools.lint.detector.api.Severity
+import com.android.tools.lint.detector.api.SourceCodeScanner
+import org.jetbrains.uast.UCatchClause
+
+class ErrorCatchDetector : Detector(), SourceCodeScanner {
+    override fun getApplicableUastTypes() = listOf(UCatchClause::class.java)
+
+    override fun createUastHandler(context: JavaContext) = object : UElementHandler() {
+        override fun visitCatchClause(node: UCatchClause) {
+            for (typeRef in node.typeReferences) {
+                if (context.evaluator.typeMatches(typeRef.type, "java.lang.Throwable")) {
+                    Incident(context)
+                        .issue(ISSUE_CATCH_TOO_GENERIC)
+                        .message(TOO_GENERIC_MESSAGE)
+                        .scope(node)
+                        .location(context.getLocation(typeRef))
+                        .report()
+                }
+
+                val clazz = context.evaluator.getTypeClass(typeRef.type)
+                if (context.evaluator.extendsClass(clazz, "java.lang.Error")) {
+                    Incident(context)
+                        .issue(ISSUE_ERROR_CAUGHT)
+                        .message(ERROR_CAUGHT_MESSAGE)
+                        .scope(node)
+                        .location(context.getLocation(typeRef))
+                        .report()
+                }
+            }
+        }
+    }
+
+    companion object {
+        private const val TOO_GENERIC_MESSAGE = "Catching Throwable will include Errors. Be more specific."
+        private const val ERROR_CAUGHT_MESSAGE = "Errors should not be caught."
+        private const val DESC = "Catch blocks should not handle java.lang.Error"
+        private fun createExplanation(message: String) = """
+            $message
+
+            Catching errors can further complicate stacktraces and error investigation
+            in general. `java.lang.Error` and subtypes should not be caught. They indicate
+            a terminal program state that is best served by crashing quickly in order to provide
+            the best view of application state that lead to the `Error` being thrown.
+        """
+
+        val ISSUE_CATCH_TOO_GENERIC = Issue.create(
+            "ThrowableCatchDetector",
+            DESC,
+            createExplanation(TOO_GENERIC_MESSAGE),
+            moreInfo = "https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Error.html",
+            category = Category.CORRECTNESS,
+            severity = Severity.ERROR,
+            implementation = Implementation(
+                ErrorCatchDetector::class.java,
+                Scope.JAVA_FILE_SCOPE,
+            ),
+        )
+
+        val ISSUE_ERROR_CAUGHT = Issue.create(
+            "ErrorCatchDetector",
+            DESC,
+            createExplanation(ERROR_CAUGHT_MESSAGE),
+            moreInfo = "https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Error.html",
+            category = Category.CORRECTNESS,
+            severity = Severity.ERROR,
+            implementation = Implementation(
+                ErrorCatchDetector::class.java,
+                Scope.JAVA_FILE_SCOPE,
+            ),
+        )
+    }
+}
diff --git a/checks/src/main/kotlin/com/faithlife/lint/IssueRegistry.kt b/checks/src/main/kotlin/com/faithlife/lint/IssueRegistry.kt
@@ -13,6 +13,8 @@ class IssueRegistry : ApiIssueRegistry() {
     )
 
     override val issues = listOf(
+        ErrorCatchDetector.ISSUE_CATCH_TOO_GENERIC,
+        ErrorCatchDetector.ISSUE_ERROR_CAUGHT,
         FiniteWhenCasesDetector.ISSUE,
         ForEachFunctionDetector.ISSUE,
         IndirectSuperCallDetector.ISSUE,

diff --git a/checks/src/test/kotlin/com/faithlife/lint/ErrorCatchDetectorTest.kt b/checks/src/test/kotlin/com/faithlife/lint/ErrorCatchDetectorTest.kt
@@ -0,0 +1,146 @@
+package com.faithlife.lint
+
+import com.android.tools.lint.checks.infrastructure.LintDetectorTest
+import org.junit.Test
+
+class ErrorCatchDetectorTest : LintDetectorTest() {
+    override fun getDetector() = ErrorCatchDetector()
+    override fun getIssues() = listOf(
+        ErrorCatchDetector.ISSUE_CATCH_TOO_GENERIC,
+        ErrorCatchDetector.ISSUE_ERROR_CAUGHT,
+    )
+
+    @Test
+    fun `test clean`() {
+        val code = """
+            package error
+
+            import java.io.File
+            import java.io.FileNotFoundException
+
+            class ErrorHandler {
+                fun throwableTester() {
+                    val androidDir : File? = try {
+                        File("~/.android")
+                    } catch (e: FileNotFoundException) {
+                        System.err.println(e.message)
+                        null
+                    }
+                }
+            }
+        """.trimIndent()
+
+        lint().files(kotlin(code))
+            .run().expectClean()
+    }
+
+    @Test
+    fun `test throwable catch statement detected`() {
+        val code = """
+            package error
+
+            import java.io.File
+
+            class ErrorHandler {
+                fun throwableTester() {
+                    val androidDir : File? = try {
+                        File("~/.android")
+                    } catch (e: Throwable) {
+                        System.err.println(e.message)
+                        null
+                    }
+                }
+            }
+        """.trimIndent()
+
+        lint().files(kotlin(code))
+            .run().expectErrorCount(1)
+    }
+
+    @Test
+    fun `test error catch statement detected`() {
+        val code = """
+            package error
+
+            import java.io.File
+
+            class ErrorHandler {
+                fun throwableTester() {
+                    val androidDir : File? = try {
+                        File("~/.android")
+                    } catch (e: OutOfMemoryError) {
+                        System.err.println(e.message)
+                        null
+                    }
+                }
+            }
+        """.trimIndent()
+
+        lint().files(kotlin(code))
+            .run().expect(
+                """src/error/ErrorHandler.kt:9: Error: Errors should not be caught. [ErrorCatchDetector]
+        } catch (e: OutOfMemoryError) {
+                    ~~~~~~~~~~~~~~~~
+1 errors, 0 warnings""",
+            )
+    }
+
+    @Test
+    fun `test consecutive catch statements detected`() {
+        val code = """
+            package error
+
+            import java.io.File
+
+            class ErrorHandler {
+                fun throwableTester() {
+                    val androidDir : File? = try {
+                        File("~/.android")
+                    } catch (e: OutOfMemoryError) {
+                        System.err.println(e.message)
+                        null
+                    } catch (t: Throwable) {
+                        System.err.println(e.message + " was thrown")
+                        null
+                    }
+                }
+            }
+        """.trimIndent()
+
+        lint().files(kotlin(code))
+            .run().expectErrorCount(2)
+    }
+
+    @Test
+    fun `test error multi-catch java statement detected`() {
+        val code = """
+            package error;
+
+            import java.io.File;
+
+            class ErrorHandler {
+                public void throwableTester() {
+                    File androidDir = null;
+                    try {
+                        androidDir = File("~/.android");
+                    } catch (OutOfMemoryError | Throwable | Exception e) {
+                        System.err.println(e.message);
+                    }
+
+                    System.out.println(androidDir != null ? androidDir.getAbsolutePath() : "");
+                }
+            }
+        """.trimIndent()
+
+        lint().files(java(code))
+            .run().expect(
+                """src/error/ErrorHandler.java:10: Error: Errors should not be caught. [ErrorCatchDetector]
+        } catch (OutOfMemoryError | Throwable | Exception e) {
+                 ~~~~~~~~~~~~~~~~
+src/error/ErrorHandler.java:10: Error: Catching Throwable will include Errors. Be more specific. [ThrowableCatchDetector]
+        } catch (OutOfMemoryError | Throwable | Exception e) {
+                                    ~~~~~~~~~
+2 errors, 0 warnings""",
+            )
+    }
+}