Repository: VKCOM/noverify
Pull request #632 (2020-08-05 10:32:25)

linter: added arrow functions processing in linter
Added support for arrow functions at the linter level.

quasilyte: @YuriyNasretdinov any final notes?
Storing []BlockWalker doesn't look elegant, but at least we don't copy scopes eagerly.
Arrow functions have different scoping/enclosing rules than closures, so they required some kludges to work correctly.
If you're OK with it, we might merge it today.
quasilyte: I don't have other change suggestions right now. Let's resolve conflicts and merge this.

Patch:
diff --git a/src/ir/irconv/irconv.go b/src/ir/irconv/irconv.go
index 5864649..c101a8a 100644
--- a/src/ir/irconv/irconv.go
+++ b/src/ir/irconv/irconv.go
@@ -601,6 +601,7 @@ func (c *Converter) convNode(n node.Node) ir.Node {
 		out.ReturnsRef = n.ReturnsRef
 		out.Static = n.Static
 		out.PhpDocComment = n.PhpDocComment
+		out.PhpDoc = c.parsePHPDoc(n.PhpDocComment)
 		out.Params = c.convNodeSlice(n.Params)
 		out.ReturnType = c.convNode(n.ReturnType)
 		out.Expr = c.convNode(n.Expr)
diff --git a/src/ir/node_types.go b/src/ir/node_types.go
index 0118c0c..e3fab64 100644
--- a/src/ir/node_types.go
+++ b/src/ir/node_types.go
@@ -409,6 +409,7 @@ type ArrowFunctionExpr struct {
 	ReturnsRef    bool
 	Static        bool
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	Params        []Node
 	ReturnType    Node
 	Expr          Node
diff --git a/src/linter/block.go b/src/linter/block.go
index d591740..b8b725a 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -100,6 +100,9 @@ type BlockWalker struct {
 	// static, global and other vars that have complex control flow.
 	// Never contains varLocal elements.
 	nonLocalVars map[string]variableKind
+
+	inArrowFunction    bool
+	parentBlockWalkers []*BlockWalker // all parent block walkers if we handle nested arrow functions.
 }
 
 func newBlockWalker(r *RootWalker, sc *meta.Scope) *BlockWalker {
@@ -285,6 +288,8 @@ func (b *BlockWalker) EnterNode(n ir.Node) (res bool) {
 		res = b.handleVariable(s)
 	case *ir.FunctionStmt:
 		res = b.handleFunction(s)
+	case *ir.ArrowFunctionExpr:
+		res = b.handleArrowFunction(s)
 	case *ir.AnonClassExpr:
 		res = !b.ignoreFunctionBodies
 	case *ir.ClassStmt:
@@ -387,6 +392,14 @@ func (b *BlockWalker) handleFunction(fun *ir.FunctionStmt) bool {
 	return b.r.enterFunction(fun)
 }
 
+func (b *BlockWalker) handleArrowFunction(fun *ir.ArrowFunctionExpr) bool {
+	if b.ignoreFunctionBodies {
+		return false
+	}
+
+	return b.enterArrowFunction(fun)
+}
+
 func (b *BlockWalker) handleReturn(ret *ir.ReturnStmt) {
 	if ret.Expr == nil {
 		// Return without explicit return value.
@@ -1195,6 +1208,19 @@ func (b *BlockWalker) handleFor(s *ir.ForStmt) bool {
 	return false
 }
 
+func (b *BlockWalker) enterArrowFunction(fun *ir.ArrowFunctionExpr) bool {
+	sc := meta.NewScope()
+
+	doc := b.r.parsePHPDoc(fun, fun.PhpDoc, fun.Params)
+	b.r.reportPhpdocErrors(fun, doc.errs)
+	phpDocParamTypes := doc.types
+
+	params, _ := b.r.parseFuncArgs(fun.Params, phpDocParamTypes, sc, nil)
+	b.r.handleArrowFuncExpr(params, fun.Expr, sc, b)
+
+	return false
+}
+
 func (b *BlockWalker) enterClosure(fun *ir.ClosureExpr, haveThis bool, thisType meta.TypesMap, closureSolver *solver.ClosureCallerInfo) bool {
 	sc := meta.NewScope()
 	sc.SetInClosure(true)
@@ -1320,20 +1346,75 @@ func (b *BlockWalker) propagateFlagsFromBranches(contexts []*blockContext, links
 }
 
 func (b *BlockWalker) handleVariable(v ir.Node) bool {
+	var varName string
 	switch v := v.(type) {
 	case *ir.Var:
 		if vv, ok := v.Expr.(*ir.SimpleVar); ok {
-			b.untrackVarName(vv.Name)
+			varName = vv.Name
+			if b.inArrowFunction {
+				for _, w := range b.parentBlockWalkers {
+					w.untrackVarName(varName)
+				}
+			}
+
+			b.untrackVarName(varName)
 		}
 	case *ir.SimpleVar:
-		b.untrackVarName(v.Name)
+		varName = v.Name
+		if b.inArrowFunction {
+			for _, w := range b.parentBlockWalkers {
+				w.untrackVarName(varName)
+			}
+		}
+
+		b.untrackVarName(varName)
 	}
 
-	if !b.ctx.sc.HaveVar(v) {
+	have := b.ctx.sc.HaveVar(v)
+
+	if !have && !b.inArrowFunction {
 		b.r.reportUndefinedVariable(v, b.ctx.sc.MaybeHaveVar(v))
 		b.ctx.sc.AddVar(v, meta.NewTypesMap("undefined"), "undefined", meta.VarAlwaysDefined)
 	}
 
+	// In case the required variable was not found in the current scope,
+	// we need to look at all scopes above up to the scope in which the given
+	// arrow function is declared or, in case it is a nested arrow function,
+	// to the scope, which contains the parent arrow function.
+	// This is necessary because arrow functions implicitly capture variables.
+	if b.inArrowFunction && !have {
+		var varNotFound bool
+		var varMaybeNotDefined bool
+
+		for _, bw := range b.parentBlockWalkers {
+			s := bw.ctx.sc
+
+			if !s.HaveVar(v) {
+				if !varMaybeNotDefined {
+					varMaybeNotDefined = s.MaybeHaveVar(v)
+				}
+				varNotFound = true
+				continue
+			}
+
+			tp, _ := s.GetVarNameType(varName)
+
+			// If a variable was found in one of the scopes,
+			// we must add it to the current scope, that is, capture the variable.
+			// Thus, by changing this variable inside the arrow function,
+			// we will not change the variable that was captured,
+			// as it should be according to the specification
+			// (www.php.net/manual/en/functions.arrow.php).
+			b.ctx.sc.AddVar(v, tp, "from_parent_scope", meta.VarAlwaysDefined)
+			return false
+		}
+
+		if varNotFound {
+			b.r.reportUndefinedVariable(v, varMaybeNotDefined)
+			b.ctx.sc.AddVar(v, meta.NewTypesMap("undefined"), "undefined", meta.VarAlwaysDefined)
+		}
+	}
+
 	return false
 }
 
diff --git a/src/linter/root.go b/src/linter/root.go
index 26798d3..4293203 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -603,6 +603,28 @@ type handleFuncResult struct {
 	callsParentConstructor bool
 }
 
+func (d *RootWalker) handleArrowFuncExpr(params []meta.FuncParam, expr ir.Node, sc *meta.Scope, parentBlockWalker *BlockWalker) handleFuncResult {
+	b := newBlockWalker(d, sc)
+	b.inArrowFunction = true
+	parentBlockWalker.parentBlockWalkers = append(parentBlockWalker.parentBlockWalkers, parentBlockWalker)
+	b.parentBlockWalkers = parentBlockWalker.parentBlockWalkers
+
+	for _, p := range params {
+		if p.IsRef {
+			b.nonLocalVars[p.Name] = varRef
+		}
+	}
+
+	b.addStatement(expr)
+	expr.Walk(b)
+
+	b.flushUnused()
+
+	return handleFuncResult{
+		returnTypes: b.returnTypes,
+	}
+}
+
 func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []ir.Node, sc *meta.Scope) handleFuncResult {
 	b := newBlockWalker(d, sc)
 	for _, createFn := range d.customBlock {
@@ -1367,6 +1389,7 @@ func (d *RootWalker) parseTypeNode(n ir.Node) (typ meta.TypesMap, ok bool) {
 	return tm, !tm.IsEmpty()
 }
 
+// callbackParamByIndex returns the description of the parameter for the function by its index.
 func (d *RootWalker) callbackParamByIndex(param ir.Node, argType meta.TypesMap) meta.FuncParam {
 	p := param.(*ir.Parameter)
 	v := p.Variable


Test patch:
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index 09cc1da..635effe 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -1557,6 +1557,86 @@ func TestFunctionJustReturns(t *testing.T) {
 	}`)
 }
 
+func TestArrowFunction(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+	class Boo {
+		/** @return int */
+		public function b() { }
+	}
+
+	function foo() {
+		$value = 10;
+
+		// simple function
+		$_ = fn($x) => $x + 5;
+
+		// with capture
+		$_ = fn($x) => $x + $value;
+
+		// reference
+		$_ = fn&($x) => $x + $value;
+
+		// with undefined variable
+		$_ = fn($x) => $x + $undefined_variable;
+
+		if ($value == 0) {
+			$maybe_defined = 100;
+		}
+
+		// with maybe defined variable
+		$_ = fn($x) => $x + $maybe_defined;
+
+		// with unused variable
+		$_ = fn($x) => $a = $x + 5;
+		$_ = fn($x) => ($a = $x + 5) && $x;
+
+		$_ = fn($x) => ($a = $x + 5) && $a + 5;
+
+		// with PHPDoc
+		/**
+		 * @param Boo $x
+		 */
+		$_ = fn($x) => $x->b();
+
+		// nested
+		$_ = fn($x) => fn($y) => fn($w) => $x * $y + $w - $value;
+
+		// nested with maybe defined variable
+		$_ = fn($x) => fn($y) => fn($w) => $x * $y + $w - $maybe_defined;
+
+		// nested with unused variable
+		$_ = fn($x) => fn($y) => fn($w) => $a = $x + $y + $w;
+
+		$_ = fn($x) => fn($y) => fn($w) => ($a = $x + 5) && $a + 5;
+
+		// ok
+		$_ = fn() => ($a = 10) && $a;
+
+		// $a is undefined
+		$_ = fn() => $a = 10 && $a;
+
+		// arguments are not visible outside of arrow function
+		echo $x; // Undefined $x
+		echo $y; // Undefined $y
+		echo $w; // Undefined $w
+	}`)
+	test.Expect = []string{
+		`Undefined variable: undefined_variable`,
+		`Variable might have not been defined: maybe_defined`,
+		`Variable a is unused (use $_ to ignore this inspection)`,
+		`Variable a is unused (use $_ to ignore this inspection)`,
+		`Variable a is unused (use $_ to ignore this inspection)`,
+		`Variable a is unused (use $_ to ignore this inspection)`,
+		`Undefined variable: a`,
+		`Undefined variable: x`,
+		`Undefined variable: y`,
+		`Undefined variable: w`,
+		`Variable might have not been defined: maybe_defined`,
+	}
+	test.RunAndMatch()
+}
+
 func TestSwitchFallthrough(t *testing.T) {
 	linttest.SimpleNegativeTest(t, `<?php
 	function withFallthrough($a) {
diff --git a/src/tests/exprtype/exprtype_test.go b/src/tests/exprtype/exprtype_test.go
index 03ea45a..4393ca2 100644
--- a/src/tests/exprtype/exprtype_test.go
+++ b/src/tests/exprtype/exprtype_test.go
@@ -2240,6 +2240,18 @@ exprtype(Roo::$d, "string");
 	runExprTypeTest(t, &exprTypeTestParams{code: code})
 }
 
+func TestArrowFunction(t *testing.T) {
+	code := `<?php
+function f() {
+   $value = 10;
+   $_ = fn($x) => $value = "probably now $value has type int|string";
+   // but, no
+   exprtype($value, "precise int"); // Ok, see specification
+}
+`
+	runExprTypeTest(t, &exprTypeTestParams{code: code})
+}
+
 func TestClosureCallbackArgumentsTypes(t *testing.T) {
 	code := `<?php
 function usort($array, $callback) {}