From 32fe933f3ae75cf8ad2019425cc1a550e02b6894 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 3 Feb 2021 14:25:59 -0800
Subject: [PATCH] Add a lint check for catching "Exception" without catching
 "Throwable"

Summary:
Ref T13588. For consistency of behavior between versions on either side of PHP7, any "catch (Exception)" block should generally have a "catch (Throwable)" block in the same catch list.

Raise a lint warning when "Exception" is caught without also catching "Throwable", since this is almost certainly not desired.

Test Plan: Added tests.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21542
---
 src/__phutil_library_map__.php                |  4 ++
 .../ArcanistPartialCatchXHPASTLinterRule.php  | 45 +++++++++++++++++++
 ...stPartialCatchXHPASTLinterRuleTestCase.php | 10 +++++
 .../catch-without-throwable.lint-test         | 17 +++++++
 4 files changed, 76 insertions(+)
 create mode 100644 src/lint/linter/xhpast/rules/ArcanistPartialCatchXHPASTLinterRule.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistPartialCatchXHPASTLinterRuleTestCase.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/partial-catch/catch-without-throwable.lint-test

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9f9914ec..0bd7549a 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -402,6 +402,8 @@ phutil_register_library_map(array(
     'ArcanistParenthesesSpacingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistParenthesesSpacingXHPASTLinterRuleTestCase.php',
     'ArcanistParseStrUseXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistParseStrUseXHPASTLinterRule.php',
     'ArcanistParseStrUseXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistParseStrUseXHPASTLinterRuleTestCase.php',
+    'ArcanistPartialCatchXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPartialCatchXHPASTLinterRule.php',
+    'ArcanistPartialCatchXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPartialCatchXHPASTLinterRuleTestCase.php',
     'ArcanistPasteRef' => 'ref/paste/ArcanistPasteRef.php',
     'ArcanistPasteSymbolRef' => 'ref/paste/ArcanistPasteSymbolRef.php',
     'ArcanistPasteWorkflow' => 'workflow/ArcanistPasteWorkflow.php',
@@ -1451,6 +1453,8 @@ phutil_register_library_map(array(
     'ArcanistParenthesesSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistParseStrUseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistParseStrUseXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistPartialCatchXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistPartialCatchXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPasteRef' => 'ArcanistRef',
     'ArcanistPasteSymbolRef' => 'ArcanistSimpleSymbolRef',
     'ArcanistPasteWorkflow' => 'ArcanistArcWorkflow',
diff --git a/src/lint/linter/xhpast/rules/ArcanistPartialCatchXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPartialCatchXHPASTLinterRule.php
new file mode 100644
index 00000000..720d68c2
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/ArcanistPartialCatchXHPASTLinterRule.php
@@ -0,0 +1,45 @@
+<?php
+
+final class ArcanistPartialCatchXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 132;
+
+  public function getLintName() {
+    return pht('Partial Catch');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_WARNING;
+  }
+
+  public function process(XHPASTNode $root) {
+    $catch_lists = $root->selectDescendantsOfType('n_CATCH_LIST');
+
+    foreach ($catch_lists as $catch_list) {
+      $catches = $catch_list->getChildrenOfType('n_CATCH');
+
+      $classes = array();
+      foreach ($catches as $catch) {
+        $class_node = $catch->getChildOfType(0, 'n_CLASS_NAME');
+        $class_name = $class_node->getConcreteString();
+        $class_name = phutil_utf8_strtolower($class_name);
+
+        $classes[$class_name] = $class_node;
+      }
+
+      $catches_exception = idx($classes, 'exception');
+      $catches_throwable = idx($classes, 'throwable');
+
+      if ($catches_exception && !$catches_throwable) {
+        $this->raiseLintAtNode(
+          $catches_exception,
+          pht(
+            'Try/catch block catches "Exception", but does not catch '.
+            '"Throwable". In PHP7 and newer, some runtime exceptions '.
+            'will escape this block.'));
+      }
+    }
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistPartialCatchXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistPartialCatchXHPASTLinterRuleTestCase.php
new file mode 100644
index 00000000..3001c403
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistPartialCatchXHPASTLinterRuleTestCase.php
@@ -0,0 +1,10 @@
+<?php
+
+final class ArcanistPartialCatchXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(dirname(__FILE__).'/partial-catch/');
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/partial-catch/catch-without-throwable.lint-test b/src/lint/linter/xhpast/rules/__tests__/partial-catch/catch-without-throwable.lint-test
new file mode 100644
index 00000000..b8b3dfe7
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/partial-catch/catch-without-throwable.lint-test
@@ -0,0 +1,17 @@
+<?php
+try {
+  f();
+} catch (Exception $ex) {
+  g();
+}
+
+try {
+  f();
+} catch (Exception $ex) {
+  g();
+} catch (Throwable $ex) {
+  h();
+}
+
+~~~~~~~~~~
+warning:4:10:XHP132