Repository: VKCOM/noverify
Pull request #645 (2020-08-07 07:42:07)

linter: added check for the correct number of arguments for magic methods
Fixed errors in tests (`TestNonPublicMagicMethods`, `TestIssue8`) that appeared due to a new check.


Patch:
diff --git a/src/linter/root.go b/src/linter/root.go
index 03624ed..d97bb31 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -778,33 +778,56 @@ func (d *RootWalker) parseMethodModifiers(meth *stmt.ClassMethod) (res methodMod
 	return res
 }
 
-func (d *RootWalker) checkMagicMethod(meth node.Node, name string, modif methodModifiers) {
+func (d *RootWalker) checkMagicMethod(meth node.Node, name string, modif methodModifiers, countArgs int) {
+	const Any = -1
 	var (
 		canBeStatic    bool
 		canBeNonPublic bool
 		mustBeStatic   bool
+
+		numArgsExpected int
 	)
 
 	switch name {
 	case "__call",
-		"__get",
-		"__set",
+		"__set":
+		canBeStatic = false
+		canBeNonPublic = false
+		numArgsExpected = 2
+
+	case "__get",
 		"__isset",
-		"__unset",
-		"__toString",
-		"__invoke",
+		"__unset":
+		canBeStatic = false
+		canBeNonPublic = false
+		numArgsExpected = 1
+
+	case "__toString":
+		canBeStatic = false
+		canBeNonPublic = false
+		numArgsExpected = 0
+
+	case "__invoke",
 		"__debugInfo":
 		canBeStatic = false
 		canBeNonPublic = false
+		numArgsExpected = Any
 
-	case "__construct", "__destruct", "__clone":
+	case "__construct":
 		canBeStatic = false
 		canBeNonPublic = true
+		numArgsExpected = Any
+
+	case "__destruct", "__clone":
+		canBeStatic = false
+		canBeNonPublic = true
+		numArgsExpected = 0
 
 	case "__callStatic":
 		canBeStatic = true
 		canBeNonPublic = false
 		mustBeStatic = true
+		numArgsExpected = 2
 
 	case "__sleep",
 		"__wakeup",
@@ -813,6 +836,7 @@ func (d *RootWalker) checkMagicMethod(meth node.Node, name string, modif methodM
 		"__set_state":
 		canBeNonPublic = true
 		canBeStatic = true
+		numArgsExpected = Any
 
 	default:
 		return // Not a magic method
@@ -827,6 +851,10 @@ func (d *RootWalker) checkMagicMethod(meth node.Node, name string, modif methodM
 	if !canBeNonPublic && modif.accessLevel != meta.Public {
 		d.Report(meth, LevelError, "magicMethodDecl", "The magic method %s() must have public visibility", name)
 	}
+
+	if countArgs != numArgsExpected && numArgsExpected != Any {
+		d.Report(meth, LevelError, "magicMethodDecl", "The magic method %s() must take exactly %d argument", name, numArgsExpected)
+	}
 }
 
 func (d *RootWalker) getClass() meta.ClassInfo {
@@ -984,7 +1012,7 @@ func (d *RootWalker) enterClassMethod(meth *stmt.ClassMethod) bool {
 
 	modif := d.parseMethodModifiers(meth)
 
-	d.checkMagicMethod(meth.MethodName, nm, modif)
+	d.checkMagicMethod(meth.MethodName, nm, modif, len(meth.Params))
 
 	sc := meta.NewScope()
 	if !modif.static {


Test patch:
diff --git a/src/tests/checkers/oop_test.go b/src/tests/checkers/oop_test.go
index ee22985..6996c12 100644
--- a/src/tests/checkers/oop_test.go
+++ b/src/tests/checkers/oop_test.go
@@ -274,7 +274,7 @@ class A {
   public static function __set($name, $value) {} // The magic method __set() cannot be static
   protected function __toString() {} // The magic method __call() must have public visibility
   public function __callStatic($name, $arguments) {} // The magic method __callStatic() must be static
-  public static function __destruct($name, $arguments) {} // The magic method __destruct() cannot be static
+  public static function __destruct() {} // The magic method __destruct() cannot be static
   public static function __construct($name, $arguments) {} // The magic method __construct() cannot be static
   private static function __call($name, $arguments) {} // The magic method __call() must have public visibility
   private static function __callStatic($name, $arguments) {} // The magic method __callStatic() must have public visibility
@@ -339,6 +339,59 @@ class A {
 }`)
 }
 
+func TestWrongNumberOfArgumentsInMagicMethods(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class Foo {
+  public function __destruct($a) {} // 0
+  public function __call($a) {} // 2
+  public static function __callStatic($a, $b, $c) {} // 2
+  public function __get($a, $b) {} // 1
+  public function __set($a) {} // 2
+  public function __isset() {} // 1
+  public function __unset($a, $b) {} // 1
+  public function __toString($a) {} // 0
+}`)
+
+	test.Expect = []string{
+		"The magic method __destruct() must take exactly 0 argument",
+		"The magic method __call() must take exactly 2 argument",
+		"The magic method __callStatic() must take exactly 2 argument",
+		"The magic method __get() must take exactly 1 argument",
+		"The magic method __set() must take exactly 2 argument",
+		"The magic method __isset() must take exactly 1 argument",
+		"The magic method __unset() must take exactly 1 argument",
+		"The magic method __toString() must take exactly 0 argument",
+	}
+	test.RunAndMatch()
+}
+
+func TestWrongNumberOfArgumentsInMagicMethodsGood(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+class Foo {
+  public function __construct($a) {}
+  public function __construct($a, $b) {}
+  public function __destruct() {} // 0
+  public function __call($a, $b) {} // 2
+  public static function __callStatic($a, $b) {} // 2
+  public function __get($a) {} // 1
+  public function __set($a, $b) {} // 2
+  public function __isset($a) {} // 1
+  public function __unset($a) {} // 1
+  public function __sleep($a) {}
+  public function __wakeup() {}
+  public function __serialize() {}
+  public function __unserialize() {}
+  public function __toString() {} // 0
+  public function __invoke() {}
+  public function __set_state() {}
+  public function __clone() {} // 0
+  public function __debugInfo() {}
+  public function __non_magic($a) {} // any
+  public function __non_magic() {} // any
+}`)
+}
+
 func TestIteratorForeach(t *testing.T) {
 	linttest.SimpleNegativeTest(t, `<?php
 interface Iterator extends Traversable {
diff --git a/src/tests/checkers/regression_test.go b/src/tests/checkers/regression_test.go
index 59fd384..d3009da 100644
--- a/src/tests/checkers/regression_test.go
+++ b/src/tests/checkers/regression_test.go
@@ -133,13 +133,13 @@ func TestIssue8(t *testing.T) {
 	linttest.SimpleNegativeTest(t, `<?php
 	class Magic
 	{
-		public function __get();
-		public function __set();
-		public function __call();
+		public function __get($a);
+		public function __set($a, $b);
+		public function __call($a, $b);
 	}
 
 	class MagicStatic {
-		public static function __callStatic();
+		public static function __callStatic($a, $b);
 	}
 
 	function test() {