Repository: VKCOM/noverify
Pull request #1143 (2021-09-02 18:09:18)

linter: fixed false positives in Chevere projects
Chevere: https://github.com/chevere/chevere

- Added support for property promotion
- `returnAssign`, `voidResultUsed`, and `parentNotFound` disabled by default and will fix and improve

## Warnings after

This seems to be correct since the result is not used.

```
<critical> WARNING discardExpr: Expression evaluated but not used at /Users/petrmakhnev/chevere/src/Chevere/Components/Action/Action.php:76
        new Arguments($this->responseParameters, ...$namedData);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Correct, `name` is static

```
<critical> WARNING callStatic: Calling static method as instance method at /Users/petrmakhnev/chevere/src/Chevere/Components/Router/Route/RouteEndpoints.php:30
            $routeEndpoint->method()->name(),
                                      ^^^^
```

~~Incorrect, an analysis is required for reflection~~. **Fixed.**

```
<critical> ERROR   undefinedMethod: Call to undefined method {\ReflectionType|null}->getName() at /Users/petrmakhnev/chevere/src/Chevere/Components/Var/VarStorable.php:106
                ? $property->getType()->getName() . ' '
                                        ^^^^^^^
```

Seems to be correct since PHPStorm is giving a warning here as well.

```
<critical> ERROR   undefinedVariable: Cannot find referenced variable $actionName at /Users/petrmakhnev/chevere/src/Chevere/Components/Workflow/WorkflowRunner.php:70
                        ->code('%action%', $actionName)
                                           ^^^^^^^^^^^
```

Correct.

```
<critical> WARNING callStatic: Calling static method as instance method at /Users/petrmakhnev/chevere/src/Chevere/Components/Router/Route/Route.php:105
        $key = $endpoint->method()->name();
                                    ^^^^
```

The `name` method does not exist in the `RoutePathInterface` interface, so the linter gives a warning, which is probably correct, since the `name` method in the new inheritor will not necessarily be, which can lead to a runtime error.

```
<critical> ERROR   undefinedMethod: Call to undefined method {\Chevere\Interfaces\Router\Route\RoutePathInterface}->name() at /Users/petrmakhnev/chevere/src/Chevere/Components/Spec/Specs/RouteSpec.php:41
        $this->path = $path->name();
                             ^^^^
<critical> ERROR   undefinedMethod: Call to undefined method {\Chevere\Interfaces\Router\Route\RoutePathInterface}->name() at /Users/petrmakhnev/chevere/src/Chevere/Components/Spec/Specs/RouteSpec.php:42
        $this->key = $path->name();
                            ^^^^
```

This seems to be correct since the result is not used.

```
<critical> WARNING discardExpr: Expression evaluated but not used at /Users/petrmakhnev/chevere/src/Chevere/Components/Var/functions.php:30
            new ObjectClonable($value);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Correct.

```
<critical> WARNING strictCmp: 3rd argument of in_array must be true when comparing strings at /Users/petrmakhnev/chevere/src/Chevere/Components/Type/Type.php:101
        if (isset(self::TYPE_VALIDATORS[$this->type]) && in_array($this->type, self::TYPE_ARGUMENTS)) {
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
MAYBE   assignOp: Could rewrite as `$this->timeTaken ??= (int) hrtime(true) - $this->startupTime` at /Users/petrmakhnev/chevere/src/Chevere/Components/Benchmark/BenchmarkRun.php:177
                $this->timeTaken = $this->timeTaken ?? (int) hrtime(true) - $this->startupTime;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Incorrect.

```
<critical> ERROR   undefinedMethod: Call to undefined method {\Ds\Map}->getIterator() at /Users/petrmakhnev/chevere/src/Chevere/Components/Benchmark/BenchmarkRun.php:241
            foreach ($this->records->getIterator() as $name => $timeTaken) {
                                     ^^^^^^^^^^^
```

Incorrect, an analysis is required for reflection.

```
<critical> ERROR   undefinedMethod: Call to undefined method {\ReflectionType|null}->getName() at /Users/petrmakhnev/chevere/src/Chevere/Components/Var/ObjectClonable.php:82
                ? $property->getType()->getName() . ' '
                                        ^^^^^^^
```

This seems to be correct since the result is not used.

```
<critical> WARNING discardExpr: Expression evaluated but not used at /Users/petrmakhnev/chevere/src/Chevere/Components/Workflow/WorkflowRun.php:68
        new Arguments(
        ^
```

Correct.

```
<critical> WARNING callStatic: Calling static method as instance method at /Users/petrmakhnev/chevere/src/Chevere/Components/Spec/Specs/RouteEndpointSpec.php:29
        $this->key = $routeEndpoint->method()->name();
                                               ^^^^
```

It is necessary to investigate.

```
<critical> WARNING parentConstructor: Missing parent::__construct() call at /Users/petrmakhnev/chevere/src/Chevere/Components/Workflow/Attributes/Provider.php:28
    public function __construct(protected string $attribute)
                    ^^^^^^^^^^^
```

Seems to be correct since PHPStorm is giving a warning here as well.

```
<critical> ERROR   undefinedClass: Class or interface named \Chevere\Components\Workflow\ActionInterface does not exist at /Users/petrmakhnev/chevere/src/Chevere/Components/Workflow/Steps.php:164
        /** @var ActionInterface $action */
                 ^^^^^^^^^^^^^^^
```

It seems to be correct, since this is an atypical format and PHPStorm does not understand it either.

```
<critical> WARNING invalidDocblockRef: @see tag refers to unknown symbol `VarDumpHighlightInterface::KEYS` at /Users/petrmakhnev/chevere/src/Chevere/Interfaces/VarDump/VarDumpHighlightInterface.php:45
     * @see `VarDumpHighlightInterface::KEYS`
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Correct.

```
        <critical> WARNING strictCmp: 3rd argument of in_array must be true when comparing strings at /Users/petrmakhnev/chevere/src/Chevere/Components/Controller/Controller.php:43
        if (in_array('', [$this->relation, $this->dispatch])) {
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

It is necessary to investigate.

```
        <critical> WARNING parentConstructor: Missing parent::__construct() call at /Users/petrmakhnev/chevere/src/Chevere/Components/Controller/Controller.php:36
        public function __construct(
                    ^^^^^^^^^^^
```

Correct.

```
        <critical> WARNING callStatic: Calling static method as instance method at /Users/petrmakhnev/chevere/src/Chevere/Components/Router/Route/RouteEndpoint.php:38
            $this->description = $method->description();
                                          ^^^^^^^^^^^

```


Patch:
diff --git a/src/cmd/embeddedrules/rules.php b/src/cmd/embeddedrules/rules.php
index ea4ed74..bcc7c0c 100644
--- a/src/cmd/embeddedrules/rules.php
+++ b/src/cmd/embeddedrules/rules.php
@@ -488,6 +488,7 @@ function emptyStringCheck() {
  * @comment Report the use of assignment in the `return` statement.
  * @before  return $a = 100;
  * @after   return $a;
+ * @disabled
  */
 function returnAssign() {
     /**
diff --git a/src/linter/report.go b/src/linter/report.go
index 18ed89f..8887255 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -59,7 +59,7 @@ return -9223372036854775808;`,
 
 		{
 			Name:     "voidResultUsed",
-			Default:  true,
+			Default:  false,
 			Quickfix: false,
 			Comment:  `Report usages of the void-type expressions`,
 			Before:   `$x = var_dump($v); // var_dump returns void.`,
@@ -1023,7 +1023,7 @@ function main(): void {
 
 		{
 			Name:     "parentNotFound",
-			Default:  true,
+			Default:  false,
 			Quickfix: false,
 			Comment:  "Report using `parent::` in a class without a parent class.",
 			Before: `class Foo {
diff --git a/src/linter/root.go b/src/linter/root.go
index 3a22029..6fc9213 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -969,6 +969,20 @@ func (d *rootWalker) enterClassConstList(list *ir.ClassConstListStmt) bool {
 }
 
 func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
+	class := d.getClass()
+	doc := phpdoctypes.Parse(meth.Doc, meth.Params, d.ctx.typeNormalizer)
+
+	if meth.MethodName.Value == "__construct" {
+		for _, p := range meth.Params {
+			param := p.(*ir.Parameter)
+			propertyPromotion := len(param.Modifiers) != 0
+
+			if propertyPromotion {
+				d.handlePropertyPromotion(param, doc, class)
+			}
+		}
+	}
+
 	nm := meth.MethodName.Value
 	_, insideInterface := d.currentClassNodeStack.Current().(*ir.InterfaceStmt)
 
@@ -980,8 +994,6 @@ 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 {
@@ -1008,7 +1020,6 @@ func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 	d.checker.CheckIdentMisspellings(meth.MethodName)
 
 	// Indexing stage.
-	doc := phpdoctypes.Parse(meth.Doc, meth.Params, d.ctx.typeNormalizer)
 	moveShapesToContext(&d.ctx, doc.Shapes)
 	d.handleClosuresFromDoc(doc.Closures)
 
@@ -1121,6 +1132,42 @@ func (d *rootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 	return false
 }
 
+func (d *rootWalker) handlePropertyPromotion(param *ir.Parameter, doc phpdoctypes.ParseResult, class meta.ClassInfo) {
+	var accessLevel meta.AccessLevel
+	for _, m := range param.Modifiers {
+		d.checker.CheckModifierKeywordCase(m)
+		switch strings.ToLower(m.Value) {
+		case "public":
+			accessLevel = meta.Public
+		case "protected":
+			accessLevel = meta.Protected
+		case "private":
+			accessLevel = meta.Private
+		}
+	}
+
+	var propType types.Map
+
+	docType, ok := doc.ParamTypes[param.Variable.Name]
+	if ok {
+		propType.Append(docType.Typ)
+	}
+	hintType, ok := d.parseTypeHintNode(param.VariableType)
+	if ok {
+		propType = propType.Append(hintType)
+	}
+
+	if param.DefaultValue != nil {
+		propType = propType.Append(solver.ExprTypeLocal(d.scope(), d.ctx.st, param.DefaultValue))
+	}
+
+	class.Properties[param.Variable.Name] = meta.PropertyInfo{
+		Pos:         d.getElementPos(param),
+		Typ:         propType.Immutable(),
+		AccessLevel: accessLevel,
+	}
+}
+
 type methodModifiers struct {
 	abstract       bool
 	static         bool


Test patch:
diff --git a/src/tests/inline/testdata/php8/propertyPromotion_any.php b/src/tests/inline/testdata/php8/propertyPromotion_any.php
new file mode 100644
index 0000000..0d5c451
--- /dev/null
+++ b/src/tests/inline/testdata/php8/propertyPromotion_any.php
@@ -0,0 +1,51 @@
+<?php
+
+class Boo {
+  /***/
+  public function f(): int {
+    return 1;
+  }
+}
+
+class Foo {
+  public $prop4;
+
+  public function __construct(public Boo $prop1, private string $prop2, int $prop3) {
+    echo $prop1->f();
+    echo $prop2;
+  }
+
+  /***/
+  public function method() {
+    echo $this->prop1->f();
+    echo $this->prop2;
+    echo $this->prop4;
+  }
+}
+
+class Goo extends Foo {}
+
+class Doo extends Goo {
+  public string $prop2;
+  public Boo $prop3;
+}
+
+function f() {
+  $foo = new Foo(new Boo, "", 10);
+  var_dump($foo->prop1);
+  var_dump($foo->prop2); // want `Cannot access private property \Foo->prop2`
+  var_dump($foo->prop3); // want `Property {\Foo}->prop3 does not exist`
+  var_dump($foo->prop4);
+
+  $goo = new Goo(new Boo, "", 10);
+  var_dump($goo->prop1);
+  var_dump($goo->prop2); // want `Cannot access private property \Foo->prop2`
+  var_dump($goo->prop3); // want `Property {\Goo}->prop3 does not exist`
+  var_dump($goo->prop4);
+
+  $doo = new Doo(new Boo, "", 10);
+  var_dump($doo->prop1);
+  var_dump($doo->prop2);
+  var_dump($doo->prop3);
+  var_dump($doo->prop4);
+}