Repository: VKCOM/noverify
Pull request #622 (2020-07-31 08:33:39)

linter: add dupGlobal checker
Finds global statements over repeated variables.

Using the same name inside one statement emits a warning-level report.
Redundant global between statements produces maybe-level report.

We only track unconditional global statements for inter-statement analysis.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/linter/block.go b/src/linter/block.go
index 21f8e89..1c4dbec 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -47,6 +47,16 @@ const (
 	FlagDie
 )
 
+type variableKind int
+
+const (
+	varLocal variableKind = iota
+	varRef
+	varGlobal
+	varCondGlobal
+	varStatic
+)
+
 // BlockWalker is used to process function/method contents.
 type BlockWalker struct {
 	ctx *blockContext
@@ -80,8 +90,11 @@ type BlockWalker struct {
 	callsParentConstructor bool
 
 	// shared state between all blocks
-	unusedVars   map[string][]node.Node
-	nonLocalVars map[string]struct{} // static, global and other vars that have complex control flow
+	unusedVars map[string][]node.Node
+
+	// static, global and other vars that have complex control flow.
+	// Never contains varLocal elements.
+	nonLocalVars map[string]variableKind
 }
 
 func (b *BlockWalker) addStatement(n node.Node) {
@@ -325,18 +338,7 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 	case *cast.Array:
 		b.checkRedundantCastArray(s.Expr)
 	case *stmt.Global:
-		b.r.checkKeywordCase(s, "global")
-		for _, v := range s.Vars {
-			nm := varToString(v)
-			if nm == "" {
-				continue
-			}
-			if _, ok := superGlobals[nm]; ok {
-				b.r.Report(v, LevelWarning, "redundantGlobal", "%s is superglobal", nm)
-			}
-			b.addVar(v, meta.NewTypesMap(meta.WrapGlobal(nm)), "global", meta.VarAlwaysDefined)
-			b.addNonLocalVar(v)
-		}
+		b.checkGlobalStmt(s)
 		res = false
 	case *stmt.Static:
 		for _, vv := range s.Vars {
@@ -347,7 +349,7 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 			// the previously assigned value.
 			typ.MarkAsImprecise()
 			b.addVarName(v, ev.Name, typ, "static", meta.VarAlwaysDefined)
-			b.addNonLocalVarName(ev.Name)
+			b.addNonLocalVarName(ev.Name, varStatic)
 			if v.Expr != nil {
 				v.Expr.Walk(b)
 			}
@@ -497,6 +499,52 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 	return res
 }
 
+func (b *BlockWalker) checkDupGlobal(s *stmt.Global) {
+	vars := make(map[string]struct{}, len(s.Vars))
+	for _, v := range s.Vars {
+		v, ok := v.(*node.SimpleVar)
+		if !ok {
+			continue
+		}
+		nm := v.Name
+
+		// Check whether this var was already global'ed.
+		// We use nonLocalVars for function-wide analysis and vars for local analysis.
+		if _, ok := vars[nm]; ok {
+			b.r.Report(v, LevelWarning, "dupGlobal", "global statement mentions $%s more than once", nm)
+		} else {
+			vars[nm] = struct{}{}
+			if b.nonLocalVars[nm] == varGlobal {
+				b.r.Report(v, LevelDoNotReject, "dupGlobal", "$%s already global'ed above", nm)
+			}
+		}
+	}
+}
+
+func (b *BlockWalker) checkGlobalStmt(s *stmt.Global) {
+	if !b.rootLevel {
+		b.checkDupGlobal(s)
+	}
+
+	b.r.checkKeywordCase(s, "global")
+	for _, v := range s.Vars {
+		nm := varToString(v)
+		if nm == "" {
+			continue
+		}
+		if _, ok := superGlobals[nm]; ok {
+			b.r.Report(v, LevelWarning, "redundantGlobal", "%s is superglobal", nm)
+		}
+
+		b.addVar(v, meta.NewTypesMap(meta.WrapGlobal(nm)), "global", meta.VarAlwaysDefined)
+		if b.path.Conditional() {
+			b.addNonLocalVar(v, varCondGlobal)
+		} else {
+			b.addNonLocalVar(v, varGlobal)
+		}
+	}
+}
+
 func (b *BlockWalker) handleInterface(int *stmt.Interface) bool {
 	for _, st := range int.Stmts {
 		switch x := st.(type) {
@@ -559,16 +607,16 @@ func (b *BlockWalker) handleContinue(s *stmt.Continue) {
 	}
 }
 
-func (b *BlockWalker) addNonLocalVarName(nm string) {
-	b.nonLocalVars[nm] = struct{}{}
+func (b *BlockWalker) addNonLocalVarName(nm string, kind variableKind) {
+	b.nonLocalVars[nm] = kind
 }
 
-func (b *BlockWalker) addNonLocalVar(v node.Node) {
+func (b *BlockWalker) addNonLocalVar(v node.Node, kind variableKind) {
 	sv, ok := v.(*node.SimpleVar)
 	if !ok {
 		return
 	}
-	b.addNonLocalVarName(sv.Name)
+	b.addNonLocalVarName(sv.Name, kind)
 }
 
 // replaceVar must be used to track assignments to conrete var nodes if they are available
@@ -909,7 +957,7 @@ func (b *BlockWalker) handleCallArgs(n node.Node, args []node.Node, fn meta.Func
 		switch a := arg.(*node.Argument).Expr.(type) {
 		case *node.Var, *node.SimpleVar:
 			if ref {
-				b.addNonLocalVar(a)
+				b.addNonLocalVar(a, varRef)
 				// TODO: variable may actually not be set by ref
 				b.addVar(a, fn.Params[i].Typ, "call_with_ref", meta.VarAlwaysDefined)
 				break
@@ -2188,7 +2236,7 @@ func (b *BlockWalker) handleAssignReference(a *assign.Reference) bool {
 		return false
 	case *node.Var, *node.SimpleVar:
 		b.addVar(v, solver.ExprTypeLocal(b.ctx.sc, b.r.ctx.st, a.Expression), "assign", meta.VarAlwaysDefined)
-		b.addNonLocalVar(v)
+		b.addNonLocalVar(v, varRef)
 	case *expr.List:
 		// TODO: figure out whether this case is reachable.
 		for _, item := range v.Items {
diff --git a/src/linter/parser.go b/src/linter/parser.go
index 65957e9..b6210a4 100644
--- a/src/linter/parser.go
+++ b/src/linter/parser.go
@@ -207,7 +207,7 @@ func AnalyzeFileRootLevel(rootNode node.Node, d *RootWalker) {
 		ctx:                  &blockContext{sc: sc},
 		r:                    d,
 		unusedVars:           make(map[string][]node.Node),
-		nonLocalVars:         make(map[string]struct{}),
+		nonLocalVars:         make(map[string]variableKind),
 		ignoreFunctionBodies: true,
 		rootLevel:            true,
 		path:                 newNodePath(),
diff --git a/src/linter/report.go b/src/linter/report.go
index 3f9af86..f933216 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -100,6 +100,14 @@ func init() {
 			After:   `[0 => 'a', 5 => 'b']`,
 		},
 
+		{
+			Name:    "dupGlobal",
+			Default: true,
+			Comment: `Report repeated global statements over variables.`,
+			Before:  `global $x, $y, $x;`,
+			After:   `global $x, $y;`,
+		},
+
 		{
 			Name:    "dupArrayKeys",
 			Default: true,
diff --git a/src/linter/root.go b/src/linter/root.go
index d574751..95039df 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -646,7 +646,7 @@ func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []node
 		ctx:          &blockContext{sc: sc},
 		r:            d,
 		unusedVars:   make(map[string][]node.Node),
-		nonLocalVars: make(map[string]struct{}),
+		nonLocalVars: make(map[string]variableKind),
 		path:         newNodePath(),
 	}
 	for _, createFn := range d.customBlock {
@@ -674,13 +674,13 @@ func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []node
 		if !byRef {
 			b.unusedVars[v.Name] = append(b.unusedVars[v.Name], v)
 		} else {
-			b.nonLocalVars[v.Name] = struct{}{}
+			b.nonLocalVars[v.Name] = varRef
 		}
 	}
 
 	for _, p := range params {
 		if p.IsRef {
-			b.nonLocalVars[p.Name] = struct{}{}
+			b.nonLocalVars[p.Name] = varRef
 		}
 	}
 	for _, s := range stmts {


Test patch:
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index 11d139f..d2eb5c3 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -12,6 +12,72 @@ import (
 	"github.com/VKCOM/noverify/src/meta"
 )
 
+func TestDupGlobalRoot(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+global $x;
+global $x;
+`)
+}
+
+func TestDupGlobalCond(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function both_conditional($cond1, $cond2) {
+  if ($cond1) {
+    global $x3;
+    return $x3;
+  }
+  if ($cond2) {
+    global $x3;
+    return $x3;
+  }
+  return 0;
+}
+`)
+}
+
+func TestDupGlobalSameStatement(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f1($cond) {
+  if ($cond) {
+    // global is conditional, but contains local duplicates.
+    global $x, $x;
+  }
+}
+`)
+	test.Expect = []string{`global statement mentions $x more than once`}
+	test.RunAndMatch()
+}
+
+func TestDupGlobal(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f1() {
+  global $x1, $x1;
+  return $x1;
+}
+
+function f2() {
+  global $x2;
+  global $x2;
+  return $x2;
+}
+
+function f3() {
+  global $x3;
+  global $x4;
+  global $x3;
+  return $x3 + $x4;
+}
+`)
+	test.Expect = []string{
+		`global statement mentions $x1 more than once`,
+		`$x2 already global'ed above`,
+		`$x3 already global'ed above`,
+	}
+	test.RunAndMatch()
+}
+
 func TestStrictCmp(t *testing.T) {
 	test := linttest.NewSuite(t)
 	test.LoadStubs = []string{`stubs/phpstorm-stubs/Core/Core_d.php`}