Skip to content

Unname every return under src/, and stop the generators emitting named ones - #140

Merged
thedavidmeister merged 7 commits into
mainfrom
2026-08-18-issue-129-unname-returns
Aug 18, 2026
Merged

Unname every return under src/, and stop the generators emitting named ones#140
thedavidmeister merged 7 commits into
mainfrom
2026-08-18-issue-129-unname-returns

Conversation

@thedavidmeister

Copy link
Copy Markdown
Contributor

Closes#129

Premise checked first

Every named return the issue lists at 369cfe9 was still there when this branched off ed91bfa, after #131 and #134. Nothing was already fixed. main then moved again — #136 landed the generated aggregate — and that merge is in here, so the sweep covers the code main has now rather than the code the issue was filed against.

What changed

Each function loses the name from its returns clause, gains an explicit return on every path, and keeps its @return tag with the name dropped from the tag text — the form zoltuAddress already used.

filefunctionname dropped
src/abstract/RainDeploySuitesBase.solcheckedCandidateSuitescandidates
src/abstract/RainDeploySuitesBase.solallSuitessuites
src/abstract/RainDeploySuitesBase.solsuiteNamesnames
src/abstract/RegistryDeploySuites.solcandidateSuitescandidates
src/abstract/RainDeployVerifyBase.solderiveDeploymentderived
src/abstract/RainDeployVerifyBase.solderiveDeploymentsderived
src/lib/LibRainDeploy.solisStartBlockisStart
src/lib/LibRainDeploy.solfindDeployBlockdeployBlock
src/lib/LibRainDeploy.soldeployZoltudeployedAddress
src/lib/LibRainDeploy.soldeployToNetworksdeployedAddress
src/lib/LibRainDeploy.soldeployAndBroadcastdeployedAddress
src/lib/LibRainDeploySnapshot.solfrozenSnapshotPathspaths
src/lib/LibRainDeploySnapshot.solsortedRecordPathssorted
src/lib/LibRainDeploySnapshot.solreleasedImportBlockimports
src/lib/LibRainDeploySnapshot.solnewestFrozenTagnewest
src/lib/LibRainDeploySnapshot.solaggregateImportBlockimports
src/interface/IAddressRegistryV1.solgetaccount
src/concrete/AddressRegistry.solgetaccount
script/Build.solgeneratedContractscontracts
script/Build.solgeneratedContractNamesnames

RegistryDeploySuites.releasedSuites is on the issue's list but is not in this diff: #136 replaced its body with return LibReleasedSuites.releasedSuites();, which is already unnamed.

No named return is left under src/ or script/. test/ is out of scope per the issue.

The two the issue calls the hard case

deployToNetworks assigned deployedAddress only inside the if/else arms of the loop over networks, safe solely because NoNetworks guarantees an iteration. Enumerating the paths shows both arms end with deployedAddress == expectedAddress — the deploy arm reverts UnexpectedDeployedAddress unless they are equal, the skip arm assigns it outright — so the accumulator carried nothing the caller had not already handed in. The deploy arm keeps a local for its own comparison, and the function returns expectedAddress after the loop.

newestFrozenTag used bytes(newest).length == 0 as an is-this-the-first-iteration flag on the default-initialised named return. It now refuses an empty record up front, seeds from paths[0] and scans from 1.

Both are also what slither's uninitialized-local detector named once the return was unnamed. Seeding either with the value its declaration already defaults to would be a line no test can kill — what the issue closed#103 over — so they are restructured rather than seeded, and no suppression is added. M21 and M22 in the table below are there because the restructure moved a live refusal's operand: they delete each refusal in turn to show it still fails a test.

The generators

Two emitters wrote named returns as string literals — the per-contract released lib and #136's aggregate over them. Both now emit an unnamed clause, a local declaration and a trailing return suites;, and the aggregate's empty-repo branch emits return new DeploySuite[](0);. src/lib/LibAddressRegistryReleased.sol, src/lib/LibMigrationRegistryReleased.sol and src/lib/LibReleasedSuites.sol are the output of forge script script/Build.sol against them, so the next build reproduces them rather than reverting this diff.

The deploy address moves

AddressRegistry.get is one of the unnamed functions, so the contract's runtime code changes — one byte longer, two jump destinations shifted. The rolling candidate snapshot is regenerated with it:

  • BYTECODE_HASH0xef8355…4ea730x6c37a6…10097
  • DEPLOYED_ADDRESS0x25aC2b82915f191dbE64e65BAeDDD68b97b68fe10x8cACfbD5d78b6D87080cE0839708ac2dA5461F78

src/generated/ holds no frozen tag directory, so no released suite and nothing on chain is contradicted by the move. checkCandidatesAnchoredToSource is what makes the regeneration mandatory rather than optional.

Beyond the issue's table

script/Build.sol is neither src/ nor the test/ the issue excludes. Its two named returns are unnamed here rather than left as the only counterexamples to a ruling that says this repo does not name return values.

Suite

runresult
main at ed91bfa, no RPC env259 passed / 51 failed, every failure *_RPC_URL not found; grep '[FAIL' | grep -vc '_RPC_URL. not found' = 0
this branch, no RPC envCI's test job is green
this branch, public RPC endpoints bound to all five aliases320 passed / 0 failed / 320 total
forge fmt --checkexit 0
CI rainixtest, legal and static (slither + fmt + single-contract) all green

Every mutation below runs against that configuration, so the fork-dependent functions in LibRainDeploy are measured rather than skipped. Two earlier runs against rate-limited public endpoints reported failures that were HTTP 429s rather than the mutation; those are re-run and the table counts only runs whose failures are assertions.

Mutation pass

One mutation at a time, each deleting a return this diff ADDS or emits, against the full suite with live RPC. tests run is the suite's own N total tests line, so a run that compiled nothing or filtered to nothing is visible rather than counted as a kill. infra failures counts RPC transport errors in the run and is 0 everywhere, so no verdict rests on a fork that failed to open.

idfunctionmutationtests runassertion failuresinfra failuresverdictkilling tests
M01RainDeploySuitesBase.checkedCandidateSuitesdelete return candidates;319160KILLEDtestAllSuitesIsReleasedThenCandidates,testCandidatesPresentAnswers
M02RainDeploySuitesBase.allSuitesdelete return suites;319110KILLEDtestAllSuitesIsReleasedThenCandidates,testDerivationLeavesNoCodeBehind
M03RainDeploySuitesBase.suiteNamesdelete return names;31950KILLEDtestEmptySuiteIsUnknown,testRunRefusesToBroadcastACandidateThatIsNotItsSource
M05RegistryDeploySuites.candidateSuitesdelete return candidates;31950KILLEDtestEveryCandidateArtifactPathResolves,testEveryCandidateHasASnapshot
M06RainDeployVerifyBase.deriveDeploymentdelete return derived;319130KILLEDtestChainCodeHashMismatchReverts,testChainIgnoresAnUndeployedCandidate
M07RainDeployVerifyBase.deriveDeploymentsdelete return derived;31950KILLEDtestChainCodeHashMismatchReverts,testChainMatrixReachesTheLastSupportedNetwork
M08LibRainDeploy.isStartBlockdelete return isStart;31930KILLEDtestFindDeployBlockZoltuFactory,testIsStartBlockAtDeployBlock
M09LibRainDeploy.findDeployBlockdelete return low;31930KILLEDtestFindDeployBlockExactZoltuBaseDeployBlock,testFindDeployBlockZoltuFactory
M10LibRainDeploy.deployZoltudelete return deployedAddress;319430KILLEDtestChainCodeHashMismatchReverts,testChainIgnoresAnUndeployedCandidate
M11LibRainDeploy.deployToNetworksdelete return expectedAddress;31960KILLEDtestDeployAndBroadcastHappyPath,testDeployToNetworksEveryDependencyPresentDeploys
M12LibRainDeploy.deployAndBroadcastdrop return from the tail call31910KILLEDtestDeployAndBroadcastHappyPath
M13LibRainDeploySnapshot.frozenSnapshotPathsdelete return paths;319110KILLEDtestCheckReleaseFollowsRecordAcceptsExactlyTheStrictlyGreater,testCheckReleaseFollowsRecordRefusesATagBelowTheNewestRelease
M14LibRainDeploySnapshot.sortedRecordPathsdelete return sorted;31930KILLEDtestRecordPathsForContractSelectsOneContractInTagOrder,testSortedRecordPathsOrdersTagsAsVersions
M15LibRainDeploySnapshot.releasedImportBlockdelete return imports;31910KILLEDtestReleasedImportBlockAliasesEveryRecord
M16LibRainDeploySnapshot.newestFrozenTagdelete return newest;31950KILLEDtestCheckReleaseFollowsRecordAcceptsExactlyTheStrictlyGreater,testCheckReleaseFollowsRecordRefusesATagBelowTheNewestRelease
M17LibRainDeploySnapshot.releasedLibraryBlockstop emitting return suites;31910KILLEDtestReleasedLibraryBlockDeclaresEveryRecord
M18AddressRegistry.getdelete return account;319240KILLEDtestCandidatesPresentAnswers,testCheckResolvedAddressesChecksEveryRead
M19Build.generatedContractsdelete return contracts;31920KILLEDtestGeneratedContractsAreExactlyTheDeclaredCandidates,testTheCommittedAggregateIsInDeclarationOrder
M20LibRainDeploySnapshot.newestFrozenTagdelete the empty-record return ""; guard31950KILLEDtestFreezeCopiesTheRegeneratedRollingSnapshot,testFreezeCutsEveryNamedContract
M21LibRainDeploy.deployToNetworksdelete the post-broadcast UnexpectedDeployedAddress refusal31910KILLEDtestDeployToNetworksFactoryReportsOtherAddressReverts
M22LibRainDeploy.deployToNetworksdelete the UnexpectedDeployedCodeHash refusal31910KILLEDtestUnexpectedDeployedCodeHashReverts
M23Build.generatedContractNamesdelete return names;32010KILLEDtestGeneratedContractNamesAreTheDeclarationInOrder
M24LibRainDeploySnapshot.aggregateImportBlockdelete return imports;31920KILLEDtestAggregateImportBlockImportsEveryReleasedLib,testTheCommittedAggregateIsWhatTheGeneratorEmits
M25LibRainDeploySnapshot.aggregateLibraryBlockstop emitting return suites;31920KILLEDtestAggregateLibraryBlockConcatenatesEveryReleasedLib,testTheCommittedAggregateIsWhatTheGeneratorEmits
M26LibRainDeploySnapshot.aggregateLibraryBlockemit new DeploySuite[](1) for an empty repo31910KILLEDtestAggregateLibraryBlockDeclaresNothingForNoContracts

AddressRegistry.get's mutant (M18) is the one where the source-anchor tests fire for free: any edit to that contract's source breaks testSnapshotMatchesSource whatever the edit does. Its kill does not rest on them — testGetReturnsRegistered, testGetOpaqueNames, testResolveRegistered and testResolveUnregistered are among the 24, and each of those reads a binding back rather than a pin.

M23 survived the first run and is the reason testGeneratedContractNamesAreTheDeclarationInOrder is in this PR. generatedContractNames() is the list cutRelease freezes and the list the aggregate is emitted from, and both reach it only through forge script, so nothing in the suite could tell it from an empty array. The new test asserts it positionally against generatedContracts() and now kills the mutant.

QA

  • Discriminating tests: testGeneratedContractNamesAreTheDeclarationInOrder — new, fails on the M23 mutant (320 tests ran, it is the only failure) and passes on this branch. testReleasedLibraryBlockDeclaresEveryRecord and testAggregateLibraryBlockConcatenatesEveryReleasedLib — existing, expectations updated to the new emitted text; each fails against the pre-change emitter, verified by M17 and M25 which revert exactly that literal.
  • Mutations applied: 25, each a return this diff adds or emits, deleted one at a time against the full suite with live RPC — 25 killed, 0 survived, 0 runs with an RPC transport failure. Full table above with the killing test for each.
  • Oracle: the emitted text is asserted character for character against the form the issue specifies, spelled literally in the test rather than computed the way the emitter computes it. Behaviour has no oracle beyond "unchanged": every function keeps its old semantics, which is what the 320/320 suite and the unchanged MigrationRegistry pins check.
  • Category check: the issue asks for (a) every named return under src/ unnamed with an explicit return on every path, (b) @return kept with the name dropped, (c) the generator changed so it stops emitting them, (d) the generated files regenerated from it. All four are covered, including the aggregate generator Move the release version under foundry's [external] section #136 added after the issue was filed and the two named returns in script/Build.sol. test/ is the issue's own exclusion and is untouched apart from the expectations that pin generated text and the one new test.

Each function loses the name from its returns clause, gains an explicit
return on every path, and keeps its @return tag without the name.
AddressRegistry.get is one of them, so the contract's runtime code is a
byte longer and its candidate deploy address and code hash move with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The released-suites emitter writes an unnamed returns clause, declares
the array as a local and returns it. Both generated libs are regenerated
from it, so the next script/Build.sol run reproduces them byte for byte.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
deployToNetworks proves deployedAddress equals expectedAddress on every
path through the loop, so the deploy branch keeps a local for its own
comparison and the function returns expectedAddress. newestFrozenTag
refuses an empty record up front and seeds from the first path.
Both were what slither's uninitialized-local detector named. Seeding them
with the value the declaration already defaults to would be a line no
test can kill.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unname-returns
# Conflicts:
#	src/abstract/RegistryDeploySuites.sol
generatedContractNames and aggregateImportBlock lose their names, and the
aggregate emitter writes an unnamed clause with a local and a trailing
return. LibReleasedSuites is regenerated from it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The name list the freeze and the aggregate read reaches both only through
forge script, so nothing asserted it against generatedContracts(). A list
that lost an entry froze one contract fewer and emitted an aggregate that
declared that contract's releases as nothing, with the suite green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thedavidmeisterthedavidmeister self-assigned this Aug 18, 2026
@coderabbitai

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in:40 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: 5c9575e8-678a-4e18-982a-0b7b689d5585

📥 Commits

Reviewing files that changed from the base of the PR and between 74260ae and a1bcccb.

⛔ Files ignored due to path filters (1)
  • src/generated/candidate/AddressRegistry.sol is excluded by !**/generated/**
📒 Files selected for processing (14)
  • script/Build.sol
  • src/abstract/RainDeploySuitesBase.sol
  • src/abstract/RainDeployVerifyBase.sol
  • src/abstract/RegistryDeploySuites.sol
  • src/concrete/AddressRegistry.sol
  • src/interface/IAddressRegistryV1.sol
  • src/lib/LibAddressRegistryReleased.sol
  • src/lib/LibMigrationRegistryReleased.sol
  • src/lib/LibRainDeploy.sol
  • src/lib/LibRainDeploySnapshot.sol
  • src/lib/LibReleasedSuites.sol
  • test/concrete/BuildHarness.sol
  • test/script/Build.t.sol
  • test/src/lib/LibRainDeploySnapshot.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

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

@thedavidmeister
thedavidmeister merged commit cc83fe0 into mainAug 18, 2026
4 checks passed
thedavidmeister pushed a commit that referenced this pull request Aug 18, 2026
…buildscript-base
#140 swept named returns out of the repo (#129: this repo does not name return
values), and #139 pinned the name list against the declaration. Both landed on
the functions this branch renames.
Conflicts, and how they were taken:
* script/Build.sol - main unnamed generatedContractNames()'s return; this
branch renamed that function to snapshotContractNames() as BuildScript's
hook. Kept the rename, took main's style: unnamed `returns (string[] memory)`
with an explicit `return names;`. generatedContracts() was already main's
unnamed form and came through the automatic merge unchanged.
* test/concrete/BuildHarness.sol - main's externalGeneratedContractNames()
wrapper and this branch's externalSnapshotContractNames() are the same seam
onto the renamed function. Kept this branch's, dropped main's: the function
it called no longer exists.
* test/script/Build.t.sol - merged without markers, but main's
testGeneratedContractNamesAreTheDeclarationInOrder called the wrapper that
went. Repointed at externalSnapshotContractNames() and renamed to match, so
main's positional assertion is not lost. Its doc reference, and the one in
testTheCommittedAggregateIsInDeclarationOrder, follow the rename.
test/concrete/BuildScriptHarness.sol is not a conflict but its
snapshotContractNames() is added by this branch with a named return, so it is
unnamed here too, with an explicit return.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister pushed a commit that referenced this pull request Aug 18, 2026
`#139` and `#140` landed under this branch, and `#126`'s aggregate writer
came with them. Four files conflicted.
- `foundry.toml`: both sides add the SAME `./fixture-lib` fs_permissions
entry, predicted by this PR's "Not in this PR" note. One entry, with a
comment covering all three writers that are pointed there.
- `.soldeerignore`: both sides add `/fixture-lib`, in different
positions. Deduped, keeping the sorted one.
- `src/lib/LibRainDeploySnapshot.sol`: main introduced `LIB_DIR`,
`pathForLib` and `releasedLibraryName` for the aggregate writer, so
this branch's own `LIB_DIR` is a second declaration of main's and its
hand-concatenated paths are main's helper spelled twice. Main's are
kept, and the two writers this PR parameterises now build their path
with `pathForLib(libDir, libraryName)`. The aggregate writer's NatSpec
claimed the other two writers "take no directory and are always
`LIB_DIR`", which this PR makes false; it now says they take the same
`libDir`.
- `test/script/Build.t.sol`: both sides reworded one paragraph. This
branch's reason (both entry points rewrite committed files other
contracts read) with main's conclusion (nothing below writes anything,
which is what main's new reading tests made true).
- `test/src/lib/LibRainDeploySnapshot.t.sol`: main added
`generatedFilePrefix()` and the aggregate emitter tests where this
branch deleted `testWriteReleasedSuitesLibWritesTheLibAtItsPath`. All
of main's is kept and the deletion stands — that test wrote the
committed `src/lib/LibAddressRegistryReleased.sol`, which is the race
this PR removes, and
`testTheCommittedReleasedLibIsWhatTheGeneratorEmits` carries its
staleness half without writing. The three tests this PR added spell the
generated header out literally; they use `generatedFilePrefix()` now.
`testTheCommittedAggregateIsWhatTheGeneratorEmits` cited the deleted
test by name and now cites its replacement.
Return style is main's throughout: unnamed `returns (...)` with an
explicit `return`, per #129 and the #140 sweep. Nothing added here names
a return, and `forge build --force` emits no solc warning at all.
`nix develop -c forge test`: 275 passed / 52 failed, every failure
`*_RPC_URL not found`; `grep '[FAIL' | grep -vc '_RPC_URL. not found'`
= 0. `git status` clean after the run and `fixture-lib/` empty, so no
test writes a committed file. `nix develop -c forge fmt --check` exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unname every return under src/, and stop the generator emitting named ones

2 participants

@thedavidmeister@claude