Repository: VKCOM/noverify
Pull request #840 (2020-12-08 22:04:11)

linter: add dupCatch and catchOrder checkers
When using multi-catch try statements, sometimes programmers get the
catch clause order wrong:

Catching derived exception after the base exception:
  https://github.com/woocommerce/woocommerce/blob/1c65696535b4de34263554280c4ae7ef8b57d8ea/includes/legacy/api/class-wc-rest-legacy-orders-controller.php#L271
WC_REST_Exception extends WC_Data_Exception:
  https://github.com/woocommerce/woocommerce/blob/1c65696535b4de34263554280c4ae7ef8b57d8ea/includes/class-wc-rest-exception.php#L16

dupCatch: report duplicated catch clauses.

	try {
	  // some code
	} catch (Exception1 $e) {
	} catch (Exception1 $e) {} // <- note the typo

	=>

	try {
	  // some code
	} catch (Exception1 $e) {
	} catch (Exception2 $e) {}

catchOrder: Report erroneous catch order in try statements.

	try {
	  // some code
	} catch (Exception $e) {
	  // bad: this will catch both Exception and TimeoutException
	} catch (TimeoutException $e) {
	  // bad: this is a dead code
	}

	=>

	try {
	  // some code
	} catch (TimeoutException $e) {
	  // good: it can catch TimeoutException
	} catch (Exception $e) {
	  // good: it will catch everything else
	}


Patch:
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index 23e93ee..31fdeb3 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -191,6 +191,52 @@ func (b *blockLinter) checkTryStmt(s *ir.TryStmt) {
 	if s.Finally != nil {
 		b.walker.r.checkKeywordCase(s.Finally, "finally")
 	}
+
+	if len(s.Catches) > 1 {
+		b.checkCatchOrder(s)
+	}
+}
+
+func (b *blockLinter) checkCatchOrder(s *ir.TryStmt) {
+	// This code has O(n^2) complexity, but there are usually no more than 3-4 catch clauses in the code.
+	// We could avoid some extra work if we would not add leaf types to the classes list,
+	// but we don't have that information available.
+
+	classes := make([]string, 0, len(s.Catches))
+
+	for _, c := range s.Catches {
+		c := c.(*ir.CatchStmt)
+		if len(c.Types) > 1 {
+			return // give up on A|B catch
+		}
+
+		class, ok := solver.GetClassName(b.walker.r.ctx.st, c.Types[0])
+		if !ok {
+			continue
+		}
+
+		add := true
+		for _, otherClass := range classes {
+			if class == otherClass {
+				b.report(c.Types[0], LevelWarning, "dupCatch", "duplicated catch on %s", class)
+				add = false
+				break
+			}
+			if solver.Extends(class, otherClass) {
+				b.report(c.Types[0], LevelWarning, "catchOrder", "catch %s block will never run as it extends %s which is caught above", class, otherClass)
+				add = false
+				break
+			}
+			if solver.Implements(class, otherClass) {
+				b.report(c.Types[0], LevelWarning, "catchOrder", "catch %s block will never run as it implements %s which is caught above", class, otherClass)
+				add = false
+				break
+			}
+		}
+		if add {
+			classes = append(classes, class)
+		}
+	}
 }
 
 func (b *blockLinter) checkBitwiseOp(n, left, right ir.Node) {
diff --git a/src/linter/report.go b/src/linter/report.go
index e22b100..ed8c290 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -554,6 +554,42 @@ $foo = new Foo();`,
 			Default: true,
 			Comment: `Report the use of variables that were supposed to be unused, like $ _.`,
 		},
+
+		{
+			Name:     "dupCatch",
+			Default:  true,
+			Quickfix: false,
+			Comment:  `Report duplicated catch clauses.`,
+			Before: `try {
+  // some code
+} catch (Exception1 $e) {
+} catch (Exception1 $e) {} // <- note the typo`,
+			After: `try {
+  // some code
+} catch (Exception1 $e) {
+} catch (Exception2 $e) {}`,
+		},
+
+		{
+			Name:     "catchOrder",
+			Default:  true,
+			Quickfix: false,
+			Comment:  `Report erroneous catch order in try statements.`,
+			Before: `try {
+  // some code
+} catch (Exception $e) {
+  // bad: this will catch both Exception and TimeoutException
+} catch (TimeoutException $e) {
+  // bad: this is a dead code
+}`,
+			After: `try {
+  // some code
+} catch (TimeoutException $e) {
+  // good: it can catch TimeoutException
+} catch (Exception $e) {
+  // good: it will catch everything else
+}`,
+		},
 	}
 
 	for _, info := range allChecks {
diff --git a/src/solver/oop.go b/src/solver/oop.go
index 7037f4b..2ab6a66 100644
--- a/src/solver/oop.go
+++ b/src/solver/oop.go
@@ -113,3 +113,23 @@ func GetConstant(cs *meta.ClassParseState, constNode ir.Node) (constName string,
 
 	return "", meta.ConstInfo{}, false
 }
+
+// Extends reports whether derived class extends the base class.
+// It returns false for the derived==base case.
+func Extends(derived, base string) bool {
+	if derived == base {
+		return false
+	}
+	class, ok := meta.Info.GetClass(derived)
+	if !ok {
+		return false
+	}
+	switch class.Parent {
+	case base:
+		return true
+	case "":
+		return false
+	default:
+		return Extends(class.Parent, base)
+	}
+}


Test patch:
diff --git a/src/tests/checkers/catch_test.go b/src/tests/checkers/catch_test.go
new file mode 100644
index 0000000..525dd46
--- /dev/null
+++ b/src/tests/checkers/catch_test.go
@@ -0,0 +1,112 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestCatchDup(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class MyException1 extends Exception {}
+class MyException2 extends Exception {}
+
+try {
+} catch (MyException1 $e) {
+} catch (MyException2 $e) {
+} catch (MyException1 $e) {
+}
+`)
+	test.Expect = []string{`duplicated catch on \MyException1`}
+	test.RunAndMatch()
+}
+
+func TestCatchOrderThrowable(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+try {
+} catch (Throwable $e) {
+} catch (Exception $e) {
+}
+`)
+	test.Expect = []string{`catch \Exception block will never run as it implements \Throwable which is caught above`}
+	test.RunAndMatch()
+}
+
+func TestCatchOrderExtends(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class MyException extends Exception {}
+
+try {
+} catch (Exception $e) {
+} catch (MyException $e) {
+}
+`)
+	test.Expect = []string{`catch \MyException block will never run as it extends \Exception which is caught above`}
+	test.RunAndMatch()
+}
+
+func TestCatchOrderExtends2(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class ExceptionBase extends Exception {}
+class ExceptionDerived extends ExceptionBase {}
+
+try {
+} catch (ExceptionBase $e) {
+} catch (ExceptionDerived $e) {
+}
+`)
+	test.Expect = []string{`catch \ExceptionDerived block will never run as it extends \ExceptionBase which is caught above`}
+	test.RunAndMatch()
+}
+
+func TestCatchOrderExtendsGood(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+class ExceptionBase extends Exception {}
+class ExceptionDerived extends ExceptionBase {}
+
+try {
+} catch (ExceptionDerived $e) {
+} catch (ExceptionBase $e) {
+}
+`)
+}
+
+func TestCatchOrderImplements(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+interface CustomException {}
+
+class ExceptionBase extends Exception implements CustomException {}
+class ExceptionDerived extends ExceptionBase {}
+
+try {
+} catch (CustomException $e) {
+} catch (ExceptionBase $e) {
+} catch (ExceptionDerived $e) {
+}
+`)
+	test.Expect = []string{
+		`catch \ExceptionBase block will never run as it implements \CustomException which is caught above`,
+		`catch \ExceptionDerived block will never run as it implements \CustomException which is caught above`,
+	}
+	test.RunAndMatch()
+}
+
+func TestCatchOrderImplementsGood(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+interface CustomException {}
+
+class ExceptionBase extends Exception implements CustomException {}
+class ExceptionDerived extends ExceptionBase {}
+
+try {
+} catch (ExceptionDerived $e) {
+} catch (ExceptionBase $e) {
+} catch (CustomException $e) {
+}
+`)
+}