Repository: VKCOM/noverify
Pull request #1217 (2024-03-22 13:57:55)

linter: new rule for dangerous bool/int condition
Sometimes during debugging we can write this kind of code
```
if(1){
}
while(true){
}
```
This code can be very dangerous.

Added:

- new rule for: if/while/do statemenst
- tests for it

Hidanio: I think we should change Warning -> notice

Patch:
diff --git a/docs/checkers_doc.md b/docs/checkers_doc.md
index 0fe2c4b..5d7b169 100644
--- a/docs/checkers_doc.md
+++ b/docs/checkers_doc.md
@@ -25,6 +25,7 @@
    - [`concatenationPrecedence` checker](#concatenationprecedence-checker)
    - [`constCase` checker (autofixable)](#constcase-checker)
    - [`countUse` checker (autofixable)](#countuse-checker)
+   - [`dangerousBoolCondition` checker](#dangerousboolcondition-checker)
    - [`deadCode` checker](#deadcode-checker)
    - [`deprecated` checker](#deprecated-checker)
    - [`discardExpr` checker](#discardexpr-checker)
@@ -498,6 +499,25 @@ if (count($arr) != 0) { ... }
 <p><br></p>
 
 
+### `dangerousBoolCondition` checker
+
+#### Description
+
+Report a dangerous condition
+
+#### Non-compliant code:
+```php
+if(true){}
+```
+
+#### Compliant code:
+```php
+$a = getCond(); // get bool value from some func
+				if($a){}
+```
+<p><br></p>
+
+
 ### `deadCode` checker
 
 #### Description
diff --git a/src/linter/block_linter.go b/src/linter/block_linter.go
index c478c7a..4353bdc 100644
--- a/src/linter/block_linter.go
+++ b/src/linter/block_linter.go
@@ -188,8 +188,10 @@ func (b *blockLinter) enterNode(n ir.Node) {
 		b.walker.r.checker.CheckKeywordCase(n, "for")
 	case *ir.WhileStmt:
 		b.walker.r.checker.CheckKeywordCase(n, "while")
+		b.checkDangerousBoolCond(n.Cond)
 	case *ir.DoStmt:
 		b.walker.r.checker.CheckKeywordCase(n, "do")
+		b.checkDangerousBoolCond(n.Cond)
 
 	case *ir.ContinueStmt:
 		b.checkContinueStmt(n)
@@ -828,10 +830,34 @@ func (b *blockLinter) checkIfStmt(s *ir.IfStmt) {
 			b.report(s, LevelWarning, "dupBranchBody", "Duplicated if/else actions")
 		}
 	}
+	b.checkDangerousBoolCond(s.Cond)
 
 	b.checkIfStmtDupCond(s)
 }
 
+func (b *blockLinter) checkDangerousBoolCond(s ir.Node) {
+	checkNodeDangerousBoolCond(s, b)
+}
+
+func checkNodeDangerousBoolCond(node ir.Node, b *blockLinter) {
+	switch n := node.(type) {
+	case *ir.ConstFetchExpr:
+		if strings.EqualFold(n.Constant.Value, "true") || strings.EqualFold(n.Constant.Value, "false") {
+			b.report(node, LevelNotice, "dangerousBoolCondition", "Potential dangerous bool value: you have constant bool value in condition")
+		}
+	case *ir.Lnumber:
+		if n.Value == "0" || n.Value == "1" {
+			b.report(node, LevelNotice, "dangerousBoolCondition", "Potential dangerous value: you have constant int value that interpreted as bool")
+		}
+	case *ir.BooleanOrExpr:
+		checkNodeDangerousBoolCond(n.Left, b)
+		checkNodeDangerousBoolCond(n.Right, b)
+	case *ir.BooleanAndExpr:
+		checkNodeDangerousBoolCond(n.Left, b)
+		checkNodeDangerousBoolCond(n.Right, b)
+	}
+}
+
 func (b *blockLinter) checkIfStmtDupCond(s *ir.IfStmt) {
 	conditions := irutil.NewNodeSet()
 	conditions.Add(s.Cond)
@@ -843,6 +869,7 @@ func (b *blockLinter) checkIfStmtDupCond(s *ir.IfStmt) {
 		if !conditions.Add(elseif.Cond) {
 			b.report(elseif.Cond, LevelWarning, "dupCond", "duplicated condition in if-else chain")
 		}
+		b.checkDangerousBoolCond(elseif.Cond)
 	}
 }
 
diff --git a/src/linter/report.go b/src/linter/report.go
index b8b9b66..37bc595 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -1203,6 +1203,16 @@ function main(): void {
 			After:    `(string)$x`,
 		},
 
+		{
+			Name:     "dangerousBoolCondition",
+			Default:  true,
+			Quickfix: false,
+			Comment:  "Report a dangerous condition",
+			Before:   "if(true){}",
+			After: `$a = getCond(); // get bool value from some func
+				if($a){}`,
+		},
+
 		{
 			Name:     "reverseAssign",
 			Default:  true,


Test patch:
diff --git a/src/tests/checkers/basic_test.go b/src/tests/checkers/basic_test.go
index d4dad7c..c204111 100644
--- a/src/tests/checkers/basic_test.go
+++ b/src/tests/checkers/basic_test.go
@@ -2184,6 +2184,7 @@ function f() {
 	test.Expect = []string{
 		`since PHP 7.4, using array_key_exists() with an object has been deprecated, use isset() or property_exists() instead`,
 		`since PHP 7.4, using array_key_exists() with an object has been deprecated, use isset() or property_exists() instead`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
 	}
 	test.RunAndMatch()
 }
@@ -2365,6 +2366,7 @@ function f() {
 `)
 	test.Expect = []string{
 		`Cannot find referenced variable $e`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
 	}
 	test.RunAndMatch()
 }
diff --git a/src/tests/checkers/dangerouse_condition_test.go b/src/tests/checkers/dangerouse_condition_test.go
new file mode 100644
index 0000000..715dded
--- /dev/null
+++ b/src/tests/checkers/dangerouse_condition_test.go
@@ -0,0 +1,117 @@
+package checkers
+
+import (
+	"testing"
+
+	"github.com/VKCOM/noverify/src/linttest"
+)
+
+func TestDangerousConditionExplicitBool(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+if(true){
+}
+
+if(1){
+}
+
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+	}
+	test.RunAndMatch()
+}
+
+func TestDangerousConditionElseIf(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+if(1){
+} elseif (true){
+
+}
+
+
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+	}
+	test.RunAndMatch()
+}
+
+func TestDangerousConditionExplicitBoolMultiOr(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+
+$a = true;
+if(true||$a){
+echo "test";
+}
+
+if(1||$a||1||true||false||0){
+}
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted`,
+	}
+	test.RunAndMatch()
+}
+
+func TestDangerousConditionExplicitBoolMultiAnd(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+
+$a = true;
+if($a && false && true && 1 && 0){
+}
+
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+	}
+	test.RunAndMatch()
+}
+
+func TestDangerousConditionExplicitBoolMultiOrAnd(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+
+$a = true;
+if($a || false && 1 || true){
+}
+
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+	}
+	test.RunAndMatch()
+}
+
+func TestDangerousConditionCycles(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AddFile(`<?php
+
+while(true){
+}
+
+do{
+
+}while(1);
+`)
+	test.Expect = []string{
+		`Potential dangerous bool value: you have constant bool value in condition`,
+		`Potential dangerous value: you have constant int value that interpreted as bool`,
+	}
+	test.RunAndMatch()
+}
diff --git a/src/tests/checkers/get_type_miss_use_test.go b/src/tests/checkers/get_type_miss_use_test.go
index eadfa26..ca17d10 100644
--- a/src/tests/checkers/get_type_miss_use_test.go
+++ b/src/tests/checkers/get_type_miss_use_test.go
@@ -43,6 +43,7 @@ function getTypeMisUse(mixed $var) {
 		`use is_object instead of 'gettype($var) === "object"'`,
 		`use is_int instead of 'gettype(getTypeMisUse($var)) === "integer"'`,
 		`use is_resource instead of 'gettype(getTypeMisUse($var)) != "resource"'`,
+		`Potential dangerous bool value: you have constant bool value in condition`,
 	}
 
 	test.RunAndMatch()
diff --git a/src/tests/golden/golden_test.go b/src/tests/golden/golden_test.go
index 88657c9..dc78e21 100644
--- a/src/tests/golden/golden_test.go
+++ b/src/tests/golden/golden_test.go
@@ -113,7 +113,7 @@ func TestGolden(t *testing.T) {
 
 		{
 			Name:    "phprocksyd",
-			Disable: []string{"missingPhpdoc"},
+			Disable: []string{"missingPhpdoc", "dangerousBoolCondition"},
 			Deps: []string{
 				`stubs/phpstorm-stubs/standard/basic.php`,
 				`stubs/phpstorm-stubs/pcntl/pcntl.php`,
diff --git a/src/tests/regression/issue78_test.go b/src/tests/regression/issue78_test.go
index 1bb38a1..1b42b3a 100644
--- a/src/tests/regression/issue78_test.go
+++ b/src/tests/regression/issue78_test.go
@@ -194,6 +194,7 @@ function trailing_exit_while($xs) {
 		"Unreachable code",
 		"Unreachable code",
 		"Unreachable code",
+		"Potential dangerous value: you have constant int value that interpreted as bool",
 	}
 
 	test.RunAndMatch()
@@ -375,6 +376,7 @@ function trailing_exit_while($xs) {
 		"Unreachable code",
 		"Unreachable code",
 		"Unreachable code",
+		"Potential dangerous value: you have constant int value that interpreted as bool",
 	}
 
 	test.RunAndMatch()
@@ -505,7 +507,8 @@ function trailing_exit_for($xs) {
 }
 
 function trailing_exit_while($xs) {
-  while (1) {
+$b = 1;
+  while ($b) {
     if ($xs[0] < 1000) {
       return "ok";
     }