Repository: VKCOM/noverify
Pull request #1167 (2021-11-01 09:46:47)

linter: added initial support for `@noverify-suppress` annotations
The syntax for the annotation is as follows:

```php
/** @noverify-suppress <checker-name> or all */
```

If `all` is specified, then all warnings in the expression or 
statement inside the function will be suppressed.

This annotation can only be used for expressions in the global 
scope and for expressions and statements in the function scope.



YuriyNasretdinov: If you are going to do this (which I don't agree that is a good idea, but that's a separate discussion), I would suggest to make syntax compatible with PHPStorm and other linters as much as possible.
i582: > If you are going to do this (which I don't agree that is a good idea, but that's a separate discussion), I would suggest to make syntax compatible with PHPStorm and other linters as much as possible.

As for compatibility, everything is rather complicated, because both PHPStorm and Psalm use their own check names and their annotations, it seems impossible to make it compatible with them.

As for the need... I kind of agree that this is not a good idea, but as a default linter feature it's not bad. In the our code it will be allowed only for a few checks, which often break push when refactoring (for example, the `deprecated` check).

Patch:
diff --git a/src/ir/irconv/utils.go b/src/ir/irconv/utils.go
index 8c79ec2..01cd13d 100644
--- a/src/ir/irconv/utils.go
+++ b/src/ir/irconv/utils.go
@@ -44,9 +44,11 @@ func namePartsToToken(parts []ast.Vertex) *token.Token {
 		switch v := v.(type) {
 		case *ast.NamePart:
 			ff = append(ff, v.StringTkn)
+			ff = append(ff, v.StringTkn.FreeFloating...)
 			valueParts = append(valueParts, v.Value)
 		case *ast.ScalarEncapsedStringPart:
 			ff = append(ff, v.EncapsedStrTkn)
+			ff = append(ff, v.EncapsedStrTkn.FreeFloating...)
 			valueParts = append(valueParts, v.Value)
 		}
 	}
diff --git a/src/linter/block.go b/src/linter/block.go
index 0dc2224..ee1401c 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -5,11 +5,12 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/VKCOM/noverify/src/phpdoctypes"
-	"github.com/VKCOM/noverify/src/utils"
 	"github.com/VKCOM/php-parser/pkg/position"
 	"github.com/VKCOM/php-parser/pkg/token"
 
+	"github.com/VKCOM/noverify/src/phpdoctypes"
+	"github.com/VKCOM/noverify/src/utils"
+
 	"github.com/VKCOM/noverify/src/ir"
 	"github.com/VKCOM/noverify/src/ir/irutil"
 	"github.com/VKCOM/noverify/src/linter/autogen"
@@ -124,6 +125,30 @@ func newBlockWalker(r *rootWalker, sc *meta.Scope) *blockWalker {
 	return b
 }
 
+func (b *blockWalker) report(n ir.Node, level int, checkName, msg string, args ...interface{}) {
+	if b.isSuppressed(n, checkName) {
+		return
+	}
+
+	b.r.Report(n, level, checkName, msg, args...)
+}
+
+func (b *blockWalker) isSuppressed(n ir.Node, checkName string) bool {
+	if containLinterSuppress(n, checkName) {
+		return true
+	}
+
+	// We go up the tree in search of a comment that disables this checker.
+	for i := 0; b.path.NthParent(i) != nil; i++ {
+		parent := b.path.NthParent(i)
+		if containLinterSuppress(parent, checkName) {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (b *blockWalker) isIndexingComplete() bool {
 	return b.r.ctx.st.Info.IsIndexingComplete()
 }
@@ -175,7 +200,47 @@ func (b *blockWalker) reportDeadCode(n ir.Node) {
 	}
 
 	b.ctx.deadCodeReported = true
-	b.r.Report(n, LevelWarning, "deadCode", "Unreachable code")
+	b.report(n, LevelWarning, "deadCode", "Unreachable code")
+}
+
+func containLinterSuppress(n ir.Node, needInspection string) bool {
+	if n == nil {
+		return false
+	}
+
+	phpdocTypeParser := phpdoc.NewTypeParser()
+	firstTkn := ir.GetFirstToken(n)
+	if firstTkn == nil {
+		return false
+	}
+
+	for _, tkn := range firstTkn.FreeFloating {
+		if !phpdoc.IsPHPDocToken(tkn) {
+			continue
+		}
+
+		if !bytes.Contains(tkn.Value, []byte("@noverify-suppress")) {
+			continue
+		}
+
+		parsed := phpdoc.Parse(phpdocTypeParser, string(tkn.Value))
+		for _, p := range parsed.Parsed {
+			part, ok := p.(*phpdoc.RawCommentPart)
+			if !ok {
+				continue
+			}
+
+			if part.Name() == "noverify-suppress" {
+				inspection := part.Params[0]
+
+				if inspection == "all" || inspection == needInspection {
+					return true
+				}
+			}
+		}
+	}
+
+	return false
 }
 
 func (b *blockWalker) containsDisableInspection(n ir.Node, needInspection string) bool {
@@ -456,11 +521,11 @@ func (b *blockWalker) checkDupGlobal(s *ir.GlobalStmt) {
 		// Check whether this var was already global'ed.
 		// We use nonLocalVars for function-wide analysis and vars for local analysis.
 		if _, ok := vars[nm]; ok {
-			b.r.Report(v, LevelWarning, "dupGlobal", "Global statement mentions $%s more than once", nm)
+			b.report(v, LevelWarning, "dupGlobal", "Global statement mentions $%s more than once", nm)
 		} else {
 			vars[nm] = struct{}{}
 			if b.nonLocalVars[nm] == varGlobal {
-				b.r.Report(v, LevelNotice, "dupGlobal", "$%s already global'ed above", nm)
+				b.report(v, LevelNotice, "dupGlobal", "$%s already global'ed above", nm)
 			}
 		}
 	}
@@ -818,7 +883,7 @@ func (b *blockWalker) checkUnreachableForFinallyReturn(tryStmt *ir.TryStmt, tryC
 	}
 
 	if catchContainsDie {
-		b.r.Report(tryStmt.Finally, LevelError, "deadCode", "Block finally is unreachable (because catch block %d contains a exit/die)", catchWithDieIndex)
+		b.report(tryStmt.Finally, LevelError, "deadCode", "Block finally is unreachable (because catch block %d contains a exit/die)", catchWithDieIndex)
 
 		// If there is an error when the finally block is unreachable,
 		// then errors due to return in finally are skipped.
@@ -833,7 +898,7 @@ func (b *blockWalker) checkUnreachableForFinallyReturn(tryStmt *ir.TryStmt, tryC
 		}
 
 		for _, point := range exitPoints {
-			b.r.Report(point.n, LevelError, "deadCode", "%s is unreachable (because finally block contains a return on line %d)", point.kind, finallyReturnPos.StartLine)
+			b.report(point.n, LevelError, "deadCode", "%s is unreachable (because finally block contains a return on line %d)", point.kind, finallyReturnPos.StartLine)
 		}
 	}
 }
@@ -1158,7 +1223,7 @@ func (b *blockWalker) handleForeach(s *ir.ForeachStmt) bool {
 
 		b.untrackVarName(key.Name)
 
-		b.r.Report(s.Key, LevelWarning, "unused", "Foreach key $%s is unused, can simplify $%s => $%s to just $%s", key.Name, key.Name, variable.Name, variable.Name)
+		b.report(s.Key, LevelWarning, "unused", "Foreach key $%s is unused, can simplify $%s => $%s to just $%s", key.Name, key.Name, variable.Name, variable.Name)
 	}
 
 	return false
@@ -1258,7 +1323,7 @@ func (b *blockWalker) enterClosure(fun *ir.ClosureExpr, haveThis bool, thisType
 		}
 
 		if !b.ctx.sc.HaveVar(v) && !byRef {
-			b.r.Report(v, LevelWarning, "undefinedVariable", "Cannot find referenced variable $%s", v.Name)
+			b.report(v, LevelWarning, "undefinedVariable", "Cannot find referenced variable $%s", v.Name)
 		}
 
 		typ, ok := b.ctx.sc.GetVarNameType(v.Name)
@@ -1397,7 +1462,7 @@ func (b *blockWalker) handleVariable(v ir.Node) bool {
 			}
 		}
 		if b.r.config.IsDiscardVar(varName) && !isSuperGlobal(varName) {
-			b.r.Report(v, LevelError, "discardVar", "Used var $%s that is supposed to be unused (rename variable if it's intended or respecify --unused-var-regex flag)", varName)
+			b.report(v, LevelError, "discardVar", "Used var $%s that is supposed to be unused (rename variable if it's intended or respecify --unused-var-regex flag)", varName)
 		}
 
 		b.untrackVarName(varName)
@@ -1783,7 +1848,7 @@ func (b *blockWalker) handleSwitch(s *ir.SwitchStmt) bool {
 				// allow the fallthrough if appropriate comment is present
 				nextCase := s.Cases[idx+1]
 				if !caseHasFallthroughComment(nextCase) {
-					b.r.Report(c, LevelWarning, "caseBreak", "Add break or '// fallthrough' to the end of the case")
+					b.report(c, LevelWarning, "caseBreak", "Add break or '// fallthrough' to the end of the case")
 				}
 			}
 
@@ -1914,7 +1979,7 @@ func (b *blockWalker) checkArrayDimFetch(s *ir.ArrayDimFetchExpr) {
 	})
 
 	if maybeHaveClasses && !haveArrayAccess {
-		b.r.Report(s.Variable, LevelNotice, "arrayAccess", "Array access to non-array type %s", typ)
+		b.report(s.Variable, LevelNotice, "arrayAccess", "Array access to non-array type %s", typ)
 	}
 }
 
@@ -2029,7 +2094,7 @@ func (b *blockWalker) paramClobberCheck(v *ir.SimpleVar) {
 		return
 	}
 	if _, ok := b.unusedParams[v.Name]; ok && !b.path.Conditional() {
-		b.r.Report(v, LevelWarning, "paramClobber", "Param $%s re-assigned before being used", v.Name)
+		b.report(v, LevelWarning, "paramClobber", "Param $%s re-assigned before being used", v.Name)
 	}
 }
 
@@ -2191,7 +2256,7 @@ func (b *blockWalker) flushUnused() {
 			}
 
 			visitedMap[n] = struct{}{}
-			b.r.Report(n, LevelWarning, "unused", `Variable $%s is unused (use $_ to ignore this inspection or specify --unused-var-regex flag)`, name)
+			b.report(n, LevelWarning, "unused", `Variable $%s is unused (use $_ to ignore this inspection or specify --unused-var-regex flag)`, name)
 		}
 	}
 }
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index a6d9bdc..c95cd4b 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -287,7 +287,7 @@ func (b *blockLinter) checkTypeCaseExpr(n *ir.TypeCastExpr) {
 }
 
 func (b *blockLinter) report(n ir.Node, level int, checkName, msg string, args ...interface{}) {
-	b.walker.r.Report(n, level, checkName, msg, args...)
+	b.walker.report(n, level, checkName, msg, args...)
 }
 
 func (b *blockLinter) checkNopStmt(n *ir.NopStmt) {
@@ -510,7 +510,7 @@ func (b *blockLinter) checkNew(e *ir.NewExpr) {
 		if ok {
 			b.report(e.Class, LevelError, "invalidNew", "Cannot instantiate trait %s", class.Name)
 		} else {
-			b.walker.r.checker.ReportUndefinedClass(e.Class, className)
+			b.report(e.Class, LevelError, "undefinedClass", "Class or interface named %s does not exist", className)
 		}
 	} else {
 		if class.IsInterface() {
@@ -1325,11 +1325,11 @@ func (b *blockLinter) checkClassConstFetch(e *ir.ClassConstFetchExpr) {
 	}
 
 	if !fetch.isFound && !b.classParseState().IsTrait {
-		b.walker.r.Report(e.ConstantName, LevelError, "undefinedConstant", "Class constant %s::%s does not exist", fetch.className, fetch.constName)
+		b.report(e.ConstantName, LevelError, "undefinedConstant", "Class constant %s::%s does not exist", fetch.className, fetch.constName)
 	}
 
 	if fetch.isFound && !canAccess(b.classParseState(), fetch.implClassName, fetch.info.AccessLevel) {
-		b.walker.r.Report(e.ConstantName, LevelError, "accessLevel", "Cannot access %s constant %s::%s", fetch.info.AccessLevel, fetch.implClassName, fetch.constName)
+		b.report(e.ConstantName, LevelError, "accessLevel", "Cannot access %s constant %s::%s", fetch.info.AccessLevel, fetch.implClassName, fetch.constName)
 	}
 }
 
@@ -1350,7 +1350,7 @@ func (b *blockLinter) checkClassSpecialNameCase(n ir.Node, className string) {
 
 	for _, name := range names {
 		if strings.EqualFold(className, name) {
-			b.walker.r.Report(n, LevelNotice, "nameMismatch", "%s should be spelled as %s", strings.TrimPrefix(className, `\`), strings.TrimPrefix(name, `\`))
+			b.report(n, LevelNotice, "nameMismatch", "%s should be spelled as %s", strings.TrimPrefix(className, `\`), strings.TrimPrefix(name, `\`))
 		}
 	}
 }
diff --git a/src/linter/root.go b/src/linter/root.go
index 0a3be47..feafef3 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -9,11 +9,12 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/VKCOM/php-parser/pkg/position"
+	"github.com/VKCOM/php-parser/pkg/token"
+
 	"github.com/VKCOM/noverify/src/attributes"
 	"github.com/VKCOM/noverify/src/phpdoctypes"
 	"github.com/VKCOM/noverify/src/utils"
-	"github.com/VKCOM/php-parser/pkg/position"
-	"github.com/VKCOM/php-parser/pkg/token"
 
 	"github.com/VKCOM/noverify/src/baseline"
 	"github.com/VKCOM/noverify/src/constfold"


Test patch:
diff --git a/src/tests/checkers/linter_suppress_test.go b/src/tests/checkers/linter_suppress_test.go
new file mode 100644
index 0000000..9e1aa6b
--- /dev/null
+++ b/src/tests/checkers/linter_suppress_test.go
@@ -0,0 +1,148 @@
+package checkers
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestLinterSuppressDeprecationCall(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+/**
+ * @deprecated some
+ */
+function f($a) {
+  return 0;
+}
+
+function g($b) {
+  /**
+   * @noverify-suppress deprecated
+   */
+  echo f(
+    f(10)
+  );
+}
+
+/**
+ * @noverify-suppress deprecated
+ */
+echo f(10);
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressAll(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function g($b) {
+  /** @noverify-suppress all */
+  $a = $b && $b;
+}
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressUndefinedClass(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+/** @noverify-suppress undefinedClass */
+echo new Foo;
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressNotAll(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function g($b) {
+  /** @noverify-suppress dupSubExpr */
+  $a = $b && $b;
+}
+`)
+	test.Expect = []string{
+		`Variable $a is unused`,
+	}
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressDeadCode(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f($b) {
+  return $b;
+
+  /**
+   * @noverify-suppress deadCode
+   */
+  return 10;
+}
+
+function f($b) {
+  return $b;
+
+  /** @noverify-suppress deadCode */
+  return 10;
+}
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressWarningInGlobalScope(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+$a = 100;
+
+/**
+ * @noverify-suppress all
+ */
+echo $a && $a;
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterSuppressWarnings(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+class Foo {
+  private string $a = "hello";
+  /** */
+  public function f() {}
+}
+
+function g($b) {
+  /** @noverify-suppress paramClobber */
+  $b = 100;
+
+  /** @noverify-suppress dupSubExpr */
+  echo $b && $b;
+
+  /** @noverify-suppress dupBranchBody */
+  if ($b) {
+    echo 1;
+  } else {
+    echo 1;
+  }
+
+  /** @noverify-suppress accessLevel */
+  echo (new Foo)->a;
+
+  /** @noverify-suppress undefinedMethod */
+  echo (new Foo)->undefined();
+
+  /** @noverify-suppress mixedArrayKeys */
+  echo [10, 20, "some" => 30];
+
+  /** @noverify-suppress all */
+  switch ($b) {
+    case 1:
+      echo 1;
+      break;
+  }
+
+  return $b;
+}
+`)
+	test.RunAndMatch()
+}