Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SuspiciousMessageMode: Don't report 0 #199

Merged
merged 17 commits into from
Nov 2, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

### Added
- `EtaLikeSimplifications` detector: PR [#198](https://github.com/nowarp/misti/pull/198)
Esorat marked this conversation as resolved.
Show resolved Hide resolved
- `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)
Expand Down
192 changes: 192 additions & 0 deletions src/detectors/builtin/etaLikeSimplifications.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
import { CompilationUnit } from "../../internals/ir";
import { forEachStatement, forEachExpression } from "../../internals/tact";
import { MistiTactWarning, Severity } from "../../internals/warnings";
import { ASTDetector } from "../detector";
import {
AstNode,
AstStatement,
AstExpression,
AstOpBinary,
idText,
AstStatementReturn,
AstConditional,
} from "@tact-lang/compiler/dist/grammar/ast";

/**
* Detects opportunities for simplifying code by eliminating redundant boolean expressions and statements.
*
* ## Why is it bad?
* Redundant code can make programs less efficient and harder to read. Simplifying such code improves readability,
* maintainability, and can prevent potential logical errors.
*
* **What it checks:**
* - `if` statements that return boolean literals directly based on a condition.
* - Comparisons of boolean expressions with boolean literals (`true` or `false`).
* - Conditional expressions (ternary operators) that return boolean literals.
*
* ## Example
*
* ```tact
* // Redundant 'if' statement:
* if (condition) {
* return true;
* } else {
* return false;
* }
* // Simplify to:
* return condition;
*
* // Redundant comparison:
* return a == true;
* // Simplify to:
* return a;
*
* // Redundant conditional expression:
* return b ? true : false;
* // Simplify to:
* return b;
* ```
*/
export class EtaLikeSimplifications extends ASTDetector {
severity = Severity.LOW;

async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
const warnings: MistiTactWarning[] = [];
const entries = cu.ast.getProgramEntries();
for (const node of entries) {
this.analyzeNode(node, warnings);
}
return warnings;
}

private analyzeNode(node: AstNode, warnings: MistiTactWarning[]): void {
forEachStatement(node, (stmt) => {
this.checkStatement(stmt, warnings);
});
forEachExpression(node, (expr) => {
this.checkExpression(expr, warnings);
});
}

private checkStatement(
stmt: AstStatement,
warnings: MistiTactWarning[],
): void {
if (stmt.kind === "statement_condition") {
const ifStmt = stmt;
if (
ifStmt.trueStatements.length === 1 &&
ifStmt.falseStatements &&
ifStmt.falseStatements.length === 1 &&
ifStmt.trueStatements[0].kind === "statement_return" &&
ifStmt.falseStatements[0].kind === "statement_return"
) {
const trueReturn = ifStmt.trueStatements[0] as AstStatementReturn;
const falseReturn = ifStmt.falseStatements[0] as AstStatementReturn;
if (
this.isBooleanLiteral(trueReturn.expression, true) &&
this.isBooleanLiteral(falseReturn.expression, false)
) {
warnings.push(
this.makeWarning(
"Simplify 'if' statement by returning the condition directly",
stmt.loc,
{
suggestion: "Replace with 'return condition;'",
},
),
);
}
}
}
}

private checkExpression(
expr: AstExpression,
warnings: MistiTactWarning[],
): void {
if (expr.kind === "op_binary") {
const binaryExpr = expr as AstOpBinary;
if (binaryExpr.op === "==" || binaryExpr.op === "!=") {
const { right } = binaryExpr;
if (this.isBooleanLiteral(right)) {
warnings.push(
this.makeWarning(
"Redundant comparison with boolean literal",
expr.loc,
{
suggestion: `Use '${this.getSimplifiedBooleanExpression(
binaryExpr,
)}' instead`,
},
),
);
}
}
}
if (expr.kind === "conditional") {
const conditionalExpr = expr as AstConditional;
if (
this.isBooleanLiteral(conditionalExpr.thenBranch, true) &&
this.isBooleanLiteral(conditionalExpr.elseBranch, false)
) {
warnings.push(
this.makeWarning(
"Simplify conditional expression by using the condition directly",
expr.loc,
{
suggestion: `Use '${this.getConditionText(
conditionalExpr.condition,
)}' instead`,
},
),
);
}
}
}

private isBooleanLiteral(
expr: AstExpression | null | undefined,
value?: boolean,
): boolean {
if (!expr) return false;
if (expr.kind === "boolean") {
if (value === undefined) {
return true;
}
return expr.value === value;
}
return false;
}

private getSimplifiedBooleanExpression(binaryExpr: AstOpBinary): string {
const exprText = (expr: AstExpression): string => {
if (expr.kind === "id") {
return idText(expr);
}
return "expression";
};

if (this.isBooleanLiteral(binaryExpr.right, true)) {
if (binaryExpr.op === "==") {
return exprText(binaryExpr.left);
} else {
return `!${exprText(binaryExpr.left)}`;
}
} else if (this.isBooleanLiteral(binaryExpr.right, false)) {
if (binaryExpr.op === "==") {
return `!${exprText(binaryExpr.left)}`;
} else {
return exprText(binaryExpr.left);
}
}
return "expression";
}

private getConditionText(expr: AstExpression): string {
if (expr.kind === "id") {
return idText(expr);
}
return "condition";
}
}
14 changes: 14 additions & 0 deletions src/detectors/builtin/suspiciousMessageMode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,20 @@ export class SuspiciousMessageMode extends ASTDetector {
expr: AstExpression,
warnings: MistiTactWarning[],
): void {
if (expr.kind === "number" && expr.value === 0n) {
warnings.push(
jubnzv marked this conversation as resolved.
Show resolved Hide resolved
this.makeWarning(
"Setting `mode` to `0` is redundant as it has no effect.",
expr.loc,
{
suggestion:
"Remove the `mode` field or set it to a meaningful value.",
},
),
);
return;
}

const flagsUsed = new Set<string>();
forEachExpression(expr, (e) => {
switch (e.kind) {
Expand Down
7 changes: 7 additions & 0 deletions src/detectors/detector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,13 @@ export const BuiltInDetectors: Record<string, DetectorEntry> = {
),
enabledByDefault: true,
},
EtaLikeSimplifications: {
loader: (ctx: MistiContext) =>
import("./builtin/etaLikeSimplifications").then(
(module) => new module.EtaLikeSimplifications(ctx),
),
enabledByDefault: true,
},
};

/**
Expand Down
26 changes: 26 additions & 0 deletions test/detectors/EtaLikeSimplifications.expected.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
[LOW] EtaLikeSimplifications: Simplify 'if' statement by returning the condition directly
test/detectors/EtaLikeSimplifications.tact:4:9:
3 | fun redundantIf(condition: Bool): Bool {
> 4 | if (condition) {
^
5 | return true;
Help: Replace with 'return condition;'
See: https://nowarp.io/tools/misti/docs/detectors/EtaLikeSimplifications

[LOW] EtaLikeSimplifications: Redundant comparison with boolean literal
test/detectors/EtaLikeSimplifications.tact:13:16:
12 | fun redundantComparison(a: Bool): Bool {
> 13 | return a == true;
^
14 | }
Help: Use 'a' instead
See: https://nowarp.io/tools/misti/docs/detectors/EtaLikeSimplifications

[LOW] EtaLikeSimplifications: Simplify conditional expression by using the condition directly
test/detectors/EtaLikeSimplifications.tact:18:16:
17 | fun redundantTernary(b: Bool): Bool {
> 18 | return b ? true : false;
^
19 | }
Help: Use 'b' instead
See: https://nowarp.io/tools/misti/docs/detectors/EtaLikeSimplifications
25 changes: 25 additions & 0 deletions test/detectors/EtaLikeSimplifications.tact
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
contract TestContract {
// Bad: Example of redundant if statement
fun redundantIf(condition: Bool): Bool {
if (condition) {
return true;
} else {
return false;
}
}

// Bad: Example of redundant boolean comparison
fun redundantComparison(a: Bool): Bool {
return a == true;
}

// Bad: Example of redundant ternary expression
fun redundantTernary(b: Bool): Bool {
return b ? true : false;
}

// Ok: Correct usage (should not trigger warnings)
fun correctUsage(condition: Bool): Bool {
return condition;
}
}
9 changes: 9 additions & 0 deletions test/detectors/SuspiciousMessageMode.expected.out
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,13 @@ test/detectors/SuspiciousMessageMode.tact:61:21:
^
62 | });
Help: Use the '|' operator (bitwise OR) to combine flags
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Setting `mode` to `0` is redundant as it has no effect.
test/detectors/SuspiciousMessageMode.tact:79:15:
78 | value: 0,
> 79 | mode: 0 // Bad: Should trigger warning about `mode: 0` being redundant
^
80 | });
Help: Remove the `mode` field or set it to a meaningful value.
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
8 changes: 8 additions & 0 deletions test/detectors/SuspiciousMessageMode.tact
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,12 @@ contract SendParametersTestContract {
mode: modeFlag // Ok
});
}

fun modeZeroUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: 0 // Bad: Should trigger warning about `mode: 0` being redundant
});
}
}
Loading