diff --git a/src/lib/LibCodeGen.sol b/src/lib/LibCodeGen.sol index df1ad56..4e605e2 100644 --- a/src/lib/LibCodeGen.sol +++ b/src/lib/LibCodeGen.sol @@ -17,10 +17,10 @@ uint256 constant MAX_LINE_LENGTH = 120; /// needs to match what formatters expect. string constant NEWLINE_DUE_TO_MAX_LENGTH = "\n "; -/// Thrown when a contract name is not a Solidity identifier. Such a name cannot -/// be interpolated into a file path or a constant declaration. +/// Thrown when a name is not a Solidity identifier. Such a name cannot be +/// interpolated into a file path or a constant declaration. /// @param name The rejected name. -error InvalidContractName(string name); +error InvalidIdentifier(string name); /// Thrown when a bytecode hash is asked for at an address that holds no code. /// @param instance The address that holds no code. @@ -34,14 +34,19 @@ error CodelessInstance(address instance); library LibCodeGen { /// Reverts unless `name` is a Solidity identifier: at least one character, /// drawn from ASCII letters, digits, `_` and `$`, and not starting with a - /// digit. A contract name is such an identifier, and restricting it to one - /// is also what makes it safe to interpolate into a path: no identifier - /// contains a path separator, and none of them is `.` or `..`. + /// digit. Every name this library interpolates verbatim into generated + /// source or into a path is one, and being an identifier is what makes that + /// interpolation safe. A declaration named by an identifier is the + /// declaration the caller asked for and no other, because no identifier + /// carries a space, a `;` or any other character that ends a declaration or + /// starts another. A path built from an identifier stays a direct child of + /// the directory it is joined to, because no identifier contains a path + /// separator and none of them is `.` or `..`. /// @param name The name to check. - function requireContractName(string memory name) internal pure { + function requireIdentifier(string memory name) internal pure { bytes memory nameBytes = bytes(name); if (nameBytes.length == 0) { - revert InvalidContractName(name); + revert InvalidIdentifier(name); } for (uint256 i = 0; i < nameBytes.length; i++) { bytes1 char = nameBytes[i]; @@ -49,7 +54,7 @@ library LibCodeGen { bool isDigit = char >= 0x30 && char <= 0x39; bool isUnderscoreOrDollar = char == 0x5F || char == 0x24; if (!(isLetter || isUnderscoreOrDollar || (isDigit && i > 0))) { - revert InvalidContractName(name); + revert InvalidIdentifier(name); } } } @@ -253,7 +258,7 @@ library LibCodeGen { /// @return A string containing the Solidity code for the described by meta /// hash constant. function describedByMetaHashConstantString(Vm vm, string memory name) internal view returns (string memory) { - requireContractName(name); + requireIdentifier(name); bytes memory describedByMeta = vm.readFileBinary(string.concat("meta/", name, ".rain.meta")); return bytes32ConstantString( vm, @@ -269,7 +274,8 @@ library LibCodeGen { /// @param vm The Vm instance used to format values as strings. /// @param comment The comment to include above the constant declaration. /// An empty comment emits no comment line. - /// @param name The name of the constant. + /// @param name The name of the constant, interpolated verbatim. Has to be a + /// Solidity identifier. /// @param data The bytes data for the constant. /// @return A string containing the Solidity code for the bytes constant. function bytesConstantString(Vm vm, string memory comment, string memory name, bytes memory data) @@ -277,6 +283,7 @@ library LibCodeGen { pure returns (string memory) { + requireIdentifier(name); string memory hexData = LibHexString.bytesToHex(vm, data); return string.concat( commentPrefix(comment), @@ -297,7 +304,8 @@ library LibCodeGen { /// @param vm The Vm instance used to format values as strings. /// @param comment The comment to include above the constant declaration. /// An empty comment emits no comment line. - /// @param name The name of the constant. + /// @param name The name of the constant, interpolated verbatim. Has to be a + /// Solidity identifier. /// @param data The uint8 data for the constant. /// @return A string containing the Solidity code for the uint8 constant. function uint8ConstantString(Vm vm, string memory comment, string memory name, uint8 data) @@ -305,6 +313,7 @@ library LibCodeGen { pure returns (string memory) { + requireIdentifier(name); string memory intString = vm.toString(data); return string.concat( commentPrefix(comment), @@ -325,7 +334,8 @@ library LibCodeGen { /// @param vm The Vm instance used to format values as strings. /// @param comment The comment to include above the constant declaration. /// An empty comment emits no comment line. - /// @param name The name of the constant. + /// @param name The name of the constant, interpolated verbatim. Has to be a + /// Solidity identifier. /// @param data The bytes32 value for the constant. /// @return A string containing the Solidity code for the bytes32 constant. function bytes32ConstantString(Vm vm, string memory comment, string memory name, bytes32 data) @@ -333,6 +343,7 @@ library LibCodeGen { pure returns (string memory) { + requireIdentifier(name); string memory hexString = vm.toString(data); return string.concat( commentPrefix(comment), @@ -354,7 +365,8 @@ library LibCodeGen { /// @param vm The Vm instance used to format values as strings. /// @param comment The comment to include above the constant declaration. /// An empty comment emits no comment line. - /// @param name The name of the constant. + /// @param name The name of the constant, interpolated verbatim. Has to be a + /// Solidity identifier. /// @param data The address for the constant. /// @return A string containing the Solidity code for the address constant. function addressConstantString(Vm vm, string memory comment, string memory name, address data) @@ -362,6 +374,7 @@ library LibCodeGen { pure returns (string memory) { + requireIdentifier(name); string memory addressString = vm.toString(data); return string.concat( commentPrefix(comment), diff --git a/src/lib/LibFs.sol b/src/lib/LibFs.sol index a4439a5..19599df 100644 --- a/src/lib/LibFs.sol +++ b/src/lib/LibFs.sol @@ -51,7 +51,7 @@ library LibFs { /// @param contractName The name of the contract, interpolated verbatim. /// @return The file path as a string. function pathForContractIn(string memory dir, string memory contractName) private pure returns (string memory) { - LibCodeGen.requireContractName(contractName); + LibCodeGen.requireIdentifier(contractName); return string.concat(dir, "/", contractName, ".sol"); } diff --git a/test/concrete/CodeGennable.sol b/test/concrete/CodeGennable.sol index dde87a0..d93b5ce 100644 --- a/test/concrete/CodeGennable.sol +++ b/test/concrete/CodeGennable.sol @@ -7,7 +7,7 @@ pragma solidity =0.8.25; /// code. It carries no behaviour of its own: what the tests want from it is a /// stable, non-zero `codehash` to feed to `bytecodeHashConstantString` and /// `buildFileForContract`. The name is asserted on in -/// `LibCodeGen.requireContractName.t.sol` and used as a contract name in +/// `LibCodeGen.requireIdentifier.t.sol` and used as a contract name in /// `LibCodeGen.describedByMetaHashConstantString.t.sol`, so it is not free to /// change. contract CodeGennable {} diff --git a/test/lib/LibCodeGenSlow.sol b/test/lib/LibCodeGenSlow.sol index 9f2854f..6fe7aa3 100644 --- a/test/lib/LibCodeGenSlow.sol +++ b/test/lib/LibCodeGenSlow.sol @@ -14,7 +14,7 @@ uint256 constant SLOW_LINE_LENGTH = 120; string constant SLOW_WRAP = "\n "; /// @dev Every character a Solidity identifier may begin with, spelled out one by -/// one rather than as byte ranges. `LibCodeGen.requireContractName` decides with +/// one rather than as byte ranges. `LibCodeGen.requireIdentifier` decides with /// range comparisons, so an off by one at either end of a range shows up here as /// a disagreement instead of moving both sides at once. string constant SLOW_HEAD_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_$"; @@ -263,7 +263,7 @@ library LibCodeGenSlow { /// True if `name` is a Solidity identifier, decided by membership of the /// written out alphabets rather than by arithmetic. - function isContractNameSlow(string memory name) internal pure returns (bool) { + function isIdentifierSlow(string memory name) internal pure returns (bool) { bytes memory nameBytes = bytes(name); if (nameBytes.length == 0) { return false; diff --git a/test/src/lib/LibCodeGen.addressConstantString.t.sol b/test/src/lib/LibCodeGen.addressConstantString.t.sol index cc7305e..a6f8706 100644 --- a/test/src/lib/LibCodeGen.addressConstantString.t.sol +++ b/test/src/lib/LibCodeGen.addressConstantString.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, MAX_LINE_LENGTH} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, MAX_LINE_LENGTH, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// @dev A checksummed address literal, 42 characters like every other, so the @@ -16,6 +16,16 @@ string constant SOME_ADDRESS_STRING = "0xc51a14251b0dcF0ae24A96b7153991378938f5F /// declaration. The output is source code that must COMPILE, so these assert the /// exact emitted text rather than that it merely contains the address. contract LibCodeGenAddressConstantStringTest is Test { + /// Reachable only through an external call so that a rejected name reverts + /// the call rather than aborting the test. + function callAddressConstantString(string memory comment, string memory name, address data) + external + pure + returns (string memory) + { + return LibCodeGen.addressConstantString(vm, comment, name, data); + } + function testAddressConstantString() external view { assertEq( LibCodeGen.addressConstantString( @@ -84,16 +94,48 @@ contract LibCodeGenAddressConstantStringTest is Test { /// declaration built from those literals, wrapped exactly when measuring the /// one line form says it does not fit. Fuzzed over every input because each /// term of the library's hand computed sum has to be right for this to hold. - function testAddressConstantStringMatchesMeasuredLine(string memory comment, string memory name, address data) + /// The name is built from the seed rather than fuzzed directly because + /// random bytes are essentially never an identifier, and a name that is not + /// one is rejected before anything is emitted at all. + function testAddressConstantStringMatchesMeasuredLine(string memory comment, bytes memory seed, address data) external view { + string memory name = LibCodeGenSlow.nameFromSeedSlow(seed); assertEq( LibCodeGen.addressConstantString(vm, comment, name, data), LibCodeGenSlow.addressConstantStringSlow(vm, comment, name, data) ); } + /// The name is interpolated verbatim into an `address constant` declaration, + /// so a name that is not a Solidity identifier is refused rather than + /// emitted. A space or a `-` produces a file that does not compile, and a + /// `;` one that compiles into a different set of declarations than the + /// caller asked for. + function testAddressConstantStringRejectsNonIdentifierName() external { + string[5] memory names = ["SOME NAME", "SOME;NAME", "SOME-NAME", "", "0LEADING"]; + for (uint256 i = 0; i < names.length; i++) { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, names[i])); + this.callAddressConstantString("/// @dev Bad name.", names[i], SOME_ADDRESS); + } + } + + /// The set of names that emit is exactly the set of Solidity identifiers. + /// Stated against the reference alphabet so that neither a check that + /// rejects too much nor one that rejects nothing at all passes. + function testAddressConstantStringNameMustBeIdentifier(string memory name, address data) external { + if (LibCodeGenSlow.isIdentifierSlow(name)) { + assertEq( + this.callAddressConstantString("/// @dev Fuzz.", name, data), + LibCodeGenSlow.addressConstantStringSlow(vm, "/// @dev Fuzz.", name, data) + ); + } else { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); + this.callAddressConstantString("/// @dev Fuzz.", name, data); + } + } + /// An empty comment emits no comment line rather than an empty one. Two /// consecutive newlines are a blank line that `forge fmt` collapses, so a /// generated file carrying one is not stable under the formatter and a diff --git a/test/src/lib/LibCodeGen.bytes32ConstantString.t.sol b/test/src/lib/LibCodeGen.bytes32ConstantString.t.sol index 4b5defa..c36066f 100644 --- a/test/src/lib/LibCodeGen.bytes32ConstantString.t.sol +++ b/test/src/lib/LibCodeGen.bytes32ConstantString.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, MAX_LINE_LENGTH} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, MAX_LINE_LENGTH, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// @dev A `bytes32` literal is 66 characters whatever the value, so the @@ -16,6 +16,16 @@ string constant SOME_HASH_STRING = "0x2573004ac3a9ee7fc8d73654d76386f1b6b99e34cd /// declaration. The output is source code that must COMPILE, so these assert the /// exact emitted text rather than that it merely contains the value. contract LibCodeGenBytes32ConstantStringTest is Test { + /// Reachable only through an external call so that a rejected name reverts + /// the call rather than aborting the test. + function callBytes32ConstantString(string memory comment, string memory name, bytes32 data) + external + pure + returns (string memory) + { + return LibCodeGen.bytes32ConstantString(vm, comment, name, data); + } + function testBytes32ConstantString() external view { assertEq( LibCodeGen.bytes32ConstantString( @@ -80,16 +90,48 @@ contract LibCodeGenBytes32ConstantStringTest is Test { /// built from those literals, wrapped exactly when measuring the one line /// form says it does not fit. Fuzzed over every input because each term of /// the library's hand computed sum has to be right for this to hold. - function testBytes32ConstantStringMatchesMeasuredLine(string memory comment, string memory name, bytes32 data) + /// The name is built from the seed rather than fuzzed directly because + /// random bytes are essentially never an identifier, and a name that is not + /// one is rejected before anything is emitted at all. + function testBytes32ConstantStringMatchesMeasuredLine(string memory comment, bytes memory seed, bytes32 data) external view { + string memory name = LibCodeGenSlow.nameFromSeedSlow(seed); assertEq( LibCodeGen.bytes32ConstantString(vm, comment, name, data), LibCodeGenSlow.bytes32ConstantStringSlow(vm, comment, name, data) ); } + /// The name is interpolated verbatim into a `bytes32 constant` declaration, + /// so a name that is not a Solidity identifier is refused rather than + /// emitted. A space or a `-` produces a file that does not compile, and a + /// `;` one that compiles into a different set of declarations than the + /// caller asked for. + function testBytes32ConstantStringRejectsNonIdentifierName() external { + string[5] memory names = ["SOME NAME", "SOME;NAME", "SOME-NAME", "", "0LEADING"]; + for (uint256 i = 0; i < names.length; i++) { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, names[i])); + this.callBytes32ConstantString("/// @dev Bad name.", names[i], SOME_HASH); + } + } + + /// The set of names that emit is exactly the set of Solidity identifiers. + /// Stated against the reference alphabet so that neither a check that + /// rejects too much nor one that rejects nothing at all passes. + function testBytes32ConstantStringNameMustBeIdentifier(string memory name, bytes32 data) external { + if (LibCodeGenSlow.isIdentifierSlow(name)) { + assertEq( + this.callBytes32ConstantString("/// @dev Fuzz.", name, data), + LibCodeGenSlow.bytes32ConstantStringSlow(vm, "/// @dev Fuzz.", name, data) + ); + } else { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); + this.callBytes32ConstantString("/// @dev Fuzz.", name, data); + } + } + /// An empty comment emits no comment line rather than an empty one. Two /// consecutive newlines are a blank line that `forge fmt` collapses, so a /// generated file carrying one is not stable under the formatter and a diff --git a/test/src/lib/LibCodeGen.bytesConstantString.t.sol b/test/src/lib/LibCodeGen.bytesConstantString.t.sol index 508c12c..f0da10e 100644 --- a/test/src/lib/LibCodeGen.bytesConstantString.t.sol +++ b/test/src/lib/LibCodeGen.bytesConstantString.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, MAX_LINE_LENGTH} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, MAX_LINE_LENGTH, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// @dev 32 bytes, so the hex literal is 64 characters and the whole declaration @@ -27,6 +27,16 @@ string constant HEX_34 = "2573004ac3a9ee7fc8d73654d76386f1b6b99e34cdf86a689c4691 /// that measures the line instead, and pin the decision either side of the /// maximum. contract LibCodeGenBytesConstantStringTest is Test { + /// Reachable only through an external call so that a rejected name reverts + /// the call rather than aborting the test. + function callBytesConstantString(string memory comment, string memory name, bytes memory data) + external + pure + returns (string memory) + { + return LibCodeGen.bytesConstantString(vm, comment, name, data); + } + /// The short case: blank line, comment, then the whole declaration on one /// line. The blank line is what separates this constant from whatever the /// caller concatenated before it. @@ -85,17 +95,49 @@ contract LibCodeGenBytesConstantStringTest is Test { /// Whatever the comment, name and data, the emitted text is the declaration /// built from those literals, wrapped exactly when measuring the one line /// form says it does not fit. Fuzzed over every input because each term of - /// the library's hand computed sum has to be right for this to hold. - function testBytesConstantStringMatchesMeasuredLine(string memory comment, string memory name, bytes memory data) + /// the library's hand computed sum has to be right for this to hold. The + /// name is built from the seed rather than fuzzed directly because random + /// bytes are essentially never an identifier, and a name that is not one is + /// rejected before anything is emitted at all. + function testBytesConstantStringMatchesMeasuredLine(string memory comment, bytes memory seed, bytes memory data) external pure { + string memory name = LibCodeGenSlow.nameFromSeedSlow(seed); assertEq( LibCodeGen.bytesConstantString(vm, comment, name, data), LibCodeGenSlow.bytesConstantStringSlow(vm, comment, name, data) ); } + /// The name is interpolated verbatim into a `bytes constant` declaration, so + /// a name that is not a Solidity identifier is refused rather than emitted. + /// A space or a `-` produces a file that does not compile, and a `;` one + /// that compiles into a different set of declarations than the caller asked + /// for. + function testBytesConstantStringRejectsNonIdentifierName() external { + string[5] memory names = ["SOME NAME", "SOME;NAME", "SOME-NAME", "", "0LEADING"]; + for (uint256 i = 0; i < names.length; i++) { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, names[i])); + this.callBytesConstantString("/// @dev Bad name.", names[i], hex"12345678"); + } + } + + /// The set of names that emit is exactly the set of Solidity identifiers. + /// Stated against the reference alphabet so that neither a check that + /// rejects too much nor one that rejects nothing at all passes. + function testBytesConstantStringNameMustBeIdentifier(string memory name, bytes memory data) external { + if (LibCodeGenSlow.isIdentifierSlow(name)) { + assertEq( + this.callBytesConstantString("/// @dev Fuzz.", name, data), + LibCodeGenSlow.bytesConstantStringSlow(vm, "/// @dev Fuzz.", name, data) + ); + } else { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); + this.callBytesConstantString("/// @dev Fuzz.", name, data); + } + } + /// The hex the declaration carries is the data, unchanged and unprefixed, so /// the constant compiles back to the bytes it was generated from. A `0x` /// inside a `hex"..."` literal does not compile at all. diff --git a/test/src/lib/LibCodeGen.describedByMetaHashConstantString.t.sol b/test/src/lib/LibCodeGen.describedByMetaHashConstantString.t.sol index 015150e..d75ee38 100644 --- a/test/src/lib/LibCodeGen.describedByMetaHashConstantString.t.sol +++ b/test/src/lib/LibCodeGen.describedByMetaHashConstantString.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, MAX_LINE_LENGTH, InvalidContractName} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, MAX_LINE_LENGTH, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// @dev `describedByMetaHashConstantString` reads `meta/.rain.meta`, and @@ -215,7 +215,7 @@ contract LibCodeGenDescribedByMetaHashConstantStringTest is Test { } function assertRejectsName(string memory name) internal { - vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, name)); + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); this.callDescribedByMetaHash(name); } diff --git a/test/src/lib/LibCodeGen.requireContractName.t.sol b/test/src/lib/LibCodeGen.requireIdentifier.t.sol similarity index 80% rename from test/src/lib/LibCodeGen.requireContractName.t.sol rename to test/src/lib/LibCodeGen.requireIdentifier.t.sol index 5e34593..9a1888d 100644 --- a/test/src/lib/LibCodeGen.requireContractName.t.sol +++ b/test/src/lib/LibCodeGen.requireIdentifier.t.sol @@ -3,32 +3,33 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, InvalidContractName} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow, SLOW_HEAD_ALPHABET, SLOW_TAIL_ALPHABET} from "test/lib/LibCodeGenSlow.sol"; -/// @title LibCodeGenRequireContractNameTest -/// @notice `requireContractName` is what stands between a caller supplied -/// contract name and a filesystem path built out of it. The rule it enforces is -/// the Solidity identifier: that is what a contract name is, and it is also a -/// character set that cannot express a path separator, a parent directory or an -/// empty basename. -contract LibCodeGenRequireContractNameTest is Test { +/// @title LibCodeGenRequireIdentifierTest +/// @notice `requireIdentifier` is what stands between a caller supplied name and +/// the filesystem path or the generated declaration built out of it. The rule it +/// enforces is the Solidity identifier: that is what a contract name and a +/// constant name both are, and it is also a character set that cannot express a +/// path separator, a parent directory, an empty basename, or a character that +/// ends one declaration and starts another. +contract LibCodeGenRequireIdentifierTest is Test { /// Reachable only through an external call so that the revert can be caught /// rather than aborting the test. - function callRequireContractName(string memory name) external pure { - LibCodeGen.requireContractName(name); + function callRequireIdentifier(string memory name) external pure { + LibCodeGen.requireIdentifier(name); } function assertAccepted(string memory name) internal view { - this.callRequireContractName(name); + this.callRequireIdentifier(name); } /// Rejection is asserted as the whole error, selector and argument: every /// rejection below pins that the revert carries the name that was rejected, /// so a build script that generates many files says which one it choked on. function assertRejected(string memory name) internal { - vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, name)); - this.callRequireContractName(name); + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); + this.callRequireIdentifier(name); } /// Asserts that no byte of `alphabet` means anything to a filesystem. The @@ -48,7 +49,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// The names real consumers pass. Every `script/Build.sol` in the Rain org /// passes the concrete contract's own name, so these are the shape that must /// keep working. - function testRequireContractNameAcceptsContractNames() external view { + function testRequireIdentifierAcceptsContractNames() external view { assertAccepted("CodeGennable"); assertAccepted("PythWords"); assertAccepted("RaindexV6SubParser"); @@ -58,7 +59,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// Every character class Solidity allows in an identifier, at both the first /// position and a later one, so the rule is Solidity's rather than a /// narrower guess at it. - function testRequireContractNameAcceptsIdentifierCharacters() external view { + function testRequireIdentifierAcceptsIdentifierCharacters() external view { assertAccepted("A"); assertAccepted("z"); assertAccepted("_"); @@ -81,7 +82,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// the ranges they sit against, because a name has to be an identifier /// apart from the one character under test for the boundary to be what /// decides it. - function testRequireContractNameRangeBoundaries() external { + function testRequireIdentifierRangeBoundaries() external { assertAccepted("A"); assertAccepted("Z"); assertAccepted("a"); @@ -99,13 +100,13 @@ contract LibCodeGenRequireContractNameTest is Test { /// An empty name would build `src/generated/.sol` and `meta/.rain.meta`: /// hidden dotfiles that no compiler picks up and `ls` does not show, written /// with a success report. - function testRequireContractNameRejectsEmpty() external { + function testRequireIdentifierRejectsEmpty() external { assertRejected(""); } /// A digit cannot open a Solidity identifier, so it cannot open a contract /// name either. - function testRequireContractNameRejectsLeadingDigit() external { + function testRequireIdentifierRejectsLeadingDigit() external { assertRejected("0Foo"); assertRejected("9"); } @@ -113,7 +114,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// A separator would target a subdirectory, and `..` would escape the /// generated directory entirely. Both are refused by the character rule /// rather than by a special case for them. - function testRequireContractNameRejectsPathCharacters() external { + function testRequireIdentifierRejectsPathCharacters() external { assertRejected("sub/Foo"); assertRejected(".."); assertRejected("."); @@ -127,7 +128,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// A name that is otherwise fine but carries anything outside the identifier /// set is refused. A dot in particular would collide with the extension the /// caller appends. - function testRequireContractNameRejectsOtherCharacters() external { + function testRequireIdentifierRejectsOtherCharacters() external { assertRejected("Foo.sol"); assertRejected("Foo Bar"); assertRejected("Foo-Bar"); @@ -141,12 +142,12 @@ contract LibCodeGenRequireContractNameTest is Test { /// character by character rather than restating the range arithmetic the /// library decides with. The name is generated as a `string` rather than as /// `bytes`, which is all that separates this from - /// `testRequireContractNameMatchesAlphabet`: uniform bytes are an identifier + /// `testRequireIdentifierMatchesAlphabet`: uniform bytes are an identifier /// only by accident, and the `string` generator lands on one several times /// more often, so it is this test that carries the accepted half of the /// domain. - function testRequireContractNameAcceptedNamesAreIdentifiers(string memory name) external { - if (LibCodeGenSlow.isContractNameSlow(name)) { + function testRequireIdentifierAcceptedNamesAreIdentifiers(string memory name) external { + if (LibCodeGenSlow.isIdentifierSlow(name)) { assertAccepted(name); } else { assertRejected(name); @@ -157,12 +158,12 @@ contract LibCodeGenRequireContractNameTest is Test { /// filesystem, so no accepted name can leave the directory it is /// interpolated into. Stated over the alphabets rather than over names the /// fuzzer happens to get accepted, so every run of the property carries the - /// whole of it. `testRequireContractNameMatchesAlphabet` is the other half: + /// whole of it. `testRequireIdentifierMatchesAlphabet` is the other half: /// it pins an accepted name to the head alphabet at its first byte and the /// tail alphabet at every later one, and together the two cover the entire /// accepted set. Both alphabets are checked because a name draws from both /// and nothing states that one contains the other. - function testRequireContractNameAlphabetCannotTraverse() external pure { + function testRequireIdentifierAlphabetCannotTraverse() external pure { assertNoFilesystemBytes(SLOW_HEAD_ALPHABET); assertNoFilesystemBytes(SLOW_TAIL_ALPHABET); } @@ -172,7 +173,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// accepted ranges is left to a chosen example, and the oracle is the /// alphabet written out character by character rather than the same range /// arithmetic the library uses. - function testRequireContractNameEveryLeadingByte() external { + function testRequireIdentifierEveryLeadingByte() external { for (uint256 i = 0; i < 256; i++) { string memory name = string(bytes.concat(bytes1(uint8(i)))); if (LibCodeGenSlow.containsSlow(SLOW_HEAD_ALPHABET, bytes1(uint8(i)))) { @@ -186,7 +187,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// Exhaustive over the trailing byte, behind a leading byte that is itself /// accepted. The digits separate this from the leading case: they are /// accepted here and rejected there. - function testRequireContractNameEveryTrailingByte() external { + function testRequireIdentifierEveryTrailingByte() external { for (uint256 i = 0; i < 256; i++) { string memory name = string(bytes.concat(bytes("A"), bytes1(uint8(i)))); if (LibCodeGenSlow.containsSlow(SLOW_TAIL_ALPHABET, bytes1(uint8(i)))) { @@ -202,9 +203,9 @@ contract LibCodeGenRequireContractNameTest is Test { /// character, so this fails if either end of any range moves, rather than /// following the library the way an inlined copy of its own arithmetic /// would. - function testRequireContractNameMatchesAlphabet(bytes memory nameBytes) external { + function testRequireIdentifierMatchesAlphabet(bytes memory nameBytes) external { string memory name = string(nameBytes); - if (LibCodeGenSlow.isContractNameSlow(name)) { + if (LibCodeGenSlow.isIdentifierSlow(name)) { assertAccepted(name); } else { assertRejected(name); @@ -216,7 +217,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// identifier, so without constructing them the accepted half of the domain /// is never exercised at all and a check that rejected everything would /// still pass. - function testRequireContractNameAcceptsGeneratedIdentifiers(bytes memory seed) external view { + function testRequireIdentifierAcceptsGeneratedIdentifiers(bytes memory seed) external view { string memory name = LibCodeGenSlow.nameFromSeedSlow(seed); assertTrue(bytes(name).length > 0, "generated an empty name"); assertAccepted(name); @@ -225,7 +226,7 @@ contract LibCodeGenRequireContractNameTest is Test { /// A single byte outside the alphabet is enough to reject a name that is /// otherwise an identifier, wherever in the name it sits. A check that only /// looked at the first or the last character would pass this. - function testRequireContractNameRejectsOneBadByte(bytes memory seed, uint256 position, uint8 badByte) external { + function testRequireIdentifierRejectsOneBadByte(bytes memory seed, uint256 position, uint8 badByte) external { bytes memory nameBytes = bytes(LibCodeGenSlow.nameFromSeedSlow(seed)); vm.assume(!LibCodeGenSlow.containsSlow(SLOW_TAIL_ALPHABET, bytes1(badByte))); nameBytes[position % nameBytes.length] = bytes1(badByte); diff --git a/test/src/lib/LibCodeGen.uint8ConstantString.t.sol b/test/src/lib/LibCodeGen.uint8ConstantString.t.sol index ba97799..307b039 100644 --- a/test/src/lib/LibCodeGen.uint8ConstantString.t.sol +++ b/test/src/lib/LibCodeGen.uint8ConstantString.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; -import {LibCodeGen, MAX_LINE_LENGTH} from "src/lib/LibCodeGen.sol"; +import {LibCodeGen, MAX_LINE_LENGTH, InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// @title LibCodeGenUint8ConstantStringTest @@ -14,6 +14,16 @@ import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; /// that measures the line instead, and pin the decision either side of the /// maximum. contract LibCodeGenUint8ConstantStringTest is Test { + /// Reachable only through an external call so that a rejected name reverts + /// the call rather than aborting the test. + function callUint8ConstantString(string memory comment, string memory name, uint8 data) + external + pure + returns (string memory) + { + return LibCodeGen.uint8ConstantString(vm, comment, name, data); + } + /// The short case: blank line, comment, then the whole declaration on one /// line, decimal rather than hex. function testUint8ConstantString() external pure { @@ -77,17 +87,48 @@ contract LibCodeGenUint8ConstantStringTest is Test { /// Whatever the comment, name and value, the emitted text is the declaration /// built from those literals, wrapped exactly when measuring the one line - /// form says it does not fit. - function testUint8ConstantStringMatchesMeasuredLine(string memory comment, string memory name, uint8 data) + /// form says it does not fit. The name is built from the seed rather than + /// fuzzed directly because random bytes are essentially never an identifier, + /// and a name that is not one is rejected before anything is emitted at all. + function testUint8ConstantStringMatchesMeasuredLine(string memory comment, bytes memory seed, uint8 data) external pure { + string memory name = LibCodeGenSlow.nameFromSeedSlow(seed); assertEq( LibCodeGen.uint8ConstantString(vm, comment, name, data), LibCodeGenSlow.uint8ConstantStringSlow(vm, comment, name, data) ); } + /// The name is interpolated verbatim into a `uint8 constant` declaration, so + /// a name that is not a Solidity identifier is refused rather than emitted. + /// A space or a `-` produces a file that does not compile, and a `;` one + /// that compiles into a different set of declarations than the caller asked + /// for. + function testUint8ConstantStringRejectsNonIdentifierName() external { + string[5] memory names = ["SOME NAME", "SOME;NAME", "SOME-NAME", "", "0LEADING"]; + for (uint256 i = 0; i < names.length; i++) { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, names[i])); + this.callUint8ConstantString("/// @dev Bad name.", names[i], 42); + } + } + + /// The set of names that emit is exactly the set of Solidity identifiers. + /// Stated against the reference alphabet so that neither a check that + /// rejects too much nor one that rejects nothing at all passes. + function testUint8ConstantStringNameMustBeIdentifier(string memory name, uint8 data) external { + if (LibCodeGenSlow.isIdentifierSlow(name)) { + assertEq( + this.callUint8ConstantString("/// @dev Fuzz.", name, data), + LibCodeGenSlow.uint8ConstantStringSlow(vm, "/// @dev Fuzz.", name, data) + ); + } else { + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, name)); + this.callUint8ConstantString("/// @dev Fuzz.", name, data); + } + } + /// The literal the declaration carries parses back to the value it was /// generated from, so the constant is not silently truncated or /// reformatted. The literal is sliced out of the emitted text, so the round diff --git a/test/src/lib/LibFs.buildFileForContract.t.sol b/test/src/lib/LibFs.buildFileForContract.t.sol index ecaa42e..025ad2d 100644 --- a/test/src/lib/LibFs.buildFileForContract.t.sol +++ b/test/src/lib/LibFs.buildFileForContract.t.sol @@ -4,7 +4,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; import {LibFs, GENERATED_DIR} from "src/lib/LibFs.sol"; -import {InvalidContractName, CodelessInstance} from "src/lib/LibCodeGen.sol"; +import {InvalidIdentifier, CodelessInstance} from "src/lib/LibCodeGen.sol"; import {CodeGennable} from "test/concrete/CodeGennable.sol"; import {LibFsExternal} from "test/concrete/LibFsExternal.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; @@ -299,7 +299,7 @@ contract LibFsBuildFileForContractTest is Test { /// asserted here is that the write inherits it, and that nothing lands on /// disk when it does. function assertNameRejected(string memory contractName) internal { - vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, contractName)); + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, contractName)); iExternal.buildFileForContract(vm, address(this), contractName, ""); } @@ -343,7 +343,7 @@ contract LibFsBuildFileForContractTest is Test { /// ones named above. function testBuildFileForContractRejectsEveryNonIdentifierName(bytes memory nameBytes) external { string memory contractName = string(nameBytes); - vm.assume(!LibCodeGenSlow.isContractNameSlow(contractName)); + vm.assume(!LibCodeGenSlow.isIdentifierSlow(contractName)); assertNameRejected(contractName); } } diff --git a/test/src/lib/LibFs.t.sol b/test/src/lib/LibFs.t.sol index 67d1823..c77f791 100644 --- a/test/src/lib/LibFs.t.sol +++ b/test/src/lib/LibFs.t.sol @@ -4,7 +4,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; import {LibFs, GENERATED_DIR} from "src/lib/LibFs.sol"; -import {InvalidContractName} from "src/lib/LibCodeGen.sol"; +import {InvalidIdentifier} from "src/lib/LibCodeGen.sol"; import {LibFsExternal} from "test/concrete/LibFsExternal.sol"; import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; @@ -133,7 +133,7 @@ contract LibFsTest is Test { /// No path is produced for the name at all, and the error carries the name /// that was rejected so a build failure says which one it was. function assertNameRejected(string memory contractName) internal { - vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, contractName)); + vm.expectRevert(abi.encodeWithSelector(InvalidIdentifier.selector, contractName)); iExternal.pathForContract(contractName); } @@ -167,7 +167,7 @@ contract LibFsTest is Test { /// string is essentially never an identifier. function testPathForContractRejectsEveryNonIdentifierName(bytes memory nameBytes) external { string memory contractName = string(nameBytes); - vm.assume(!LibCodeGenSlow.isContractNameSlow(contractName)); + vm.assume(!LibCodeGenSlow.isIdentifierSlow(contractName)); assertNameRejected(contractName); }