Repository: VKCOM/noverify
Pull request #968 (2021-04-12 18:50:55)

linter: improve oldStyleConstructor checker
Now the check does not work if there is a namespace. 
I think this can also be included in this PR, since this is 
also a kind of improvement 😄 

quasilyte: If you want to add this change, please make this code trigger a method name case warning:
```
class Foo {
  public function foo() {} // it's old-style constructor
  public function__construct() {}
}
```
People may do this without knowing that `foo` will be considered to be a constructor, not a normal method.

I would actually not allow such "legacy compatibility" constructors at all. We don't support such old versions of PHP anyway in noverify. Our target is somewhere in PHP7-PHP8 range.
i582: > I would actually not allow such "legacy compatibility" constructors at all. We don't support such old versions of PHP anyway in noverify. Our target is somewhere in PHP7-PHP8 range.

In that case, then it probably really doesn't make sense to have an 
exception for backward compatibility.

Patch:
diff --git a/src/linter/root.go b/src/linter/root.go
index 293c7e3..741feec 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -889,21 +889,28 @@ func (d *rootWalker) enterClassConstList(list *ir.ClassConstListStmt) bool {
 	return true
 }
 
-func (d *rootWalker) checkOldStyleConstructor(meth *ir.ClassMethodStmt, nm string) {
-	lastDelim := strings.IndexByte(d.ctx.st.CurrentClass, '\\')
-	if strings.EqualFold(d.ctx.st.CurrentClass[lastDelim+1:], nm) {
-		_, isClass := d.currentClassNode.(*ir.ClassStmt)
-		if isClass {
-			d.Report(meth.MethodName, LevelNotice, "oldStyleConstructor", "Old-style constructor usage, use __construct instead")
-		}
+func (d *rootWalker) checkOldStyleConstructor(meth *ir.ClassMethodStmt) {
+	lastDelim := strings.LastIndexByte(d.ctx.st.CurrentClass, '\\')
+	methodName := meth.MethodName.Value
+	className := d.ctx.st.CurrentClass[lastDelim+1:]
+
+	if !strings.EqualFold(className, methodName) {
+		return
 	}
+
+	_, inClass := d.currentClassNode.(*ir.ClassStmt)
+	if !inClass {
+		return
+	}
+
+	d.Report(meth.MethodName, LevelNotice, "oldStyleConstructor", "Old-style constructor usage, use __construct instead")
 }
 
 func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 	nm := meth.MethodName.Value
 	_, insideInterface := d.currentClassNode.(*ir.InterfaceStmt)
 
-	d.checkOldStyleConstructor(meth, nm)
+	d.checkOldStyleConstructor(meth)
 
 	pos := ir.GetPosition(meth)
 


Test patch:
diff --git a/src/tests/checkers/oop_test.go b/src/tests/checkers/oop_test.go
index 4b32792..f8d9804 100644
--- a/src/tests/checkers/oop_test.go
+++ b/src/tests/checkers/oop_test.go
@@ -141,11 +141,51 @@ class t3 {
   /** inverse of the T2 test case */
   public function T3() {}
 }
+
+trait TraitWithNameMatchingMethod {
+  /** ok */
+  public function TraitWithNameMatchingMethod() {}
+}
+
+interface InterfaceWithNameMatchingMethod {
+  /** ok */
+  public function InterfaceWithNameMatchingMethod();
+}
+
+namespace SameWithNamespace {
+  class T1 {
+    /** simple constructor */
+    public function T1() {}
+  }
+  
+  class T2 {
+    /** constructor name is in lower case */
+    public function t2() {}
+  }
+  
+  class t3 {
+    /** inverse of the T2 test case */
+    public function T3() {}
+  }
+
+  trait TraitWithNameMatchingMethod {
+    /** ok */
+    public function TraitWithNameMatchingMethod() {}
+  }
+  
+  interface InterfaceWithNameMatchingMethod {
+    /** ok */
+    public function InterfaceWithNameMatchingMethod();
+  }
+}
 `)
 	test.Expect = []string{
 		`Old-style constructor usage, use __construct instead`,
 		`Old-style constructor usage, use __construct instead`,
 		`Old-style constructor usage, use __construct instead`,
+		`Old-style constructor usage, use __construct instead`,
+		`Old-style constructor usage, use __construct instead`,
+		`Old-style constructor usage, use __construct instead`,
 	}
 	test.RunAndMatch()
 }