Repository: VKCOM/noverify
Pull request #851 (2020-12-14 13:06:25)

linter: added a new way of return type inference for functions and methods
The types are now inferred as follows:
1. If there is an `@param` annotation, then its value becomes
   the return type;

2. If there is a type hint, then it is added to the types
   from the `@param`. If the `@param` is empty, then the type
   matches the type hint itself;

3. If there is no `@param` annotation and type hint, then
   the return type is equal to the union of the types that
   are returned from the function by return.

Part of #849 

The next step, adding type checking.

quasilyte: 1. Please increase the cache version as we changed the way how function results are inferred.
2. Add something in a cache_test so we get a different result for the type inference before and after this change.

I believe the cache_test is not failing because we don't have a function there that would be inferred differently.

Patch:
diff --git a/src/linter/cache.go b/src/linter/cache.go
index 186f13f..4ad192d 100644
--- a/src/linter/cache.go
+++ b/src/linter/cache.go
@@ -41,7 +41,8 @@ import (
 //     43 - define'd const values stored in cache
 //     44 - rename ConstantInfo => ConstInfo
 //     45 - added Mixins field to meta.ClassInfo
-const cacheVersion = 45
+//     46 - changed the way of inferring the return type of functions and methods
+const cacheVersion = 46
 
 var (
 	errWrongVersion = errors.New("Wrong cache version")
diff --git a/src/linter/root.go b/src/linter/root.go
index ede7905..430118d 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -1015,9 +1015,9 @@ func (d *RootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 		sc.SetInInstanceMethod(true)
 	}
 
-	var specifiedReturnType meta.TypesMap
+	var hintReturnType meta.TypesMap
 	if typ, ok := d.parseTypeNode(meth.ReturnType); ok {
-		specifiedReturnType = typ
+		hintReturnType = typ
 	}
 
 	if meth.PhpDocComment == "" && modif.accessLevel == meta.Public {
@@ -1086,11 +1086,9 @@ func (d *RootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 
 	d.addScope(meth, sc)
 
+	returnTypes := functionReturnType(phpdocReturnType, hintReturnType, actualReturnTypes)
+
 	// TODO: handle duplicate method
-	returnType := meta.MergeTypeMaps(phpdocReturnType, actualReturnTypes, specifiedReturnType)
-	if returnType.IsEmpty() {
-		returnType = meta.VoidType
-	}
 	var funcFlags meta.FuncFlags
 	if modif.static {
 		funcFlags |= meta.FuncStatic
@@ -1108,7 +1106,7 @@ func (d *RootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 		Params:       params,
 		Name:         nm,
 		Pos:          d.getElementPos(meth),
-		Typ:          returnType.Immutable(),
+		Typ:          returnTypes.Immutable(),
 		MinParamsCnt: minParamsCnt,
 		AccessLevel:  modif.accessLevel,
 		Flags:        funcFlags,
@@ -1117,7 +1115,7 @@ func (d *RootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 	})
 
 	if nm == "getIterator" && meta.IsIndexingComplete() && solver.Implements(d.ctx.st.CurrentClass, `\IteratorAggregate`) {
-		implementsTraversable := returnType.Find(func(typ string) bool {
+		implementsTraversable := returnTypes.Find(func(typ string) bool {
 			return solver.Implements(typ, `\Traversable`)
 		})
 
@@ -1584,9 +1582,9 @@ func (d *RootWalker) enterFunction(fun *ir.FunctionStmt) bool {
 		d.Report(fun.FunctionName, LevelDoNotReject, "complexity", "Too big function: more than %d lines", maxFunctionLines)
 	}
 
-	var specifiedReturnType meta.TypesMap
+	var hintReturnType meta.TypesMap
 	if typ, ok := d.parseTypeNode(fun.ReturnType); ok {
-		specifiedReturnType = typ
+		hintReturnType = typ
 	}
 
 	d.checkCommentMisspellings(fun.FunctionName, fun.PhpDocComment)
@@ -1612,10 +1610,7 @@ func (d *RootWalker) enterFunction(fun *ir.FunctionStmt) bool {
 	exitFlags := funcInfo.prematureExitFlags
 	d.addScope(fun, sc)
 
-	returnType := meta.MergeTypeMaps(phpdocReturnType, actualReturnTypes, specifiedReturnType)
-	if returnType.IsEmpty() {
-		returnType = meta.VoidType
-	}
+	returnTypes := functionReturnType(phpdocReturnType, hintReturnType, actualReturnTypes)
 
 	for _, param := range fun.Params {
 		d.checkFuncParam(param.(*ir.Parameter))
@@ -1629,7 +1624,7 @@ func (d *RootWalker) enterFunction(fun *ir.FunctionStmt) bool {
 		Params:       params,
 		Name:         nm,
 		Pos:          d.getElementPos(fun),
-		Typ:          returnType.Immutable(),
+		Typ:          returnTypes.Immutable(),
 		MinParamsCnt: minParamsCnt,
 		Flags:        funcFlags,
 		ExitFlags:    exitFlags,
diff --git a/src/linter/utils.go b/src/linter/utils.go
index 867505c..fc7572c 100644
--- a/src/linter/utils.go
+++ b/src/linter/utils.go
@@ -90,6 +90,62 @@ func typesMapToTypeExpr(p *phpdoc.TypeParser, m meta.TypesMap) phpdoc.Type {
 	return p.Parse(typeString)
 }
 
+// MergeTypeMaps merges two typesmaps without losing information.
+// So merging int[] and array will give int[], and Foo and object will give Foo.
+func MergeTypeMaps(left meta.TypesMap, right meta.TypesMap) meta.TypesMap {
+	var hasAtLeastOneArray bool
+	var hasAtLeastOneClass bool
+	newMap := meta.NewEmptyTypesMap(left.Len() + right.Len())
+
+	left.Iterate(func(typ string) {
+		if typ[0] == meta.WArrayOf {
+			hasAtLeastOneArray = true
+		}
+		if typ[0] == '\\' {
+			hasAtLeastOneClass = true
+		}
+		newMap.AppendString(typ)
+	})
+
+	right.Iterate(func(typ string) {
+		if typ[0] == meta.WArrayOf && meta.UnwrapArrayOf(typ) == "mixed" && hasAtLeastOneArray {
+			return
+		}
+		if typ == "object" && hasAtLeastOneClass {
+			return
+		}
+		newMap.AppendString(typ)
+	})
+
+	return newMap
+}
+
+// functionReturnType returns the return type of a function over computed types
+// according to the convention below:
+//
+// The types are inferred as follows:
+// 1. If there is an @return annotation, then its value becomes the return type;
+//
+// 2. If there is a type hint, then it is added to the types from the @return.
+//    If the @return is empty, then the type matches the type hint itself;
+//
+// 3. If there is no @return annotation and type hint, then the return type is equal to
+//    the union of the types that are returned from the function by return.
+func functionReturnType(phpdocReturnType meta.TypesMap, hintReturnType meta.TypesMap, actualReturnTypes meta.TypesMap) meta.TypesMap {
+	var returnTypes meta.TypesMap
+	if !phpdocReturnType.IsEmpty() || !hintReturnType.IsEmpty() {
+		returnTypes = MergeTypeMaps(phpdocReturnType, hintReturnType)
+	} else {
+		returnTypes = actualReturnTypes
+	}
+
+	if returnTypes.IsEmpty() {
+		returnTypes = meta.VoidType
+	}
+
+	return returnTypes
+}
+
 type funcCallInfo struct {
 	funcName   string
 	info       meta.FuncInfo


Test patch:
diff --git a/src/linter/cache_test.go b/src/linter/cache_test.go
index 7f8c46d..6561bc7 100644
--- a/src/linter/cache_test.go
+++ b/src/linter/cache_test.go
@@ -101,6 +101,18 @@ function as_shape($x) {
   return $x;
 }
 
+/** @param int|null */
+function return_null() {
+  return null;
+}
+
+class ByNull {
+  /** @param int|null */
+  public function return_null() {
+    return null;
+  }
+}
+
 main();
 `
 
@@ -126,7 +138,7 @@ main();
 		//
 		// If cache encoding changes, there is a very high chance that
 		// encoded data lengh will change as well.
-		wantLen := 5030
+		wantLen := 5332
 		haveLen := buf.Len()
 		if haveLen != wantLen {
 			t.Errorf("cache len mismatch:\nhave: %d\nwant: %d", haveLen, wantLen)
@@ -135,7 +147,7 @@ main();
 		// 2. Check cache "strings" hash.
 		//
 		// It catches new fields in cached types, field renames and encoding of additional named attributes.
-		wantStrings := "62321eae8ac6f753221d367a0a0e2a3190202ef49c608ceef6e03dd6b09906c64bf20a1470abef9e26b917836a68415a5459491208fb692d8773524cdbe6d239"
+		wantStrings := "cfbe2074534e9b4116dc9e4a629e5fcfba2344c363314643dce1f90156c9b20006a006cb4e7d724d9d5fca6c6034d078d95127a9cbb002f79fb68debc2a59def"
 		haveStrings := collectCacheStrings(buf.String())
 		if haveStrings != wantStrings {
 			t.Errorf("cache strings mismatch:\nhave: %q\nwant: %q", haveStrings, wantStrings)
diff --git a/src/tests/exprtype/exprtype_test.go b/src/tests/exprtype/exprtype_test.go
index e36b042..0ef2fb0 100644
--- a/src/tests/exprtype/exprtype_test.go
+++ b/src/tests/exprtype/exprtype_test.go
@@ -1694,11 +1694,11 @@ exprtype(get_array(), 'mixed[]');
 
 /** @return array */
 function get_array_or_null() { return null; }
-exprtype(get_array_or_null(), 'mixed[]|null');
+exprtype(get_array_or_null(), 'mixed[]');
 
 /** @return null */
 function get_null_or_array() { return []; }
-exprtype(get_null_or_array(), 'mixed[]|null');
+exprtype(get_null_or_array(), 'null');
 `
 	runExprTypeTest(t, &exprTypeTestParams{code: code})
 }
@@ -2707,6 +2707,74 @@ function f1() {
 	runExprTypeTest(t, &exprTypeTestParams{code: code, stubs: "<?php /* no code */"})
 }
 
+func TestNewFunctionReturnExprType(t *testing.T) {
+	code := `<?php
+class Foo {}
+
+/** @return int */
+function f1() {
+	return 5;
+}
+exprtype(f1(), "int");
+
+
+/** @return int */
+function f2(): float {
+	return 5;
+}
+exprtype(f2(), "float|int");
+
+
+/** Without @return */
+function f3(): float {
+	if (1) {
+		return new Foo;
+	}
+	return 5;
+}
+exprtype(f3(), "float");
+
+
+/** Without @return and type hint */
+function f4() {
+	if (1) {
+		return 10;
+	}
+	return new Foo;
+}
+exprtype(f4(), "\Foo|int");
+
+
+/** Without @return and type hint and return */
+function f5() {
+	
+}
+exprtype(f5(), "void");
+
+
+/** @return int[] */
+function f6(): array {
+	return [1,2,4];
+}
+exprtype(f6(), "int[]");
+
+
+/** @return int[] */
+function f7(): array {
+	return f6();
+}
+exprtype(f7(), "int[]");
+
+
+/** @return Foo */
+function f8(): object {
+	return new Foo;
+}
+exprtype(f8(), "\Foo");
+`
+	runExprTypeTest(t, &exprTypeTestParams{code: code})
+}
+
 func runExprTypeTest(t *testing.T, params *exprTypeTestParams) {
 	meta.ResetInfo()
 	if params.stubs != "" {
diff --git a/src/tests/exprtype/kphp_test.go b/src/tests/exprtype/kphp_test.go
index bd6f705..a4b8cef 100644
--- a/src/tests/exprtype/kphp_test.go
+++ b/src/tests/exprtype/kphp_test.go
@@ -18,8 +18,8 @@ function get_any_arr() {
   return [[1]];
 }
 
-exprtype(get_any(), 'int|mixed');
-exprtype(get_any_arr(), 'int[][]|mixed[][]');
+exprtype(get_any(), 'mixed');
+exprtype(get_any_arr(), 'mixed[][]');
 `
 	runKPHPExprTypeTest(t, &exprTypeTestParams{code: code})
 }