diff --git a/CHANGELOG.md b/CHANGELOG.md index da870355..9af4d05f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts new file mode 100644 index 00000000..95f3344d --- /dev/null +++ b/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 { + 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; + } +} diff --git a/src/detectors/detector.ts b/src/detectors/detector.ts index 6c2780a5..70b55fc3 100644 --- a/src/detectors/detector.ts +++ b/src/detectors/detector.ts @@ -378,6 +378,13 @@ export const BuiltInDetectors: Record = { ), enabledByDefault: true, }, + ShortCircuitCondition: { + loader: (ctx: MistiContext) => + import("./builtin/shortCircuitCondition").then( + (module) => new module.ShortCircuitCondition(ctx), + ), + enabledByDefault: true, + }, }; /** diff --git a/test/detectors/ShortCircuitCondition.expected.out b/test/detectors/ShortCircuitCondition.expected.out new file mode 100644 index 00000000..eddb7dd8 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/detectors/ShortCircuitCondition.tact b/test/detectors/ShortCircuitCondition.tact new file mode 100644 index 00000000..e8c3fcd1 --- /dev/null +++ b/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; + } +}