Refuse to generate beside another artifact for the same contract

README.md

@@ -37,6 +37,28 @@ read a tree that never stops changing as a cycle that does not converge rather
 than one more pass to run. Committing part-way records a deployed-bytecode hash
 for a contract compiled against a different pass of the same file.
 
+## Generated paths
+
+`LibFs.pathForContract` names `src/generated/<Contract>.sol`, and
+`buildFileForContract` writes that file and only that file. Consumers commit it
+and import it by path from `src/**`, so the path is a cross repo contract:
+moving it, or moving `GENERATED_DIR`, breaks every repo holding the artifact at
+the old path.
+
+`buildFileForContract` therefore refuses to generate while `src/generated/`
+holds another artifact for the same contract — any direct child named for that
+contract, in full, followed by a `.` and anything other than `sol`. Nothing
+regenerates such a file, while the consumer's imports keep resolving to it, so
+the build fails with `OrphanedGeneratedArtifact` naming the file rather than
+generating beside it. Delete it and repoint the imports at
+`src/generated/<Contract>.sol` in the same commit.
+
+Only direct children are read, so a generation into `src/generated/` never reads
+inside the per release snapshot directories that sit there, and never refuses
+one of them either, because a tag carries no `.`. `buildFileForTaggedContract`
+writes into one of those directories, and reads that directory rather than
+`src/generated/`, so each is checked against its own contents.
+
 ## Formatter requirements
 
 `LibCodeGen` wraps the declarations it emits itself, deciding against

src/lib/LibFs.sol

@@ -10,6 +10,14 @@ import {LibCodeGen} from "./LibCodeGen.sol";
 /// path, so it is a cross repo contract rather than an internal detail.
 string constant GENERATED_DIR = "src/generated";
 
+/// Thrown when the generated directory holds an artifact for a contract other
+/// than the one `pathForContract` names for it. Consumers commit these files
+/// and import them by path, so an artifact this library does not write is one
+/// nothing regenerates: its imports keep resolving to a frozen codehash while
+/// the build reports success.
+/// @param path The artifact nothing regenerates, relative to the project root.
+error OrphanedGeneratedArtifact(string path);
+
 /// Thrown when a tag is not a single path segment drawn from the tag alphabet.
 /// Such a tag cannot be interpolated into a directory path.
 /// @param tag The rejected tag.
@@ -106,6 +114,97 @@ library LibFs {
         return string.concat(dir, "/", contractName, ".sol");
     }
 
+    /// The final segment of `path`: the bytes after its last `/`, or all of
+    /// `path` when it has none. `vm.readDir` reports absolute paths, so this is
+    /// what carries an entry's own name.
+    /// @param path The path to take the final segment of.
+    /// @return The final segment.
+    function lastPathSegment(string memory path) internal pure returns (string memory) {
+        bytes memory pathBytes = bytes(path);
+        uint256 start = 0;
+        for (uint256 i = 0; i < pathBytes.length; i++) {
+            if (pathBytes[i] == "/") {
+                start = i + 1;
+            }
+        }
+        bytes memory segment = new bytes(pathBytes.length - start);
+        for (uint256 i = 0; i < segment.length; i++) {
+            segment[i] = pathBytes[start + i];
+        }
+        return string(segment);
+    }
+
+    /// @notice Reverts if `GENERATED_DIR` holds an artifact for `contractName`
+    /// other than the one at `pathForContract(contractName)`.
+    ///
+    /// An artifact for a contract is a direct child of `GENERATED_DIR` named
+    /// for that contract: the name in full, then a `.`, then anything. The full
+    /// name up to the `.` is what separates one contract's artifact from
+    /// another's, so a repo generating both `Foo` and `FooBar` keeps both.
+    ///
+    /// `pathForContract` names exactly one such child, and that one is asked
+    /// for here rather than spelled out again, so whichever file that function
+    /// names is the current artifact and every other one is a file this library
+    /// does not write. Nothing regenerates those, while the consumer's `src/**`
+    /// keeps importing them, so they are refused rather than left frozen beside
+    /// a fresh generation.
+    ///
+    /// Only direct children are read. `pathForContract` never names anything
+    /// deeper, so a per release snapshot directory holds nothing this library
+    /// wrote and is left alone.
+    ///
+    /// Inherits `pathForContract`'s refusal to produce a path for a name that
+    /// is not a Solidity identifier.
+    /// @param vm The Vm instance for file operations.
+    /// @param contractName The name of the contract.
+    function requireNoOrphanedArtifact(Vm vm, string memory contractName) internal view {
+        requireNoOrphanedArtifactIn(vm, GENERATED_DIR, contractName);
+    }
+
+    /// @notice Reverts if `dir` holds an artifact for `contractName` other than
+    /// the one at `pathForContractIn(dir, contractName)`.
+    /// @dev `requireNoOrphanedArtifact` is this function applied to
+    /// `GENERATED_DIR`, so everything stated there holds here too, of `dir`
+    /// rather than of `GENERATED_DIR`. It is private for the same reason
+    /// `pathForContractIn` is: the only directory a consumer of this library
+    /// writes to is `GENERATED_DIR`, and `dir` is interpolated verbatim and is
+    /// not checked.
+    ///
+    /// The whole check is one read of `dir`, and `vm.readDir` does not revert
+    /// when that read fails: it returns a single entry naming `dir` itself and
+    /// carrying an `errorMessage`. No artifact name matches that entry, so a
+    /// directory that cannot be read is accepted rather than refused. That is
+    /// the answer wanted for a repo with no generated directory yet, and it is
+    /// why `buildFileForContract` creates `dir` before calling this: after a
+    /// `vm.createDir` that did not itself revert, the read is of a directory
+    /// that is there.
+    /// @param vm The Vm instance for file operations.
+    /// @param dir The directory to read, without a trailing separator,
+    /// interpolated verbatim.
+    /// @param contractName The name of the contract.
+    function requireNoOrphanedArtifactIn(Vm vm, string memory dir, string memory contractName) private view {
+        bytes32 currentArtifact = keccak256(bytes(lastPathSegment(pathForContractIn(dir, contractName))));
+        bytes memory prefix = bytes(string.concat(contractName, "."));
+        //forge-lint: disable-next-line(unsafe-cheatcode)
+        Vm.DirEntry[] memory entries = vm.readDir(dir);
+        for (uint256 i = 0; i < entries.length; i++) {
+            bytes memory name = bytes(lastPathSegment(entries[i].path));
+            if (name.length < prefix.length || keccak256(name) == currentArtifact) {
+                continue;
+            }
+            bool isArtifact = true;
+            for (uint256 j = 0; j < prefix.length; j++) {
+                if (name[j] != prefix[j]) {
+                    isArtifact = false;
+                    break;
+                }
+            }
+            if (isArtifact) {
+                revert OrphanedGeneratedArtifact(string.concat(dir, "/", string(name)));
+            }
+        }
+    }
+
     /// @notice Constructs the file path for a contract's generated file inside a
     /// tag's directory, which is the layout per release deploy pin snapshots
     /// use.
@@ -178,6 +277,13 @@ library LibFs {
     /// nothing is created, unlinked or written unless there is content to
     /// write.
     ///
+    /// Another artifact for the same contract already in `GENERATED_DIR`
+    /// refuses the whole call, before anything at the path is unlinked or
+    /// written, so a generation never lands beside a file that nothing
+    /// regenerates. The directory is created first, because the check is a read
+    /// of it and a consumer generating for the first time has neither the
+    /// directory nor an orphan in it.
+    ///
     /// The path is unlinked until it holds nothing, then written, so a symlink
     /// there is replaced by a regular file rather than written through to its
     /// target, whether or not that target exists, and the path does not exist
@@ -222,8 +328,9 @@ library LibFs {
     /// inside `GENERATED_DIR`.
     /// @dev Identical to `buildFileForContract` in every other respect, and
     /// that function is this one applied to `GENERATED_DIR`: `dir` is what gets
-    /// created when it is missing, and what the file is written a direct child
-    /// of. `dir` is interpolated verbatim and is not checked, so a caller
+    /// created when it is missing, what is read for another artifact of the
+    /// same contract, and what the file is written a direct child of. `dir` is
+    /// interpolated verbatim and is not checked, so a caller
     /// passing something other than a directory it means to own gets whatever
     /// `fs_permissions` allows; `contractName` is still required to be a
     /// Solidity identifier, so the name can never carry the file out of `dir`.
@@ -259,6 +366,7 @@ library LibFs {
         );
         //forge-lint: disable-next-line(unsafe-cheatcode)
         vm.createDir(dir, true);
+        requireNoOrphanedArtifactIn(vm, dir, contractName);
         // `vm.removeFile` resolves the path before it acts, so on a live symlink
         // it takes what the link points at and leaves the link, now dangling.
         // Every pass removes something the next one no longer finds, so this

test/concrete/LibFsExternal.sol

@@ -20,10 +20,26 @@ contract LibFsExternal {
         LibFs.buildFileForContract(vm, instance, contractName, spdxLicenseIdentifier, copyrightText, body);
     }
 
+    function buildFileForContract(
+        Vm vm,
+        address instance,
+        string memory dir,
+        string memory contractName,
+        string memory spdxLicenseIdentifier,
+        string memory copyrightText,
+        string memory body
+    ) external {
+        LibFs.buildFileForContract(vm, instance, dir, contractName, spdxLicenseIdentifier, copyrightText, body);
+    }
+
     function pathForContract(string memory contractName) external pure returns (string memory) {
         return LibFs.pathForContract(contractName);
     }
 
+    function requireNoOrphanedArtifact(Vm vm, string memory contractName) external view {
+        LibFs.requireNoOrphanedArtifact(vm, contractName);
+    }
+
     function buildFileForTaggedContract(
         Vm vm,
         address instance,

test/src/lib/LibFs.buildFileForContract.t.sol

@@ -3,7 +3,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 {LibFs, GENERATED_DIR, OrphanedGeneratedArtifact} from "src/lib/LibFs.sol";
 import {
     InvalidIdentifier,
     InvalidSpdxLicenseIdentifier,
@@ -466,4 +466,147 @@ contract LibFsBuildFileForContractTest is Test {
         vm.assume(!LibCodeGenSlow.isIdentifierSlow(contractName));
         assertNameRejected(contractName);
     }
+
+    /// A consumer holds the artifact this library used to write committed and
+    /// imported from `src/**`. Generating beside it would leave those imports
+    /// resolving to a file nothing regenerates, with the build reporting
+    /// success, so the generation is refused instead and nothing is written.
+    ///
+    /// The refusal lands before the unlink as well as before the write, which
+    /// is what the pre-existing file at the generated path pins: a check placed
+    /// after the unlink would leave a consumer with neither artifact.
+    function testBuildFileForContractRefusesToOrphanAnotherArtifact() external {
+        string memory name = "LibFsBuildOrphan";
+        string memory orphan = string.concat(GENERATED_DIR, "/", name, ".pointers.sol");
+        //REUSE-IgnoreStart
+        string memory existing = "// SPDX-License-Identifier: LicenseRef-DCL-1.0\npragma solidity ^0.8.25;\n";
+        //REUSE-IgnoreEnd
+        cleanupPath(orphan);
+        cleanup(name);
+        vm.writeFile(orphan, existing);
+        vm.writeFile(LibFs.pathForContract(name), "PRE-EXISTING");
+        address instance = address(new CodeGennable());
+
+        // Caught rather than expected, and everything read off disk before any
+        // assertion, so the orphan is removed on the runs that fail too. It is
+        // a file this refusal fires on, and every test here shares the one
+        // generated directory, so one left behind is a precondition the next
+        // run does not get to choose.
+        bytes memory outcome;
+        try iExternal.buildFileForContract(
+            vm, instance, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, "\n// body\n"
+        ) {}
+        catch (bytes memory reason) {
+            outcome = reason;
+        }
+        bool currentPathIsPresent = vm.exists(LibFs.pathForContract(name));
+        string memory currentPathContent = currentPathIsPresent ? vm.readFile(LibFs.pathForContract(name)) : "";
+        string memory orphanContent = vm.readFile(orphan);
+
+        cleanupPath(orphan);
+        cleanup(name);
+
+        assertEq(
+            outcome,
+            abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan),
+            string.concat("not refused as OrphanedGeneratedArtifact(", orphan, ")")
+        );
+        assertTrue(currentPathIsPresent, "a refused generation unlinked the file at the generated path");
+        assertEq(currentPathContent, "PRE-EXISTING", "a refused generation wrote a second artifact");
+        assertEq(orphanContent, existing, "the artifact that was already there was touched");
+    }
+
+    /// The refusal is the exception and not the rule: a generated directory
+    /// holding nothing for this contract generates, and one holding another
+    /// contract's artifact generates too. Driven through `buildFileForContract`
+    /// rather than through the check directly, so the call site is what is
+    /// asserted to accept.
+    function testBuildFileForContractGeneratesWhenNoOtherArtifactExists() external {
+        string memory name = "LibFsBuildNoOrphan";
+        string memory otherContract = string.concat(GENERATED_DIR, "/LibFsBuildNoOrphanOther.pointers.sol");
+        cleanup(name);
+        cleanupPath(string.concat(GENERATED_DIR, "/", name, ".pointers.sol"));
+        cleanupPath(otherContract);
+        vm.writeFile(otherContract, "// another contract's artifact\n");
+        address instance = address(new CodeGennable());
+        string memory body = "\n// no orphan\n";
+
+        LibFs.buildFileForContract(vm, instance, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body);
+
+        string memory written = vm.readFile(LibFs.pathForContract(name));
+        cleanup(name);
+        cleanupPath(otherContract);
+        assertEq(written, expectedFile(instance, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body));
+    }
+
+    /// The `dir` overload reads the directory it writes into, not
+    /// `GENERATED_DIR`. A consumer's snapshot directory and the live one hold
+    /// artifacts for the same contract names by design, so a check keyed on
+    /// `GENERATED_DIR` would refuse every snapshot generation for the live
+    /// artifact sitting beside it, and would miss an orphan in the snapshot
+    /// directory itself.
+    function testBuildFileForContractChecksTheDirectoryItWritesTo() external {
+        string memory name = "LibFsBuildDirOrphan";
+        string memory dir = string.concat(GENERATED_DIR, "/LibFsBuildDirOrphanSnapshot");
+        string memory liveOrphan = string.concat(GENERATED_DIR, "/", name, ".pointers.sol");
+        string memory dirOrphan = string.concat(dir, "/", name, ".pointers.sol");
+        cleanupPath(dir);
+        cleanupPath(liveOrphan);
+        cleanup(name);
+        address instance = address(new CodeGennable());
+        string memory body = "\n// snapshot\n";
+
+        // An orphan in `GENERATED_DIR` does not refuse a write into `dir`.
+        vm.writeFile(liveOrphan, "// live orphan\n");
+        LibFs.buildFileForContract(vm, instance, dir, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body);
+        string memory written = vm.readFile(string.concat(dir, "/", name, ".sol"));
+
+        // An orphan in `dir` does refuse it.
+        vm.writeFile(dirOrphan, "// snapshot orphan\n");
+        bytes memory outcome;
+        try iExternal.buildFileForContract(vm, instance, dir, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body) {}
+        catch (bytes memory reason) {
+            outcome = reason;
+        }
+
+        cleanupPath(dir);
+        cleanupPath(liveOrphan);
+
+        assertEq(
+            written,
+            expectedFile(instance, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body),
+            "an orphan outside the directory written to refused the write"
+        );
+        assertEq(
+            outcome,
+            abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, dirOrphan),
+            string.concat("not refused as OrphanedGeneratedArtifact(", dirOrphan, ")")
+        );
+    }
+
+    /// The check reads what is inside the directory, never the directory's own
+    /// name, and it reads it after `vm.createDir` rather than before.
+    ///
+    /// `vm.readDir` does not revert on a directory that is not there: it
+    /// returns one entry whose `path` is that directory and whose
+    /// `errorMessage` says why. A check placed ahead of the create therefore
+    /// reads that one entry, and for a directory whose own final segment reads
+    /// as an artifact for the contract it refuses the very first generation —
+    /// naming, as the orphan, the directory it was about to create. Driven
+    /// through such a directory, because that is the only shape the two
+    /// orderings disagree on.
+    function testBuildFileForContractReadsTheDirectoryNotItsOwnName() external {
+        string memory name = "LibFsBuildDirName";
+        string memory dir = string.concat(GENERATED_DIR, "/", name, ".snapshot");
+        cleanupPath(dir);
+        assertFalse(vm.exists(dir), "dirty precondition");
+        address instance = address(new CodeGennable());
+        string memory body = "\n// dir name\n";
+
+        LibFs.buildFileForContract(vm, instance, dir, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body);
+
+        string memory written = vm.readFile(string.concat(dir, "/", name, ".sol"));
+        cleanupPath(dir);
+        assertEq(written, expectedFile(instance, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body));
+    }
 }