Repository: VKCOM/noverify
Pull request #579 (2020-07-14 10:46:46)

linter: add compliant/non-compliant code examples for every checker doc
linter.CheckerInfo gets 2 new fields: Before and After.
Before is a non-compliant code example (before the fix).
After is a compliant code example (after the fix).

A new package, lintdoc, provides utilities to render
check info in a human-readable format. It's meant to be
used in NoVerify as well is in custom NoVerify-based tools
that want to work with checks metadata.

Fixes #579 

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/lintdoc/lintdoc.go b/src/lintdoc/lintdoc.go
new file mode 100644
index 0000000..d614b91
--- /dev/null
+++ b/src/lintdoc/lintdoc.go
@@ -0,0 +1,34 @@
+package lintdoc
+
+import (
+	"io"
+	"text/template"
+
+	"github.com/VKCOM/noverify/src/linter"
+)
+
+// RenderCheckDocumentation pretty-prints info to the provided writer.
+func RenderCheckDocumentation(w io.Writer, info linter.CheckInfo) error {
+	if info.Before == "" {
+		return templateShort.Execute(w, info)
+	}
+	return templateFull.Execute(w, info)
+}
+
+var templateShort = template.Must(template.New("short").Parse(`
+{{- .Name}} checker documentation
+
+{{.Comment -}}
+`))
+
+var templateFull = template.Must(template.New("short").Parse(`
+{{- .Name}} checker documentation
+
+{{.Comment}}
+
+Non-compliant code:
+{{.Before}}
+
+Compliant code:
+{{.After -}}
+`))
diff --git a/src/linter/custom.go b/src/linter/custom.go
index 755f88f..6d187d0 100644
--- a/src/linter/custom.go
+++ b/src/linter/custom.go
@@ -56,6 +56,14 @@ type CheckInfo struct {
 	// Comment is a short summary of what this diagnostic does.
 	// A single descriptive sentence is a perfect format for it.
 	Comment string
+
+	// Before is a non-compliant code example (before the fix).
+	// Optional, but if present, After should also be non-empty.
+	Before string
+
+	// After is a compliant code example (after the fix).
+	// Optional, but if present, Before should also be non-empty.
+	After string
 }
 
 // BlockChecker is a custom linter that is called on block level
@@ -294,6 +302,12 @@ func DeclareCheck(info CheckInfo) {
 	if _, ok := checksInfoRegistry[info.Name]; ok {
 		panic(fmt.Sprintf("check %q already declared", info.Name))
 	}
+	if info.Before != "" && info.After == "" {
+		panic(fmt.Sprintf("%s: Before is set, but After is empty", info.Name))
+	}
+	if info.After != "" && info.Before == "" {
+		panic(fmt.Sprintf("%s: After is set, but Before is empty", info.Name))
+	}
 	checksInfoRegistry[info.Name] = info
 }
 
diff --git a/src/linter/report.go b/src/linter/report.go
index 5dce6e8..e3d48bb 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -22,138 +22,243 @@ func init() {
 			Name:    "discardExpr",
 			Default: true,
 			Comment: `Report expressions that are evaluated but not used.`,
+			Before: `if ($cond) {
+  [$v, $err];
+}`,
+			After: `if ($cond) {
+  return [$v, $err];
+}`,
 		},
 
 		{
 			Name:    "precedence",
 			Default: true,
 			Comment: `Report potential operation precedence issues.`,
+			Before:  `$x & $mask == 0; // == has higher precedence than &`,
+			After:   `($x & $mask) == 0`,
 		},
 
 		{
 			Name:    "voidResultUsed",
 			Default: true,
 			Comment: `Report usages of the void-type expressions`,
+			Before:  `$x = var_dump($v); // var_dump returns void`,
+			After:   `$x = print_r($v, true);`,
 		},
 
 		{
 			Name:    "keywordCase",
 			Default: true,
 			Comment: `Report keywords that are not in the lower case.`,
+			Before:  `RETURN $x;`,
+			After:   `return $x;`,
 		},
 
 		{
 			Name:    "accessLevel",
 			Default: true,
 			Comment: `Report erroneous member access.`,
+			Before:  `$x->_run(); // _run() is private, can't be accessed`,
+			After:   `$x->run(); // run() is public`,
 		},
 
 		{
 			Name:    "argCount",
 			Default: true,
 			Comment: `Report mismatching args count inside call expressions.`,
+			Before:  `array_combine($keys)`,
+			After:   `array_combine($keys, $values)`,
 		},
 
 		{
 			Name:    "redundantGlobal",
 			Default: true,
 			Comment: `Report global statement over superglobal variables (which is redundant).`,
+			Before:  `global $Foo, $_GET; // $_GET is superglobal`,
+			After:   `global $Foo;`,
 		},
 
 		{
 			Name:    "arrayAccess",
 			Default: true,
 			Comment: `Report array access to non-array objects.`,
+			Before:  `return $foo[0]; // $foo value may not implement ArrayAccess`,
+			After:   `if ($foo instanceof ArrayAccess) { return $foo[0]; }`,
 		},
 
 		{
 			Name:    "bitwiseOps",
 			Default: true,
 			Comment: `Report suspicious usage of bitwise operations.`,
+			Before:  `if ($isURL & $verify) ...`,
+			After:   `if ($isURL && $veriry) ...`,
 		},
 
 		{
 			Name:    "mixedArrayKeys",
 			Default: true,
 			Comment: `Report array literals that have both implicit and explicit keys.`,
+			Before:  `['a', 5 => 'b']`,
+			After:   `[0 => 'a', 5 => 'b']`,
 		},
 
 		{
 			Name:    "dupArrayKeys",
 			Default: true,
 			Comment: `Report duplicated keys in array literals.`,
+			Before:  `[A => 1, B => 2, A => 3]`,
+			After:   `[A => 1, B => 2, C => 3]`,
 		},
 
 		{
 			Name:    "dupCond",
 			Default: true,
 			Comment: `Report duplicated conditions in switch and if/else statements.`,
+			Before: `if ($status == OK) {
+  return "OK";
+} elseif ($status == OK) { // Duplicated condition
+  return "NOT OK";
+} else {
+  return "UNKNOWN";
+}`,
+			After: `if ($status == OK) {
+  return "OK";
+} elseif ($status == NOT_OK) {
+  return "NOT OK";
+} else {
+  return "UNKNOWN";
+}`,
 		},
 
 		{
 			Name:    "dupBranchBody",
 			Default: true,
 			Comment: `Report suspicious conditional branches that execute the same action.`,
+			Before:  `$pickLeft ? foo($left) : foo($left)`,
+			After:   `$pickLeft ? foo($left) : foo($right)`,
 		},
 
 		{
 			Name:    "dupSubExpr",
 			Default: true,
 			Comment: `Report suspicious duplicated operands in expressions.`,
+			Before:  `return $x[$i] < $x[$i]; // $x[$i] is duplicated`,
+			After:   `return $x[$i] < $x[$j];`,
 		},
 
 		{
 			Name:    "arraySyntax",
 			Default: true,
 			Comment: `Report usages of old array() syntax.`,
+			Before:  `array(1, 2)`,
+			After:   `[1, 2]`,
 		},
 
 		{
 			Name:    "bareTry",
 			Default: true,
 			Comment: `Report try blocks without catch/finally.`,
+			Before: `try {
+  doit();
+}`,
+			After: `try {
+  doit();
+} catch (Exception $e) {
+  // Handle $e.
+}`,
 		},
 
 		{
 			Name:    "caseBreak",
 			Default: true,
 			Comment: `Report switch cases without break.`,
+			Before: `switch ($v) {
+case 1:
+  echo "one"; // May want to insert a "break" here
+case 2:
+  echo "this fallthrough is intentional";
+  // fallthrough
+case 3:
+  echo "two or three";
+}`,
+			After: `switch ($v) {
+case 1:
+  echo "one";
+  break;
+case 2:
+  echo "this fallthrough is intentional";
+  // fallthrough
+case 3:
+  echo "two or three";
+}`,
 		},
 
 		{
 			Name:    "complexity",
 			Default: true,
 			Comment: `Report funcs/methods that are too complex.`,
+			Before: `function checkRights() {
+  // Super big function.
+}`,
+			After: `function checkRights() {
+  return true; // Or 42 if you need int-typed result
+}`,
 		},
 
 		{
 			Name:    "deadCode",
 			Default: true,
 			Comment: `Report potentially unreachable code.`,
+			Before: `
+thisFunctionExits(); // Always exits
+foo(); // Dead code`,
+			After: `
+foo();
+thisFunctionExits();`,
 		},
 
 		{
 			Name:    "phpdocLint",
 			Default: true,
 			Comment: `Report malformed phpdoc comments.`,
+			Before:  `@property $foo`,
+			After:   `@property Foo $foo`,
 		},
 
 		{
 			Name:    "phpdocType",
 			Default: true,
 			Comment: `Report potential issues in phpdoc types.`,
+			Before:  `@var []int $xs`,
+			After:   `@var int[] $xs`,
 		},
 
 		{
 			Name:    "phpdocRef",
 			Default: true,
 			Comment: `Report invalid symbol references inside phpdoc.`,
+			Before:  `@see MyClass`,
+			After:   `@see \Foo\MyClass`,
 		},
 
 		{
 			Name:    "phpdoc",
 			Default: true,
 			Comment: `Report missing phpdoc on public methods.`,
+			Before: `public function process($acts, $config) {
+  // Does something very complicated.
+}`,
+			After: `
+/**
+ * process executes all $acts in a new context.
+ * Processed $acts should never be processed again.
+ *
+ * @param Act[] $acts - acts to execute
+ * @param array $config - options
+ */
+public function process($acts, $config) {
+  // Does something very complicated.
+}`,
 		},
 
 		{
@@ -166,48 +271,78 @@ func init() {
 			Name:    "unimplemented",
 			Default: true,
 			Comment: `Report classes that don't implement their contract.`,
+			Before: `class MyObj implements Serializable {
+  public function serialize() { /* ... */ }
+}`,
+			After: `class MyObj implements Serializable {
+  public function serialize() { /* ... */ }
+  public function unserialize(string $s) { /* ... */ }
+}`,
 		},
 
 		{
 			Name:    "syntax",
 			Default: true,
 			Comment: `Report syntax errors.`,
+			Before:  `foo(1]`,
+			After:   `foo(1)`,
 		},
 
 		{
 			Name:    "undefined",
 			Default: true,
 			Comment: `Report usages of potentially undefined symbols.`,
+			Before: `if ($cond) {
+  $v = 10;
+}
+return $v; // $v may be undefined`,
+			After: `$v = 0; // Default value
+if ($cond) {
+  $v = 10;
+}
+return $v;`,
 		},
 
 		{
 			Name:    "unused",
 			Default: true,
 			Comment: `Report potentially unused variables.`,
+			Before: `$result = calculateResult(); // Unused $result
+return [$err];`,
+			After: `$result = calculateResult();
+return [$result, $err];`,
 		},
 
 		{
 			Name:    "redundantCast",
 			Default: false,
 			Comment: `Report redundant type casts.`,
+			Before:  `return (int)10;`,
+			After:   `return 10;`,
 		},
 
 		{
 			Name:    "newAbstract",
 			Default: true,
 			Comment: `Report abstract classes usages in new expressions.`,
+			Before:  `return new AbstractFactory();`,
+			After:   `return new NonAbstractFactory();`,
 		},
 
 		{
 			Name:    "regexpSimplify",
 			Default: true,
 			Comment: `Report regular expressions that can be simplified.`,
+			Before:  `preg_match('/x(?:a|b|c){0,}/', $s)`,
+			After:   `preg_match('/x[abc]*/', $s)`,
 		},
 
 		{
 			Name:    "regexpVet",
 			Default: true,
 			Comment: `Report suspicious regexp patterns.`,
+			Before:  `preg_match('a\d+a', $s); // 'a' is not a valid delimiter`,
+			After:   `preg_match('/\d+/', $s);`,
 		},
 
 		{
@@ -220,48 +355,101 @@ func init() {
 			Name:    "caseContinue",
 			Default: true,
 			Comment: `Report suspicious 'continue' usages inside switch cases.`,
+			Before: `switch ($v) {
+case STOP:
+  continue; // This acts like a "break"
+case INC:
+  $x++;
+  break;
+}`,
+			After: `switch ($v) {
+case STOP:
+  break;
+case INC:
+  $x++;
+  break;
+}`,
 		},
 
 		{
 			Name:    "deprecated",
 			Default: false, // Experimental
 			Comment: `Report usages of deprecated symbols.`,
+			Before:  `ereg($pat, $s)`,
+			After:   `preg_match($pat, $s)`,
 		},
 
 		{
 			Name:    "callStatic",
 			Default: true,
 			Comment: `Report static calls of instance methods and vice versa.`,
+			Before:  `$object::instance_method()`,
+			After:   `$object->instance_method()`,
 		},
 
 		{
 			Name:    "parentConstructor",
 			Default: true,
 			Comment: `Report missing parent::__construct calls in class constructors.`,
+			Before: `class Foo extends Bar {
+  public function __construct($x, $y) {
+    $this->y = $y;
+  }
+}`,
+			After: `class Foo extends Bar {
+  public function __construct($x, $y) {
+    parent::__construct($x);
+    $this->y = $y;
+  }
+}`,
 		},
 
 		{
 			Name:    "oldStyleConstructor",
 			Default: true,
 			Comment: `Report old-style (PHP4) class constructors.`,
+			Before: `class Foo {
+  public function Foo($v) { $this->v = $v; }
+}`,
+			After: `class Foo {
+  public function __construct($v) { $this->v = $v; }
+}`,
 		},
 
 		{
 			Name:    "misspellName",
 			Default: true,
 			Comment: `Report commonly misspelled words in symbol names.`,
+			Before:  `function performace_test() ...`,
+			After:   `function performance_test() ...`,
 		},
 
 		{
 			Name:    "misspellComment",
 			Default: true,
 			Comment: `Report commonly misspelled words in comments.`,
+			Before: `/** This is our performace test. */
+function performance_test() {}`,
+			After: `/** This is our performance test. */
+function performance_test() {}`,
 		},
 
 		{
 			Name:    "nonPublicInterfaceMember",
 			Default: true,
 			Comment: `Report illegal non-public access level in interfaces.`,
+			Before: `interface Iface {
+  function a(); // OK, public implied
+  public function b(); // OK, public
+  private function c();
+  protected function d();
+}`,
+			After: `interface Iface {
+  function a(); // OK, public implied
+  public function b(); // OK, public
+  public function c();
+  public function d();
+}`,
 		},
 	}
 


Test patch:
diff --git a/src/lintdoc/lintdoc_test.go b/src/lintdoc/lintdoc_test.go
new file mode 100644
index 0000000..568929e
--- /dev/null
+++ b/src/lintdoc/lintdoc_test.go
@@ -0,0 +1,65 @@
+package lintdoc
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linter"
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestRenderCheckDocumentation(t *testing.T) {
+	runTest := func(info linter.CheckInfo, expect string) {
+		t.Helper()
+		var buf strings.Builder
+		RenderCheckDocumentation(&buf, info)
+		if diff := cmp.Diff(buf.String(), expect); diff != "" {
+			t.Errorf("%#v:\n%s", info, diff)
+		}
+	}
+
+	runTest(linter.CheckInfo{
+		Name:    "shortExample",
+		Comment: "Report nothing, but test short info rendering.",
+	}, `shortExample checker documentation
+
+Report nothing, but test short info rendering.`)
+
+	runTest(linter.CheckInfo{
+		Name:    "fullExample",
+		Comment: "Report nothing, but test full info rendering.",
+		Before:  `ereg($pat, $s)`,
+		After:   `preg_match($pat, $s)`,
+	}, `fullExample checker documentation
+
+Report nothing, but test full info rendering.
+
+Non-compliant code:
+ereg($pat, $s)
+
+Compliant code:
+preg_match($pat, $s)`)
+
+	runTest(linter.CheckInfo{
+		Name:    "fullExampleMultiLine",
+		Comment: "Report nothing, but test full info rendering.",
+		Before: `class Foo {
+  public function Foo($v) { $this->v = $v; }
+}`,
+		After: `class Foo {
+  public function __construct($v) { $this->v = $v; }
+}`,
+	}, `fullExampleMultiLine checker documentation
+
+Report nothing, but test full info rendering.
+
+Non-compliant code:
+class Foo {
+  public function Foo($v) { $this->v = $v; }
+}
+
+Compliant code:
+class Foo {
+  public function __construct($v) { $this->v = $v; }
+}`)
+}