diff --git a/packages/cashc/src/Errors.ts b/packages/cashc/src/Errors.ts index 05ca2d22e..fd3551216 100644 --- a/packages/cashc/src/Errors.ts +++ b/packages/cashc/src/Errors.ts @@ -22,7 +22,7 @@ import { IntLiteralNode, TupleAssignmentNode, } from './ast/AST.js'; -import { Symbol, SymbolType } from './ast/SymbolTable.js'; +import { SymbolType } from './ast/SymbolTable.js'; import { Location } from './ast/Location.js'; import { BinaryOperator } from './ast/Operator.js'; @@ -98,14 +98,6 @@ export class ImportResolutionError extends CashScriptError { } } -export class UnusedVariableError extends CashScriptError { - constructor( - public symbol: Symbol, - ) { - super(symbol.definition as Node, `Unused variable ${symbol.name}`); - } -} - export class EmptyContractError extends CashScriptError { constructor( public node: ContractNode, diff --git a/packages/cashc/src/Warnings.ts b/packages/cashc/src/Warnings.ts new file mode 100644 index 000000000..68eb7e147 --- /dev/null +++ b/packages/cashc/src/Warnings.ts @@ -0,0 +1,41 @@ +import { IdentifierNode, Node } from './ast/AST.js'; +import { Symbol } from './ast/SymbolTable.js'; + +export class CashScriptWarning { + name: string; + message: string; + + constructor( + public node: Node, + message: string, + ) { + if (node.location) { + message += ` at ${node.location.start}`; + } + + this.name = this.constructor.name; + this.message = message; + } +} + +export class UnusedVariableWarning extends CashScriptWarning { + constructor( + public symbol: Symbol, + ) { + super(symbol.definition as Node, `Unused variable '${symbol.name}'`); + } +} + +export class UnusedAssignmentWarning extends CashScriptWarning { + constructor( + public identifier: IdentifierNode, + ) { + super(identifier, `Value assigned to '${identifier.name}' is never read`); + } +} + +export type CashScriptWarningListener = (warning: CashScriptWarning) => void; + +export const defaultWarningListener: CashScriptWarningListener = (warning) => { + console.warn(`Warning: ${warning.message}`); +}; diff --git a/packages/cashc/src/ast/AST.ts b/packages/cashc/src/ast/AST.ts index b10c36ec5..864db595b 100644 --- a/packages/cashc/src/ast/AST.ts +++ b/packages/cashc/src/ast/AST.ts @@ -120,6 +120,8 @@ export class FunctionDefinitionNode extends Node implements Named { } export class ParameterNode extends Node implements Named, Typed { + symbol?: Symbol; + constructor( public type: Type, public modifiers: Modifier[], @@ -140,6 +142,8 @@ export abstract class ControlStatementNode extends StatementNode { } export abstract class NonControlStatementNode extends StatementNode { } export class VariableDefinitionNode extends NonControlStatementNode implements Named, Typed { + symbol?: Symbol; + constructor( public type: Type, public modifiers: Modifier[], diff --git a/packages/cashc/src/ast/SymbolTable.ts b/packages/cashc/src/ast/SymbolTable.ts index 136169b41..32bf1b163 100644 --- a/packages/cashc/src/ast/SymbolTable.ts +++ b/packages/cashc/src/ast/SymbolTable.ts @@ -10,8 +10,18 @@ import { import { Modifier } from './Globals.js'; import { functionReturnType } from '../utils.js'; +export enum ReferenceKind { + READ = 'read', + WRITE = 'write', +} + +export interface Reference { + kind: ReferenceKind; + node: IdentifierNode; +} + export class Symbol { - references: IdentifierNode[] = []; + references: Reference[] = []; inlinedFrame?: DebugFrame; private constructor( @@ -30,6 +40,14 @@ export class Symbol { && this.definition.modifiers.includes(modifier); } + getReferences(kind: ReferenceKind): Reference[] { + return this.references.filter((reference) => reference.kind === kind); + } + + isUnused(): boolean { + return this.getReferences(ReferenceKind.READ).length === 0; + } + static variable(node: VariableDefinitionNode | ParameterNode): Symbol { return new Symbol(node.name, node.type, SymbolType.VARIABLE, node); } @@ -103,10 +121,10 @@ export class SymbolTable { return `[${Array.from(this.symbols).map((e) => e[1])}]`; } - unusedSymbols(): Symbol[] { + getUnmarkedUnusedSymbols(): Symbol[] { return Array.from(this.symbols) .map((e) => e[1]) .filter((s) => !s.hasModifier(Modifier.UNUSED)) - .filter((s) => s.references.length === 0); + .filter((s) => s.isUnused()); } } diff --git a/packages/cashc/src/compiler.ts b/packages/cashc/src/compiler.ts index 6724d8c18..dcfcf5fac 100644 --- a/packages/cashc/src/compiler.ts +++ b/packages/cashc/src/compiler.ts @@ -18,6 +18,7 @@ import { Ast } from './ast/AST.js'; import { checkVersionConstraints } from './ast/Pragma.js'; import { CashScriptErrorListener } from './ast/error-listeners.js'; import { MissingContractError } from './Errors.js'; +import { CashScriptWarningListener, defaultWarningListener } from './Warnings.js'; import { parseCode } from './parser.js'; import { createDiskResolver, @@ -28,6 +29,7 @@ import { import GenerateTargetTraversal from './generation/GenerateTargetTraversal.js'; import { FoldGlobalConstantsTraversal } from './semantic/FoldGlobalConstantsTraversal.js'; import SymbolTableTraversal from './semantic/SymbolTableTraversal.js'; +import UnusedCodeWarningsTraversal from './semantic/UnusedCodeTraversal.js'; import TypeCheckTraversal from './semantic/TypeCheckTraversal.js'; import EnsureFinalRequireTraversal from './semantic/EnsureFinalRequireTraversal.js'; import EnsureFunctionsSafeTraversal from './semantic/EnsureFunctionsSafeTraversal.js'; @@ -42,6 +44,7 @@ export const DEFAULT_COMPILER_OPTIONS: CompilerOptions = { export interface CompileOptions extends CompilerOptions { errorListener?: CashScriptErrorListener; + warningListener?: CashScriptWarningListener; } export interface CompileStringOptions extends CompileOptions { @@ -55,6 +58,8 @@ export interface CompileStringOptions extends CompileOptions { * @param compilerOptions - Optional compiler options that override the defaults. * @returns The compiled CashScript artifact, including ABI, bytecode and debug information. * @throws If the source code contains a syntax, semantic, or type error, or an import cannot be resolved. + * @remarks Compilation warnings (e.g. unused variables) are passed to the `warningListener` compiler + * option, or printed with `console.warn` when no listener is provided. */ export const compileString: (code: string, compilerOptions?: CompileStringOptions) => Artifact = compileStringInternal; @@ -103,7 +108,7 @@ function compileCode( resolver: ImportResolver, compilerOptions: CompileOptions & InternalCompilerOptions, ): Artifact { - const { errorListener, disableInlining, ...artifactCompilerOptions } = compilerOptions; + const { errorListener, warningListener, disableInlining, ...artifactCompilerOptions } = compilerOptions; const mergedCompilerOptions = { ...DEFAULT_COMPILER_OPTIONS, ...artifactCompilerOptions }; // Lexing + parsing @@ -117,7 +122,10 @@ function compileCode( // Semantic analysis ast = ast.accept(new FoldGlobalConstantsTraversal()) as Ast; + ast = ast.accept(new SymbolTableTraversal()) as Ast; + ast = ast.accept(new UnusedCodeWarningsTraversal(warningListener ?? defaultWarningListener)) as Ast; + ast = ast.accept(new TypeCheckTraversal()) as Ast; ast = ast.accept(new EnsureFunctionsSafeTraversal()) as Ast; ast = ast.accept(new EnsureFinalRequireTraversal()) as Ast; diff --git a/packages/cashc/src/generation/GenerateTargetTraversal.ts b/packages/cashc/src/generation/GenerateTargetTraversal.ts index c8ff35fdd..ddc25171a 100644 --- a/packages/cashc/src/generation/GenerateTargetTraversal.ts +++ b/packages/cashc/src/generation/GenerateTargetTraversal.ts @@ -63,7 +63,7 @@ import { ForNode, } from '../ast/AST.js'; import AstTraversal from '../ast/AstTraversal.js'; -import { GlobalFunction, Class, Modifier } from '../ast/Globals.js'; +import { GlobalFunction, Class } from '../ast/Globals.js'; import { BinaryOperator } from '../ast/Operator.js'; import { compileBinaryOp, @@ -428,7 +428,7 @@ export default class GenerateTargetTraversal extends AstTraversal { private dropUnusedParameters(parameters: ParameterNode[]): void { parameters - .filter((parameter) => parameter.modifiers.includes(Modifier.UNUSED)) + .filter((parameter) => parameter.symbol!.isUnused()) .sort((a, b) => this.getStackIndex(a.name) - this.getStackIndex(b.name)) .forEach((parameter) => { const stackIndex = this.getStackIndex(parameter.name); @@ -481,7 +481,7 @@ export default class GenerateTargetTraversal extends AstTraversal { } shouldEnforceFunctionParameterType(node: ParameterNode): boolean { - if (node.modifiers.includes(Modifier.UNUSED)) return false; + if (node.symbol!.isUnused()) return false; if (node.type === PrimitiveType.BOOL) return true; if (node.type instanceof BytesType && node.type.bound !== undefined) return true; return false; @@ -495,7 +495,7 @@ export default class GenerateTargetTraversal extends AstTraversal { visitVariableDefinition(node: VariableDefinitionNode): Node { node.expression = this.visit(node.expression); - if (node.modifiers.includes(Modifier.UNUSED)) { + if (node.symbol!.isUnused()) { this.emit(Op.OP_DROP, { location: node.location, positionHint: PositionHint.END }); this.popFromStack(); return node; @@ -523,10 +523,11 @@ export default class GenerateTargetTraversal extends AstTraversal { const reversedTargets = [...node.targets].reverse(); reversedTargets.forEach((target) => { - if (target.isReassignment) { - this.emitReplace(this.getStackIndex(target.identifier.name), node); - } else if (target.modifiers.includes(Modifier.UNUSED)) { + // Unused variables are never added the stack, so their defined or re-assigned value is dropped + if (target.identifier.symbol!.isUnused()) { this.emit(Op.OP_DROP, locationData); + } else if (target.isReassignment) { + this.emitReplace(this.getStackIndex(target.identifier.name), node); } else { this.emit(Op.OP_TOALTSTACK, locationData); parkedDeclarations.push(target.identifier.name); @@ -546,7 +547,7 @@ export default class GenerateTargetTraversal extends AstTraversal { const locationData = { location: node.location, positionHint: PositionHint.END }; node.targets - .filter((target) => target.modifiers.includes(Modifier.UNUSED)) + .filter((target) => target.identifier.symbol!.isUnused()) .sort((a, b) => this.getStackIndex(a.identifier.name) - this.getStackIndex(b.identifier.name)) .forEach((target) => { const stackIndex = this.getStackIndex(target.identifier.name); @@ -559,6 +560,14 @@ export default class GenerateTargetTraversal extends AstTraversal { visitAssign(node: AssignNode): Node { node.expression = this.visit(node.expression); + + // An unused variable never gets added to the stack, so the assigned value is dropped as well + if (node.identifier.symbol!.isUnused()) { + this.emit(Op.OP_DROP, { location: node.location, positionHint: PositionHint.END }); + this.popFromStack(); + return node; + } + if (this.scopeDepth > 0) { this.emitReplace(this.getStackIndex(node.identifier.name), node); this.popFromStack(); diff --git a/packages/cashc/src/index.ts b/packages/cashc/src/index.ts index 85f011e49..773eddcc2 100644 --- a/packages/cashc/src/index.ts +++ b/packages/cashc/src/index.ts @@ -1,4 +1,5 @@ export * from './Errors.js'; +export * from './Warnings.js'; export * as utils from '@cashscript/utils'; export { compileFile, compileString, type CompileOptions, type CompileStringOptions, diff --git a/packages/cashc/src/semantic/LowerGlobalConstantsTraversal.ts b/packages/cashc/src/semantic/LowerGlobalConstantsTraversal.ts index c6f4e68f9..7a38b0577 100644 --- a/packages/cashc/src/semantic/LowerGlobalConstantsTraversal.ts +++ b/packages/cashc/src/semantic/LowerGlobalConstantsTraversal.ts @@ -71,7 +71,6 @@ export class LowerGlobalConstantsTraversal extends AstTraversal { identifier.location = node.location; identifier.type = node.type; identifier.symbol = symbol; - symbol.references.push(identifier); const call = new FunctionCallNode(identifier, []); call.location = node.location; diff --git a/packages/cashc/src/semantic/SymbolTableTraversal.ts b/packages/cashc/src/semantic/SymbolTableTraversal.ts index 2dc708379..b95bf2b07 100644 --- a/packages/cashc/src/semantic/SymbolTableTraversal.ts +++ b/packages/cashc/src/semantic/SymbolTableTraversal.ts @@ -21,12 +21,13 @@ import { TupleAssignmentTarget, } from '../ast/AST.js'; import AstTraversal from '../ast/AstTraversal.js'; -import { SymbolTable, Symbol, SymbolType } from '../ast/SymbolTable.js'; +import { + SymbolTable, Symbol, SymbolType, ReferenceKind, +} from '../ast/SymbolTable.js'; import { createConstantLiteral } from './LowerGlobalConstantsTraversal.js'; import { RedefinitionError, UndefinedReferenceError, - UnusedVariableError, InvalidSymbolTypeError, ConstantModificationError, DuplicateTupleTargetError, @@ -70,11 +71,6 @@ export default class SymbolTableTraversal extends AstTraversal { node.parameters = this.visitList(node.parameters) as ParameterNode[]; node.functions = this.visitList(node.functions) as FunctionDefinitionNode[]; - const unusedSymbols = node.symbolTable.unusedSymbols(); - if (unusedSymbols.length !== 0) { - throw new UnusedVariableError(unusedSymbols[0]); - } - this.symbolTables.shift(); return node; } @@ -86,7 +82,8 @@ export default class SymbolTableTraversal extends AstTraversal { validateModifiers(node, node.modifiers, [Modifier.UNUSED]); - this.symbolTables[0].set(Symbol.variable(node)); + node.symbol = Symbol.variable(node); + this.symbolTables[0].set(node.symbol); return node; } @@ -106,11 +103,6 @@ export default class SymbolTableTraversal extends AstTraversal { node.parameters = this.visitList(node.parameters) as ParameterNode[]; node.body = this.visit(node.body); - const unusedSymbols = node.symbolTable.unusedSymbols(); - if (unusedSymbols.length !== 0) { - throw new UnusedVariableError(unusedSymbols[0]); - } - this.symbolTables.shift(); return node; } @@ -121,11 +113,6 @@ export default class SymbolTableTraversal extends AstTraversal { node.statements = this.visitOptionalList(node.statements) as StatementNode[]; - const unusedSymbols = node.symbolTable.unusedSymbols(); - if (unusedSymbols.length !== 0) { - throw new UnusedVariableError(unusedSymbols[0]); - } - this.symbolTables.shift(); return node; } @@ -139,11 +126,6 @@ export default class SymbolTableTraversal extends AstTraversal { node.update = this.visit(node.update) as AssignNode; node.block = this.visit(node.block); - const unusedSymbols = node.symbolTable.unusedSymbols(); - if (unusedSymbols.length !== 0) { - throw new UnusedVariableError(unusedSymbols[0]); - } - this.symbolTables.shift(); return node; } @@ -157,23 +139,16 @@ export default class SymbolTableTraversal extends AstTraversal { node.expression = this.visit(node.expression); - this.symbolTables[0].set(Symbol.variable(node)); + node.symbol = Symbol.variable(node); + this.symbolTables[0].set(node.symbol); return node; } visitAssign(node: AssignNode): Node { - const symbol = this.symbolTables[0].get(node.identifier.name); - - if (!symbol) { - throw new UndefinedReferenceError(node.identifier); - } - - if (symbol.hasModifier(Modifier.CONSTANT)) { - throw new ConstantModificationError(node, node.identifier.name); - } - - super.visitAssign(node); + node.identifier.symbol = this.resolveAssignmentTarget(node, node.identifier); + node.expression = this.visit(node.expression); + this.addReference(ReferenceKind.WRITE, node.identifier); return node; } @@ -186,12 +161,8 @@ export default class SymbolTableTraversal extends AstTraversal { seenTargetNames.add(target.identifier.name); if (target.isReassignment) { - if (this.symbolTables[0].get(target.identifier.name)?.hasModifier(Modifier.CONSTANT)) { - throw new ConstantModificationError(node, target.identifier.name); - } - - target.identifier = this.visit(target.identifier) as IdentifierNode; - target.type = target.identifier.symbol!.type; + target.identifier.symbol = this.resolveAssignmentTarget(node, target.identifier); + target.type = target.identifier.symbol.type; } else { const definition = createTupleVariableDefinition(node, target); @@ -201,11 +172,17 @@ export default class SymbolTableTraversal extends AstTraversal { validateModifiers(definition, definition.modifiers, [Modifier.CONSTANT, Modifier.UNUSED]); - this.symbolTables[0].set(Symbol.variable(definition)); + target.identifier.symbol = Symbol.variable(definition); + this.symbolTables[0].set(target.identifier.symbol); } }); node.tuple = this.visit(node.tuple); + + node.targets + .filter((target) => target.isReassignment) + .forEach((target) => this.addReference(ReferenceKind.WRITE, target.identifier)); + return node; } @@ -255,7 +232,7 @@ export default class SymbolTableTraversal extends AstTraversal { } node.symbol = symbol; - node.symbol.references.push(node); + this.addReference(ReferenceKind.READ, node); // Keep track of final use of variables for code generation (excluding console statements) if (!this.insideConsoleStatement) { @@ -264,6 +241,36 @@ export default class SymbolTableTraversal extends AstTraversal { return node; } + + private addReference(kind: ReferenceKind, node: IdentifierNode): void { + node.symbol!.references.push({ kind, node }); + } + + // Assignment targets are resolved without counting as a use of the variable, since only reads count + private resolveAssignmentTarget(node: AssignNode | TupleAssignmentNode, identifier: IdentifierNode): Symbol { + const symbol = this.symbolTables[0].get(identifier.name); + + if (!symbol) { + throw new UndefinedReferenceError(identifier); + } + + if (symbol.hasModifier(Modifier.CONSTANT)) { + throw new ConstantModificationError(node, identifier.name); + } + + if (symbol.symbolType !== SymbolType.VARIABLE) { + throw new InvalidSymbolTypeError(identifier, SymbolType.VARIABLE); + } + + if (symbol.hasModifier(Modifier.UNUSED)) { + throw new InvalidModifierError(identifier, `Cannot assign to variable '${identifier.name}' because it is marked 'unused'`); + } + + // An assignment still needs the variable to be on the stack, so it does count as its final use for code generation + this.currentFunction.opRolls.set(identifier.name, identifier); + + return symbol; + } } function validateModifiers( diff --git a/packages/cashc/src/semantic/UnusedCodeTraversal.ts b/packages/cashc/src/semantic/UnusedCodeTraversal.ts new file mode 100644 index 000000000..32276d932 --- /dev/null +++ b/packages/cashc/src/semantic/UnusedCodeTraversal.ts @@ -0,0 +1,129 @@ +import { + AssignNode, + BlockNode, + ContractNode, + DoWhileNode, + ForNode, + FunctionDefinitionNode, + IdentifierNode, + Node, + TupleAssignmentNode, + WhileNode, +} from '../ast/AST.js'; +import AstTraversal from '../ast/AstTraversal.js'; +import { Symbol, SymbolTable } from '../ast/SymbolTable.js'; +import { CashScriptWarningListener, UnusedAssignmentWarning, UnusedVariableWarning } from '../Warnings.js'; + +export default class UnusedCodeWarningsTraversal extends AstTraversal { + private pendingWrites: Map> = new Map>(); + private readWrites: Set = new Set(); + private reportedScopes: Set = new Set(); + + constructor(private warningListener: CashScriptWarningListener) { + super(); + } + + visitContract(node: ContractNode): Node { + super.visitContract(node); + this.collectUnusedVariableWarnings(node.symbolTable!); + return node; + } + + visitFunctionDefinition(node: FunctionDefinitionNode): Node { + super.visitFunctionDefinition(node); + this.collectUnusedVariableWarnings(node.symbolTable!); + this.collectUnusedAssignmentWarnings(); + return node; + } + + visitBlock(node: BlockNode): Node { + super.visitBlock(node); + this.collectUnusedVariableWarnings(node.symbolTable!); + return node; + } + + visitFor(node: ForNode): Node { + this.visit(node.init); + this.visitLoop(() => { + this.visit(node.condition); + this.visit(node.block); + this.visit(node.update); + }); + this.collectUnusedVariableWarnings(node.symbolTable!); + return node; + } + + visitWhile(node: WhileNode): Node { + this.visitLoop(() => { + this.visit(node.condition); + this.visit(node.block); + }); + return node; + } + + visitDoWhile(node: DoWhileNode): Node { + this.visitLoop(() => { + this.visit(node.block); + this.visit(node.condition); + }); + return node; + } + + // Loops are visited twice (in execution order), so that a read at the start of the loop is seen to follow + // a write later in the loop, as it does in the next iteration + private visitLoop(visitIteration: () => void): void { + visitIteration(); + visitIteration(); + } + + visitAssign(node: AssignNode): Node { + // The expression is visited before the write is recorded, so that reads inside the expression + // (e.g. x = x + 1) do not count as reading the assigned value + this.visit(node.expression); + this.recordWrite(node.identifier); + return node; + } + + visitTupleAssignment(node: TupleAssignmentNode): Node { + this.visit(node.tuple); + node.targets + .filter((target) => target.isReassignment) + .forEach((target) => this.recordWrite(target.identifier)); + return node; + } + + visitIdentifier(node: IdentifierNode): Node { + this.pendingWrites.get(node.symbol!)?.forEach((write) => this.readWrites.add(write)); + this.pendingWrites.delete(node.symbol!); + return node; + } + + private recordWrite(identifier: IdentifierNode): void { + if (this.readWrites.has(identifier)) return; + + const symbol = identifier.symbol!; + const pendingWrites = this.pendingWrites.get(symbol) ?? new Set(); + pendingWrites.add(identifier); + this.pendingWrites.set(symbol, pendingWrites); + } + + // Each scope is reported once, even though scopes inside loops are visited twice + private collectUnusedVariableWarnings(symbolTable: SymbolTable): void { + if (this.reportedScopes.has(symbolTable)) return; + this.reportedScopes.add(symbolTable); + + symbolTable.getUnmarkedUnusedSymbols().forEach((symbol) => this.warningListener(new UnusedVariableWarning(symbol))); + } + + // At the end of a function, every read that could follow its assignments has been visited + private collectUnusedAssignmentWarnings(): void { + this.pendingWrites.forEach((writes, symbol) => { + // Writes to a variable that is never read at all are covered by its unused variable warning + if (symbol.isUnused()) return; + writes.forEach((write) => this.warningListener(new UnusedAssignmentWarning(write))); + }); + + this.pendingWrites.clear(); + this.readWrites.clear(); + } +} diff --git a/packages/cashc/test/compiler/compiler.test.ts b/packages/cashc/test/compiler/compiler.test.ts index 19fef16ce..7c5050007 100644 --- a/packages/cashc/test/compiler/compiler.test.ts +++ b/packages/cashc/test/compiler/compiler.test.ts @@ -1,6 +1,7 @@ import { URL } from 'url'; import { getSubdirectories, readCashFiles } from '../test-utils.js'; import * as Errors from '../../src/Errors.js'; +import * as Warnings from '../../src/Warnings.js'; import { compileString } from '../../src/index.js'; import type { CashScriptErrorListener } from '../../src/index.js'; @@ -12,6 +13,14 @@ contract Test() { } `; const INVALID_SOURCE = 'contract Test() { function unlock() { require(true) } }'; +const UNUSED_VARIABLE_SOURCE = ` +contract Test() { + function hello(sig s, pubkey pk) { + string x = 'Hello'; + require(checkSig(s, pk)); + } +} +`; describe('Compiler', () => { describe('Compilation errors', () => { @@ -23,7 +32,6 @@ describe('Compiler', () => { it(`${file.fn} should throw ${errorType}`, () => { // Retrieve the correct Error constructor from the Errors.ts file const expectedError = Errors[errorType as keyof typeof Errors]; - if (!expectedError) throw new Error(`Invalid test configuration: error ${errorType} does not exist`); expect(() => compileString(file.contents)).toThrow(expectedError); @@ -33,6 +41,31 @@ describe('Compiler', () => { }); }); + describe('Compilation warnings', () => { + const warningTypes = getSubdirectories(new URL('../warnings/', import.meta.url)); + + warningTypes.forEach((warningType) => { + describe(warningType.toString(), () => { + readCashFiles(new URL(`../warnings/${warningType}`, import.meta.url)).forEach((file) => { + it(`${file.fn} should report ${warningType}`, () => { + // Retrieve the correct Warning constructor from the Warnings.ts file + const expectedWarning = Warnings[warningType as keyof typeof Warnings]; + if (!expectedWarning) throw new Error(`Invalid test configuration: warning ${warningType} does not exist`); + + const warnings: Warnings.CashScriptWarning[] = []; + try { + compileString(file.contents, { warningListener: (warning) => { warnings.push(warning); } }); + } catch { + // ignore compilation errors from later phases + } + + expect(warnings).toContainEqual(expect.any(expectedWarning)); + }); + }); + }); + }); + }); + describe('Custom error listener', () => { it('uses the custom error listener for parse errors', () => { const errors: string[] = []; @@ -59,14 +92,15 @@ describe('Compiler', () => { expect(errors).toHaveLength(1); }); - it('does not include custom error listeners in compiler artifact options', () => { + it('does not include custom error or warning listeners in compiler artifact options', () => { const errorListener: CashScriptErrorListener = { syntaxError(): void { throw new Error('Unexpected parse error'); }, }; - const artifact = compileString(VALID_SOURCE, { enforceLocktimeGuard: false, errorListener }); + const compileOptions = { enforceLocktimeGuard: false, errorListener, warningListener: () => {} }; + const artifact = compileString(VALID_SOURCE, compileOptions); expect(artifact.compiler.options).toEqual({ enforceFunctionParameterTypes: true, @@ -74,4 +108,27 @@ describe('Compiler', () => { }); }); }); + + describe('Custom warning listener', () => { + it('uses the custom warning listener for compilation warnings', () => { + const warnings: Warnings.CashScriptWarning[] = []; + compileString(UNUSED_VARIABLE_SOURCE, { warningListener: (warning) => { warnings.push(warning); } }); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toBeInstanceOf(Warnings.UnusedVariableWarning); + expect(warnings[0].message).toEqual("Unused variable 'x' at Line 4, Column 8"); + }); + + it('prints warnings with console.warn when no warning listener is provided', () => { + const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + try { + compileString(UNUSED_VARIABLE_SOURCE); + expect(consoleWarn).toHaveBeenCalledTimes(1); + expect(consoleWarn).toHaveBeenCalledWith("Warning: Unused variable 'x' at Line 4, Column 8"); + } finally { + consoleWarn.mockRestore(); + } + }); + }); }); diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/loop_reassignment_after_final_read.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/loop_reassignment_after_final_read.cash new file mode 100644 index 000000000..84bc843bf --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/loop_reassignment_after_final_read.cash @@ -0,0 +1,12 @@ +contract Test() { + function spend(int a) { + int x = 1; + require(x == 1); + int i = 0; + while (i < a) { + x = i; + i = i + 1; + } + require(true); + } +} diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/parameter_reassignment.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/parameter_reassignment.cash new file mode 100644 index 000000000..0c6c3fb8e --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/parameter_reassignment.cash @@ -0,0 +1,7 @@ +contract Test() { + function spend(int a) { + require(a == 1); + a = 2; + require(true); + } +} diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_after_final_read.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_after_final_read.cash new file mode 100644 index 000000000..605a93048 --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_after_final_read.cash @@ -0,0 +1,8 @@ +contract Test() { + function spend() { + int x = 1; + require(x == 1); + x = 2; + require(true); + } +} diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_in_branch.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_in_branch.cash new file mode 100644 index 000000000..4dfdcf798 --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/reassignment_in_branch.cash @@ -0,0 +1,10 @@ +contract Test(int a) { + function spend() { + int x = 1; + require(x == 1); + if (a > 0) { + x = 2; + } + require(true); + } +} diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/self_referencing_reassignment.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/self_referencing_reassignment.cash new file mode 100644 index 000000000..6d865705e --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/self_referencing_reassignment.cash @@ -0,0 +1,8 @@ +contract Test() { + function spend() { + int x = 1; + require(x == 1); + x = x + 1; + require(true); + } +} diff --git a/packages/cashc/test/warnings/UnusedAssignmentWarning/tuple_reassignment_after_final_read.cash b/packages/cashc/test/warnings/UnusedAssignmentWarning/tuple_reassignment_after_final_read.cash new file mode 100644 index 000000000..e04d9fa2a --- /dev/null +++ b/packages/cashc/test/warnings/UnusedAssignmentWarning/tuple_reassignment_after_final_read.cash @@ -0,0 +1,9 @@ +contract Test() { + function spend() { + bytes head = 0x00; + bytes tail = 0x01; + require(head == 0x00 && tail == 0x01); + (head, tail) = 0x1234.split(1); + require(true); + } +} diff --git a/packages/cashc/test/compiler/UnusedVariableError/final_variable_definition.cash b/packages/cashc/test/warnings/UnusedVariableWarning/final_variable_definition.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/final_variable_definition.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/final_variable_definition.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_contract_parameter.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_contract_parameter.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_contract_parameter.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_contract_parameter.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_function_parameter.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_function_parameter.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_function_parameter.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_function_parameter.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_global_function_local.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_global_function_local.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_global_function_local.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_global_function_local.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_global_function_parameter.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_global_function_parameter.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_global_function_parameter.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_global_function_parameter.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_scope_variable.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_scope_variable.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_scope_variable.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_scope_variable.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_tuple_target.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_tuple_target.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_tuple_target.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_tuple_target.cash diff --git a/packages/cashc/test/compiler/UnusedVariableError/unused_variable.cash b/packages/cashc/test/warnings/UnusedVariableWarning/unused_variable.cash similarity index 100% rename from packages/cashc/test/compiler/UnusedVariableError/unused_variable.cash rename to packages/cashc/test/warnings/UnusedVariableWarning/unused_variable.cash diff --git a/packages/cashc/test/warnings/UnusedVariableWarning/write_only_scoped_variables.cash b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_scoped_variables.cash new file mode 100644 index 000000000..ce3b7e0b2 --- /dev/null +++ b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_scoped_variables.cash @@ -0,0 +1,12 @@ +contract Test() { + function spend(int a) { + int x = 0; + bytes head = 0x00; + bytes tail = 0x01; + if (a > 0) { + x = a; + (head, tail) = 0x1234.split(1); + } + require(tail == 0x34); + } +} diff --git a/packages/cashc/test/warnings/UnusedVariableWarning/write_only_tuple_target.cash b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_tuple_target.cash new file mode 100644 index 000000000..b6a490b9c --- /dev/null +++ b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_tuple_target.cash @@ -0,0 +1,8 @@ +contract Test() { + function spend() { + bytes head = 0x00; + bytes tail = 0x01; + (head, tail) = 0x1234.split(1); + require(tail == 0x34); + } +} diff --git a/packages/cashc/test/warnings/UnusedVariableWarning/write_only_variable.cash b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_variable.cash new file mode 100644 index 000000000..f52980095 --- /dev/null +++ b/packages/cashc/test/warnings/UnusedVariableWarning/write_only_variable.cash @@ -0,0 +1,7 @@ +contract Test() { + function spend() { + int x = 1; + x = 2; + require(true); + } +} diff --git a/website/docs/compiler/compiler.md b/website/docs/compiler/compiler.md index 3f509b762..262083fc0 100644 --- a/website/docs/compiler/compiler.md +++ b/website/docs/compiler/compiler.md @@ -127,6 +127,21 @@ const Doubler = compileString(source, { files: { './math.cash': mathSource } }); Imports inside imported files are resolved relative to the *importing* file, but their keys in `files` remain relative to the main source. For example, if `lib/a.cash` contains `import "./b.cash";`, that file must be provided under the key `lib/b.cash`. Package imports such as `import "@example/math-lib/math.cash"` are looked up verbatim, so they must be provided under exactly that key. ::: +### Compilation Warnings + +Some issues, such as unused variables that are not marked [`unused`](/docs/language/contracts#intentionally-unused-values), do not prevent compilation but produce a compiler warning instead. By default these warnings are printed with `console.warn`. When compiling from JavaScript, a custom `warningListener` can be passed as a compiler option to capture the structured warnings instead. It is called for each warning. The default listener is exported as `defaultWarningListener`, so a custom listener can compose with it to keep the standard console output. + +```ts +import { compileString, defaultWarningListener } from 'cashc'; + +const P2PKH = compileString(source, { + warningListener: (warning) => { + defaultWarningListener(warning); // still print the warning to the console + myDiagnostics.push(warning); + }, +}); +``` + ### Compiler Options ```ts interface CompilerOptions { diff --git a/website/docs/language/contracts.md b/website/docs/language/contracts.md index 4bec99458..5712dd939 100644 --- a/website/docs/language/contracts.md +++ b/website/docs/language/contracts.md @@ -265,7 +265,7 @@ contract P2PKH(bytes20 pkh) { Variables can be declared by specifying their type and name. All variables need to be initialised at the time of their declaration, but can be reassigned later on — unless specifying the `constant` keyword. Since CashScript is strongly typed and has no type inference, it is not possible to use keywords such as `var` or `let` to declare variables. :::note -CashScript disallows variable shadowing and unused variables unless they are explicitly marked `unused`. +CashScript disallows variable shadowing, and the compiler emits a warning for unused variables unless they are explicitly marked `unused`. ::: #### Example @@ -276,7 +276,11 @@ string constant myString = 'Bitcoin Cash'; ### Intentionally unused values -Parameters and local variables that intentionally have no references can use the `unused` modifier. These values are dropped from the stack immediately after their declaration. A declaration marked `unused` cannot be referenced later. Some use cases for this include padding the contract bytecode in order to get a higher opcost budget, or nonces in order to differentiate between similar contracts. +A parameter or local variable that is declared but never used results in a compiler warning. If the variable is intended to be unused, this warning can be silenced by marking the variable as `unused`. Unused parameters and local variables are dropped from the stack immediately after their declaration, and no [parameter type enforcement](/docs/compiler#enforcefunctionparametertypes) is generated for unused parameters. + +Some use cases for intentionally unused values include padding the contract bytecode in order to get a higher opcost budget, or nonces in order to differentiate between similar contracts. + +The compiler also warns when a value is assigned to a variable that is never read afterwards, since such an assignment has no effect on the contract. #### Example diff --git a/website/docs/releases/release-notes.md b/website/docs/releases/release-notes.md index 5f8cf21da..00432bf0e 100644 --- a/website/docs/releases/release-notes.md +++ b/website/docs/releases/release-notes.md @@ -12,6 +12,9 @@ title: Release Notes - :sparkles: Add support for top-level global constants. - :sparkles: Allow for simple arithmetic / concatenation operations in global constant definitions. - :sparkles: Add `unused` modifier for parameters or variables that are intentionally unused. +- :hammer_and_wrench: Unused variables that are not marked `unused` now produce a compiler warning instead of a compilation error. Only reads count as usage, so variables that are only assigned to are reported as well. Warnings are printed with `console.warn`, or passed to the new `warningListener` compiler option. +- :sparkles: Add a compiler warning for values assigned to a variable that are never read afterwards. +- :racehorse: Treat parameters and variables that are never read the same as explicitly `unused`-marked ones: they are dropped from the stack immediately, and no parameter type enforcement is generated for them. - :sparkles: Add support for `import` directives to share user-defined functions across files. - :sparkles: Resolve package imports (e.g. `import "pkg/math.cash"`) from `node_modules`, so contract libraries can be installed as npm packages. - :sparkles: Add support for reassigning existing variables in tuple destructuring (e.g. `(a, b) = swap(a, b)`), optionally mixed with fresh declarations.