feat(detectors) Add `ShortCircuitCondition' (#202)

Closes #144

Co-authored-by: Georgiy Komarov <[email protected]>

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+- `ShortCircuitCondition` detector: PR [#202](https://github.com/nowarp/misti/pull/202)
+
 ## [0.5.0] - 2024-10-31
 
 ### Added

src/detectors/builtin/shortCircuitCondition.ts

@@ -0,0 +1,130 @@
+import { CompilationUnit } from "../../internals/ir";
+import {
+  evalsToValue,
+  foldStatements,
+  forEachExpression,
+  findInExpressions,
+} from "../../internals/tact";
+import { MistiTactWarning, Severity } from "../../internals/warnings";
+import { ASTDetector } from "../detector";
+import { AstExpression } from "@tact-lang/compiler/dist/grammar/ast";
+import { prettyPrint } from "@tact-lang/compiler/dist/prettyPrinter";
+
+/**
+ * A detector that suggests optimizing boolean expressions to leverage short-circuit evaluation.
+ *
+ * ## Why is it bad?
+ * TVM supports short-circuit operations. When using logical AND (`&&`) or logical OR (`||`) operations,
+ * placing constant or cheaper conditions first can prevent unnecessary execution
+ * of expensive operations when the result is already determined.
+ *
+ * ## Example
+ * ```tact
+ * // Bad: Expensive operation is always executed
+ * if (expensive_function() && constant_false) {
+ *   // ...
+ * }
+ * ```
+ *
+ * Use instead:
+ * ```tact
+ * // Good: Expensive operation is skipped when constant_false is false
+ * if (constant_false && expensive_function()) {
+ *   // ...
+ * }
+ * ```
+ */
+export class ShortCircuitCondition extends ASTDetector {
+  severity = Severity.LOW;
+
+  async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
+    return Array.from(cu.ast.getFunctions()).reduce(
+      (acc, fun) => acc.concat(this.checkFunction(fun)),
+      [] as MistiTactWarning[],
+    );
+  }
+
+  private checkFunction(fun: any): MistiTactWarning[] {
+    return foldStatements(
+      fun,
+      (acc, stmt) => {
+        forEachExpression(stmt, (expr) => {
+          if (
+            expr.kind === "op_binary" &&
+            (expr.op === "&&" || expr.op === "||")
+          ) {
+            const leftExpensive = this.containsExpensiveCall(expr.left);
+            const rightExpensive = this.containsExpensiveCall(expr.right);
+            const leftIsConstant = this.isConstantExpression(expr.left);
+            const rightIsConstant = this.isConstantExpression(expr.right);
+            if (
+              leftExpensive &&
+              !rightExpensive &&
+              !this.containsInitOf(expr.right)
+            ) {
+              acc.push(
+                this.makeWarning(
+                  `Consider reordering: Move expensive function call to the end`,
+                  expr.loc,
+                  {
+                    suggestion: `Place cheaper conditions on the left to leverage short-circuiting: ${prettyPrint(
+                      expr.right,
+                    )} ${expr.op} ${prettyPrint(expr.left)}`,
+                  },
+                ),
+              );
+            }
+            if (
+              !leftIsConstant &&
+              rightIsConstant &&
+              !this.containsInitOf(expr.left)
+            ) {
+              acc.push(
+                this.makeWarning(
+                  `Consider reordering: Move constant to the left`,
+                  expr.loc,
+                  {
+                    suggestion: `Reorder to optimize ${expr.op} condition short-circuiting: ${prettyPrint(
+                      expr.right,
+                    )} ${expr.op} ${prettyPrint(expr.left)}`,
+                  },
+                ),
+              );
+            }
+          }
+        });
+        return acc;
+      },
+      [] as MistiTactWarning[],
+    );
+  }
+
+  private containsExpensiveCall(expr: AstExpression | null): boolean {
+    if (!expr) return false;
+    return (
+      findInExpressions(
+        expr,
+        (e) =>
+          (e.kind === "method_call" || e.kind === "static_call") &&
+          !this.containsInitOf(e),
+      ) !== null
+    );
+  }
+
+  private isConstantExpression(expr: AstExpression | null): boolean {
+    if (!expr) return false;
+    return (
+      evalsToValue(expr, "boolean", true) ||
+      evalsToValue(expr, "boolean", false) ||
+      expr.kind === "boolean" ||
+      expr.kind === "number" ||
+      expr.kind === "string" ||
+      expr.kind === "null"
+    );
+  }
+
+  private containsInitOf(expr: AstExpression | null): boolean {
+    if (!expr) return false;
+    return findInExpressions(expr, (e) => e.kind === "init_of") !== null;
+  }
+}

src/detectors/detector.ts

@@ -378,6 +378,13 @@ export const BuiltInDetectors: Record<string, DetectorEntry> = {
       ),
     enabledByDefault: true,
   },
+  ShortCircuitCondition: {
+    loader: (ctx: MistiContext) =>
+      import("./builtin/shortCircuitCondition").then(
+        (module) => new module.ShortCircuitCondition(ctx),
+      ),
+    enabledByDefault: true,
+  },
 };
 
 /**

test/detectors/ShortCircuitCondition.expected.out

@@ -0,0 +1,53 @@
+[LOW] ShortCircuitCondition: Consider reordering: Move expensive function call to the end
+test/detectors/ShortCircuitCondition.tact:6:17:
+  5 |     fun testCondition1(): Bool {
+> 6 |         return (self.expensiveCheck() && (self.a > 10)); // Bad
+                      ^
+  7 |     }
+Help: Place cheaper conditions on the left to leverage short-circuiting: self.a > 10 && self.expensiveCheck()
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition
+
+[LOW] ShortCircuitCondition: Consider reordering: Move expensive function call to the end
+test/detectors/ShortCircuitCondition.tact:16:17:
+  15 |     fun testCondition3(): Bool {
+> 16 |         return ((self.a > 0) && self.expensiveCheck() && false); //Bad: Should warn for moving false left
+                       ^
+  17 |     }
+Help: Place cheaper conditions on the left to leverage short-circuiting: false && self.a > 0 && self.expensiveCheck()
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition
+
+[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left
+test/detectors/ShortCircuitCondition.tact:16:17:
+  15 |     fun testCondition3(): Bool {
+> 16 |         return ((self.a > 0) && self.expensiveCheck() && false); //Bad: Should warn for moving false left
+                       ^
+  17 |     }
+Help: Reorder to optimize && condition short-circuiting: false && self.a > 0 && self.expensiveCheck()
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition
+
+[LOW] ShortCircuitCondition: Consider reordering: Move expensive function call to the end
+test/detectors/ShortCircuitCondition.tact:21:13:
+  20 |     fun testCondition4(): Bool {
+> 21 |         if ((self.expensiveCheck() && (self.a < 3)) || ((self.a > 10) && true)) {
+                   ^
+  22 |             return true; // Bad: Should warn for moving constants to the left
+Help: Place cheaper conditions on the left to leverage short-circuiting: self.a > 10 && true || self.expensiveCheck() && self.a < 3
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition
+
+[LOW] ShortCircuitCondition: Consider reordering: Move expensive function call to the end
+test/detectors/ShortCircuitCondition.tact:21:14:
+  20 |     fun testCondition4(): Bool {
+> 21 |         if ((self.expensiveCheck() && (self.a < 3)) || ((self.a > 10) && true)) {
+                    ^
+  22 |             return true; // Bad: Should warn for moving constants to the left
+Help: Place cheaper conditions on the left to leverage short-circuiting: self.a < 3 && self.expensiveCheck()
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition
+
+[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left
+test/detectors/ShortCircuitCondition.tact:21:57:
+  20 |     fun testCondition4(): Bool {
+> 21 |         if ((self.expensiveCheck() && (self.a < 3)) || ((self.a > 10) && true)) {
+                                                               ^
+  22 |             return true; // Bad: Should warn for moving constants to the left
+Help: Reorder to optimize && condition short-circuiting: true && self.a > 10
+See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition

test/detectors/ShortCircuitCondition.tact

@@ -0,0 +1,35 @@
+contract ShortCircuitTest {
+    a: Int = 5;
+
+    // Test 1: Unoptimized && condition, should warn to reorder
+    fun testCondition1(): Bool {
+        return (self.expensiveCheck() && (self.a > 10)); // Bad
+    }
+
+    // Test 2: Unoptimized || condition, should warn to reorder
+    fun testCondition2(): Bool {
+        return (true || self.expensiveCheck());  // Bad
+    }
+
+    // Test 3: Complex condition with both optimized and unoptimized parts
+    fun testCondition3(): Bool {
+        return ((self.a > 0) && self.expensiveCheck() && false); //Bad: Should warn for moving false left
+    }
+
+    // Test 4: Nested conditions
+    fun testCondition4(): Bool {
+        if ((self.expensiveCheck() && (self.a < 3)) || ((self.a > 10) && true)) {
+            return true; // Bad: Should warn for moving constants to the left
+        }
+        return false;
+    }
+
+    // Test 5: No optimization needed, should not warn
+    fun testCondition5(): Bool {
+        return (self.a > 0 && self.expensiveCheck()); // Ok
+    }
+
+    fun expensiveCheck(): Bool {
+        return true;
+    }
+}