Document the return value of every tooling builder - #134
Conversation
The five builders on the four published tooling interfaces each return `bytes memory` whose encoding is two byte function pointers, and none of them tagged that fact as a return, so solc emitted an empty `methods` object into their devdoc and a consumer received an artifact that documents the return value nowhere. Each builder now carries a `@return`, and each is asserted against the devdoc of its own compiled artifact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe interfaces now document encoded function-pointer return values. A test library reads compiled devdoc artifacts, and new tests verify the documented return text for all five tooling functions. Foundry can read the ChangesDevdoc documentation and verification
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This PR documents the tooling builders’ byte-encoded return values and verifies the generated artifacts without changing runtime behavior or interface IDs. No actionable merge-blocking risk remains after normal checks. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
# Conflicts: # src/interface/IIntegrityToolingV1.sol # src/interface/IOpcodeToolingV1.sol # src/interface/IParserToolingV1.sol # src/interface/ISubParserToolingV1.sol
|
@coderabbitai review |
|
The fixer brief that drove this change required every fix to ship a test. That was wrong for a change with no behaviour, and the tests it produced here were hand-rolled static analysis in Solidity: `test/lib/LibDevdoc.sol` and four `.devdoc.t.sol` suites pinned five file-level constants byte-for-byte against the compiled artifacts. Only the five `@return` tags remain. `foundry.toml` returns to `main`'s three-entry `fs_permissions` block exactly.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Closes #94
The finding
The five builders on the four published tooling interfaces all return
bytes memory. The encoding of those bytes — two bytes per function pointer,positionally indexed — is the whole contract of the call: the caller receives an
undifferentiated byte string and has to know how to cut it up. None of the five
tagged that fact as a return, so
solchad nothing to put indevdoc.Measured on
origin/main(b422d97),forge build --forcethen reading eachartifact's
metadata.output:devdoc.methodsis empty on all four, and the prose that was written landed inuserdocas a notice, because an untagged///line continues the preceding tagrather than starting a return. Reading the source by eye, the functions look
documented. The artifact a consumer compiles against documents the return value
nowhere.
What changed
A
@returnon each of the five declarations. Ten lines, four files, and nothingelse —
git diff origin/main --staton this branch:@returnIIntegrityToolingV1.buildIntegrityFunctionPointersIOpcodeToolingV1.buildOpcodeFunctionPointersIParserToolingV1.buildOperandHandlerFunctionPointersIParserToolingV1.buildLiteralParserFunctionPointersISubParserToolingV1.buildSubParserWordParsersThe same build on this branch, same command, same key:
No test guards this, and it can regress silently
This PR ships no test. Nothing in this repo's suite reads
out/, so nothinghere observes the compiled artifact at all. Three regressions therefore pass
silently:
@returndeleted, which puts the interface straight back to the state thisPR is fixing;
@returnreworded to state an encoding that is not the one the consumersimplement;
@returnmoved above the untagged///paragraph that currentlyprecedes it. That paragraph then stops beginning a docblock of its own and is
appended to the return documentation instead, so
returns._0becomes theencoding sentence plus three sentences about state mutability. This is not
hypothetical: it is the exact conflict resolution this branch had to get right
when Declare every tooling builder
viewso an implementation can read state #124 was merged in, and a future edit to those docblocks faces the samechoice with nothing checking it.
In every case the full suite stays green and the artifact is wrong.
solcemitsno diagnostic for an absent return tag — building
origin/main's interfaces,which have none, produces only
Warning (2018)mutability notices and nothingabout NatSpec — so
forge buildwill not flag it either. After this PR nothingin this repository will catch any of the three.
An earlier revision of this PR carried
test/lib/LibDevdoc.soland fourtest/src/interface/<Interface>.devdoc.t.solsuites, 192 lines, that read thecompiled artifact and asserted the return text against five file-level string
constants. They are removed here, along with the
{ access = "read", path = "out" }fs_permissionsgrant that existed only tolet them read
out/.foundry.tomlis now byte-identical toorigin/main.They were hand-rolled static analysis written in Solidity, and they pinned the
prose byte-for-byte, so every future wording change would have had to be made
twice, in two files, or the suite goes red for a documentation edit that is
correct.
The half of that which generalises — every published interface function with a
return value carries a
@returnthat reachesdevdocin its artifact — is arepo-wide lint rule with no home in this repo. It is rehomed to
rainlanguage/rainix#317 ("Six repos' worth of static analysis is being
hand-rolled in Solidity because
rainix-statichas no home for repo-wide lintchecks"), which is where the presence check belongs and where it can run against
every repo rather than this one. The prose pinning is not rehomed anywhere:
asserting that documentation says a particular sentence, by writing that sentence
down a second time, is a duplicate rather than a check.
Where the issue's proposed fix needed correcting
buildOperandHandlerFunctionPointersandbuildLiteralParserFunctionPointersonly, and said the other three take"equivalents".
LibCodeGenstates the two byte width for exactly those two(
src/lib/LibCodeGen.sol:151,:177-179) and says nothing about the width ofthe other three, so their wording is derived from the consumers instead:
LibEval.sol:110dispatches opcodes atmul(mod(opcode, fsCount), 2),LibIntegrityCheck.sol:166atmul(opcodeIndex, 2), andBaseRainlangSubParser.sol:206reads a word parser atmul(add(index, 1), 2),all in
rainlanguage/rain.interpreteratmain. Two bytes, positionallyindexed, holds for all five.
the literal, rather than a full word lookup". The second clause is about how
the parser is implemented, not about how the returned bytes are encoded, so it
is dropped;
LibCodeGenstill emits it into the generated constant's comment,which is where it describes something.
Interface ids do not move
#119 pins these four ids, so this was checked rather than assumed. Each
interface's
abiwas extracted from its artifact onorigin/mainand again onthis branch, sorted and diffed:
An interface id is the exclusive or of the ABI selectors, and a selector is a
function name and its parameter types, so an identical ABI is an identical id.
NatSpec cannot reach any of it.
Merged
origin/main, and how the conflict was resolved#124 (issue #92) landed on all four of these files while this branch was open, so
origin/mainis merged in at726a86e(merge, not rebase). Every conflict wasin the same five docblocks: #124 added an untagged mutability paragraph and moved
three declarations from
puretoview.Both sides are kept. The ordering is load bearing rather than cosmetic — an
untagged
///line continues whatever tag precedes it, so #124's paragraph sitsbefore the
@returntag. Written the other way round its three sentenceswould be swallowed into the return documentation. The artifact output quoted
above is that ordering holding: each
returns._0is exactly the encodingsentence and nothing else.
Suite
nix develop -c forge teston this branch:Before the strip the same command reported
22 test suites … 147 tests passed.The drop is 5 tests across 4 suites, which is exactly the five deleted devdoc
tests and their four suites; no other test changed.
nix develop -c forge fmt --checkexits 0.CI on the head of this branch:
rainix / test,rainix / static,rainix / legalandbuild-pointers / copy-artifactsall pass.Left to the sibling issues
The same five docblocks are the subject of two other open findings, and neither
is touched here: #93 (the description folded into
@title, so the contract level@noticeis emitted empty) and #95 (the.github/workflows/build-pointers.yamlreference that.soldeerignorestripsfrom the published package, still present on all five). #92 is in this branch
only because it landed as #124 and was merged in, unchanged.
CodeRabbit
Reviewed. Its first attempt on this PR was rate limited ("you've reached your PR
review limit"), which is an absence of review rather than a passed one, so a
review was requested again once the quota reset. That one ran: "No actionable
comments were generated in the recent review", and its commit status on the head
of this branch reads
success/ "Review completed". Unresolved threads werechecked over GraphQL rather than taken from the green check —
reviewThreadsreturnstotalCount: 0, so there is nothing open.QA
by decision. The regression it can suffer, and the fact that nothing here would
catch it, are stated in full above. The presence check is rehomed to
Six repos' worth of static analysis is being hand-rolled in Solidity because rainix-static has no home for repo-wide lint checks rainix#317.
that source prose was not reaching the artifact.
devdoc.methodsis{}onall four interfaces on
origin/main, and carries the five expectedreturns._0strings on this branch. Both measurements are quoted above, from the same
forge build --forceon the same tree.rainlanguage/rain.interpreter(
LibEval.sol:110,LibIntegrityCheck.sol:166,BaseRainlangSubParser.sol:206) and the commentsLibCodeGenalready emits —not the interfaces being documented.
forge fmt --checkexits 0. Run on the tip, which hasorigin/mainmerged in.@returnon each of the five declarationscarrying the encoding. Covered: all five. Nothing outside those five docblocks
is changed —
foundry.tomlis byte-identical toorigin/mainand no test fileis added.