fix: rollback code we are need to check constant expression as before

nowarp · Oct 31, 2024 · 23e871b · 23e871b
1 parent 024a801
commit 23e871b
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 4 deletions.
diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts
@@ -1,5 +1,6 @@
 import { CompilationUnit } from "../../internals/ir";
 import {
+  evalsToValue,
   foldStatements,
   forEachExpression,
   findInExpressions,
@@ -14,21 +15,21 @@ import { prettyPrint } from "@tact-lang/compiler/dist/prettyPrinter";
  *
  * ## Why is it bad?
  * TVM supports short-circuit operations. When using logical AND (`&&`) or logical OR (`||`) operations,
- * placing cheaper conditions first can prevent unnecessary execution
+ * 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() && cheap_condition) {
+ * if (expensive_function() && constant_false) {
  *   // ...
  * }
  * ```
  *
  * Use instead:
  * ```tact
- * // Good: Expensive operation is skipped when cheap_condition is false
- * if (cheap_condition && expensive_function()) {
+ * // Good: Expensive operation is skipped when constant_false is false
+ * if (constant_false && expensive_function()) {
  *   // ...
  * }
  * ```
@@ -54,6 +55,8 @@ export class ShortCircuitCondition extends ASTDetector {
           ) {
             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 &&
@@ -71,6 +74,23 @@ export class ShortCircuitCondition extends ASTDetector {
                 ),
               );
             }
+            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;
@@ -91,6 +111,18 @@ export class ShortCircuitCondition extends ASTDetector {
     );
   }
 
+  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;

diff --git a/test/detectors/ShortCircuitCondition.expected.out b/test/detectors/ShortCircuitCondition.expected.out
@@ -16,6 +16,15 @@ test/detectors/ShortCircuitCondition.tact:16: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 {
@@ -32,4 +41,13 @@ test/detectors/ShortCircuitCondition.tact:21:14:
                     ^
   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