linter: add finally return check for unreachable checker

src/linter/block.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/z7zmey/php-parser/pkg/position"
 	"github.com/z7zmey/php-parser/pkg/token"
 
 	"github.com/VKCOM/noverify/src/ir"
@@ -682,17 +683,19 @@ func (b *blockWalker) handleTry(s *ir.TryStmt) bool {
 		b.ctx.containsExitFlags |= ctx.containsExitFlags
 	}
 
-	ctx := b.withNewContext(func() {
+	tryCtx := b.withNewContext(func() {
 		for _, s := range s.Stmts {
 			b.addStatement(s)
 			s.Walk(b)
 		}
 	})
-	if ctx.exitFlags == 0 {
+	if tryCtx.exitFlags == 0 {
 		linksCount++
 	}
 
-	contexts = append(contexts, ctx)
+	b.checkUnreachableForFinallyReturn(s, tryCtx, finallyCtx, contexts)
+
+	contexts = append(contexts, tryCtx)
 
 	varTypes := make(map[string]types.Map, b.ctx.sc.Len())
 	defCounts := make(map[string]int, b.ctx.sc.Len())
@@ -723,16 +726,120 @@ func (b *blockWalker) handleTry(s *ir.TryStmt) bool {
 		})
 	}
 
-	if othersExit && ctx.exitFlags != 0 {
+	if othersExit && tryCtx.exitFlags != 0 {
 		b.ctx.exitFlags |= prematureExitFlags
-		b.ctx.exitFlags |= ctx.exitFlags
+		b.ctx.exitFlags |= tryCtx.exitFlags
 	}
 
-	b.ctx.containsExitFlags |= ctx.containsExitFlags
+	b.ctx.containsExitFlags |= tryCtx.containsExitFlags
 
 	return false
 }
 
+func (b *blockWalker) checkUnreachableForFinallyReturn(tryStmt *ir.TryStmt, tryCtx *blockContext, finallyCtx *blockContext, catchContexts []*blockContext) {
+	if finallyCtx == nil {
+		return
+	}
+
+	var exitPoints []exitPoint
+
+	// If try block contains some other return/die statements.
+	containsOtherNoThrowExitPoints := tryCtx.containsExitFlags&FlagReturn != 0 ||
+		tryCtx.containsExitFlags&FlagDie != 0
+
+	if containsOtherNoThrowExitPoints {
+		exitFlagsWithoutThrow := tryCtx.containsExitFlags ^ FlagThrow
+		points := b.findExitPointsByFlags(&ir.StmtList{Stmts: tryStmt.Stmts}, exitFlagsWithoutThrow)
+		exitPoints = append(exitPoints, points...)
+	}
+
+	var catchContainsDie bool
+	var catchWithDieIndex int
+
+	for i, context := range catchContexts {
+		containsDie := context.exitFlags&FlagDie != 0
+		exitFlagsWithoutDie := context.exitFlags ^ FlagDie
+		containsOtherNoDieExitPoints := exitFlagsWithoutDie != 0
+
+		if containsOtherNoDieExitPoints {
+			points := b.findExitPointsByFlags(tryStmt.Catches[i], exitFlagsWithoutDie)
+			exitPoints = append(exitPoints, points...)
+		}
+
+		if containsDie {
+			catchContainsDie = true
+			catchWithDieIndex = i + 1
+		}
+	}
+
+	if catchContainsDie {
+		b.r.Report(tryStmt.Finally, LevelError, "unreachable", "block finally is unreachable (because catch block %d contains a exit/die)", catchWithDieIndex)
+
+		// If there is an error when the finally block is unreachable,
+		// then errors due to return in finally are skipped.
+		return
+	}
+
+	if finallyCtx.exitFlags == FlagReturn {
+		var finallyReturnPos position.Position
+		finallyReturns := b.findExitPointsByFlags(tryStmt.Finally, FlagReturn)
+		if len(finallyReturns) > 0 {
+			finallyReturnPos = *ir.GetPosition(finallyReturns[0].n)
+		}
+
+		for _, point := range exitPoints {
+			b.r.Report(point.n, LevelError, "unreachable", "%s is unreachable (because finally block contains a return on line %d)", point.kind, finallyReturnPos.StartLine)
+		}
+	}
+}
+
+type exitPoint struct {
+	n    ir.Node
+	kind string
+}
+
+func (b *blockWalker) findExitPointsByFlags(where ir.Node, exitFlags int) (points []exitPoint) {
+	irutil.Inspect(where, func(n ir.Node) bool {
+		if exitFlags&FlagReturn != 0 {
+			ret, ok := n.(*ir.ReturnStmt)
+			if ok {
+				points = append(points, exitPoint{
+					n:    ret,
+					kind: "return",
+				})
+			}
+		}
+
+		if exitFlags&FlagThrow != 0 {
+			thr, ok := n.(*ir.ThrowStmt)
+			if ok {
+				points = append(points, exitPoint{
+					n:    thr,
+					kind: "throw",
+				})
+			}
+		}
+
+		if exitFlags&FlagDie != 0 {
+			exit, ok := n.(*ir.ExitExpr)
+			if ok {
+				typ := "exit"
+				if exit.Die {
+					typ = "die"
+				}
+
+				points = append(points, exitPoint{
+					n:    exit,
+					kind: typ,
+				})
+			}
+		}
+		return true
+	})
+
+	return points
+}
+
 func (b *blockWalker) handleCatch(s *ir.CatchStmt) {
 	typeList := make([]types.Type, 0, len(s.Types))
 	for _, t := range s.Types {

src/tests/inline/testdata/checkers/finallyReturn.php

@@ -0,0 +1,130 @@
+<?php
+
+/**
+ * @throws Exception
+ */
+function throwException() { throw new Exception; }
+
+function finallyReturnBadReturnInCatch(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        return 2; // want `return is unreachable (because finally block contains a return on line 14)`
+    } finally {
+        return 1;
+    }
+}
+
+function finallyReturnBadThrowInCatch(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        throw new Exception(); // want `throw is unreachable (because finally block contains a return on line 24)`
+    } finally {
+        return 1;
+    }
+}
+
+function finallyReturnBadDieInCatch(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        die();
+    } finally { // want `block finally is unreachable (because catch block 1 contains a exit/die)`
+        return 1;
+    }
+}
+
+function finallyReturnBadMultiplyExitPointInCatch(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        if (1) {
+            die();
+        } else {
+            return 2;
+        }
+    } finally { // want `block finally is unreachable (because catch block 1 contains a exit/die)`
+        return 1;
+    }
+}
+
+function finallyReturnBadMultiplyExitPointInTry(): int {
+    try {
+        if (0) {
+            throwException();
+        } else {
+            return 1; // want `return is unreachable (because finally block contains a return on line 61)`
+        }
+    } catch (Exception $_) {
+    } finally {
+        return 1;
+    }
+}
+
+function finallyReturnBadMultiplyCatch(): int {
+    try {
+        throwException();
+    } catch (RuntimeException $_) {
+        return 2; // want `return is unreachable (because finally block contains a return on line 73)`
+    } catch (Exception $_) {
+       return 3; // want `return is unreachable (because finally block contains a return on line 73)`
+    } finally {
+        return 1;
+    }
+}
+
+function finallyReturnBadMultiplyCatchWithDie(): int {
+    try {
+        throwException();
+    } catch (RuntimeException $_) {
+        return 2;
+    } catch (Exception $_) {
+        die();
+    } finally { // want `block finally is unreachable (because catch block 2 contains a exit/die)`
+        return 1;
+    }
+}
+
+function finallyReturnBadMultiplyCatchWithExit(): int {
+    try {
+        throwException();
+    } catch (RuntimeException $_) {
+        return 2;
+    } catch (Exception $_) {
+        exit();
+    } finally { // want `block finally is unreachable (because catch block 2 contains a exit/die)`
+        return 1;
+    }
+}
+
+function finallyReturnOkWithoutReturnInTryCatch(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+    } finally {
+        return 1; // ok, catch and try blocks don't contain return/exceptions/die/etc
+    }
+}
+
+function finallyReturnOkFinallyWithoutReturn(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        return 1;
+    } finally {
+        echo 1; /* ok, finally don't contain return */
+    }
+}
+
+function finallyReturnOkFinallyWithConditionalReturn(): int {
+    try {
+        throwException();
+    } catch (Exception $_) {
+        return 1;
+    } finally {
+        if (2) {
+            return 2;
+        }
+    }
+}