Repository: VKCOM/noverify
Pull request #926 (2021-02-11 11:43:47)

linter: add emptyStmt checker
Report redundant empty empty statements (nop statements).


Patch:
diff --git a/src/ir/node_types.go b/src/ir/node_types.go
index b6a5687..e98060f 100644
--- a/src/ir/node_types.go
+++ b/src/ir/node_types.go
@@ -1438,6 +1438,7 @@ type NamespaceStmt struct {
 
 // NopStmt is a `;` statement.
 // It's also known as "empty statement".
+// It could also be a `?>` (script closing marker).
 type NopStmt struct {
 	FreeFloating freefloating.Collection
 	Position     *position.Position
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index 61b3327..6e40520 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -180,6 +180,9 @@ func (b *blockLinter) enterNode(n ir.Node) {
 	case *ir.InterfaceStmt:
 		b.checkInterfaceStmt(n)
 
+	case *ir.NopStmt:
+		b.checkNopStmt(n)
+
 	case *ir.BadString:
 		b.report(n, LevelError, "syntax", "%s", n.Error)
 	}
@@ -189,6 +192,24 @@ func (b *blockLinter) report(n ir.Node, level int, checkName, msg string, args .
 	b.walker.r.Report(n, level, checkName, msg, args...)
 }
 
+func (b *blockLinter) checkNopStmt(n *ir.NopStmt) {
+	switch b.walker.path.Parent().(type) {
+	case *ir.DeclareStmt, *ir.IfStmt, *ir.ForStmt, *ir.ForeachStmt, *ir.WhileStmt, *ir.DoStmt:
+		return
+	}
+
+	// Check whether it's `?>`; it it is, ignore it.
+	if ffs := n.GetFreeFloating(); ffs != nil {
+		for _, tok := range (*ffs)[freefloating.SemiColon] {
+			if tok.StringType == freefloating.TokenType && strings.HasPrefix(tok.Value, "?>") {
+				return
+			}
+		}
+	}
+
+	b.report(n, LevelNotice, "emptyStmt", "semicolon (;) is not needed here, it can be safely removed")
+}
+
 func (b *blockLinter) checkCoalesceExpr(n *ir.CoalesceExpr) {
 	lhsType := solver.ExprType(b.walker.ctx.sc, b.classParseState(), n.Left)
 	if !lhsType.IsPrecise() {
diff --git a/src/linter/report.go b/src/linter/report.go
index ffb1951..e6a3db2 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -16,6 +16,15 @@ const (
 
 func addBuiltinCheckers(reg *CheckersRegistry) {
 	allChecks := []CheckerInfo{
+		{
+			Name:     "emptyStmt",
+			Default:  true,
+			Quickfix: false,
+			Comment:  `Report redundant empty statements that can be safely removed.`,
+			Before:   `echo $foo;;`,
+			After:    `echo $foo;`,
+		},
+
 		{
 			Name:     "intOverflow",
 			Default:  true,


Test patch:
diff --git a/src/tests/checkers/emptystmt_test.go b/src/tests/checkers/emptystmt_test.go
new file mode 100644
index 0000000..cc99cbd
--- /dev/null
+++ b/src/tests/checkers/emptystmt_test.go
@@ -0,0 +1,98 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestEmptyStmtBadComment(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+/** @var $foo int */;
+global $foo;
+`)
+	test.Expect = []string{
+		`semicolon (;) is not needed here, it can be safely removed`,
+	}
+	test.RunAndMatch()
+}
+
+func TestEmptyStmtBadStmtEnd(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+if ($argv) {
+};
+while ($argv) {
+};
+class Foo {};
+`)
+	test.Expect = []string{
+		`semicolon (;) is not needed here, it can be safely removed`,
+		`semicolon (;) is not needed here, it can be safely removed`,
+		`semicolon (;) is not needed here, it can be safely removed`,
+	}
+	test.RunAndMatch()
+}
+
+func TestEmptyStmtBadRequire1(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+require_once 'foo.php';;
+`)
+	test.Expect = []string{
+		`semicolon (;) is not needed here, it can be safely removed`,
+	}
+	test.RunAndMatch()
+}
+
+func TestEmptyStmtBadRequire2(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+require_once 'foo.php'; ; ;
+`)
+	test.Expect = []string{
+		`semicolon (;) is not needed here, it can be safely removed`,
+		`semicolon (;) is not needed here, it can be safely removed`,
+	}
+	test.RunAndMatch()
+}
+
+func TestEmptyStmtBadStmtSeparator(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+echo 1;
+; echo 2;
+echo 3; ;
+`)
+	test.Expect = []string{
+		`semicolon (;) is not needed here, it can be safely removed`,
+		`semicolon (;) is not needed here, it can be safely removed`,
+	}
+	test.RunAndMatch()
+}
+
+func TestEmptyStmtScriptEnd1(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+function f() {}
+?>
+`)
+}
+
+func TestEmptyStmtScriptEnd2(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+echo 123;
+?>
+`)
+}
+
+func TestEmptyStmtGood(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+if ($argv) ;
+while ($argv) ;
+for (;;) ;
+foreach ($argv as $_) ;
+do ; while ($argv);
+declare (strict_types=1);
+`)
+}
diff --git a/src/tests/checkers/oop_test.go b/src/tests/checkers/oop_test.go
index b5ce255..0a41fc9 100644
--- a/src/tests/checkers/oop_test.go
+++ b/src/tests/checkers/oop_test.go
@@ -1421,8 +1421,8 @@ func TestGroupUse(t *testing.T) {
 	test.AddFile(`<?php
 namespace Test;
 
-class TestClass {};
-class TestClass2 {};
+class TestClass {}
+class TestClass2 {}
 
 function testFunction() {}
 function testFunction2() {}
@@ -1431,8 +1431,8 @@ function testFunction2() {}
 	test.AddFile(`<?php
 namespace Test\Something;
 
-class TestSomethingClass {};
-class TestSomethingClass2 {};
+class TestSomethingClass {}
+class TestSomethingClass2 {}
 
 function testSomethingFunction() {}
 function testSomethingFunction2() {}