Repository: bazelbuild/buildtools
Pull request #517 (2019-01-15 17:34:46)

Warnings for unreachable statements
Warn about statements that are not reachable by the execution.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index e4cf1ef..c897f9a 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -33,6 +33,7 @@
   * [return-value](#return-value)
   * [same-origin-load](#same-origin-load)
   * [string-iteration](#string-iteration)
+  * [unreachable](#unreachable)
   * [unsorted-dict-items](#unsorted-dict-items)
   * [unused-variable](#unused-variable)
 
@@ -631,6 +632,16 @@ Use string indexing and `len` instead:
 
 --------------------------------------------------------------------------------
 
+## <a name="unreachable"></a>The statement is unreachable
+
+  * Category_name: `unreachable`
+  * Automatic fix: no
+
+The statement is unreachable because it follows a `return`, `break`, `continue`,
+or `fail()` statement.
+
+--------------------------------------------------------------------------------
+
 ## <a name="unused-variable"></a>Variable is unused
 
   * Category_name: `unused-variable`
diff --git a/warn/warn.go b/warn/warn.go
index 284dc76..e751942 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -1036,6 +1036,63 @@ The function may terminate by an implicit return in the end.`, true, nil))
 	return findings
 }
 
+// findUnreachableStatements searches for unreachable statements (i.e. statements that immediately
+// follow `return`, `break`, `continue`, and `fail()` statements and calls `callback` on them.
+// If there are several consequent unreachable statements, it only reports the first of them.
+// Returns whether the execution is terminated explicitly.
+func findUnreachableStatements(stmts []build.Expr, callback func(build.Expr)) bool {
+	unreachable := false
+	for _, stmt := range stmts {
+		if unreachable {
+			callback(stmt)
+			return true
+		}
+		switch stmt := stmt.(type) {
+		case *build.ReturnStmt:
+			unreachable = true
+		case *build.CallExpr:
+			ident, ok := stmt.X.(*build.Ident)
+			if ok && ident.Name == "fail" {
+				unreachable = true
+			}
+		case *build.Ident:
+			switch stmt.Name {
+			case "continue", "break":
+				unreachable = true
+			}
+		case *build.ForStmt:
+			findUnreachableStatements(stmt.Body, callback)
+		case *build.IfStmt:
+			// Save to separate values to avoid short circuit evaluation
+			term1 := findUnreachableStatements(stmt.True, callback)
+			term2 := findUnreachableStatements(stmt.False, callback)
+			if term1 && term2 {
+				unreachable = true
+			}
+		}
+	}
+	return unreachable
+}
+
+func unreachableStatementWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	for _, stmt := range f.Stmt {
+		function, ok := stmt.(*build.DefStmt)
+		if !ok {
+			continue
+		}
+
+		findUnreachableStatements(function.Body, func(expr build.Expr) {
+			start, end := expr.Span()
+			findings = append(findings,
+				makeFinding(f, start, end, "unreachable",
+					`The statement is unreachable.`, true, nil))
+		})
+	}
+	return findings
+}
+
 // RuleWarningMap lists the warnings that run on a single rule.
 // These warnings run only on BUILD files (not bzl files).
 var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr) *Finding{
@@ -1075,6 +1132,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"repository-name":     repositoryNameWarning,
 	"same-origin-load":    sameOriginLoadWarning,
 	"string-iteration":    stringIterationWarning,
+	"unreachable":         unreachableStatementWarning,
 	"unsorted-dict-items": unsortedDictItemsWarning,
 	"unused-variable":     unusedVariableWarning,
 }


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 53103d5..2a1df23 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -1235,3 +1235,77 @@ def foobar():
     fail()
 `, []string{}, scopeEverywhere)
 }
+
+func TestUnreachableStatementWarning(t *testing.T) {
+	// after return
+	checkFindings(t, "unreachable", `
+def foo():
+  return
+  bar()
+  baz()
+`, []string{
+		`:3: The statement is unreachable.`,
+	}, scopeEverywhere)
+
+	// two returns
+	checkFindings(t, "unreachable", `
+def foo():
+  return 1
+  return 2
+`, []string{
+		`:3: The statement is unreachable.`,
+	}, scopeEverywhere)
+
+	// after fail()
+	checkFindings(t, "unreachable", `
+def foo():
+  fail("die")
+  bar()
+  baz()
+`, []string{
+		`:3: The statement is unreachable.`,
+	}, scopeEverywhere)
+
+	// after break and continue
+	checkFindings(t, "unreachable", `
+def foo():
+  for x in y:
+    if x:
+      break
+      bar()  # unreachable
+    if y:
+      continue
+      bar()  # unreachable
+
+def bar():
+  for x in y:
+    if x:
+      break
+    elif y:
+      continue
+    else:
+      return x
+
+    foo()  # unreachable
+  foobar()  # potentially reachable
+`, []string{
+		`:5: The statement is unreachable.`,
+		`:8: The statement is unreachable.`,
+		`:19: The statement is unreachable.`,
+	}, scopeEverywhere)
+
+	// ok
+	checkFindings(t, "unreachable", `
+def foo():
+  if x:
+    return
+  bar()
+`, []string{}, scopeEverywhere)
+
+	// ok
+	checkFindings(t, "unreachable", `
+def foo():
+  x() or fail("maybe")
+  bar()
+`, []string{}, scopeEverywhere)
+}