From 7f8e03f5226fb470511cf5104921a84f166691bf Mon Sep 17 00:00:00 2001 From: Iskander Sharipov Date: Sat, 21 Mar 2020 01:40:52 +0300 Subject: [PATCH] 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 --- src/linter/assign_walker.go | 28 ++++++++ src/linter/block.go | 70 ++++++++----------- src/linttest/basic_test.go | 63 +++++++++++++++++ src/linttest/oop_test.go | 5 +- src/linttest/regression_test.go | 120 ++++++++++++++++++++++++++++++++ src/meta/scope.go | 42 ++++++++--- 6 files changed, 274 insertions(+), 54 deletions(-) create mode 100644 src/linter/assign_walker.go diff --git a/src/linter/assign_walker.go b/src/linter/assign_walker.go new file mode 100644 index 000000000..fd834b3dd --- /dev/null +++ b/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) {} diff --git a/src/linter/block.go b/src/linter/block.go index 9cbb30b37..5eff73ba7 100644 --- a/src/linter/block.go +++ b/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,16 +1493,14 @@ 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) } } @@ -1515,14 +1508,13 @@ func (a *andWalker) EnterNode(w walker.Walkable) (res bool) { 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 } diff --git a/src/linttest/basic_test.go b/src/linttest/basic_test.go index ea652c88b..98f6ba756 100644 --- a/src/linttest/basic_test.go +++ b/src/linttest/basic_test.go @@ -10,6 +10,69 @@ import ( "github.com/VKCOM/noverify/src/meta" ) +func TestIfCondAssign(t *testing.T) { + linttest.SimpleNegativeTest(t, `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(`example(); // Bad: inside else branch it's known *not* to be C + } + + if ($c instanceof C) { + $c->example(); // OK + $c->example2(); // Not OK + } else if ($c instanceof C2) { + $c->example(); // OK + $c->example2(); // OK + } else { + $c->example2(); + } + + $c->example(); // Bad: using $c outside of if statements +} +`) + test.Expect = []string{ + `Call to undefined method {mixed}->example()`, + `Call to undefined method {\C}->example2()`, + `Call to undefined method {mixed}->example2()`, + `Call to undefined method {mixed}->example()`, + } + test.RunAndMatch() +} + func TestIssue170(t *testing.T) { linttest.SimpleNegativeTest(t, `a; +} elseif ($globf instanceof Fooo2) { + echo $globf->a; // Bad: $globf is not Fooo1 + echo $globf->b; +} else { + echo $globf->a; // Bad: $globf type is unknown +} +`) + test.Expect = []string{ + `Property {\Fooo2}->a does not exist`, + `Property {mixed}->a does not exist`, + } + test.RunAndMatch() +} diff --git a/src/meta/scope.go b/src/meta/scope.go index 5f17da878..97123868b 100644 --- a/src/meta/scope.go +++ b/src/meta/scope.go @@ -15,6 +15,12 @@ type scopeVar struct { typesMap TypesMap alwaysDefined bool noReplace bool // do not replace variable upon assignment (used for phpdoc @var declaration) + + // whether a variable is implicitly defined (by isset or instanceof). + // Implicit vars are not tracked and they are not required to be "used". + // They're also more scoped than normal variables and rarely outlive + // the block context in which they were introduced. + implicit bool } // Scope contains variables with their types in the respective scope @@ -117,6 +123,14 @@ func (s *Scope) Iterate(cb func(varName string, typ TypesMap, alwaysDefined bool } } +func (s *Scope) IterateExplicit(cb func(varName string, typ TypesMap, alwaysDefined bool)) { + for varName, v := range s.vars { + if !v.implicit { + cb(varName, v.typesMap, v.alwaysDefined) + } + } +} + func (s *Scope) Len() int { return len(s.vars) } @@ -130,6 +144,16 @@ func (s *Scope) AddVar(v node.Node, typ TypesMap, reason string, alwaysDefined b s.AddVarName(name, typ, reason, alwaysDefined) } +// AddImplicitVar adds implicit variable with specified types to scope +func (s *Scope) AddImplicitVar(varNode node.Node, typ TypesMap, reason string, alwaysDefined bool) { + name, ok := scopeVarName(varNode) + if !ok { + return + } + v := s.addVarName(name, typ, reason, alwaysDefined) + v.implicit = true +} + // ReplaceVar replaces variable with specified types to scope func (s *Scope) ReplaceVar(v node.Node, typ TypesMap, reason string, alwaysDefined bool) { name, ok := scopeVarName(v) @@ -173,38 +197,36 @@ func (s *Scope) ReplaceVarName(name string, typ TypesMap, reason string, alwaysD } // AddVarName adds variable with specified types to the scope -func (s *Scope) addVarName(name string, typ TypesMap, reason string, alwaysDefined, noReplace bool) { +func (s *Scope) addVarName(name string, typ TypesMap, reason string, alwaysDefined bool) *scopeVar { v, ok := s.vars[name] if !ok { - s.vars[name] = &scopeVar{ + v := &scopeVar{ typesMap: typ, alwaysDefined: alwaysDefined, - noReplace: noReplace, } - return + s.vars[name] = v + return v } if !v.alwaysDefined && alwaysDefined { v.alwaysDefined = true } - if !v.noReplace && noReplace { - v.noReplace = true - } - v.typesMap = v.typesMap.Append(typ) s.vars[name] = v + return v } // AddVarName adds variable with specified types to the scope func (s *Scope) AddVarName(name string, typ TypesMap, reason string, alwaysDefined bool) { - s.addVarName(name, typ, reason, alwaysDefined, false) + s.addVarName(name, typ, reason, alwaysDefined) } // AddVarFromPHPDoc adds variable with specified types to the scope func (s *Scope) AddVarFromPHPDoc(name string, typ TypesMap, reason string) { - s.addVarName(name, typ, reason, true, true) + v := s.addVarName(name, typ, reason, true) + v.noReplace = true } // HaveVar checks whether or not specified variable is present in the scope and that it is always defined