linter: improve if-related implicit vars handling

This change attempts to improve handleIf behavior.

We introduce implicit kind of vars to express automatically
defined vars that can be handled in a special way.

andWalker.varsToDelete are removed because we're running
all branches in a separate block contexts. No extra
cleanup is required.

Changed code requires to evaluate if statement condition
separately, so we can get variables defined inside it
into the parent block context.
assignWalker type does that.

Fixes #369
Fixes #363
Fixes #370

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>

src/linter/assign_walker.go

@@ -0,0 +1,28 @@
+package linter
+
+import (
+	"github.com/VKCOM/noverify/src/php/parser/node"
+	"github.com/VKCOM/noverify/src/php/parser/node/expr/assign"
+	"github.com/VKCOM/noverify/src/php/parser/walker"
+	"github.com/VKCOM/noverify/src/solver"
+)
+
+// assignWalker handles assignments by pushing all defined variables
+// to the associated block scope.
+type assignWalker struct {
+	b *BlockWalker
+}
+
+func (a *assignWalker) EnterNode(w walker.Walkable) (res bool) {
+	b := a.b
+	switch n := w.(type) {
+	case *assign.Assign:
+		switch v := n.Variable.(type) {
+		case *node.Var, *node.SimpleVar:
+			b.ctx.sc.ReplaceVar(v, solver.ExprTypeLocal(b.ctx.sc, b.r.st, n.Expression), "assign", true)
+		}
+	}
+	return true
+}
+
+func (a *assignWalker) LeaveNode(w walker.Walkable) {}

src/linter/block.go

@@ -1462,15 +1462,10 @@ func (b *BlockWalker) propagateFlagsFromBranches(contexts []*blockContext, links
 	}
 }
 
-// andWalker walks if conditions and adds isset/!empty/instanceof variables
+// andWalker walks if conditions and adds isset/!empty/instanceof implicit variables
 // to the associated block walker.
-//
-// All variables defined by andWalker should be removed after
-// if body is handled, this is why we collect varsToDelete.
 type andWalker struct {
 	b *BlockWalker
-
-	varsToDelete []node.Node
 }
 
 func (a *andWalker) EnterNode(w walker.Walkable) (res bool) {
@@ -1498,31 +1493,28 @@ func (a *andWalker) EnterNode(w walker.Walkable) (res bool) {
 
 			switch v := v.(type) {
 			case *node.SimpleVar:
-				a.b.addVar(v, meta.NewTypesMap("isset_$"+v.Name), "isset", true)
-				a.varsToDelete = append(a.varsToDelete, v)
+				a.b.ctx.sc.AddImplicitVar(v, meta.NewTypesMap("isset_$"+v.Name), "isset", true)
 			case *node.Var:
 				a.b.handleVariable(v.Expr)
 				vv, ok := v.Expr.(*node.SimpleVar)
 				if !ok {
 					continue
 				}
-				a.b.addVar(v, meta.NewTypesMap("isset_$$"+vv.Name), "isset", true)
-				a.varsToDelete = append(a.varsToDelete, v)
+				a.b.ctx.sc.AddImplicitVar(v, meta.NewTypesMap("isset_$$"+vv.Name), "isset", true)
 			}
 		}
 
 	case *expr.InstanceOf:
 		if className, ok := solver.GetClassName(a.b.r.st, n.Class); ok {
 			switch v := n.Expr.(type) {
 			case *node.Var, *node.SimpleVar:
-				a.b.ctx.sc.AddVar(v, meta.NewTypesMap(className), "instanceof", false)
+				a.b.ctx.sc.AddImplicitVar(v, meta.NewTypesMap(className), "instanceof", false)
 			default:
 				a.b.ctx.customTypes = append(a.b.ctx.customTypes, solver.CustomType{
 					Node: n.Expr,
 					Typ:  meta.NewTypesMap(className),
 				})
 			}
-			// TODO: actually this needs to be present inside if body only
 		}
 
 	case *expr.BooleanNot:
@@ -1542,8 +1534,7 @@ func (a *andWalker) EnterNode(w walker.Walkable) (res bool) {
 		if a.b.ctx.sc.HaveVar(v) {
 			break
 		}
-		a.b.addVar(v, meta.NewTypesMap("isset_$"+v.Name), "!empty", true)
-		a.varsToDelete = append(a.varsToDelete, v)
+		a.b.ctx.sc.AddImplicitVar(v, meta.NewTypesMap("isset_$"+v.Name), "!empty", true)
 	}
 
 	w.Walk(a.b)
@@ -1564,36 +1555,19 @@ func (b *BlockWalker) handleVariable(v node.Node) bool {
 
 	if !b.ctx.sc.HaveVar(v) {
 		b.r.reportUndefinedVariable(v, b.ctx.sc.MaybeHaveVar(v))
-		b.ctx.sc.AddVar(v, meta.NewTypesMap("undefined"), "undefined", true)
+		b.ctx.sc.AddImplicitVar(v, meta.NewTypesMap("undefined"), "undefined", true)
 	}
 
 	return false
 }
 
 func (b *BlockWalker) handleIf(s *stmt.If) bool {
-	var varsToDelete []node.Node
-	customMethods := len(b.ctx.customMethods)
-	// Remove all isset'ed variables after we're finished with this if statement.
-	defer func() {
-		for _, v := range varsToDelete {
-			b.ctx.sc.DelVar(v, "isset/!empty")
-		}
-		b.ctx.customMethods = b.ctx.customMethods[:customMethods]
-	}()
-	walkCond := func(cond node.Node) {
-		a := &andWalker{b: b}
-		cond.Walk(a)
-		varsToDelete = append(varsToDelete, a.varsToDelete...)
-	}
-
-	// first condition is always executed, so run it in base context
-	if s.Cond != nil {
-		walkCond(s.Cond)
-	}
+	// TODO: revisit this method when "else if" and "elseif" are
+	// parsed as a signle type (365).
 
 	var contexts []*blockContext
 
-	walk := func(n node.Node) (links int) {
+	walk := func(cond, n node.Node) (links int) {
 		// handle if (...) smth(); else other_thing(); // without braces
 		if els, ok := n.(*stmt.Else); ok {
 			b.addStatement(els.Stmt)
@@ -1604,11 +1578,16 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 		}
 
 		ctx := b.withNewContext(func() {
-			if elsif, ok := n.(*stmt.ElseIf); ok {
-				walkCond(elsif.Cond)
+			customMethods := len(b.ctx.customMethods)
+			a := &andWalker{b: b}
+			if cond != nil {
+				cond.Walk(a)
 			}
 			n.Walk(b)
 			b.r.addScope(n, b.ctx.sc)
+			if cond != nil {
+				b.ctx.customMethods = b.ctx.customMethods[:customMethods]
+			}
 		})
 
 		contexts = append(contexts, ctx)
@@ -1623,17 +1602,22 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 	linksCount := 0
 
 	if s.Stmt != nil {
-		linksCount += walk(s.Stmt)
+		// We evaluate s.Cond inside a new context to avoid
+		// redundant variables propagating into the else branches.
+		// We will grab all variables from after we finish with
+		// all if statement branches (see assignWalker usage below).
+		linksCount += walk(s.Cond, s.Stmt)
 	} else {
 		linksCount++
 	}
 
 	for _, n := range s.ElseIf {
-		linksCount += walk(n)
+		n := n.(*stmt.ElseIf)
+		linksCount += walk(n.Cond, n)
 	}
 
 	if s.Else != nil {
-		linksCount += walk(s.Else)
+		linksCount += walk(nil, s.Else)
 	} else {
 		linksCount++
 	}
@@ -1648,7 +1632,7 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 			continue
 		}
 
-		ctx.sc.Iterate(func(nm string, typ meta.TypesMap, alwaysDefined bool) {
+		ctx.sc.IterateExplicit(func(nm string, typ meta.TypesMap, alwaysDefined bool) {
 			varTypes[nm] = varTypes[nm].Append(typ)
 			if alwaysDefined {
 				defCounts[nm]++
@@ -1660,6 +1644,10 @@ func (b *BlockWalker) handleIf(s *stmt.If) bool {
 		b.ctx.sc.AddVarName(nm, types, "all branches", defCounts[nm] == linksCount)
 	}
 
+	// Collect all variable definitions from the if condition into the current context.
+	aw := assignWalker{b: b}
+	s.Cond.Walk(&aw)
+
 	return false
 }
 

src/linttest/basic_test.go

@@ -10,6 +10,69 @@ import (
 	"github.com/VKCOM/noverify/src/meta"
 )
 
+func TestIfCondAssign(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function f1($v) {
+  if ($x = $v) {}
+  echo $x;
+}
+function f2($v) {
+  if ($x = $v) {
+    exit(0);
+  }
+  echo $x;
+}
+`)
+}
+
+func TestElseIf1CondAssign(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f1($v) {
+  if ($v) {
+  } elseif ($x = 10) {}
+  echo $x;
+}
+function f2($v) {
+  if ($v) {
+  } elseif ($x = 10) {
+    exit(0);
+  }
+  echo $x;
+}
+`)
+	// It could be more precise to report 2 "might have not been defined",
+	// but at least we report both usages. Can be improved in future.
+	test.Expect = []string{
+		`Undefined variable: x`,
+		`Variable might have not been defined: x`,
+	}
+	test.RunAndMatch()
+}
+
+func TestElseIf2CondAssign(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f1($v) {
+  if ($v) {
+  } else if ($x = 10) {}
+  echo $x;
+}
+function f2($v) {
+  if ($v) {
+  } else if ($x = 10) {
+    exit(0);
+  }
+  echo $x;
+}
+`)
+	test.Expect = []string{
+		`Variable might have not been defined: x`,
+		`Variable might have not been defined: x`,
+	}
+	test.RunAndMatch()
+}
+
 func TestForeachEmpty(t *testing.T) {
 	test := linttest.NewSuite(t)
 	test.AddFile(`<?php

src/linttest/oop_test.go

@@ -562,9 +562,9 @@ function fn4($f4) {
 }`)
 	test.Expect = []string{
 		`Call to undefined method {\File}->name()`,
-		`Call to undefined method {\File|\Video}->filename()`,
+		`Call to undefined method {\Video}->filename()`,
 		`Call to undefined method {\File}->name()`,
-		`Call to undefined method {\File|\Video}->filename()`,
+		`Call to undefined method {\Video}->filename()`,
 	}
 	test.RunAndMatch()
 }
@@ -655,7 +655,6 @@ func TestInstanceOf(t *testing.T) {
 	runFilterMatch(test, "undefined")
 }
 
-
 func TestNullableTypes(t *testing.T) {
 	test := linttest.NewSuite(t)
 	test.AddFile(`<?php