Repository: VKCOM/noverify
Pull request #593 (2020-07-17 07:59:57)

linter: traverse elseif cond only once
Since Cond is a part of `n` node, when we do n.Walk,
we also walk the condition. In elseif case we need to
handle condition with walkCond and then walk the rest
with n.Stmt.Walk, so the double walk for the cond
doesn't happen.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/linter/block.go b/src/linter/block.go
index d9a46cb..8130a31 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -387,15 +387,7 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 	case *stmt.Else:
 		b.r.checkKeywordCase(s, "else")
 	case *stmt.ElseIf:
-		if s.Merged {
-			b.r.checkKeywordCase(s, "else")
-			if ff := (*s.GetFreeFloating())[freefloating.Else]; len(ff) != 0 {
-				rightmostPos := ff[len(ff)-1].Position
-				b.r.checkKeywordCasePos(s, rightmostPos.EndPos, "if")
-			}
-		} else {
-			b.r.checkKeywordCase(s, "elseif")
-		}
+		b.handleElseIf(s)
 	case *stmt.If:
 		// TODO: handle constant if expressions
 		// TODO: maybe try to handle when variables are defined and used with the same condition
@@ -1914,8 +1906,11 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 		ctx := b.withNewContext(func() {
 			if elsif, ok := n.(*stmt.ElseIf); ok {
 				walkCond(elsif.Cond)
+				b.handleElseIf(elsif)
+				elsif.Stmt.Walk(b)
+			} else {
+				n.Walk(b)
 			}
-			n.Walk(b)
 			b.r.addScope(n, b.ctx.sc)
 		})
 
@@ -1973,6 +1968,18 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 	return false
 }
 
+func (b *BlockWalker) handleElseIf(s *stmt.ElseIf) {
+	if s.Merged {
+		b.r.checkKeywordCase(s, "else")
+		if ff := (*s.GetFreeFloating())[freefloating.Else]; len(ff) != 0 {
+			rightmostPos := ff[len(ff)-1].Position
+			b.r.checkKeywordCasePos(s, rightmostPos.EndPos, "if")
+		}
+	} else {
+		b.r.checkKeywordCase(s, "elseif")
+	}
+}
+
 func (b *BlockWalker) getCaseStmts(c node.Node) (cond node.Node, list []node.Node) {
 	switch c := c.(type) {
 	case *stmt.Case:


Test patch:
diff --git a/src/linttest/rules_test.go b/src/linttest/rules_test.go
index e07a150..d391656 100644
--- a/src/linttest/rules_test.go
+++ b/src/linttest/rules_test.go
@@ -9,6 +9,42 @@ import (
 	"github.com/VKCOM/noverify/src/rules"
 )
 
+func TestRuleIfElseif(t *testing.T) {
+	rfile := `<?php
+function testrule() {
+  /** @warning bad function called */
+  bad();
+}
+`
+	test := linttest.NewSuite(t)
+
+	test.AddFile(`<?php
+function bad() { return 0; }
+function good() { return 1; }
+`)
+
+	addNamedFile(test, `/elseif_cond1.php`, `<?php
+if (good()) {
+} elseif (bad()) {}`)
+
+	addNamedFile(test, `/elseif_cond2.php`, `<?php
+if (good()) {
+} elseif (bad()) {
+} elseif (bad()) {}`)
+
+	addNamedFile(test, `/if_cond.php`, `<?php
+	if (bad()) {
+	} elseif (good()) {}`)
+
+	test.Expect = []string{
+		`bad function called at /elseif_cond1.php`,
+		`bad function called at /elseif_cond2`,
+		`bad function called at /elseif_cond2`,
+		`bad function called at /if_cond.php`,
+	}
+	runRulesTest(t, test, rfile)
+}
+
 func TestRulePathFilter(t *testing.T) {
 	rfile := `<?php
 /**
@@ -26,7 +62,7 @@ eval(${"var"});
         `
 	addNamedFile(test, "/home/john/my/site/foo.php", code)
 	addNamedFile(test, "/home/john/my/site/ads_foo.php", code)
-	addNamedFile(test, "/home/jogh/my/site/ads_bar.php", code)
+	addNamedFile(test, "/home/john/my/site/ads_bar.php", code)
 	test.Expect = []string{
 		`don't eval from variable`,
 		`don't eval from variable`,