linter: added checks for functions

src/linter/block.go

@@ -64,10 +64,14 @@ type BlockWalker struct {
 	// state
 	statements map[node.Node]struct{}
 
-	// whether a function has a return without explit expression.
+	// whether a function has a return without explicit expression.
 	// Required to make a decision in void vs null type selection,
 	// since "return" is the same as "return null".
 	bareReturn bool
+	// when the function contains at least one return or yield.
+	containsReturnOrYield bool
+	// when the function contains throw.
+	containsThrow bool
 	// whether a function has a return with explicit expression.
 	// When can't infer precise type, can use mixed.
 	returnsValue bool
@@ -458,10 +462,13 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 	case *stmt.Goto:
 		b.r.checkKeywordCase(s, "goto")
 	case *stmt.Throw:
+		b.handleThrow(s)
 		b.r.checkKeywordCase(s, "throw")
 	case *expr.Yield:
+		b.handleYield(s)
 		b.r.checkKeywordCase(s, "yield")
 	case *expr.YieldFrom:
+		b.handleYieldFrom(s)
 		b.r.checkKeywordCase(s, "yield")
 	case *expr.Include:
 		b.r.checkKeywordCase(n, "include")
@@ -491,6 +498,18 @@ func (b *BlockWalker) EnterNode(w walker.Walkable) (res bool) {
 	return res
 }
 
+func (b *BlockWalker) handleYield(yield *expr.Yield) {
+	b.containsReturnOrYield = true
+}
+
+func (b *BlockWalker) handleYieldFrom(yield *expr.YieldFrom) {
+	b.containsReturnOrYield = true
+}
+
+func (b *BlockWalker) handleThrow(throw *stmt.Throw) {
+	b.containsThrow = true
+}
+
 func (b *BlockWalker) handleInterface(int *stmt.Interface) bool {
 	for _, st := range int.Stmts {
 		switch x := st.(type) {
@@ -525,9 +544,51 @@ func (b *BlockWalker) handleFunction(fun *stmt.Function) bool {
 }
 
 func (b *BlockWalker) handleReturn(ret *stmt.Return) {
+	b.containsReturnOrYield = true
+
 	if ret.Expr == nil {
 		// Return without explicit return value.
 		b.bareReturn = true
+
+		if !meta.IsIndexingComplete() {
+			return
+		}
+
+		funcName := b.r.ctx.st.CurrentFunction
+		currentClass := b.r.ctx.st.CurrentClass
+		currentMethod := b.r.ctx.st.CurrentMethod
+
+		var isBareReturnError bool
+		if len(currentClass) != 0 {
+			funcName = currentMethod
+
+			class, ok := meta.Info.GetClass(currentClass)
+			if !ok {
+				return
+			}
+			fun, ok := class.Methods.Get(funcName)
+			if !ok {
+				return
+			}
+
+			if !fun.Typ.Is("void") {
+				isBareReturnError = true
+			}
+		} else {
+			fun, ok := meta.Info.GetFunction(funcName)
+			if !ok {
+				return
+			}
+
+			if !fun.Typ.Is("void") {
+				isBareReturnError = true
+			}
+		}
+
+		if isBareReturnError {
+			b.r.Report(ret, LevelWarning, "bareReturn", "Replace 'return' with 'return null'. The '%s' function has @return annotation with non-'void' value, so 'return null' is preferred.", funcName)
+		}
+
 		return
 	}
 	b.returnsValue = true
@@ -1634,7 +1695,7 @@ func (b *BlockWalker) enterClosure(fun *expr.Closure, haveThis bool, thisType me
 
 	params, _ := b.r.parseFuncArgs(fun.Params, phpDocParamTypes, sc)
 
-	b.r.handleFuncStmts(params, closureUses, fun.Stmts, sc)
+	b.r.handleFuncStmts(fun, params, closureUses, fun.Stmts, sc, &doc)
 	b.r.addScope(fun, sc)
 
 	return false

src/linter/report.go

@@ -455,6 +455,67 @@ function performance_test() {}`,
 			Default: true,
 			Comment: `Report linter error.`,
 		},
+
+		{
+			Name:    "bareReturn",
+			Default: true,
+			Comment: `Report when function has bare return and non-void return type.`,
+			Before: `/**
+ * @param string $x
+ * @return string|null
+ */
+function f($x) {
+    if ($x === "0") {
+        return; // Better to write as "return null"
+    }
+    if ($x === "1") {
+        return; // Better to write as "return null"
+    }
+    return "q";
+}`,
+			After: `/**
+ * @param string $x
+ * @return string|null
+ */
+function f($x) {
+    if ($x === "0") {
+        return null; // Ok
+    }
+    if ($x === "1") {
+        return null; // Ok
+    }
+    return "q";
+}`,
+		},
+
+		{
+			Name:    "mismatchingReturn",
+			Default: true,
+			Comment: `Report when function has @return annotation with non-void value, but the function does not contain return statements.`,
+			Before: `/**
+ * @param int $x
+ * @return string|null
+ */
+function f($x) {
+  echo $x;
+}`,
+			After: `/**
+ * @param int $x
+ * @return string|null
+ */
+function f($x) {
+  echo $x;
+  return $x; // Ok
+}
+// or
+/**
+ * @param int $x
+ * @return void
+ */
+function f($x) {
+  echo $x; // Ok
+}`,
+		},
 	}
 
 	for _, info := range allChecks {

src/linter/root.go

@@ -118,11 +118,22 @@ func NewWalkerForReferencesSearcher(filename string, block BlockCheckerCreateFun
 // InitFromParser initializes common fields that are needed for RootWalker work
 func (d *RootWalker) InitFromParser(contents []byte, parser *php7.Parser) {
 	lines := bytes.Split(contents, []byte("\n"))
+
+	trimedLines := make([]bool, len(lines))
+	for i := range lines {
+		trimedLines[i] = bytes.HasSuffix(lines[i], []byte("\r"))
+		lines[i] = bytes.TrimSuffix(lines[i], []byte("\r"))
+	}
+
 	linesPositions := make([]int, len(lines))
 	pos := 0
 	for idx, ln := range lines {
 		linesPositions[idx] = pos
 		pos += len(ln) + 1
+
+		if trimedLines[idx] {
+			pos += 1
+		}
 	}
 
 	d.fileContents = contents
@@ -446,10 +457,6 @@ func (d *RootWalker) report(n node.Node, lineNumber int, level int, checkName, m
 		startChar = 0
 		endChar = len(startLn)
 
-		if strings.HasSuffix(string(startLn), "\r") {
-			endChar = endChar - 1
-		}
-
 		pos = position.Position{
 			StartLine: lineNumber,
 			EndLine:   lineNumber,
@@ -529,8 +536,8 @@ func (d *RootWalker) reportHash(pos *position.Position, startLine []byte, checkN
 	// want such methods to be considered as a single "scope".
 	scope := "file"
 	switch {
-	case d.ctx.st.CurrentClass != "" && d.ctx.st.CurrentFunction != "":
-		scope = d.ctx.st.CurrentClass + "::" + d.ctx.st.CurrentFunction
+	case d.ctx.st.CurrentClass != "" && d.ctx.st.CurrentMethod != "":
+		scope = d.ctx.st.CurrentClass + "::" + d.ctx.st.CurrentMethod
 	case d.ctx.st.CurrentFunction != "":
 		scope = d.ctx.st.CurrentFunction
 	}
@@ -641,7 +648,7 @@ type handleFuncResult struct {
 	callsParentConstructor bool
 }
 
-func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []node.Node, sc *meta.Scope) handleFuncResult {
+func (d *RootWalker) handleFuncStmts(fun node.Node, params []meta.FuncParam, uses, stmts []node.Node, sc *meta.Scope, phpDoc *phpDocParseResult) handleFuncResult {
 	b := &BlockWalker{
 		ctx:          &blockContext{sc: sc},
 		r:            d,
@@ -698,7 +705,12 @@ func (d *RootWalker) handleFuncStmts(params []meta.FuncParam, uses, stmts []node
 		prematureExitFlags = cleanFlags
 	}
 
+	// if the function contains only one statement and this statement is throw.
+	oneThrowStatementFunction := b.containsThrow && len(stmts) == 1
+
 	switch {
+	case !b.containsReturnOrYield && !phpDoc.returnType.IsEmpty() && !phpDoc.returnType.Is("void") && len(stmts) != 0 && !oneThrowStatementFunction:
+		d.Report(fun, LevelWarning, "mismatchingReturn", "Mismatching @return annotations")
 	case b.bareReturn && b.returnsValue:
 		b.returnTypes = b.returnTypes.AppendString("null")
 	case b.returnTypes.IsEmpty() && b.returnsValue:
@@ -1000,7 +1012,7 @@ func (d *RootWalker) enterClassMethod(meth *stmt.ClassMethod) bool {
 	if stmtList, ok := meth.Stmt.(*stmt.StmtList); ok {
 		stmts = stmtList.Stmts
 	}
-	funcInfo := d.handleFuncStmts(params, nil, stmts, sc)
+	funcInfo := d.handleFuncStmts(meth, params, nil, stmts, sc, &doc)
 	actualReturnTypes := funcInfo.returnTypes
 	exitFlags := funcInfo.prematureExitFlags
 	if nm == `__construct` {
@@ -1454,7 +1466,7 @@ func (d *RootWalker) enterFunction(fun *stmt.Function) bool {
 
 	params, minParamsCnt := d.parseFuncArgs(fun.Params, phpDocParamTypes, sc)
 
-	funcInfo := d.handleFuncStmts(params, nil, fun.Stmts, sc)
+	funcInfo := d.handleFuncStmts(fun, params, nil, fun.Stmts, sc, &doc)
 	actualReturnTypes := funcInfo.returnTypes
 	exitFlags := funcInfo.prematureExitFlags
 	d.addScope(fun, sc)