Repository: VKCOM/noverify
Pull request #919 (2021-01-29 17:29:45)

linter: treat trigger_error($msg, E_USER_ERROR) as exit()
Since some projects may use some recovery with set_error_handler,
there is a `-ignore-trigger-error` flag to disable this behavior.


Patch:
diff --git a/src/cmd/flags.go b/src/cmd/flags.go
index 616a678..d8da748 100644
--- a/src/cmd/flags.go
+++ b/src/cmd/flags.go
@@ -179,10 +179,10 @@ func bindFlags(config *linter.Config, ruleSets []*rules.Set, args *cmdlineArgume
 	flag.StringVar(&config.StubsDir, "stubs-dir", "", "Directory with phpstorm-stubs")
 	flag.StringVar(&config.CacheDir, "cache-dir", DefaultCacheDir(), "Directory for linter cache (greatly improves indexing speed)")
 	flag.BoolVar(&args.disableCache, "disable-cache", false, "If set, cache is not used and cache-dir is ignored")
+	flag.BoolVar(&config.IgnoreTriggerError, "ignore-trigger-error", false, "If set, trigger_error control flow will be ignored")
 
 	flag.StringVar(&args.unusedVarPattern, "unused-var-regex", `^_$`,
 		"Variables that match such regexp are marked as discarded; not reported as unused, but should not be used as values")
-
 	flag.BoolVar(&args.version, "version", false, "Show version info and exit")
 
 	flag.StringVar(&args.cpuProfile, "cpuprofile", "", "Write cpu profile to `file`")
diff --git a/src/linter/block.go b/src/linter/block.go
index cd9651d..1689c27 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -1820,23 +1820,7 @@ func (b *blockWalker) LeaveNode(w ir.Node) {
 	b.path.Pop()
 
 	if b.ctx.exitFlags == 0 {
-		switch w.(type) {
-		case *ir.ReturnStmt:
-			b.ctx.exitFlags |= FlagReturn
-			b.ctx.containsExitFlags |= FlagReturn
-		case *ir.ExitExpr:
-			b.ctx.exitFlags |= FlagDie
-			b.ctx.containsExitFlags |= FlagDie
-		case *ir.ThrowStmt:
-			b.ctx.exitFlags |= FlagThrow
-			b.ctx.containsExitFlags |= FlagThrow
-		case *ir.ContinueStmt:
-			b.ctx.exitFlags |= FlagContinue
-			b.ctx.containsExitFlags |= FlagContinue
-		case *ir.BreakStmt:
-			b.ctx.exitFlags |= FlagBreak
-			b.ctx.containsExitFlags |= FlagBreak
-		}
+		b.updateExitFlags(w)
 	}
 
 	for _, c := range b.custom {
@@ -1844,6 +1828,48 @@ func (b *blockWalker) LeaveNode(w ir.Node) {
 	}
 }
 
+func (b *blockWalker) updateExitFlags(n ir.Node) {
+	switch n := n.(type) {
+	case *ir.ReturnStmt:
+		b.ctx.exitFlags |= FlagReturn
+		b.ctx.containsExitFlags |= FlagReturn
+	case *ir.ExitExpr:
+		b.ctx.exitFlags |= FlagDie
+		b.ctx.containsExitFlags |= FlagDie
+	case *ir.ThrowStmt:
+		b.ctx.exitFlags |= FlagThrow
+		b.ctx.containsExitFlags |= FlagThrow
+	case *ir.ContinueStmt:
+		b.ctx.exitFlags |= FlagContinue
+		b.ctx.containsExitFlags |= FlagContinue
+	case *ir.BreakStmt:
+		b.ctx.exitFlags |= FlagBreak
+		b.ctx.containsExitFlags |= FlagBreak
+	case *ir.ExpressionStmt:
+		b.updateExitFlags(n.Expr)
+	case *ir.FunctionCallExpr:
+		if b.r.config.IgnoreTriggerError {
+			return
+		}
+		nm, ok := n.Function.(*ir.Name)
+		if !ok {
+			return
+		}
+		// We can't use solver.GetFuncName here as PHP function names
+		// lookup requires full symbol table information => we can't use
+		// it during the indexing.
+		funcName := strings.TrimPrefix(nm.Value, `\`)
+		if (funcName != `trigger_error` && funcName != `user_error`) || len(n.Args) != 2 {
+			return
+		}
+		errorLevel, ok := n.Arg(1).Expr.(*ir.ConstFetchExpr)
+		// TODO: add meta.GetConstName() func and use it here.
+		if ok && errorLevel.Constant.Value == `E_USER_ERROR` {
+			b.ctx.exitFlags |= FlagDie
+		}
+	}
+}
+
 func (b *blockWalker) sideEffectFree(n ir.Node) bool {
 	return solver.SideEffectFree(b.ctx.sc, b.r.ctx.st, b.ctx.customTypes, n)
 }
diff --git a/src/linter/conf.go b/src/linter/conf.go
index b2726a7..734aa7a 100644
--- a/src/linter/conf.go
+++ b/src/linter/conf.go
@@ -59,6 +59,8 @@ type Config struct {
 	PhpExtensions []string
 
 	Checkers *CheckersRegistry
+
+	IgnoreTriggerError bool
 }
 
 func NewConfig() *Config {


Test patch:
diff --git a/src/tests/checkers/trigger_error_test.go b/src/tests/checkers/trigger_error_test.go
new file mode 100644
index 0000000..a5d909a
--- /dev/null
+++ b/src/tests/checkers/trigger_error_test.go
@@ -0,0 +1,68 @@
+package checkers_test
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestTriggerNonError(t *testing.T) {
+	linttest.SimpleNegativeTest(t, `<?php
+trigger_error('notice');
+trigger_error('also notice', E_USER_NOTICE);
+trigger_error('a warning', E_USER_WARNING);
+`)
+}
+
+func TestTriggerErrorFQN(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+\trigger_error('error', E_USER_ERROR);
+echo 'unreachable';
+`)
+	test.Expect = []string{
+		`Unreachable code`,
+	}
+	test.RunAndMatch()
+}
+
+func TestTriggerError(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+trigger_error('error', E_USER_ERROR);
+echo 'unreachable';
+`)
+	test.Expect = []string{
+		`Unreachable code`,
+	}
+	test.RunAndMatch()
+}
+
+func TestTriggerErrorTransitive(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+function f($msg) {
+  trigger_error($msg, E_USER_ERROR);
+}
+
+f('error');
+echo 'unreachable';
+`)
+	test.Expect = []string{
+		`Unreachable code`,
+	}
+	test.RunAndMatch()
+}
+
+func TestUserError(t *testing.T) {
+	// user_error is a trigger_error alias.
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+user_error('error', E_USER_ERROR);
+echo 'unreachable';
+`)
+	test.Expect = []string{
+		`Unreachable code`,
+	}
+	test.RunAndMatch()
+}