From fb34286f7eb775578c550d11e90fa30f247a19f5 Mon Sep 17 00:00:00 2001 From: Esorat Date: Sun, 27 Oct 2024 17:38:10 +0700 Subject: [PATCH 01/10] `shortCircuitCondition' detector that suggests optimizing boolean expressions to leverage short-circuit evaluation --- .../builtin/shortCircuitCondition.ts | 82 +++++++++++++++++++ src/detectors/detector.ts | 7 ++ .../ShortCircuitCondition.expected.out | 26 ++++++ test/detectors/ShortCircuitCondition.tact | 35 ++++++++ 4 files changed, 150 insertions(+) create mode 100644 src/detectors/builtin/shortCircuitCondition.ts create mode 100644 test/detectors/ShortCircuitCondition.expected.out create mode 100644 test/detectors/ShortCircuitCondition.tact diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts new file mode 100644 index 00000000..dd970599 --- /dev/null +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -0,0 +1,82 @@ +import { CompilationUnit } from "../../internals/ir"; +import { + evalsToValue, + foldStatements, + forEachExpression, +} from "../../internals/tact"; +import { MistiTactWarning, Severity } from "../../internals/warnings"; +import { ASTDetector } from "../detector"; +import { AstExpression } from "@tact-lang/compiler/dist/grammar/ast"; + +/** + * 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 (&&) 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 === "&&") { + const leftConst = this.isConstantExpression(expr.left); + const rightConst = this.isConstantExpression(expr.right); + if (!leftConst && rightConst) { + acc.push( + this.makeWarning( + "Consider moving constant condition to the left for short-circuit optimization", + expr.loc, + { + suggestion: + "Reorder conditions to evaluate constants first", + }, + ), + ); + } + } + }); + return acc; + }, + [] as MistiTactWarning[], + ); + } + + private isConstantExpression(expr: AstExpression): boolean { + return ( + evalsToValue(expr, "boolean", true) || + evalsToValue(expr, "boolean", false) || + expr.kind === "boolean" || + (expr.kind === "op_binary" && + ["==", "!=", ">", "<", ">=", "<="].includes(expr.op)) + ); + } +} diff --git a/src/detectors/detector.ts b/src/detectors/detector.ts index 2fe575eb..cc988d53 100644 --- a/src/detectors/detector.ts +++ b/src/detectors/detector.ts @@ -385,6 +385,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..efff1693 --- /dev/null +++ b/test/detectors/ShortCircuitCondition.expected.out @@ -0,0 +1,26 @@ +[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +test/detectors/ShortCircuitCondition.tact:6:17: + 5 | fun testCondition1(): Bool { +> 6 | return (self.expensiveCheck() && (self.a > 10)); + ^ + 7 | } +Help: Reorder conditions to evaluate constants first +See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition + +[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +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 conditions to evaluate constants first +See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition + +[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +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: Reorder conditions to evaluate constants first +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..0edf9545 --- /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)); + } + + // Test 2: Unoptimized || condition, should warn to reorder + fun testCondition2(): Bool { + return (true || self.expensiveCheck()); + } + + // 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()); + } + + fun expensiveCheck(): Bool { + return true; + } +} From 399fa9ffe67147af96fbd618e53d929554a9270f Mon Sep 17 00:00:00 2001 From: Esorat Date: Sun, 27 Oct 2024 23:58:22 +0700 Subject: [PATCH 02/10] Refactor for cover all test cases in contract --- .../builtin/shortCircuitCondition.ts | 27 +++++++++++++++---- .../ShortCircuitCondition.expected.out | 23 +++++++++++----- test/detectors/ShortCircuitCondition.tact | 6 ++--- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index dd970599..d055720c 100644 --- a/src/detectors/builtin/shortCircuitCondition.ts +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -47,17 +47,27 @@ export class ShortCircuitCondition extends ASTDetector { fun, (acc, stmt) => { forEachExpression(stmt, (expr) => { - if (expr.kind === "op_binary" && expr.op === "&&") { + if ( + expr.kind === "op_binary" && + (expr.op === "&&" || expr.op === "||") + ) { const leftConst = this.isConstantExpression(expr.left); const rightConst = this.isConstantExpression(expr.right); - if (!leftConst && rightConst) { + const leftExpensive = this.isExpensiveFunction(expr.left); + if ( + (expr.op === "&&" && !leftConst && rightConst) || + (expr.op === "||" && leftConst && !rightConst) + ) { acc.push( this.makeWarning( - "Consider moving constant condition to the left for short-circuit optimization", + `Consider reordering: ${ + leftExpensive + ? "Move expensive function call to the end." + : "Move constant to the left." + }`, expr.loc, { - suggestion: - "Reorder conditions to evaluate constants first", + suggestion: `Reorder to optimize ${expr.op} condition short-circuiting`, }, ), ); @@ -70,6 +80,13 @@ export class ShortCircuitCondition extends ASTDetector { ); } + private isExpensiveFunction(expr: AstExpression): boolean { + const expensiveFunctions = ["long_running_fun", "expensive_function"]; + return ( + expr.kind === "method_call" && expensiveFunctions.includes(expr.kind) + ); + } + private isConstantExpression(expr: AstExpression): boolean { return ( evalsToValue(expr, "boolean", true) || diff --git a/test/detectors/ShortCircuitCondition.expected.out b/test/detectors/ShortCircuitCondition.expected.out index efff1693..e736c8e5 100644 --- a/test/detectors/ShortCircuitCondition.expected.out +++ b/test/detectors/ShortCircuitCondition.expected.out @@ -1,26 +1,35 @@ -[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. test/detectors/ShortCircuitCondition.tact:6:17: 5 | fun testCondition1(): Bool { -> 6 | return (self.expensiveCheck() && (self.a > 10)); +> 6 | return (self.expensiveCheck() && (self.a > 10)); // Bad ^ 7 | } -Help: Reorder conditions to evaluate constants first +Help: Reorder to optimize && condition short-circuiting See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition -[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. +test/detectors/ShortCircuitCondition.tact:11:17: + 10 | fun testCondition2(): Bool { +> 11 | return (true || self.expensiveCheck()); // Bad + ^ + 12 | } +Help: Reorder to optimize || condition short-circuiting +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 conditions to evaluate constants first +Help: Reorder to optimize && condition short-circuiting See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition -[LOW] ShortCircuitCondition: Consider moving constant condition to the left for short-circuit optimization +[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. 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: Reorder conditions to evaluate constants first +Help: Reorder to optimize && condition short-circuiting 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 index 0edf9545..e8c3fcd1 100644 --- a/test/detectors/ShortCircuitCondition.tact +++ b/test/detectors/ShortCircuitCondition.tact @@ -3,12 +3,12 @@ contract ShortCircuitTest { // Test 1: Unoptimized && condition, should warn to reorder fun testCondition1(): Bool { - return (self.expensiveCheck() && (self.a > 10)); + return (self.expensiveCheck() && (self.a > 10)); // Bad } // Test 2: Unoptimized || condition, should warn to reorder fun testCondition2(): Bool { - return (true || self.expensiveCheck()); + return (true || self.expensiveCheck()); // Bad } // Test 3: Complex condition with both optimized and unoptimized parts @@ -26,7 +26,7 @@ contract ShortCircuitTest { // Test 5: No optimization needed, should not warn fun testCondition5(): Bool { - return (self.a > 0 && self.expensiveCheck()); + return (self.a > 0 && self.expensiveCheck()); // Ok } fun expensiveCheck(): Bool { From 10740c00ceb6c2cadbe5d507e49fae6f40ce2235 Mon Sep 17 00:00:00 2001 From: Esorat Date: Mon, 28 Oct 2024 11:49:18 +0700 Subject: [PATCH 03/10] Fix: Refactoring short-circuit optimization logic for catching complex cases --- .../builtin/shortCircuitCondition.ts | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index d055720c..ba2bff06 100644 --- a/src/detectors/builtin/shortCircuitCondition.ts +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -6,7 +6,10 @@ import { } from "../../internals/tact"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; -import { AstExpression } from "@tact-lang/compiler/dist/grammar/ast"; +import { + AstExpression, + AstMethodCall, +} from "@tact-lang/compiler/dist/grammar/ast"; /** * A detector that suggests optimizing boolean expressions to leverage short-circuit evaluation. @@ -81,10 +84,23 @@ export class ShortCircuitCondition extends ASTDetector { } private isExpensiveFunction(expr: AstExpression): boolean { - const expensiveFunctions = ["long_running_fun", "expensive_function"]; - return ( - expr.kind === "method_call" && expensiveFunctions.includes(expr.kind) - ); + if (expr.kind === "method_call") { + if (expr.args.length > 2) return true; + return this.hasNestedCallsOrRecursion(expr); + } + return false; + } + + private hasNestedCallsOrRecursion(expr: AstMethodCall): boolean { + for (const arg of expr.args) { + if ( + arg.kind === "method_call" || + (arg.kind === "id" && arg.text === expr.method.text) + ) { + return true; + } + } + return false; } private isConstantExpression(expr: AstExpression): boolean { From 9bf45d4ab2e96846bd7b269534a80196ac2ee0ec Mon Sep 17 00:00:00 2001 From: Esorat Date: Tue, 29 Oct 2024 13:24:28 +0700 Subject: [PATCH 04/10] Fix: - Update containsExpensiveCall to correctly narrow down AstExpression types before accessing properties - Ensure that properties like 'left', 'right', and 'operand' are only accessed when safe - Improve recursive traversal to detect expensive function calls in nested expressions --- .../builtin/shortCircuitCondition.ts | 121 +++++++++++++----- .../ShortCircuitCondition.expected.out | 24 +++- 2 files changed, 109 insertions(+), 36 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index ba2bff06..d538440d 100644 --- a/src/detectors/builtin/shortCircuitCondition.ts +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -6,16 +6,13 @@ import { } from "../../internals/tact"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; -import { - AstExpression, - AstMethodCall, -} from "@tact-lang/compiler/dist/grammar/ast"; +import { AstExpression } from "@tact-lang/compiler/dist/grammar/ast"; /** * 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 (&&) operations, + * 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. * @@ -56,18 +53,47 @@ export class ShortCircuitCondition extends ASTDetector { ) { const leftConst = this.isConstantExpression(expr.left); const rightConst = this.isConstantExpression(expr.right); - const leftExpensive = this.isExpensiveFunction(expr.left); + const leftExpensive = this.containsExpensiveCall(expr.left); + const rightExpensive = this.containsExpensiveCall(expr.right); if ( + expr.op === "&&" && + leftExpensive && + !rightExpensive && + !leftConst && + !rightConst + ) { + 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.`, + }, + ), + ); + } else if ( + expr.op === "||" && + leftExpensive && + !rightExpensive && + !leftConst && + !rightConst + ) { + 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.`, + }, + ), + ); + } else if ( (expr.op === "&&" && !leftConst && rightConst) || (expr.op === "||" && leftConst && !rightConst) ) { acc.push( this.makeWarning( - `Consider reordering: ${ - leftExpensive - ? "Move expensive function call to the end." - : "Move constant to the left." - }`, + `Consider reordering: Move constant to the left.`, expr.loc, { suggestion: `Reorder to optimize ${expr.op} condition short-circuiting`, @@ -83,33 +109,62 @@ export class ShortCircuitCondition extends ASTDetector { ); } - private isExpensiveFunction(expr: AstExpression): boolean { - if (expr.kind === "method_call") { - if (expr.args.length > 2) return true; - return this.hasNestedCallsOrRecursion(expr); - } - return false; - } - - private hasNestedCallsOrRecursion(expr: AstMethodCall): boolean { - for (const arg of expr.args) { - if ( - arg.kind === "method_call" || - (arg.kind === "id" && arg.text === expr.method.text) - ) { - return true; + private containsExpensiveCall(expr: AstExpression): boolean { + let isExpensive = false; + const checkExpensive = (e: AstExpression) => { + // Early exit if found + if (isExpensive) return; + if (e.kind === "method_call" || e.kind === "static_call") { + isExpensive = true; + } else if (e.kind === "op_binary") { + checkExpensive(e.left); + checkExpensive(e.right); + } else if (e.kind === "op_unary") { + checkExpensive(e.operand); + } else if (e.kind === "field_access") { + checkExpensive(e.aggregate); + } else if (e.kind === "conditional") { + checkExpensive(e.condition); + checkExpensive(e.thenBranch); + checkExpensive(e.elseBranch); + } else if (e.kind === "struct_instance") { + e.args.forEach((arg) => { + checkExpensive(arg.initializer); + }); + } else if (e.kind === "init_of") { + e.args.forEach((arg) => { + checkExpensive(arg); + }); } - } - return false; + }; + checkExpensive(expr); + return isExpensive; } private isConstantExpression(expr: AstExpression): boolean { - return ( + if ( evalsToValue(expr, "boolean", true) || evalsToValue(expr, "boolean", false) || - expr.kind === "boolean" || - (expr.kind === "op_binary" && - ["==", "!=", ">", "<", ">=", "<="].includes(expr.op)) - ); + expr.kind === "boolean" + ) { + return true; + } + if ( + expr.kind === "number" || + expr.kind === "string" || + expr.kind === "null" + ) { + return true; + } + if ( + expr.kind === "op_binary" && + ["==", "!=", ">", "<", ">=", "<="].includes(expr.op) + ) { + return ( + this.isConstantExpression(expr.left) && + this.isConstantExpression(expr.right) + ); + } + return false; } } diff --git a/test/detectors/ShortCircuitCondition.expected.out b/test/detectors/ShortCircuitCondition.expected.out index e736c8e5..3d0dfc47 100644 --- a/test/detectors/ShortCircuitCondition.expected.out +++ b/test/detectors/ShortCircuitCondition.expected.out @@ -1,10 +1,10 @@ -[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. +[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: Reorder to optimize && condition short-circuiting +Help: Place cheaper conditions on the left to leverage short-circuiting. See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition [LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. @@ -25,11 +25,29 @@ test/detectors/ShortCircuitCondition.tact:16:17: Help: Reorder to optimize && condition short-circuiting See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition -[LOW] ShortCircuitCondition: Consider reordering: Move constant to the left. +[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. +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. +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 See: https://nowarp.io/tools/misti/docs/detectors/ShortCircuitCondition \ No newline at end of file From bb95de08e4aeea9f29b7c2add94e0f4af7e080bf Mon Sep 17 00:00:00 2001 From: Esorat Date: Thu, 31 Oct 2024 16:44:41 +0700 Subject: [PATCH 05/10] FIX: improve short-circuit condition optimization suggestions - Enhance suggestion messages with prettier formatting and clearer explanations - Simplify expensive call detection using findInExpressions - Update condition reordering logic to focus on function call costs - Remove redundant constant expression checks - Add explicit handling for init_of expressions - Improve warning messages formatting and clarity --- .../builtin/shortCircuitCondition.ts | 116 ++++++------------ .../ShortCircuitCondition.expected.out | 32 ++--- 2 files changed, 54 insertions(+), 94 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index d538440d..95f3344d 100644 --- a/src/detectors/builtin/shortCircuitCondition.ts +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -3,10 +3,12 @@ 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. @@ -51,52 +53,40 @@ export class ShortCircuitCondition extends ASTDetector { expr.kind === "op_binary" && (expr.op === "&&" || expr.op === "||") ) { - const leftConst = this.isConstantExpression(expr.left); - const rightConst = this.isConstantExpression(expr.right); 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 ( - expr.op === "&&" && leftExpensive && !rightExpensive && - !leftConst && - !rightConst + !this.containsInitOf(expr.right) ) { acc.push( this.makeWarning( - `Consider reordering: Move expensive function call to the end.`, + `Consider reordering: Move expensive function call to the end`, expr.loc, { - suggestion: `Place cheaper conditions on the left to leverage short-circuiting.`, + suggestion: `Place cheaper conditions on the left to leverage short-circuiting: ${prettyPrint( + expr.right, + )} ${expr.op} ${prettyPrint(expr.left)}`, }, ), ); - } else if ( - expr.op === "||" && - leftExpensive && - !rightExpensive && - !leftConst && - !rightConst - ) { - 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.`, - }, - ), - ); - } else if ( - (expr.op === "&&" && !leftConst && rightConst) || - (expr.op === "||" && leftConst && !rightConst) + } + if ( + !leftIsConstant && + rightIsConstant && + !this.containsInitOf(expr.left) ) { acc.push( this.makeWarning( - `Consider reordering: Move constant to the left.`, + `Consider reordering: Move constant to the left`, expr.loc, { - suggestion: `Reorder to optimize ${expr.op} condition short-circuiting`, + suggestion: `Reorder to optimize ${expr.op} condition short-circuiting: ${prettyPrint( + expr.right, + )} ${expr.op} ${prettyPrint(expr.left)}`, }, ), ); @@ -109,62 +99,32 @@ export class ShortCircuitCondition extends ASTDetector { ); } - private containsExpensiveCall(expr: AstExpression): boolean { - let isExpensive = false; - const checkExpensive = (e: AstExpression) => { - // Early exit if found - if (isExpensive) return; - if (e.kind === "method_call" || e.kind === "static_call") { - isExpensive = true; - } else if (e.kind === "op_binary") { - checkExpensive(e.left); - checkExpensive(e.right); - } else if (e.kind === "op_unary") { - checkExpensive(e.operand); - } else if (e.kind === "field_access") { - checkExpensive(e.aggregate); - } else if (e.kind === "conditional") { - checkExpensive(e.condition); - checkExpensive(e.thenBranch); - checkExpensive(e.elseBranch); - } else if (e.kind === "struct_instance") { - e.args.forEach((arg) => { - checkExpensive(arg.initializer); - }); - } else if (e.kind === "init_of") { - e.args.forEach((arg) => { - checkExpensive(arg); - }); - } - }; - checkExpensive(expr); - return isExpensive; + 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): boolean { - if ( + private isConstantExpression(expr: AstExpression | null): boolean { + if (!expr) return false; + return ( evalsToValue(expr, "boolean", true) || evalsToValue(expr, "boolean", false) || - expr.kind === "boolean" - ) { - return true; - } - if ( + expr.kind === "boolean" || expr.kind === "number" || expr.kind === "string" || expr.kind === "null" - ) { - return true; - } - if ( - expr.kind === "op_binary" && - ["==", "!=", ">", "<", ">=", "<="].includes(expr.op) - ) { - return ( - this.isConstantExpression(expr.left) && - this.isConstantExpression(expr.right) - ); - } - return false; + ); + } + + 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 index 3d0dfc47..eddb7dd8 100644 --- a/test/detectors/ShortCircuitCondition.expected.out +++ b/test/detectors/ShortCircuitCondition.expected.out @@ -1,53 +1,53 @@ -[LOW] ShortCircuitCondition: Consider reordering: Move expensive function call to the end. +[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. +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 constant to the left. -test/detectors/ShortCircuitCondition.tact:11:17: - 10 | fun testCondition2(): Bool { -> 11 | return (true || self.expensiveCheck()); // Bad +[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 ^ - 12 | } -Help: Reorder to optimize || condition short-circuiting + 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. +[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 +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. +[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. +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. +[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. +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. +[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 +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 From 024a8018abc683ca97f9e80f0f5b4f673c6bbd9e Mon Sep 17 00:00:00 2001 From: Esorat Date: Thu, 31 Oct 2024 19:35:23 +0700 Subject: [PATCH 06/10] fix: remove isConstantExpression since we focus on expensive calls --- .../builtin/shortCircuitCondition.ts | 40 ++----------------- .../ShortCircuitCondition.expected.out | 18 --------- 2 files changed, 4 insertions(+), 54 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index 95f3344d..34a51cc3 100644 --- a/src/detectors/builtin/shortCircuitCondition.ts +++ b/src/detectors/builtin/shortCircuitCondition.ts @@ -1,6 +1,5 @@ import { CompilationUnit } from "../../internals/ir"; import { - evalsToValue, foldStatements, forEachExpression, findInExpressions, @@ -15,21 +14,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 constant or cheaper conditions first can prevent unnecessary execution + * placing 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) { + * if (expensive_function() && cheap_condition) { * // ... * } * ``` * * Use instead: * ```tact - * // Good: Expensive operation is skipped when constant_false is false - * if (constant_false && expensive_function()) { + * // Good: Expensive operation is skipped when cheap_condition is false + * if (cheap_condition && expensive_function()) { * // ... * } * ``` @@ -55,8 +54,6 @@ 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 && @@ -74,23 +71,6 @@ 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; @@ -111,18 +91,6 @@ 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 index eddb7dd8..f20fe39e 100644 --- a/test/detectors/ShortCircuitCondition.expected.out +++ b/test/detectors/ShortCircuitCondition.expected.out @@ -16,15 +16,6 @@ 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 { @@ -41,13 +32,4 @@ 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 \ No newline at end of file From 23e871b7ac5c22cfb5eba23865752bfcb6b78198 Mon Sep 17 00:00:00 2001 From: Esorat Date: Thu, 31 Oct 2024 19:52:40 +0700 Subject: [PATCH 07/10] fix: rollback code we are need to check constant expression as before --- .../builtin/shortCircuitCondition.ts | 40 +++++++++++++++++-- .../ShortCircuitCondition.expected.out | 18 +++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/detectors/builtin/shortCircuitCondition.ts b/src/detectors/builtin/shortCircuitCondition.ts index 34a51cc3..95f3344d 100644 --- 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 index f20fe39e..eddb7dd8 100644 --- 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 \ No newline at end of file From 0ebbe46fcab81a5f2588b2120448ad1661cbbe60 Mon Sep 17 00:00:00 2001 From: Esorat Date: Fri, 1 Nov 2024 21:34:26 +0700 Subject: [PATCH 08/10] Chore: Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bfa1904..99dde28e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ 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) - `SuspiciousMessageMode` detector: PR [#193](https://github.com/nowarp/misti/pull/193) - `SendInLoop` detector: PR [#168](https://github.com/nowarp/misti/pull/168) - `CellOverflow` detector: PR [#177](https://github.com/nowarp/misti/pull/177) From c1480d62b09f915c832c5750fd99aa0a028eaa04 Mon Sep 17 00:00:00 2001 From: Esorat Date: Fri, 1 Nov 2024 22:06:31 +0700 Subject: [PATCH 09/10] Chore: update CHANGELOG --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d501e1..ceb6675f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] - +- `shortCircuitCondition` detector: PR [#202](https://github.com/nowarp/misti/pull/202) ## [0.5.0] - 2024-10-31 ### Added -- `shortCircuitCondition` detector: PR [#202](https://github.com/nowarp/misti/pull/202) - `SuspiciousMessageMode` detector: PR [#193](https://github.com/nowarp/misti/pull/193) - `SendInLoop` detector: PR [#168](https://github.com/nowarp/misti/pull/168) - `CellOverflow` detector: PR [#177](https://github.com/nowarp/misti/pull/177) From b78e9beacad596e221d25e9e747caa552576a5c4 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Fri, 1 Nov 2024 12:09:01 -0400 Subject: [PATCH 10/10] Update CHANGELOG.md --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ceb6675f..9af4d05f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] -- `shortCircuitCondition` detector: PR [#202](https://github.com/nowarp/misti/pull/202) + +### Added +- `ShortCircuitCondition` detector: PR [#202](https://github.com/nowarp/misti/pull/202) + ## [0.5.0] - 2024-10-31 ### Added