Repository: bazelbuild/buildtools
Pull request #701 (2019-08-13 23:23:58)

Update the warning about unused statements
If a no-effect warning is triggered by string expressions, additional explanation about docstrings is shown.


Patch:
diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go
index 33e5ce6..892a8cb 100644
--- a/warn/warn_control_flow.go
+++ b/warn/warn_control_flow.go
@@ -136,14 +136,15 @@ func unreachableStatementWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
-func noEffectStatementsCheck(f *build.File, body []build.Expr, isTopLevel, isFunc bool, findings []*LinterFinding) []*LinterFinding {
+func noEffectStatementsCheck(body []build.Expr, isTopLevel, isFunc bool, findings []*LinterFinding) []*LinterFinding {
 	seenNonComment := false
 	for _, stmt := range body {
 		if stmt == nil {
 			continue
 		}
 
-		if _, ok := stmt.(*build.StringExpr); ok {
+		_, isString := stmt.(*build.StringExpr)
+		if isString {
 			if !seenNonComment && (isTopLevel || isFunc) {
 				// It's a docstring.
 				seenNonComment = true
@@ -165,26 +166,30 @@ func noEffectStatementsCheck(f *build.File, body []build.Expr, isTopLevel, isFun
 			}
 			continue
 		}
-		findings = append(findings,
-			makeLinterFinding(stmt, "Expression result is not used."))
+
+		msg := "Expression result is not used."
+		if isString {
+			msg += " Docstrings should be the first statemensts of a file or a function (they may follow comment lines)."
+		}
+		findings = append(findings, makeLinterFinding(stmt, msg))
 	}
 	return findings
 }
 
 func noEffectWarning(f *build.File) []*LinterFinding {
 	findings := []*LinterFinding{}
-	findings = noEffectStatementsCheck(f, f.Stmt, true, false, findings)
+	findings = noEffectStatementsCheck(f.Stmt, true, false, findings)
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
 		// The AST should have a ExprStmt node.
 		// Since we don't have that, we match on the nodes that contain a block to get the list of statements.
 		switch expr := expr.(type) {
 		case *build.ForStmt:
-			findings = noEffectStatementsCheck(f, expr.Body, false, false, findings)
+			findings = noEffectStatementsCheck(expr.Body, false, false, findings)
 		case *build.DefStmt:
-			findings = noEffectStatementsCheck(f, expr.Function.Body, false, true, findings)
+			findings = noEffectStatementsCheck(expr.Function.Body, false, true, findings)
 		case *build.IfStmt:
-			findings = noEffectStatementsCheck(f, expr.True, false, false, findings)
-			findings = noEffectStatementsCheck(f, expr.False, false, false, findings)
+			findings = noEffectStatementsCheck(expr.True, false, false, findings)
+			findings = noEffectStatementsCheck(expr.False, false, false, findings)
 		}
 	})
 	return findings
diff --git a/warn/warn_operation.go b/warn/warn_operation.go
index 289e0c5..9fdcfab 100644
--- a/warn/warn_operation.go
+++ b/warn/warn_operation.go
@@ -133,7 +133,7 @@ func stringEscapeWarning(f *build.File) []*LinterFinding {
 			value = str.Token[1 : len(str.Token)-1]
 		}
 
-		var problems []int  // positions of the problems (unidentified escape sequences)
+		var problems []int // positions of the problems (unidentified escape sequences)
 
 		escaped := false
 		// This for-loop doesn't correctly check for a backlash at the end of the string literal, but
@@ -168,7 +168,7 @@ func stringEscapeWarning(f *build.File) []*LinterFinding {
 				"Invalid escape sequence \\%s at position %d.",
 				string(value[problems[0]]),
 				problems[0],
-				)
+			)
 		} else {
 			var builder strings.Builder
 			builder.WriteString("Invalid escape sequences:\n")
@@ -177,7 +177,7 @@ func stringEscapeWarning(f *build.File) []*LinterFinding {
 					"    \\%s at position %d\n",
 					string(value[pos]),
 					pos,
-					))
+				))
 			}
 			msg = builder.String()
 		}


Test patch:
diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go
index 5e2662b..cb4aaa5 100644
--- a/warn/warn_control_flow_test.go
+++ b/warn/warn_control_flow_test.go
@@ -253,10 +253,14 @@ def bar():
 def bar():
     """A docstring"""
     foo
+    """ Not a docstring"""
     return foo
 `,
-		[]string{":7:", ":11:"},
-		scopeEverywhere)
+		[]string{
+			":7: Expression result is not used. Docstrings should be the first statemensts of a file or a function (they may follow comment lines).",
+			":11: Expression result is not used.",
+			":12: Expression result is not used. Docstrings should be the first statemensts of a file or a function (they may follow comment lines).",
+		}, scopeEverywhere)
 
 	checkFindings(t, "no-effect", `
 foo == bar