Skip to content

Commit

Permalink
fix(neverAccessedVariables): False positive on unused fields
Browse files Browse the repository at this point in the history
  • Loading branch information
byakuren-hijiri committed Jul 26, 2024
1 parent f237d2c commit 8618b0c
Show file tree
Hide file tree
Showing 14 changed files with 261 additions and 175 deletions.
4 changes: 2 additions & 2 deletions src/detectors/builtin/neverAccessedVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class NeverAccessedVariables extends Detector {
return Array.from(cu.ast.getFunctions()).reduce((acc, fun) => {
forEachExpression(fun, (expr) => {
if (expr.kind === "op_field" && expr.src.kind === "id") {
acc.add(expr.src.value);
acc.add(expr.name);
}
});
return acc;
Expand All @@ -179,7 +179,7 @@ export class NeverAccessedVariables extends Detector {
}

collectDefinedConstants(cu: CompilationUnit): Set<[ConstantName, ASTRef]> {
return Array.from(cu.ast.getConstants(false)).reduce((acc, constant) => {
return Array.from(cu.ast.getConstants()).reduce((acc, constant) => {
acc.add([constant.name, constant.ref]);
return acc;
}, new Set<[ConstantName, ASTRef]>());
Expand Down
2 changes: 1 addition & 1 deletion src/detectors/builtin/unboundLoops.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ export class UnboundLoops extends Detector {
cu: CompilationUnit,
ctx: Context<ASTRef>,
): void {
for (const c of cu.ast.getConstants(/* allowStdlib = */ true)) {
for (const c of cu.ast.getConstants({ includeStdlib: true })) {
ctx.addFact("constDef", Fact.from([c.name], c.ref));
}
}
Expand Down
47 changes: 34 additions & 13 deletions src/internals/ir.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type EntryOrigin = "user" | "stdlib";
export class TactASTStore {
/**
* Constructs a TactASTStore with mappings to all major AST components.
* @param stdlibConstants Identifiers of constants defined in stdlib.
* @param stdlibIds Identifiers of AST elements defined in stdlib.
* @param programEntries Identifiers of AST elements defined on the top-level.
* @param functions Functions and methods including user-defined and special methods.
* @param constants Constants defined across the compilation unit.
Expand All @@ -36,7 +36,7 @@ export class TactASTStore {
* @param statements All executable statements within all functions of the project.
*/
constructor(
private stdlibConstants = new Set<number>(),
private stdlibIds = new Set<number>(),
private programEntries: Set<number>,
private functions: Map<number, ASTFunction | ASTReceive | ASTInitFunction>,
private constants: Map<number, ASTConstant>,
Expand Down Expand Up @@ -73,27 +73,48 @@ export class TactASTStore {
return acc;
}, [] as ASTNode[]);
}
/**
* Returns all the items defined within the program.
* @param items The collection of items (functions or constants).
* @param params Additional parameters:
* - includeStdlib: If true, includes items defined in stdlib.
* @returns An iterator for the items.
*/
private getItems<T extends { id: number }>(
items: Map<number, T>,
params: Partial<{ includeStdlib: boolean }> = {},
): IterableIterator<T> {
const { includeStdlib = false } = params;
if (includeStdlib) {
return items.values();
}
const userItems = Array.from(items.values()).filter(
(c) => !this.stdlibIds.has(c.id),
);
return userItems.values();
}

/**
* Returns all the functions and methods defined within the program.
* @param params Additional parameters:
* - includeStdlib: If true, includes functions defined in stdlib.
*/
getFunctions(): IterableIterator<ASTFunction | ASTReceive | ASTInitFunction> {
return this.functions.values();
getFunctions(
params: Partial<{ includeStdlib: boolean }> = {},
): IterableIterator<ASTFunction | ASTReceive | ASTInitFunction> {
return this.getItems(this.functions, params);
}

/**
* Returns all the constants defined within the program, including top-level constants
* and contract constants.
* @param allowStdlib If true, includes constants defined in stdlib.
* @param params Additional parameters:
* - includeStdlib: If true, includes constants defined in stdlib.
*/
getConstants(allowStdlib: boolean = true): IterableIterator<ASTConstant> {
if (allowStdlib) {
return this.constants.values();
}
const userConstants = Array.from(this.constants.values()).filter(
(c) => !this.stdlibConstants.has(c.id),
);
return userConstants.values();
getConstants(
params: Partial<{ includeStdlib: boolean }> = {},
): IterableIterator<ASTConstant> {
return this.getItems(this.constants, params);
}

getContracts(): IterableIterator<ASTContract> {
Expand Down
12 changes: 9 additions & 3 deletions src/internals/tactIRBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ function hasSubdirs(filePath: string, subdirs: string[]): boolean {
*/
export class ASTMapper {
private programEntries = new Set<number>();
private stdlibConstants = new Set<number>();
private stdlibIds = new Set<number>();
private functions = new Map<
number,
ASTFunction | ASTReceive | ASTInitFunction
Expand All @@ -118,6 +118,12 @@ export class ASTMapper {
constructor(private ast: TactAST) {
this.ast.functions.forEach((func) => {
this.programEntries.add(func.id);
if (
func.ref.file !== null &&
hasSubdirs(func.ref.file, STDLIB_PATH_ELEMENTS)
) {
this.stdlibIds.add(func.id);
}
if (func.kind == "def_function") {
this.processFunction(func);
} else {
Expand All @@ -129,7 +135,7 @@ export class ASTMapper {
constant.ref.file !== null &&
hasSubdirs(constant.ref.file, STDLIB_PATH_ELEMENTS)
) {
this.stdlibConstants.add(constant.id);
this.stdlibIds.add(constant.id);
}
this.programEntries.add(constant.id);
this.constants.set(constant.id, constant);
Expand All @@ -142,7 +148,7 @@ export class ASTMapper {

public getASTStore(): TactASTStore {
return new TactASTStore(
this.stdlibConstants,
this.stdlibIds,
this.programEntries,
this.functions,
this.constants,
Expand Down
39 changes: 39 additions & 0 deletions test/contracts/never-accessed-2.cfg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"projectName": "never-accessed-2",
"functions": [],
"contracts": [
{
"name": "FieldTest",
"methods": [
{
"name": "FieldTest.init",
"cfg": {
"nodes": [
{
"id": 1263,
"stmtID": 16730,
"srcEdges": [],
"dstEdges": []
}
],
"edges": []
}
},
{
"name": "FieldTest.use_f1",
"cfg": {
"nodes": [
{
"id": 1264,
"stmtID": 16735,
"srcEdges": [],
"dstEdges": []
}
],
"edges": []
}
}
]
}
]
}
8 changes: 8 additions & 0 deletions test/contracts/never-accessed-2.expected.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
test/contracts/never-accessed-2.tact:3:5:
2 | f1: Address;
> 3 | f2: Int as uint256 = 0;
^
4 |
Field is never used
Help: Consider removing the field
See: https://nowarp.github.io/docs/misti/docs/detectors/NeverAccessedVariables
12 changes: 12 additions & 0 deletions test/contracts/never-accessed-2.tact
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
contract FieldTest {
f1: Address;
f2: Int as uint256 = 0;

init() {
self.f1 = sender();
}

get fun use_f1(): Address {
return self.f1;
}
}
88 changes: 44 additions & 44 deletions test/contracts/readonly-1.cfg.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,101 +6,101 @@
"cfg": {
"nodes": [
{
"id": 1262,
"stmtID": 16729,
"id": 1415,
"stmtID": 18935,
"srcEdges": [],
"dstEdges": [
1264
1417
]
},
{
"id": 1263,
"stmtID": 16732,
"id": 1416,
"stmtID": 18938,
"srcEdges": [
1264
1417
],
"dstEdges": [
1266
1419
]
},
{
"id": 1265,
"stmtID": 16735,
"id": 1418,
"stmtID": 18941,
"srcEdges": [
1266
1419
],
"dstEdges": [
1268
1421
]
},
{
"id": 1267,
"stmtID": 16742,
"id": 1420,
"stmtID": 18948,
"srcEdges": [
1268
1421
],
"dstEdges": [
1270,
1274
1423,
1427
]
},
{
"id": 1269,
"stmtID": 16741,
"id": 1422,
"stmtID": 18947,
"srcEdges": [
1270
1423
],
"dstEdges": [
1272
1425
]
},
{
"id": 1271,
"stmtID": 16744,
"id": 1424,
"stmtID": 18950,
"srcEdges": [
1272
1425
],
"dstEdges": []
},
{
"id": 1273,
"stmtID": 16744,
"id": 1426,
"stmtID": 18950,
"srcEdges": [
1274
1427
],
"dstEdges": []
}
],
"edges": [
{
"id": 1264,
"src": 1262,
"dst": 1263
"id": 1417,
"src": 1415,
"dst": 1416
},
{
"id": 1266,
"src": 1263,
"dst": 1265
"id": 1419,
"src": 1416,
"dst": 1418
},
{
"id": 1268,
"src": 1265,
"dst": 1267
"id": 1421,
"src": 1418,
"dst": 1420
},
{
"id": 1270,
"src": 1267,
"dst": 1269
"id": 1423,
"src": 1420,
"dst": 1422
},
{
"id": 1272,
"src": 1269,
"dst": 1271
"id": 1425,
"src": 1422,
"dst": 1424
},
{
"id": 1274,
"src": 1267,
"dst": 1273
"id": 1427,
"src": 1420,
"dst": 1426
}
]
}
Expand Down
Loading

0 comments on commit 8618b0c

Please sign in to comment.