Validate the emitted constant name in all four emitters

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,22 +34,27 @@ 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];
             bool isLetter = (char >= 0x41 && char <= 0x5A) || (char >= 0x61 && char <= 0x7A);
             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,14 +274,16 @@ 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)
         internal
         pure
         returns (string memory)
     {
+        requireIdentifier(name);
         string memory hexData = LibHexString.bytesToHex(vm, data);
         return string.concat(
             commentPrefix(comment),
@@ -297,14 +304,16 @@ 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)
         internal
         pure
         returns (string memory)
     {
+        requireIdentifier(name);
         string memory intString = vm.toString(data);
         return string.concat(
             commentPrefix(comment),
@@ -325,14 +334,16 @@ 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)
         internal
         pure
         returns (string memory)
     {
+        requireIdentifier(name);
         string memory hexString = vm.toString(data);
         return string.concat(
             commentPrefix(comment),
@@ -354,14 +365,16 @@ 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)
         internal
         pure
         returns (string memory)
     {
+        requireIdentifier(name);
         string memory addressString = vm.toString(data);
         return string.concat(
             commentPrefix(comment),

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");
     }
 

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 {}

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;

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

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