Repository: VKCOM/noverify
Pull request #1061 (2021-07-15 22:45:22)

linter: added `implicitModifiers` checker
It checks that an explicit access level is specified for the methods and properties of the class


Patch:
diff --git a/src/linter/report.go b/src/linter/report.go
index 6c34db9..904bd49 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -790,6 +790,19 @@ function f(array $a) {}`,
   case 1:
     // do something
     break;
+}`,
+		},
+
+		{
+			Name:     "implicitModifiers",
+			Default:  true,
+			Quickfix: false,
+			Comment:  `Report implicit modifiers.`,
+			Before: `class Foo {
+  function f() {} // The access modifier is implicit.
+}`,
+			After: `class Foo {
+  public function f() {}
 }`,
 		},
 	}
diff --git a/src/linter/root.go b/src/linter/root.go
index b31f32c..d3dfc81 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -721,14 +721,16 @@ func (d *rootWalker) getElementPos(n ir.Node) meta.ElementPosition {
 }
 
 type methodModifiers struct {
-	abstract    bool
-	static      bool
-	accessLevel meta.AccessLevel
-	final       bool
+	abstract       bool
+	static         bool
+	accessLevel    meta.AccessLevel
+	final          bool
+	accessImplicit bool
 }
 
 func (d *rootWalker) parseMethodModifiers(meth *ir.ClassMethodStmt) (res methodModifiers) {
 	res.accessLevel = meta.Public
+	res.accessImplicit = true
 
 	for _, m := range meth.Modifiers {
 		switch d.lowerCaseModifier(m) {
@@ -738,10 +740,13 @@ func (d *rootWalker) parseMethodModifiers(meth *ir.ClassMethodStmt) (res methodM
 			res.static = true
 		case "public":
 			res.accessLevel = meta.Public
+			res.accessImplicit = false
 		case "private":
 			res.accessLevel = meta.Private
+			res.accessImplicit = false
 		case "protected":
 			res.accessLevel = meta.Protected
+			res.accessImplicit = false
 		case "final":
 			res.final = true
 		default:
@@ -886,20 +891,32 @@ func (d *rootWalker) enterPropertyList(pl *ir.PropertyListStmt) bool {
 
 	isStatic := false
 	accessLevel := meta.Public
+	accessImplicit := true
 
 	for _, m := range pl.Modifiers {
 		switch d.lowerCaseModifier(m) {
 		case "public":
 			accessLevel = meta.Public
+			accessImplicit = false
 		case "protected":
 			accessLevel = meta.Protected
+			accessImplicit = false
 		case "private":
 			accessLevel = meta.Private
+			accessImplicit = false
 		case "static":
 			isStatic = true
 		}
 	}
 
+	if accessImplicit {
+		target := "property"
+		if len(pl.Properties) > 1 {
+			target = "properties"
+		}
+		d.Report(pl, LevelNotice, "implicitModifiers", "Specify the access modifier for %s explicitly", target)
+	}
+
 	d.checkCommentMisspellings(pl, pl.Doc.Raw)
 	phpDocType := d.parsePHPDocVar(pl, pl.Doc)
 
@@ -1045,8 +1062,15 @@ func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 		d.Report(meth.MethodName, LevelNotice, "complexity", "Too big method: more than %d lines", maxFunctionLines)
 	}
 
+	class := d.getClass()
+
 	modif := d.parseMethodModifiers(meth)
 
+	if modif.accessImplicit {
+		methodFQN := class.Name + "::" + nm
+		d.Report(meth.MethodName, LevelNotice, "implicitModifiers", "Specify the access modifier for %s method explicitly", methodFQN)
+	}
+
 	d.checkMagicMethod(meth.MethodName, nm, modif, len(meth.Params))
 
 	sc := meta.NewScope()
@@ -1055,8 +1079,6 @@ func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 		sc.SetInInstanceMethod(true)
 	}
 
-	class := d.getClass()
-
 	if meth.Doc.Raw == "" && modif.accessLevel == meta.Public {
 		// Permit having "__call" and other magic method without comments.
 		if !insideInterface && !strings.HasPrefix(nm, "_") {


Test patch:
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index ad965cd..6361e28 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -2315,7 +2315,7 @@ func TestComplexInstanceOf(t *testing.T) {
 	test.AddFile(`<?php
 class Boo {
   /** @return int */
-  function b() { return 0; }
+  public function b() { return 0; }
 }
 
 function f($a) {
diff --git a/src/tests/checkers/oop_test.go b/src/tests/checkers/oop_test.go
index 575f9a8..391fc79 100644
--- a/src/tests/checkers/oop_test.go
+++ b/src/tests/checkers/oop_test.go
@@ -1388,7 +1388,6 @@ trait AbstractTraitAB {
 func TestInterfaceRules(t *testing.T) {
 	test := linttest.NewSuite(t)
 	test.AddFile(`<?php
-
 interface WithConstants {
   const r = 10000; // ok
   public const v = 1; // ok
@@ -1423,7 +1422,7 @@ interface WithoutAnyModifier {
 		`'bad1' can't be private`,
 		`'bad2' can't be protected`,
 	}
-	test.RunAndMatch()
+	linttest.RunFilterMatch(test, "nonPublicInterfaceMember")
 }
 
 func TestMixinAnnotation(t *testing.T) {
@@ -1757,3 +1756,42 @@ function f($arg) {
 	}
 	test.RunAndMatch()
 }
+
+func TestImplicitAccessModifiers(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class Foo {
+  const FOO = 100; // ok
+  public const FOO1 = 100; // ok
+  private const FOO2 = 100; // ok
+  protected const FOO3 = 100; // ok
+  
+  var int $prop = 100;
+  public int $prop1 = 100;
+  private int $prop2 = 100;
+  protected int $prop3 = 100;
+
+  var int $prop4, $prop5 = 100;
+  public int $prop6, $prop7 = 100;
+  private int $prop8, $prop9 = 100;
+  protected int $prop10, $prop11 = 100;
+  
+  function f1() {}
+  public function f2() {}
+  private function f3() {}
+  protected function f4() {}
+
+  static function f5() {}
+  public static function f6() {}
+  private static function f7() {}
+  protected static function f8() {}
+}
+`)
+	test.Expect = []string{
+		`Specify the access modifier for property explicitly`,
+		`Specify the access modifier for properties explicitly`,
+		`Specify the access modifier for \Foo::f1 method explicitly`,
+		`Specify the access modifier for \Foo::f5 method explicitly`,
+	}
+	linttest.RunFilterMatch(test, "implicitModifiers")
+}
diff --git a/src/tests/checkers/typehint_test.go b/src/tests/checkers/typehint_test.go
index 3b53f4d..db14990 100644
--- a/src/tests/checkers/typehint_test.go
+++ b/src/tests/checkers/typehint_test.go
@@ -13,14 +13,14 @@ abstract class FooAbstract {
     /**
      * @return string[]
      */
-    abstract function f(): array;
+    public abstract function f(): array;
 }
 
 class Foo extends FooAbstract {
     /**
      * @inheritdoc
      */
-    function f(): array { // skipped
+    public function f(): array { // skipped
         return [];
     }
 }
@@ -29,7 +29,7 @@ class Foo2 extends FooAbstract {
     /**
      * {@inheritdoc}
      */
-    function f(): array { // skipped
+    public function f(): array { // skipped
         return [];
     }
 }
@@ -47,24 +47,24 @@ class A {
   /**
    * @param int[] $a
    */
-  function f(array $a) {}
+  public function f(array $a) {}
 
   /**
    * @param mixed[] $a
    */
-  function f1(array $a) {}
+  public function f1(array $a) {}
 
   /**
    * @param mixed[] $a
    * @return mixed[]
    */
-  function f2(array $a): array {}
+  public function f2(array $a): array {}
 
   /**
    * @param mixed[] $a
    * @return int[]
    */
-  function f3(array $a): array {}
+  public function f3(array $a): array {}
 }
 
 /**
@@ -106,8 +106,8 @@ class A {
   public array $a = [];
   public array $b, $c = [];
 
-  function f(array $a) {}
-  function f1(): array {}
+  public function f(array $a) {}
+  public function f1(): array {}
 }
 
 function f(array $a) {}
diff --git a/src/tests/golden/testdata/math/golden.txt b/src/tests/golden/testdata/math/golden.txt
index 9d47501..ae3f8b9 100644
--- a/src/tests/golden/testdata/math/golden.txt
+++ b/src/tests/golden/testdata/math/golden.txt
@@ -7,6 +7,9 @@ MAYBE   trailingComma: Last element in a multi-line array should have a trailing
 MAYBE   ternarySimplify: Could rewrite as `$matches['fractional'] ?? ''` at testdata/math/src/BigNumber.php:90
             $fractional = isset($matches['fractional']) ? $matches['fractional'] : '';
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Brick\Math\Internal\Calculator::powmod method explicitly at testdata/math/src/Internal/Calculator.php:260
+    abstract function powmod(string $base, string $exp, string $mod) : string;
+                      ^^^^^^
 MAYBE   assignOp: Could rewrite as `$number ^= $xor` at testdata/math/src/Internal/Calculator.php:622
         $number = $number ^ $xor;
         ^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/src/tests/golden/testdata/parsedown/golden.txt b/src/tests/golden/testdata/parsedown/golden.txt
index c13f69b..817f78e 100644
--- a/src/tests/golden/testdata/parsedown/golden.txt
+++ b/src/tests/golden/testdata/parsedown/golden.txt
@@ -1,3 +1,21 @@
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::text method explicitly at testdata/parsedown/parsedown.php:24
+    function text($text)
+             ^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::setBreaksEnabled method explicitly at testdata/parsedown/parsedown.php:59
+    function setBreaksEnabled($breaksEnabled)
+             ^^^^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::setMarkupEscaped method explicitly at testdata/parsedown/parsedown.php:68
+    function setMarkupEscaped($markupEscaped)
+             ^^^^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::setUrlsLinked method explicitly at testdata/parsedown/parsedown.php:77
+    function setUrlsLinked($urlsLinked)
+             ^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::setSafeMode method explicitly at testdata/parsedown/parsedown.php:86
+    function setSafeMode($safeMode)
+             ^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::setStrictMode method explicitly at testdata/parsedown/parsedown.php:95
+    function setStrictMode($strictMode)
+             ^^^^^^^^^^^^^
 MAYBE   typeHint: Specify the type for the parameter $lines in PHPDoc, 'array' type hint too generic at testdata/parsedown/parsedown.php:162
     protected function lines(array $lines)
                        ^^^^^
@@ -139,6 +157,9 @@ MAYBE   ternarySimplify: Could rewrite as `$Element['autobreak'] ?? isset($Eleme
 WARNING strictCmp: 3rd argument of in_array must be true when comparing strings at testdata/parsedown/parsedown.php:1807
         if ( ! in_array('', $lines)
                ^^^^^^^^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::parse method explicitly at testdata/parsedown/parsedown.php:1854
+    function parse($text)
+             ^^^^^
 MAYBE   typeHint: Specify the type for the parameter $Element in PHPDoc, 'array' type hint too generic at testdata/parsedown/parsedown.php:1861
     protected function sanitiseElement(array $Element)
                        ^^^^^^^^^^^^^^^
@@ -148,6 +169,9 @@ WARNING unused: Variable $val is unused (use $_ to ignore this inspection or spe
 MAYBE   typeHint: Specify the type for the parameter $Element in PHPDoc, 'array' type hint too generic at testdata/parsedown/parsedown.php:1900
     protected function filterUnsafeUrlInAttribute(array $Element, $attribute)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
+MAYBE   implicitModifiers: Specify the access modifier for \Parsedown::instance method explicitly at testdata/parsedown/parsedown.php:1938
+    static function instance($name = 'default')
+                    ^^^^^^^^
 ERROR   classMembersOrder: Property $breaksEnabled must go before methods in the class Parsedown at testdata/parsedown/parsedown.php:66
     protected $breaksEnabled;
     ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/src/tests/regression/issue778_test.go b/src/tests/regression/issue778_test.go
index 5f864d3..aa02c07 100644
--- a/src/tests/regression/issue778_test.go
+++ b/src/tests/regression/issue778_test.go
@@ -10,14 +10,14 @@ func TestIssue778(t *testing.T) {
 	linttest.SimpleNegativeTest(t, `<?php
 trait FooStatic {
     /** @return void */
-    static function f() {
+    public static function f() {
         self::g(); // g() is expected to be defined outside of the FooStatic trait
     }
 }
 
 trait FooInstance {
     /** @return void */
-    function f() {
+    public function f() {
         $this->g(); // g() is expected to be defined outside of the FooInstance trait
     }
 }