Repository: VKCOM/noverify
Pull request #756 (2020-09-03 07:59:13)

rules: improve type matching rules
Instead of adding a new type filter, make the default type
filter work in a more predictable way.

We can handle both (1) and (2) cases described in the #753
with the proposed approach.

Refs #753

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/rules/typematch.go b/src/rules/typematch.go
index 3578fc3..8fdf847 100644
--- a/src/rules/typematch.go
+++ b/src/rules/typematch.go
@@ -15,6 +15,15 @@ func TypeIsCompatible(dst, val phpdoc.TypeExpr) bool {
 		val = val.Args[0]
 	}
 
+	if val.Value == "mixed" {
+		// Mixed type usually mean the we failed to infer the expression type.
+		// We conservatively assume that mixed could be anything,
+		// including something that is assignable to dst.
+		// We may want to add a strict version of the type filter
+		// that chooses a different stratage here.
+		return false
+	}
+
 	switch dst.Kind {
 	case phpdoc.ExprParen:
 		return TypeIsCompatible(dst.Args[0], val)
@@ -34,6 +43,12 @@ func TypeIsCompatible(dst, val phpdoc.TypeExpr) bool {
 		return !TypeIsCompatible(dst.Args[0], val)
 
 	case phpdoc.ExprNullable:
+		if val.Value == "null" {
+			return true
+		}
+		if TypeIsCompatible(dst.Args[0], val) {
+			return true
+		}
 		return val.Kind == dst.Kind && TypeIsCompatible(dst.Args[0], val.Args[0])
 
 	case phpdoc.ExprArray:


Test patch:
diff --git a/src/rules/typematch_test.go b/src/rules/typematch_test.go
index 4fbff82..b5a5e47 100644
--- a/src/rules/typematch_test.go
+++ b/src/rules/typematch_test.go
@@ -6,99 +6,113 @@ import (
 	"github.com/VKCOM/noverify/src/phpdoc"
 )
 
-func TestTypeFilter(t *testing.T) {
-	type testCase struct {
-		dst string
-		val string
+func TestTypeIsCompatible(t *testing.T) {
+	tests := []struct {
+		dst    string
+		val    string
+		result bool
+	}{
+		// Identical types are always compatible.
+		{`int`, `int`, true},
+		{`string`, `string`, true},
+		{`int`, `string`, false},
+
+		// Parens do not change the semantics (but they do affect parsing).
+		{`(int)`, `int`, true},
+		{`((int))`, `int`, true},
+		{`int`, `(int)`, true},
+		{`((int))`, `string`, false},
+
+		// "object" special type is compatible with any kind of object.
+		{`object`, `object`, true},
+		{`object`, `\Foo`, true},
+		{`object`, `\Bar`, true},
+		{`object`, `string`, false},
+
+		// "array" special type is compatible with any kind of array.
+		{`array`, `mixed[]`, true},
+		{`array`, `int[]`, true},
+		{`array`, `string`, false},
+
+		// Nullable types ?T are compatible with T and null.
+		{`?int`, `?int`, true},
+		{`?int`, `int`, true},
+		{`?int`, `null`, true},
+		{`?(int|float)`, `int`, true},
+		{`?(int|float)`, `float`, true},
+		{`?(int|float)`, `null`, true},
+		{`?(int|float)`, `?(int|float)`, true},
+		{`?(int|float)`, `?(float|int)`, true},
+		{`?(int|float)`, `string`, false},
+
+		// For union types all alternatives are compatible.
+		{`int|string`, `int`, true},
+		{`string|int`, `int`, true},
+		{`int|string`, `string`, true},
+		{`string|int`, `string`, true},
+		{`string|int`, `float`, false},
+		{`\A|\B|\C`, `\A`, true},
+		{`\A|\B|\C`, `\B`, true},
+		{`\A|\B|\C`, `\C`, true},
+		{`\A|\B|\C`, `\D`, false},
+
+		// Union types can be matched by the identical union types.
+		// The order of the alternatives doesn't matter.
+		{`\A|\B|\C`, `\A|\B|\C`, true},
+		{`\A|\C|\B`, `\A|\B|\C`, true},
+		{`\C|\B|\A`, `\A|\B|\C`, true},
+
+		// When a single type is asserted against a union, it always fails.
+		{`int`, `int|null`, false},
+		{`string`, `int|null`, false},
+
+		// When dst and val are unions, we assume val compatible
+		// if all of its variants are compatible.
+		{`int|float`, `int|float`, true},
+		{`int|float`, `float|int`, true},
+		{`object|array`, `\Foo|int[]`, true},
+		{`object|array`, `int[]|\Foo`, true},
+		{`object|array`, `object|float`, false},
+		{`object|array`, `float|object`, false},
+		{`object|array`, `int|int[]`, false},
+		{`int|float`, `string|\A|int`, false},
+		{`int|float`, `string|float|\B`, false},
+		{`object|array`, `string|float`, false},
+
+		// Type negation inverts the function result.
+		{`!int`, `int`, false},
+		{`!int`, `string`, true},
+		{`!(int|float)`, `int`, false},
+		{`!(int|float)`, `float`, false},
+		{`!(int|float)`, `int|float`, false},
+		{`!(int|float)`, `(float|int)`, false},
+		{`!(int|float)`, `string`, true},
+
+		// Double negation works, but no one should ever use that.
+		{`!!int`, `int`, true},
+
+		// Mixed val type is assumed to be incompatible with everything.
+		// The negation doesn't change that fact.
+		{`object`, `mixed`, false},
+		{`!object`, `mixed`, false},
+
+		// This is a little compicated case.
+		// We resolve it to false for now.
+		{`int|null`, `?int`, false},
+
+		// TODO:
+		// {`?int`, `int|null`, true},
 	}
 
-	matchingTests := []testCase{
-		{`array`, `mixed[]`},
-		{`array`, `int[]`},
-		{`array`, `\Foo[]`},
-
-		{`object`, `object`},
-		{`object`, `\Foo`},
-		{`object`, `\Foo\Bar`},
-
-		{`!int`, `string`},
-		{`!int`, `mixed`},
-		{`!array`, `int`},
-		{`!array`, `string`},
-
-		{`int[]`, `int[]`},
-
-		{`int`, `(int)`},
-		{`(int)`, `int`},
-		{`(int)`, `((int))`},
-
-		{`int|float`, `int`},
-		{`int|float`, `float`},
-		{`float|int`, `int`},
-		{`int|float`, `float`},
-
-		{`!(int|float)`, `string`},
-		{`!(int|float)`, `int[]`},
-		{`!(object|array)`, `int`},
-		{`!(object|array)`, `string`},
-
-		{`a|b`, `a|b`},
-		{`object|array`, `\Foo|int[]`},
-		{`object|array`, `object|float[]`},
-
-		{`a|b|c`, `a|b|c`},
-		{`a|c|b`, `a|b|c`},
-		{`c|b|a`, `a|b|c`},
-	}
-
-	nonMatchingTests := []testCase{
-		{`array`, `int`},
-		{`array`, `mixed`},
-		{`array`, `\Foo`},
-
-		{`object`, `int`},
-		{`object`, `\Foo[]`},
-		{`object`, `mixed`},
-
-		{`!int`, `int`},
-		{`!array`, `mixed[]`},
-		{`!array`, `int[]`},
-
-		{`int[]`, `float[]`},
-		{`int[]`, `mixed[]`},
-
-		{`int|float`, `string`},
-		{`int|float`, `\Foo`},
-		{`float|int`, `int[]`},
-		{`int|float`, `float[]`},
-		{`int|float`, `mixed`},
-
-		{`!(int|float)`, `int`},
-		{`!(int|float)`, `float`},
-		{`!(object|array)`, `object`},
-		{`!(object|array)`, `int[]`},
-		{`!(object|array)`, `\Foo`},
-		{`!(object|array)`, `\Foo[]`},
-
-		{`object|array`, `int|int[]`},
-		{`object|array`, `object|float`},
-		{`object|array`, `string|float`},
-	}
-
-	runTests := func(want bool, tests []testCase) {
-		p := phpdoc.NewTypeParser()
-		for _, test := range tests {
-			val := p.Parse(test.val).Clone()
-			dst := p.Parse(test.dst).Clone()
-
-			have := TypeIsCompatible(dst.Expr, val.Expr)
-			if have != want {
-				t.Errorf("incorrect result: compatible(%s, %s) => %v",
-					test.dst, test.val, have)
-			}
+	p := phpdoc.NewTypeParser()
+	for _, test := range tests {
+		dstType := p.Parse(test.dst).Clone()
+		valType := p.Parse(test.val).Clone()
+		have := TypeIsCompatible(dstType.Expr, valType.Expr)
+		want := test.result
+		if have != want {
+			t.Errorf("incorrect result: compatible(%s, %s) => %v",
+				test.dst, test.val, have)
 		}
 	}
-
-	runTests(true, matchingTests)
-	runTests(false, nonMatchingTests)
 }
diff --git a/src/tests/golden/testdata/embeddedrules/ternarySimplify.php b/src/tests/golden/testdata/embeddedrules/ternarySimplify.php
index 331f16e..fb50bd5 100644
--- a/src/tests/golden/testdata/embeddedrules/ternarySimplify.php
+++ b/src/tests/golden/testdata/embeddedrules/ternarySimplify.php
@@ -2,7 +2,7 @@
 
 function sink($_) {}
 
-function ternarySimplify($x, $y) {
+function ternarySimplify(int $x, $y) {
     sink($x ? true : false);
     sink((bool)$x);