Repository: VKCOM/noverify
Pull request #1086 (2021-08-17 18:26:15)

linter: add `--strict-mixed` flag
Previously, by default, we would give an undefined method
or property error even if the type of the variable was `mixed`,
which led to the fact that poorly typed code gave a large
number of errors.

Now this behavior has been taken under the `--strict-mixed` flag,
without it in these cases an error will not be thrown.


Patch:
diff --git a/docs/configuration.md b/docs/configuration.md
index fdc0ab3..af913a7 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -19,6 +19,7 @@ This page is dedicated to some technical details.
   * [How to change the cache directory](#how-to-change-the-cache-directory)
   * [How to disable caching](#how-to-disable-caching)
   * [How to use PHP 7](#how-to-use-php-7)
+  * [How to use strict-mixed mode](#how-to-use-strict-mixed-mode)
 - [Hard level options](#hard-level-options)
   * [How to use dynamic rules](#how-to-use-dynamic_rules)
   * [How to use `baseline` mode](#how-to-use--baseline--mode)
@@ -242,6 +243,30 @@ noverify check --php7 ./src
 
 By default, all code is parsed as 8.1, however, some projects use names that have become reserved in PHP 8, so they need to be parsed as PHP 7.
 
+### How to use strict-mixed mode
+
+It looks like this:
+
+```shell
+noverify check --strict-mixed ./src
+```
+
+By default, if a method is called using a variable with the `mixed` or  `object` type (or property fetched), then the linter does not give a warning about an undefined method or property.
+
+```php
+// noverify check ./src
+function f($a) {
+  $a->foo(); // ok
+  $a->boo;   // ok
+}
+
+// noverify check --strict-mixed ./src
+function f1($a) {
+  $a->foo(); // error: undefined method 'foo'
+  $a->boo;   // error: undefined property 'boo'
+}
+```
+
 <p><br></p>
 
 ## Hard level options
diff --git a/docs/getting_started.md b/docs/getting_started.md
index dc83584..e7efd17 100644
--- a/docs/getting_started.md
+++ b/docs/getting_started.md
@@ -50,7 +50,7 @@ This will lead to an errors:
 <critical> WARNING parentConstructor: Missing parent::__construct() call at swiftmailer/lib/classes/Swift/Attachment.php:27
     public function __construct($data = null, $filename = null, $contentType = null)
                     ^^^^^^^^^^^
-2021/07/08 16:13:19 Found 140 critical and 10 minor reports
+2021/07/08 16:13:19 Found 113 critical and 10 minor reports
 ```
 
 From the errors, you can understand on which lines NoVerify gives errors, and also understand what kind of error it is. Also, you may notice that the errors occurred in different files.
diff --git a/src/cmd/check.go b/src/cmd/check.go
index 7266798..7e38db9 100644
--- a/src/cmd/check.go
+++ b/src/cmd/check.go
@@ -54,4 +54,5 @@ func bindConfigValuesWithFlags(ctx *AppContext, config *linter.Config) {
 	config.ConservativeBaseline = ctx.ParsedFlags.ConservativeBaseline
 	config.ApplyQuickFixes = ctx.ParsedFlags.ApplyQuickFixes
 	config.KPHP = ctx.ParsedFlags.KPHP
+	config.StrictMixed = ctx.ParsedFlags.StrictMixed
 }
diff --git a/src/cmd/flags.go b/src/cmd/flags.go
index 60108df..3cb7245 100644
--- a/src/cmd/flags.go
+++ b/src/cmd/flags.go
@@ -19,6 +19,8 @@ type ParsedFlags struct {
 	CPUProfile string
 	MemProfile string
 
+	StrictMixed bool
+
 	IgnoreVendor bool
 
 	MaxFileSize int
@@ -166,6 +168,7 @@ func RegisterCheckFlags(ctx *AppContext) (*flag.FlagSet, *FlagsGroups) {
 	fs.BoolVar(&ctx.ParsedFlags.IgnoreVendor, "ignore-vendor", false,
 		"Do not use the vendor folder to get information about functions and classes (not recommended as type inference will work much worse)")
 	fs.BoolVar(&ctx.ParsedFlags.PHP7, "php7", false, "Analyze as PHP 7")
+	fs.BoolVar(&ctx.ParsedFlags.StrictMixed, "strict-mixed", false, "Use strict mixed mode")
 
 	groups.Add("Analyze setting", "cores")
 	groups.Add("Analyze setting", "cache-dir")
@@ -175,6 +178,7 @@ func RegisterCheckFlags(ctx *AppContext) (*flag.FlagSet, *FlagsGroups) {
 	groups.Add("Analyze setting", "stubs-dir")
 	groups.Add("Analyze setting", "ignore-vendor")
 	groups.Add("Analyze setting", "php7")
+	groups.Add("Analyze setting", "strict-mixed")
 
 	// Additional group.
 	fs.BoolVar(&ctx.ParsedFlags.CheckAutoGenerated, "check-auto-generated", false, "Check auto-generated PHP files")
diff --git a/src/linter/block.go b/src/linter/block.go
index 9ed257f..5a0c8a9 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -1008,7 +1008,7 @@ func (b *blockWalker) handleCompactCallArgs(args []ir.Node) {
 }
 
 func (b *blockWalker) handleMethodCall(e *ir.MethodCallExpr) bool {
-	call := resolveMethodCall(b.ctx.sc, b.r.ctx.st, b.ctx.customTypes, e)
+	call := resolveMethodCall(b.ctx.sc, b.r.ctx.st, b.ctx.customTypes, e, b.r.strictMixed)
 
 	e.Variable.Walk(b)
 	e.Method.Walk(b)
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index 5db1fce..740a020 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -1160,7 +1160,7 @@ func (b *blockLinter) checkStripTags(e *ir.FunctionCallExpr) {
 func (b *blockLinter) checkMethodCall(e *ir.MethodCallExpr) {
 	parseState := b.classParseState()
 
-	call := resolveMethodCall(b.walker.ctx.sc, parseState, b.walker.ctx.customTypes, e)
+	call := resolveMethodCall(b.walker.ctx.sc, parseState, b.walker.ctx.customTypes, e, b.walker.r.strictMixed)
 	if !call.canAnalyze {
 		return
 	}
@@ -1170,9 +1170,11 @@ func (b *blockLinter) checkMethodCall(e *ir.MethodCallExpr) {
 	}
 
 	if !call.isFound && !call.isMagic && !parseState.IsTrait && !b.walker.isThisInsideClosure(e.Variable) {
-		// The method is undefined but we permit calling it if `method_exists`
+		needShowUndefinedMethod := !call.callerTypeIsMixed || b.walker.r.strictMixed
+
+		// The method is undefined, but we permit calling it if `method_exists`
 		// was called prior to that call.
-		if !b.walker.ctx.customMethodExists(e.Variable, call.methodName) {
+		if !b.walker.ctx.customMethodExists(e.Variable, call.methodName) && needShowUndefinedMethod {
 			b.report(e.Method, LevelError, "undefined", "Call to undefined method {%s}->%s()", call.methodCallerType, call.methodName)
 		}
 	} else if !call.isMagic && !parseState.IsTrait {
@@ -1234,12 +1236,17 @@ func (b *blockLinter) checkStaticCall(e *ir.StaticCallExpr) {
 }
 
 func (b *blockLinter) checkPropertyFetch(e *ir.PropertyFetchExpr) {
-	fetch := resolvePropertyFetch(b.walker.ctx.sc, b.classParseState(), b.walker.ctx.customTypes, e)
+	fetch := resolvePropertyFetch(b.walker.ctx.sc, b.classParseState(), b.walker.ctx.customTypes, e, b.walker.r.strictMixed)
 	if !fetch.canAnalyze {
 		return
 	}
 
-	if !fetch.isFound && !fetch.isMagic && !b.classParseState().IsTrait && !b.walker.isThisInsideClosure(e.Variable) {
+	needShowUndefinedProperty := !fetch.callerTypeIsMixed || b.walker.r.strictMixed
+
+	if !fetch.isFound && !fetch.isMagic &&
+		!b.classParseState().IsTrait &&
+		!b.walker.isThisInsideClosure(e.Variable) &&
+		needShowUndefinedProperty {
 		b.report(e.Property, LevelError, "undefined", "Property {%s}->%s does not exist", fetch.propertyFetchType, fetch.propertyNode.Value)
 	}
 
diff --git a/src/linter/conf.go b/src/linter/conf.go
index 52aaa45..9055f2e 100644
--- a/src/linter/conf.go
+++ b/src/linter/conf.go
@@ -64,6 +64,8 @@ type Config struct {
 	IgnoreTriggerError bool
 
 	PhpVersion *version.Version
+
+	StrictMixed bool
 }
 
 func NewConfig(ver string) *Config {
diff --git a/src/linter/root.go b/src/linter/root.go
index 64f726b..24e7d1a 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -66,6 +66,7 @@ type rootWalker struct {
 
 	// strictTypes is true if file contains `declare(strict_types=1)`.
 	strictTypes bool
+	strictMixed bool
 
 	reports []*Report
 
diff --git a/src/linter/utils.go b/src/linter/utils.go
index 29e44da..a05e8f7 100644
--- a/src/linter/utils.go
+++ b/src/linter/utils.go
@@ -224,16 +224,17 @@ func resolveFunctionCall(sc *meta.Scope, st *meta.ClassParseState, customTypes [
 }
 
 type methodCallInfo struct {
-	methodName       string
-	className        string
-	info             meta.FuncInfo
-	methodCallerType types.Map
-	isFound          bool
-	isMagic          bool
-	canAnalyze       bool
+	methodName        string
+	className         string
+	info              meta.FuncInfo
+	methodCallerType  types.Map
+	isFound           bool
+	isMagic           bool
+	canAnalyze        bool
+	callerTypeIsMixed bool
 }
 
-func resolveMethodCall(sc *meta.Scope, st *meta.ClassParseState, customTypes []solver.CustomType, e *ir.MethodCallExpr) methodCallInfo {
+func resolveMethodCall(sc *meta.Scope, st *meta.ClassParseState, customTypes []solver.CustomType, e *ir.MethodCallExpr, strictMixed bool) methodCallInfo {
 	if !st.Info.IsIndexingComplete() {
 		return methodCallInfo{canAnalyze: false}
 	}
@@ -256,6 +257,12 @@ func resolveMethodCall(sc *meta.Scope, st *meta.ClassParseState, customTypes []s
 	)
 
 	methodCallerType := solver.ExprTypeCustom(sc, st, e.Variable, customTypes)
+	if !strictMixed && isMixedLikeType(methodCallerType) {
+		return methodCallInfo{
+			canAnalyze:        true,
+			callerTypeIsMixed: true,
+		}
+	}
 
 	methodCallerType.Find(func(typ string) bool {
 		m, isMagic, ok := findMethod(st.Info, typ, methodName)
@@ -389,9 +396,10 @@ type propertyFetchInfo struct {
 	isFound           bool
 	isMagic           bool
 	canAnalyze        bool
+	callerTypeIsMixed bool
 }
 
-func resolvePropertyFetch(sc *meta.Scope, st *meta.ClassParseState, customTypes []solver.CustomType, e *ir.PropertyFetchExpr) propertyFetchInfo {
+func resolvePropertyFetch(sc *meta.Scope, st *meta.ClassParseState, customTypes []solver.CustomType, e *ir.PropertyFetchExpr, strictMixed bool) propertyFetchInfo {
 	propertyNode, ok := e.Property.(*ir.Identifier)
 	if !ok {
 		return propertyFetchInfo{canAnalyze: false}
@@ -404,6 +412,13 @@ func resolvePropertyFetch(sc *meta.Scope, st *meta.ClassParseState, customTypes
 	var info meta.PropertyInfo
 
 	propertyFetchType := solver.ExprTypeCustom(sc, st, e.Variable, customTypes)
+	if !strictMixed && isMixedLikeType(propertyFetchType) {
+		return propertyFetchInfo{
+			canAnalyze:        true,
+			callerTypeIsMixed: true,
+		}
+	}
+
 	propertyFetchType.Find(func(typ string) bool {
 		p, isMagic, ok := findProperty(st.Info, typ, propertyNode.Value)
 		if !ok {
@@ -627,6 +642,16 @@ func cloneRulesForFile(filename string, ruleSet *rules.ScopedSet) *rules.ScopedS
 	return &clone
 }
 
+func isMixedLikeType(typ types.Map) bool {
+	return typ.Find(func(typ string) bool {
+		if typ == "mixed" || typ == "object" || typ == "undefined" || typ == "unknown_from_list" {
+			return true
+		}
+
+		return false
+	})
+}
+
 // List taken from https://wiki.php.net/rfc/context_sensitive_lexer
 var phpKeywords = map[string]bool{
 	"callable":     true,
diff --git a/src/linter/worker.go b/src/linter/worker.go
index 4ef2b4a..84a335d 100644
--- a/src/linter/worker.go
+++ b/src/linter/worker.go
@@ -51,6 +51,8 @@ type Worker struct {
 	reParserNoLiterals *syntax.Parser
 	reParser           *syntax.Parser
 
+	strictMixed bool
+
 	needReports bool
 
 	AllowDisable *regexp.Regexp
@@ -76,6 +78,7 @@ func newWorker(config *Config, info *meta.Info, id int, checkersFilter *Checkers
 			NoLiterals: false,
 		}),
 		checkersFilter: checkersFilter,
+		strictMixed:    config.StrictMixed,
 	}
 }
 
@@ -280,6 +283,7 @@ func (w *Worker) analyzeFile(file *workspace.File, rootNode *ir.Root) (*rootWalk
 			parser: w.reParserNoLiterals,
 			out:    &strings.Builder{},
 		},
+		strictMixed: w.strictMixed,
 
 		allowDisabledRegexp: w.AllowDisable,
 		checkersFilter:      w.checkersFilter,


Test patch:
diff --git a/src/linttest/golden_linttest.go b/src/linttest/golden_linttest.go
index 1a3e1a8..ecf8e19 100644
--- a/src/linttest/golden_linttest.go
+++ b/src/linttest/golden_linttest.go
@@ -286,6 +286,7 @@ func (s *GoldenE2ETestSuite) RunOnlyTests() {
 					"--output-json",
 					"--disable-cache", // TODO: test with cache as well
 					"--allow-all-checks",
+					"--strict-mixed",
 					"--output", outputFilename,
 				}
 				if len(test.Disable) != 0 {
diff --git a/src/tests/checkers/oop_test.go b/src/tests/checkers/oop_test.go
index 391fc79..06c7007 100644
--- a/src/tests/checkers/oop_test.go
+++ b/src/tests/checkers/oop_test.go
@@ -824,6 +824,7 @@ func TestInheritanceLoop(t *testing.T) {
 
 func TestClosureLateBinding(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 	class Example
 	{
diff --git a/src/tests/checkers/shape_test.go b/src/tests/checkers/shape_test.go
index a5eb78f..c2d66e6 100644
--- a/src/tests/checkers/shape_test.go
+++ b/src/tests/checkers/shape_test.go
@@ -29,6 +29,7 @@ function f($s) {
 
 func TestShapeDimFetch(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 class Foo {
   public $x = 10;
@@ -52,6 +53,7 @@ function f() {
 
 func TestShapeIntKey(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 class Box { public $value; }
 
@@ -74,6 +76,7 @@ function f() {
 
 func TestShapeSyntax(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 class Box { public $value; }
 
diff --git a/src/tests/checkers/strict_mixed_test.go b/src/tests/checkers/strict_mixed_test.go
new file mode 100644
index 0000000..c9fd8a9
--- /dev/null
+++ b/src/tests/checkers/strict_mixed_test.go
@@ -0,0 +1,80 @@
+package checkers
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestStrictMixedEnabled(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
+	test.AddFile(`<?php
+class Foo {}
+
+function f($a) {
+  $a->f();
+}
+
+function f(object $a) {
+  $a->f();
+}
+
+function f() {
+  $a->f();
+}
+
+function f($b) {
+  [$a,] = $b;
+  $a->f();
+}
+
+function f(Foo $a) {
+  $a->f();
+}
+`,
+	)
+	test.Expect = []string{
+		"Call to undefined method {mixed}->f()",
+		"Call to undefined method {object}->f()",
+		"Undefined variable $a",
+		"Call to undefined method {undefined}->f()",
+		"Call to undefined method {unknown_from_list}->f()",
+		"Call to undefined method {\\Foo}->f()",
+	}
+	test.RunAndMatch()
+}
+
+func TestStrictMixedDisabled(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class Foo {}
+
+function f($a) {
+  $a->f();
+}
+
+function f(object $a) {
+  $a->f();
+}
+
+function f() {
+  $a->f();
+}
+
+function f($b) {
+  [$a,] = $b;
+  $a->f();
+}
+
+function f(Foo $a) {
+  $a->f();
+}
+`,
+	)
+	test.Expect = []string{
+		"Undefined variable $a",
+		"Call to undefined method {\\Foo}->f()",
+	}
+	test.RunAndMatch()
+}
diff --git a/src/tests/golden/golden_test.go b/src/tests/golden/golden_test.go
index e2e1ada..26ddd7e 100644
--- a/src/tests/golden/golden_test.go
+++ b/src/tests/golden/golden_test.go
@@ -198,6 +198,8 @@ func TestGolden(t *testing.T) {
 	e2eSuite := linttest.NewGoldenE2ETestSuite(t)
 
 	linterConfig := linter.NewConfig("8.1")
+	linterConfig.StrictMixed = true
+
 	err := linttest.InitEmbeddedRules(linterConfig)
 	if err != nil {
 		t.Fatal(err)
diff --git a/src/tests/regression/issue128_test.go b/src/tests/regression/issue128_test.go
index 8c9b613..3b6894d 100644
--- a/src/tests/regression/issue128_test.go
+++ b/src/tests/regression/issue128_test.go
@@ -8,6 +8,7 @@ import (
 
 func TestIssue128(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 class Value {
   public $x;
diff --git a/src/tests/regression/issue16_test.go b/src/tests/regression/issue16_test.go
index 720b8ec..fc86b23 100644
--- a/src/tests/regression/issue16_test.go
+++ b/src/tests/regression/issue16_test.go
@@ -8,6 +8,7 @@ import (
 
 func TestIssue16(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 	interface DateTimeInterface {
 		public function format($fmt);
diff --git a/src/tests/regression/issue288_test.go b/src/tests/regression/issue288_test.go
index 38dbb3e..7e79d1c 100644
--- a/src/tests/regression/issue288_test.go
+++ b/src/tests/regression/issue288_test.go
@@ -8,6 +8,7 @@ import (
 
 func TestIssue288(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 class Box {
   public $item1;
diff --git a/src/tests/regression/issue362_test.go b/src/tests/regression/issue362_test.go
index 138668d..f0930a1 100644
--- a/src/tests/regression/issue362_test.go
+++ b/src/tests/regression/issue362_test.go
@@ -41,6 +41,7 @@ if (method_exists($x, 'x1')) {
 
 func TestIssue362_2(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 function method_exists($object, $method_name) { return 1 != 0; }
 
diff --git a/src/tests/regression/issue390_test.go b/src/tests/regression/issue390_test.go
index f878ef5..9d06f60 100644
--- a/src/tests/regression/issue390_test.go
+++ b/src/tests/regression/issue390_test.go
@@ -8,6 +8,7 @@ import (
 
 func TestIssue390(t *testing.T) {
 	test := linttest.NewSuite(t)
+	test.Config().StrictMixed = true
 	test.AddFile(`<?php
 $cond = 1;
 if ($cond && isset($a1[0])) {