diff --git a/packages/cashc/src/compiler.ts b/packages/cashc/src/compiler.ts index dcc21d97b..38770b027 100644 --- a/packages/cashc/src/compiler.ts +++ b/packages/cashc/src/compiler.ts @@ -108,8 +108,8 @@ function compileCode( resolver: ImportResolver, compilerOptions: CompileOptions & InternalCompilerOptions, ): Artifact { - const { errorListener, warningListener, disableInlining, ...artifactCompilerOptions } = compilerOptions; - const mergedCompilerOptions = { ...DEFAULT_COMPILER_OPTIONS, ...artifactCompilerOptions }; + const { errorListener, warningListener, disableInlining } = compilerOptions; + const mergedCompilerOptions = mergeCompilerOptions(compilerOptions); // Lexing + parsing let ast = parseCode(code, errorListener); @@ -170,3 +170,13 @@ function compileCode( return generateArtifact(ast, optimisationResult.script, code, debug, mergedCompilerOptions, fingerprint); } + +// Only the known compiler options are recorded in the artifact, and an option that is passed as undefined +// falls back to its default value (rather than disabling it) +function mergeCompilerOptions(compilerOptions: CompilerOptions): CompilerOptions { + return { + enforceFunctionParameterTypes: + compilerOptions.enforceFunctionParameterTypes ?? DEFAULT_COMPILER_OPTIONS.enforceFunctionParameterTypes, + enforceLocktimeGuard: compilerOptions.enforceLocktimeGuard ?? DEFAULT_COMPILER_OPTIONS.enforceLocktimeGuard, + }; +} diff --git a/packages/cashc/src/generation/GenerateTargetTraversal.ts b/packages/cashc/src/generation/GenerateTargetTraversal.ts index 442596b4b..1e6e825c0 100644 --- a/packages/cashc/src/generation/GenerateTargetTraversal.ts +++ b/packages/cashc/src/generation/GenerateTargetTraversal.ts @@ -519,6 +519,9 @@ export default class GenerateTargetTraversal extends AstTraversal { const scopedReassign = this.scopeDepth > 0 && node.targets.some((target) => target.isReassignment); if (!scopedReassign) { this.popFromStack(node.targets.length); + node.targets + .filter((target) => target.isReassignment) + .forEach((target) => this.renameReassignedVariable(target.identifier.name)); node.targets.forEach((target) => this.pushToStack(target.identifier.name)); this.dropUnusedTupleTargets(node); return node; @@ -579,11 +582,19 @@ export default class GenerateTargetTraversal extends AstTraversal { this.popFromStack(); } else { this.popFromStack(); + this.renameReassignedVariable(node.identifier.name); this.pushToStack(node.identifier.name); } return node; } + // Outside of a loop/branch, a reassignment leaves the old value on the stack (it gets dropped with the rest of the + // stack at the end of the function). We make that old slot anonymous, so it can never be read as the new value. + private renameReassignedVariable(name: string): void { + const stackIndex = this.getStackIndex(name, true); + if (stackIndex !== -1) this.stack[stackIndex] = '(value)'; + } + // This algorithm can be optimised for hardcoded depths // See thesis for explanation emitReplace(index: number, node: Node): void { diff --git a/packages/cashc/src/semantic/SymbolTableTraversal.ts b/packages/cashc/src/semantic/SymbolTableTraversal.ts index f52edb7ec..80a2e3832 100644 --- a/packages/cashc/src/semantic/SymbolTableTraversal.ts +++ b/packages/cashc/src/semantic/SymbolTableTraversal.ts @@ -164,7 +164,15 @@ export default class SymbolTableTraversal extends AstTraversal { if (target.isReassignment) { target.identifier.symbol = this.resolveAssignmentTarget(node, target.identifier); target.type = target.identifier.symbol.type; - } else { + } + }); + + // The new targets are only declared after the right-hand side, so they cannot be used in their own declaration + node.tuple = this.visit(node.tuple); + + node.targets + .filter((target) => !target.isReassignment) + .forEach((target) => { const definition = createTupleVariableDefinition(node, target); if (this.symbolTables[0].get(target.identifier.name)) { @@ -175,10 +183,7 @@ export default class SymbolTableTraversal extends AstTraversal { 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) diff --git a/packages/cashc/test/compiler/UndefinedReferenceError/tuple_target_used_in_own_declaration.cash b/packages/cashc/test/compiler/UndefinedReferenceError/tuple_target_used_in_own_declaration.cash new file mode 100644 index 000000000..41b5424e3 --- /dev/null +++ b/packages/cashc/test/compiler/UndefinedReferenceError/tuple_target_used_in_own_declaration.cash @@ -0,0 +1,7 @@ +contract Test() { + function spend(bytes x) { + // `a` is declared by this statement, so it cannot be used on its right-hand side + bytes a, bytes b = a.split(1); + require(a + b == x); + } +} diff --git a/packages/cashc/test/compiler/compiler.test.ts b/packages/cashc/test/compiler/compiler.test.ts index 7c5050007..70c1f909d 100644 --- a/packages/cashc/test/compiler/compiler.test.ts +++ b/packages/cashc/test/compiler/compiler.test.ts @@ -3,7 +3,7 @@ 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'; +import type { CashScriptErrorListener, CompileStringOptions } from '../../src/index.js'; const VALID_SOURCE = ` contract Test() { @@ -109,6 +109,39 @@ describe('Compiler', () => { }); }); + describe('Compiler options', () => { + const LOCKTIME_SOURCE = ` +contract Test() { + function unlock() { + require(tx.locktime >= 1); + } +} +`; + + it('uses the default value for options that are passed as undefined', () => { + const artifact = compileString(LOCKTIME_SOURCE, { + enforceFunctionParameterTypes: undefined, + enforceLocktimeGuard: undefined, + }); + + expect(artifact.bytecode).toEqual(compileString(LOCKTIME_SOURCE).bytecode); + expect(artifact.compiler.options).toEqual({ + enforceFunctionParameterTypes: true, + enforceLocktimeGuard: true, + }); + }); + + it('only includes known options in compiler artifact options', () => { + const compileOptions = { enforceLocktimeGuard: false, unknownOption: true } as CompileStringOptions; + const artifact = compileString(VALID_SOURCE, compileOptions); + + expect(artifact.compiler.options).toEqual({ + enforceFunctionParameterTypes: true, + enforceLocktimeGuard: false, + }); + }); + }); + describe('Custom warning listener', () => { it('uses the custom warning listener for compilation warnings', () => { const warnings: Warnings.CashScriptWarning[] = []; diff --git a/packages/cashc/test/generation/final-use.test.ts b/packages/cashc/test/generation/final-use.test.ts new file mode 100644 index 000000000..050cb6ed8 --- /dev/null +++ b/packages/cashc/test/generation/final-use.test.ts @@ -0,0 +1,113 @@ +/* final-use.test.ts + * + * - The symbol table records the final use of every variable, and code generation rolls the variable off the stack + * (rather than picking it) at that read. This only works if both traversals visit the children of every node in + * the same order, so this file checks that no variable is read again after it was rolled. + * - The checks run on every valid contract file, and on a contract that reads the same variable in every child of + * each expression node type. + */ + +import { URL } from 'url'; +import { FunctionDefinitionNode, IdentifierNode } from '../../src/ast/AST.js'; +import GenerateTargetTraversal from '../../src/generation/GenerateTargetTraversal.js'; +import { compileFile, compileString } from '../../src/index.js'; +import { readCashFiles } from '../test-utils.js'; + +const EVERY_EXPRESSION_SOURCE = ` +function add(int a, int b) returns (int) { + return a + b; +} + +function sliceInFunction(bytes b, int v) returns (bytes) { + return b.slice(v, v + 1); +} + +contract EveryExpression() { + function slice(bytes b, int v) { + require(b.slice(v, v + 1) == 0x11); + } + + function sliceAfterReassignment(bytes b, int v) { + v = v + 1; + require(b.slice(v, v + 1) == 0x11); + } + + function binaryOp(int v) { + require(v - (v + 1) == -1); + } + + function split(bytes b, int v) { + require(b.split(v)[0].split(v)[1] == 0x); + } + + function builtinFunctionCall(int v) { + require(within(v, v - 1, v + 1)); + } + + function userFunctionCall(bytes b, int v) { + require(add(v, v + 1) == 3); + require(sliceInFunction(b, v) == 0x11); + } + + function multiSig(sig s, pubkey pk) { + require(checkMultiSig([s, s], [pk, pk])); + } + + function instantiation(bytes b, bytes20 pkh) { + require(new LockingBytecodeNullData([b, pkh, b + pkh]) == new LockingBytecodeP2PKH(pkh) + b); + } + + function tupleAssignment(bytes b, int v) { + bytes x, bytes y = b.split(v + v); + (x, y) = y.split(v); + require(x + y == b); + } +} +`; + +describe('Final use of variables', () => { + it('should not read a variable after its final use in a contract that reads it in every child of each expression', () => { + expectNoReadsAfterFinalUse(() => compileString(EVERY_EXPRESSION_SOURCE, { warningListener: () => {} })); + }); + + readCashFiles(new URL('../valid-contract-files', import.meta.url)).forEach(({ fn }) => { + it(`should not read a variable after its final use in ${fn}`, () => { + const sourceFile = new URL(`../valid-contract-files/${fn}`, import.meta.url); + expectNoReadsAfterFinalUse(() => compileFile(sourceFile, { warningListener: () => {} })); + }); + }); +}); + +interface VariableRead { + node: IdentifierNode; + isFinalUse: boolean; +} + +function expectNoReadsAfterFinalUse(compile: () => void): void { + const readsPerFunction = new Map(); + + // Code generation checks isOpRoll() for every variable read, so we use it to record the reads in order + const { isOpRoll } = GenerateTargetTraversal.prototype; + const spy = vi.spyOn(GenerateTargetTraversal.prototype, 'isOpRoll').mockImplementation( + function (this: GenerateTargetTraversal, node: IdentifierNode) { + const isFinalUse = isOpRoll.call(this, node); + const { currentFunction } = this as unknown as { currentFunction: FunctionDefinitionNode }; + readsPerFunction.set(currentFunction, [...readsPerFunction.get(currentFunction) ?? [], { node, isFinalUse }]); + return isFinalUse; + }, + ); + + try { + compile(); + } finally { + spy.mockRestore(); + } + + readsPerFunction.forEach((reads, func) => { + reads.forEach(({ node, isFinalUse }, index) => { + if (!isFinalUse) return; + const laterReads = reads.slice(index + 1).filter((read) => read.node.name === node.name); + expect(laterReads, `'${node.name}' is read after its final use in ${func.name}`).toEqual([]); + }); + }); +} diff --git a/packages/cashscript/test/debugging.test.ts b/packages/cashscript/test/debugging.test.ts index 887233fb5..5cbb1cfbb 100644 --- a/packages/cashscript/test/debugging.test.ts +++ b/packages/cashscript/test/debugging.test.ts @@ -75,6 +75,14 @@ describe('Debugging tests', () => { expect(transaction).not.toLog(expectedLog); }); + it('should log the current value of a reassigned variable after its final use', async () => { + const transaction = new TransactionBuilder({ provider }) + .addInput(contractUtxo, contractTestLogs.unlock.test_log_reassigned_after_final_use(1n)) + .addOutput({ to: contractTestLogs.address, amount: 10000n }); + + expect(transaction).toLog(new RegExp('^\\[Input #0] Test.cash:63 v: 2$')); + }); + it('should only log console.log statements from the called function', async () => { const transaction = new TransactionBuilder({ provider }) .addInput(contractUtxo, contractTestLogs.unlock.secondFunction()) diff --git a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts index 644d721e1..34215caef 100644 --- a/packages/cashscript/test/fixture/debugging/debugging_contracts.ts +++ b/packages/cashscript/test/fixture/debugging/debugging_contracts.ts @@ -468,6 +468,14 @@ contract Test(pubkey owner) { require(tx.outputs[this.activeInputIndex].value == inputValue - 1000); } } + + function test_log_reassigned_after_final_use(int a) { + int v = a; + v = v + 1; + require(v == 2); + console.log('v:', v); + require(a == 1); + } } `;