Validate the emitted constant name in all four emitters

[thedavidmeister]

Closes #80

What was wrong

bytesConstantString, uint8ConstantString, bytes32ConstantString and
addressConstantString interpolate name verbatim into a generated Solidity
declaration and never check it, while the contract name reaching the same
generated file is checked on every path that carries it. A name with a space or
a - emits a file that does not compile, with the error pointing at generated
code nobody wrote; a name with a ; emits a file that compiles into a
different set of declarations than the caller asked for.

What changed

All four emitters now call the identifier check the contract name already used,
so every name this library interpolates — into a path or into a declaration —
is a Solidity identifier or is a revert.

The check is renamed requireIdentifier and its error InvalidContractName is
renamed InvalidIdentifier. See the decision below.

Decision: rename, and why the error had to move with it

The issue offered the rename as a choice — its proposed fix ends
"if the shared name reads badly, rename it requireIdentifier and keep one
implementation". Taken, on this evidence:

• The misnomer becomes the majority. After this change the check guards six
  call sites, and four of them are constant names, not contract names.
• The error is what a user actually sees, and it was about to lie. A build
  script that derives a constant name from data and gets it wrong would have
  reverted with InvalidContractName("MY CONSTANT"), pointing the author at the
  contract name — a parameter that was fine. That is a diagnostic defect, not a
  cosmetic one, so renaming the function without the error would have been half
  a fix.
• The rule is one property, not two rules. A contract name and a constant
  name are both exactly a Solidity identifier. One implementation, named for the
  property it enforces.

Ledger

• Build cost. The check plus the rename across LibCodeGen.sol,
  LibFs.sol and the suite, and
  test/src/lib/LibCodeGen.requireContractName.t.sol renamed to
  test/src/lib/LibCodeGen.requireIdentifier.t.sol so the file still names the
  function under test. 248 insertions, 67 deletions over 12 files, no new files.
• Breaking-change cost: measured at zero. requireContractName and
  InvalidContractName are internal/file-level in a published soldeer package,
  so a rename is a source break for any consumer that names them. Every .sol
  file under /home/gildlab/code — all local Rain-org clones plus their vendored
  dependency trees, including chains as deep as cyclo.site → ethgild → rain.flare → rain.interpreter → rain.interpreter.interface → rain.sol.codegen — was swept for both symbols. They occur in 20 files and
  every one of them is a working clone of rain.sol.codegen itself. No
  consumer and no vendored copy under any dependencies/ tree names either
  symbol.
• Carrying cost. None beyond the one check per emitted declaration, which is
  pure and runs at build time only.
• Cost to remove later. The same rename in reverse, plus deleting six call
  sites. Nothing is layered on top of it.

Does the tightening break a live consumer? No.

Applying the check narrows what a published library accepts, so I went looking
for a name that would now revert rather than reasoning about it. The name
argument was extracted at every call site of the four emitters across every
.sol file on this box (all local Rain-org clones plus their vendored
dependency trees), 40 distinct (emitter, name) pairs:

• Every name a consumer passes is a string literal that is already an
  identifier: DEPLOYED_ADDRESS, CREATION_CODE, RUNTIME_CODE,
  DEPENDENCIES, PARSE_META, PARSE_META_BUILD_DEPTH, LOG_TABLES,
  LOG_TABLES_SMALL, LOG_TABLES_SMALL_ALT, ANTI_LOG_TABLES,
  ANTI_LOG_TABLES_SMALL, plus this library's own *_FUNCTION_POINTERS,
  BYTECODE_HASH and DESCRIBED_BY_META_HASH.
• The only non-literal name arguments anywhere are in this repo's own test
  suite, which is updated here.
• st0x.deploy's tagSuffix() — the derived-name case the issue names — does
  build constant names from a tag, but it concatenates them into vm.writeLine
  in its own BuildPointers.sol, not into LibCodeGen. It is the shape of
  consumer that would be caught, and it is not currently affected.

Caveat, stated rather than papered over: GitHub code search returned
total_count: 0 for terms that certainly exist in this repo, including
repo:-scoped queries, so it was not usable as a second oracle. The sweep above
is a local-clone sweep, not a global one.

QA

All numbers below are measured on the merge commit against main at 89cb0a2,
not on the branch as originally written. The QA that was here before this merge
cited test/lib/*.t.sol paths that #56 has since moved and suite totals from a
tree that no longer exists, so it has been replaced rather than annotated.

• Discriminating tests: testBytesConstantStringRejectsNonIdentifierName,
  testBytesConstantStringNameMustBeIdentifier,
  testUint8ConstantStringRejectsNonIdentifierName,
  testUint8ConstantStringNameMustBeIdentifier,
  testBytes32ConstantStringRejectsNonIdentifierName,
  testBytes32ConstantStringNameMustBeIdentifier,
  testAddressConstantStringRejectsNonIdentifierName,
  testAddressConstantStringNameMustBeIdentifier. Each pair is shown below to
  be the only thing standing between its emitter's check and a silent pass.
• Oracle: the accept/reject decision comes from LibCodeGenSlow.isIdentifierSlow,
  which decides membership against the identifier alphabets written out
  character by character, not from the range arithmetic requireIdentifier
  uses; the emitted text comes from LibCodeGenSlow.*ConstantStringSlow, which
  builds the line and measures it rather than reproducing the library's sum. The
  five spelled-out rejected names ("SOME NAME", "SOME;NAME", "SOME-NAME",
  "", "0LEADING") come from the hazards named in the issue, not from the
  implementation.
• Category check: issue asks for the name parameter of bytesConstantString,
  uint8ConstantString, bytes32ConstantString and addressConstantString to
  be validated, and names the shared-name decision; covered all four emitters
  plus the decision (rename taken, ledger above), and the pre-existing
  describedByMetaHashConstantString / pathForContract callers of the same
  check are re-verified by mutant 1 rather than assumed.

1. Suite before and after. nix develop -c forge test:

tree	result
origin/main at ff69c10	Ran 23 test suites: 165 tests passed, 0 failed, 0 skipped (165 total tests)
this branch before re-merging main (2801340)	Ran 23 test suites: 163 tests passed, 0 failed, 0 skipped (163 total tests)
this merge commit (3325514)	Ran 23 test suites: 173 tests passed, 0 failed, 0 skipped (173 total tests)

The eight new tests are the whole difference against main, and no pre-existing
test was deleted. Compared by test name rather than by count:

• merge commit against main at ff69c10: exactly eight added — the
  …NameMustBeIdentifier and …RejectsNonIdentifierName pair for each of the
  four emitters — plus the fourteen testRequireContractName* renamed 1:1 to
  testRequireIdentifier*. Nothing removed.
• merge commit against this branch's pre-merge head 2801340: exactly ten
  added, every one of them main's own — #100's
  testBuildFileForContractFailedBuildKeepsExistingFile and nine
  testBytesToHex*. Nothing removed. So the re-merge dropped nothing from
  either side.

The whole suite was run on the merge commit rather than only the merged files,
because a semantic conflict does not appear between conflict markers.

2. Mutation matrix. Each requireIdentifier(name); call removed one at a
time, the whole suite run each time, then restored. Every run reports
163 total tests, which is what proves the suite compiled and ran rather than
erroring out and reading as "survived".

Measured on this branch at 2801340, when the suite was 163 tests. The later
re-merge of main did not disturb it: the ten tests main added are #100's
buildFileForContract ordering test and nine LibHexString.bytesToHex tests,
none of which reach an emitter's name parameter, and main changed no line
any of these five mutants touches.

#	mutation	suite result	tests that failed
—	baseline, unmutated	163 passed, 0 failed (163 total)	—
1	drop requireIdentifier(name) from describedByMetaHashConstantString (the pre-existing call)	162 passed, 1 failed (163 total)	testDescribedByMetaHashConstantStringRejectsNonIdentifierName
2	drop requireIdentifier(name) from bytesConstantString	158 passed, 5 failed (163 total)	testBytesConstantStringRejectsNonIdentifierName, testBytesConstantStringNameMustBeIdentifier, + 3 unrelated (see below)
3	drop requireIdentifier(name) from uint8ConstantString	158 passed, 5 failed (163 total)	testUint8ConstantStringRejectsNonIdentifierName, testUint8ConstantStringNameMustBeIdentifier, + the same 3
4	drop requireIdentifier(name) from bytes32ConstantString	158 passed, 5 failed (163 total)	testBytes32ConstantStringRejectsNonIdentifierName, testBytes32ConstantStringNameMustBeIdentifier, + the same 3
5	drop requireIdentifier(name) from addressConstantString	158 passed, 5 failed (163 total)	testAddressConstantStringRejectsNonIdentifierName, testAddressConstantStringNameMustBeIdentifier, + the same 3

Every mutant is killed, and each of the four new mutants is killed by exactly
the two tests written for that emitter and by nothing else — so the four call
sites are covered independently rather than by one test that happens to reach
them all.

Mutant 1 is the check that already existed, included to show the new one in
bytes32ConstantString did not make it redundant and untested. Without it a
traversing name reaches vm.readFileBinary before any constant name is checked,
and the test catches the filesystem error in place of the revert:

[FAIL: Error != expected error: vm.readFileBinary: failed to open file ".../meta/.rain.meta": No such file or directory (os error 2) != InvalidIdentifier("")] testDescribedByMetaHashConstantStringRejectsNonIdentifierName()

The 3 unrelated failures, stated rather than filtered out. From mutant 2
onward every run also reported:

[FAIL: vm.etch: failed to create bytecode: Unsupported Eip7702 version.; counterexample: args=[0xef019859b34fc7671b4946cb3b139e0f41c26ba183c944]] testBytecodeHashConstantStringHashesCode(bytes)
[FAIL: ... same] testBytecodeHashConstantStringIdempotent(bytes)
[FAIL: ... same] testBytecodeHashConstantStringMatchesMeasuredLine(bytes)

These are in test/src/lib/LibCodeGen.bytecodeHashConstantString.t.sol, which
this PR does not touch, and none of the mutations reaches that code. A fuzz run
drew the counterexample by seed and forge then cached and replayed it, which
is why it appears from mutant 2 onward and in the restore run. It reproduces on
unmodified origin/main at c816251 with the identical counterexample:
8 passed, 3 failed for that suite alone. It is a pre-existing gap in that
suite's own assumeEtchableCode, which excludes 0xef01… only when the length
is not 23 — forge also rejects a 23-byte 0xef01… whose third byte is not
0x00, which is exactly this counterexample. Out of scope here and not fixed;
reported so it is not mistaken for something this branch did. The cache was
cleared afterwards and this tree is clean.

3. Formatting and coverage. nix develop -c forge fmt --check exits 0 on the
merge commit. forge coverage on the merge commit:

| File                     | % Lines         | % Statements      | % Branches      | % Funcs         |
| src/lib/LibCodeGen.sol   | 100.00% (53/53) | 100.00% (66/66)   | 100.00% (3/3)   | 100.00% (14/14) |
| src/lib/LibFs.sol        | 100.00% (22/22) | 100.00% (19/19)   | 100.00% (4/4)   | 100.00% (5/5)   |
| src/lib/LibHexString.sol | 100.00% (21/21) | 100.00% (25/25)   | 100.00% (4/4)   | 100.00% (1/1)   |
| Total                    | 100.00% (96/96) | 100.00% (110/110) | 100.00% (11/11) | 100.00% (20/20) |

4. Merging origin/main. main moved a long way under this branch —
#138 deleted the worked example (script/Build.sol,
src/generated/CodeGennable.sol, .github/workflows/build-pointers.yaml),
#112 split LibFs into a dir-taking overload with a private
pathForContractIn, #56 moved every .t.sol into the test/src/ mirror
tree, and #100 reordered buildFileForContract so the content is built before
any disk mutation. origin/main is merged in, not rebased, twice — once at
89cb0a2 and again at ff69c10 after #100 landed and made this branch
conflict. Resolutions:

• src/lib/LibFs.sol (content): main's structure taken. The rename now lands
  on pathForContractIn, which is where the check moved to under Cover the generated directory being created, by making the directory injectable #112, rather
  than on the flat pathForContract this branch was written against.
• src/lib/LibFs.sol again, after LibFs: build the generated file content before touching disk #100 landed: LibFs: build the generated file content before touching disk #100 hoists the generated
  content into a local before createDir, the unlink and the write, so a
  build that reverts cannot destroy the file already there. That ordering is
  taken as-is and is the only shape in the merged function; nothing was moved
  above the content computation and the compute-at-write shape was not restored.
  This PR's only line in that file remains the requireContractName →
  requireIdentifier rename inside pathForContractIn, which buildFileForContract
  calls before it builds the content, so an invalid name still reverts before
  anything on disk is touched. LibFs: build the generated file content before touching disk #100's discriminating test,
  testBuildFileForContractFailedBuildKeepsExistingFile, passes on the merge
  commit.
• LibCodeGen.requireContractName.t.sol (rename/rename): both sides renamed the
  same file to different places. Resolved onto
  test/src/lib/LibCodeGen.requireIdentifier.t.sol — the test/src/ mirror
  Move every .t.sol into the test/src/lib mirror tree #56 requires, under the name this PR gives the function. main's content is
  taken whole: its isIdentifierSlow oracle and its alphabet-stated
  …AlphabetCannotTraverse replace the hand-rolled versions this branch
  carried, with only the rename and this PR's own NatSpec applied on top.
• The four *ConstantString.t.sol files, LibFs.buildFileForContract.t.sol and
  LibFs.t.sol (imports and one doc comment): main's import paths and
  main's rewritten doc comments taken, this PR's InvalidIdentifier and its
  new tests kept. Nothing main landed today in those files was dropped: each
  file's diff against main is only the rename and this PR's own additions.
• test/concrete/CodeGennable.sol: its doc comment names the test file that
  asserts on the contract's name, so it follows the rename.
• #126 (forge-std 1.16.1 → 1.16.2) landed while this was being resolved and
  rewrote the versioned import prefix in every file. Every file this PR touches
  carries forge-std-1.16.2/ after the merge, including the renamed test file,
  which git could not carry main's edit onto by itself. forge-std-1.16.1
  has zero hits anywhere outside dependencies/, out/ and cache/.

No reference to requireContractName, InvalidContractName or
isContractNameSlow survives anywhere in src/ or test/.

Where the issue's proposed fix needed correcting

• The proposed diff added requireContractName(name) to the four emitters and
  stopped. It did not account for the four existing fuzz tests
  (test*MatchesMeasuredLine) that fuzz name as an arbitrary string — under
  the fix those names are rejected, so every one of them would have reverted.
  They now build the name from a fuzzed seed via
  LibCodeGenSlow.nameFromSeedSlow, which is the helper the suite already had
  for exactly this, and as a result they now exercise the accepted half of the
  domain instead of a domain the library no longer accepts.
• The proposed fix did not mention the error, which had to be renamed with the
  function for the reason given above.

Found, not touched

• comment is interpolated verbatim into the same generated file with no check
  at all, and unlike name there is no rule stated anywhere for what a comment
  may contain. A comment carrying a bare newline emits lines that are not
  comments. Outside this issue; not filed.
• The assumeEtchableCode gap in
  test/src/lib/LibCodeGen.bytecodeHashConstantString.t.sol described under QA.
  Pre-existing on main, outside this issue; not filed.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 21453e92-5f9a-4202-b413-518006c53a25

📥 Commits

Reviewing files that changed from the base of the PR and between ff69c10 and 3325514.

📒 Files selected for processing (12)

• src/lib/LibCodeGen.sol
• src/lib/LibFs.sol
• test/concrete/CodeGennable.sol
• test/lib/LibCodeGenSlow.sol
• test/src/lib/LibCodeGen.addressConstantString.t.sol
• test/src/lib/LibCodeGen.bytes32ConstantString.t.sol
• test/src/lib/LibCodeGen.bytesConstantString.t.sol
• test/src/lib/LibCodeGen.describedByMetaHashConstantString.t.sol
• test/src/lib/LibCodeGen.requireIdentifier.t.sol
• test/src/lib/LibCodeGen.uint8ConstantString.t.sol
• test/src/lib/LibFs.buildFileForContract.t.sol
• test/src/lib/LibFs.t.sol

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}