Implement PR feedback

Author: antico5

v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts

@@ -139,29 +139,31 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
     const { compilationJobsPerFile, indexedIndividualJobs } =
       compilationJobsResult;
 
-    const compilationJobs = [...new Set(compilationJobsPerFile.values())];
+    const runnableCompilationJobs = [
+      ...new Set(compilationJobsPerFile.values()),
+    ];
 
     // NOTE: We precompute the build ids in parallel here, which are cached
     // internally in each compilation job
     await Promise.all(
-      compilationJobs.map(async (compilationJob) =>
-        compilationJob.getBuildId(),
+      runnableCompilationJobs.map(async (runnableCompilationJob) =>
+        runnableCompilationJob.getBuildId(),
       ),
     );
 
     const runCompilationJobOptions: RunCompilationJobOptions = {
       quiet: options?.quiet,
     };
     const results: CompilationResult[] = await pMap(
-      compilationJobs,
-      async (compilationJob) => {
+      runnableCompilationJobs,
+      async (runnableCompilationJob) => {
         const compilerOutput = await this.runCompilationJob(
-          compilationJob,
+          runnableCompilationJob,
           runCompilationJobOptions,
         );
 
         return {
-          compilationJob,
+          compilationJob: runnableCompilationJob,
           compilerOutput,
           cached: false,
         };
@@ -276,7 +278,7 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
 
     if (options?.quiet !== true) {
       if (isSuccessfulBuild) {
-        await this.#printCompilationResult(compilationJobs);
+        await this.#printCompilationResult(runnableCompilationJobs);
       }
     }
 
@@ -348,7 +350,7 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
 
     // build job for each root file. At this point subgraphsWithConfig are 1 root file each
     const indexedIndividualJobs: Map<string, CompilationJob> = new Map();
-    const contentHashes = new Map<string, string>();
+    const sharedContentHashes = new Map<string, string>();
     await Promise.all(
       subgraphsWithConfig.map(async ([config, subgraph]) => {
         const solcLongVersion = solcVersionToLongVersion.get(config.version);
@@ -358,15 +360,15 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
           "solcLongVersion should not be undefined",
         );
 
-        const compilationJob = new CompilationJobImplementation(
+        const individualJob = new CompilationJobImplementation(
           subgraph,
           config,
           solcLongVersion,
           this.#hooks,
-          contentHashes,
+          sharedContentHashes,
         );
 
-        await compilationJob.getBuildId(); // precompute
+        await individualJob.getBuildId(); // precompute
 
         assertHardhatInvariant(
           subgraph.getRoots().size === 1,
@@ -375,7 +377,7 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
 
         const rootFilePath = Array.from(subgraph.getRoots().keys())[0];
 
-        indexedIndividualJobs.set(rootFilePath, compilationJob);
+        indexedIndividualJobs.set(rootFilePath, individualJob);
       }),
     );
 
@@ -483,18 +485,18 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
         "solcLongVersion should not be undefined",
       );
 
-      const compilationJob = new CompilationJobImplementation(
+      const runnableCompilationJob = new CompilationJobImplementation(
         subgraph,
         solcConfig,
         solcLongVersion,
         this.#hooks,
-        contentHashes,
+        sharedContentHashes,
       );
 
       for (const [userSourceName, root] of subgraph.getRoots().entries()) {
         compilationJobsPerFile.set(
           formatRootPath(userSourceName, root),
-          compilationJob,
+          runnableCompilationJob,
         );
       }
     }
@@ -503,34 +505,37 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
   }
 
   public async runCompilationJob(
-    compilationJob: CompilationJob,
+    runnableCompilationJob: CompilationJob,
     options?: RunCompilationJobOptions,
   ): Promise<CompilerOutput> {
     await this.#downloadConfiguredCompilers(options?.quiet);
 
     let numberOfFiles = 0;
-    for (const _ of compilationJob.dependencyGraph.getAllFiles()) {
+    for (const _ of runnableCompilationJob.dependencyGraph.getAllFiles()) {
       numberOfFiles++;
     }
 
-    const numberOfRootFiles = compilationJob.dependencyGraph.getRoots().size;
+    const numberOfRootFiles =
+      runnableCompilationJob.dependencyGraph.getRoots().size;
 
-    const compiler = await getCompiler(compilationJob.solcConfig.version);
+    const compiler = await getCompiler(
+      runnableCompilationJob.solcConfig.version,
+    );
 
     log(
-      `Compiling ${numberOfRootFiles} root files and ${numberOfFiles - numberOfRootFiles} dependency files with solc ${compilationJob.solcConfig.version} using ${compiler.compilerPath}`,
+      `Compiling ${numberOfRootFiles} root files and ${numberOfFiles - numberOfRootFiles} dependency files with solc ${runnableCompilationJob.solcConfig.version} using ${compiler.compilerPath}`,
     );
 
     assertHardhatInvariant(
-      compilationJob.solcLongVersion === compiler.longVersion,
+      runnableCompilationJob.solcLongVersion === compiler.longVersion,
       "The long version of the compiler should match the long version of the compilation job",
     );
 
-    return compiler.compile(await compilationJob.getSolcInput());
+    return compiler.compile(await runnableCompilationJob.getSolcInput());
   }
 
   public async remapCompilerError(
-    compilationJob: CompilationJob,
+    runnableCompilationJob: CompilationJob,
     error: CompilerOutputError,
     shouldShortenPaths: boolean = false,
   ): Promise<CompilerOutputError> {
@@ -544,7 +549,7 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
         /(-->\s+)([^\s:\n]+)/g,
         (_match, prefix, inputSourceName) => {
           const file =
-            compilationJob.dependencyGraph.getFileByInputSourceName(
+            runnableCompilationJob.dependencyGraph.getFileByInputSourceName(
               inputSourceName,
             );
 
@@ -563,17 +568,17 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
   }
 
   public async emitArtifacts(
-    compilationJob: CompilationJob,
+    runnableCompilationJob: CompilationJob,
     compilerOutput: CompilerOutput,
   ): Promise<EmitArtifactsResult> {
     const artifactsPerFile = new Map<string, string[]>();
     const typeFilePaths = new Map<string, string>();
-    const buildId = await compilationJob.getBuildId();
+    const buildId = await runnableCompilationJob.getBuildId();
 
     // We emit the artifacts for each root file, first emitting one artifact
     // for each contract, and then one declaration file for the entire file,
     // which defines their types and augments the ArtifactMap type.
-    for (const [userSourceName, root] of compilationJob.dependencyGraph
+    for (const [userSourceName, root] of runnableCompilationJob.dependencyGraph
       .getRoots()
       .entries()) {
       const fileFolder = path.join(this.#options.artifactsPath, userSourceName);
@@ -648,15 +653,15 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
     // concurrently, and keep their lifetimes separated and small.
     await Promise.all([
       (async () => {
-        const buildInfo = await getBuildInfo(compilationJob);
+        const buildInfo = await getBuildInfo(runnableCompilationJob);
 
         // TODO: Maybe formatting the build info is slow, but it's mostly
         // strings, so it probably shouldn't be a problem.
         await writeJsonFile(buildInfoPath, buildInfo);
       })(),
       (async () => {
         const buildInfoOutput = await getBuildInfoOutput(
-          compilationJob,
+          runnableCompilationJob,
           compilerOutput,
         );
 
@@ -923,13 +928,13 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
     }
   }
 
-  async #printCompilationResult(compilationJobs: CompilationJob[]) {
+  async #printCompilationResult(runnableCompilationJobs: CompilationJob[]) {
     const jobsPerVersionAndEvmVersion = new Map<
       string,
       Map<string, CompilationJob[]>
     >();
 
-    for (const job of compilationJobs) {
+    for (const job of runnableCompilationJobs) {
       const solcVersion = job.solcConfig.version;
       const solcInput = await job.getSolcInput();
       const evmVersion =

v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/cache.ts

@@ -6,6 +6,12 @@ import {
   writeJsonFileAsStream,
 } from "@nomicfoundation/hardhat-utils/fs";
 
+/**
+ * This object is used to store what source files produced which output files
+ * and what was the last compiled jobHash (buildId) for the individual root file
+ * The keys are the root file paths, as passed to the compile task. For project
+ * files this would be the user source names.
+ */
 export type CompileCache = Record<string, CompileCacheEntry>;
 
 export interface CompileCacheEntry {

v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/compilation-job.ts

@@ -25,7 +25,9 @@ export class CompilationJobImplementation implements CompilationJob {
   public readonly solcLongVersion: string;
 
   readonly #hooks: HookManager;
-  readonly #contentHashes: Map<string, string>;
+  // This map is shared across compilation jobs and is meant to store content hashes of source files
+  // It is used to speed up the hashing of compilation jobs
+  readonly #sharedContentHashes: Map<string, string>;
 
   #buildId: string | undefined;
   #solcInput: CompilerInput | undefined;
@@ -35,13 +37,13 @@ export class CompilationJobImplementation implements CompilationJob {
     solcConfig: SolcConfig,
     solcLongVersion: string,
     hooks: HookManager,
-    contentHashes: Map<string, string> = new Map(),
+    sharedContentHashes: Map<string, string> = new Map(),
   ) {
     this.dependencyGraph = dependencyGraph;
     this.solcConfig = solcConfig;
     this.solcLongVersion = solcLongVersion;
     this.#hooks = hooks;
-    this.#contentHashes = contentHashes;
+    this.#sharedContentHashes = sharedContentHashes;
   }
 
   public async getSolcInput(): Promise<CompilerInput> {
@@ -197,15 +199,10 @@ export class CompilationJobImplementation implements CompilationJob {
     // the format of the BuildInfo type.
     const format: BuildInfo["_format"] = "hh3-sol-build-info-1";
 
-    // NOTE: Historically, we used the source content hashes instead of the full
-    // source contents inside the solc input used to compute the build id here.
-    // This was an optimization that sped up the build ID computation in a case
-    // where multiple compilation jobs share some source files. We decided to
-    // remove it once the code coverage was added because it simplified the
-    // implementation and because we expect the caching logic to change.
     const solcInput = await this.getSolcInput();
     const smallerSolcInput = { ...solcInput };
 
+    // We replace the source files content with their hashes for speeding up the build id computation
     smallerSolcInput.sources = Object.fromEntries(
       Object.entries(solcInput.sources).map(([sourceName, _source]) => [
         sourceName,
@@ -227,13 +224,13 @@ export class CompilationJobImplementation implements CompilationJob {
   }
 
   #getSourceContentHash(sourceName: string, text: string): any {
-    let hash = this.#contentHashes.get(sourceName);
+    let hash = this.#sharedContentHashes.get(sourceName);
 
     if (hash !== undefined) {
       return hash;
     }
     hash = createHash("sha1").update(text).digest("hex");
-    this.#contentHashes.set(sourceName, hash);
+    this.#sharedContentHashes.set(sourceName, hash);
     return hash;
   }
 }

v-next/hardhat/src/types/solidity/build-system.ts

@@ -173,7 +173,7 @@ export interface GetCompilationJobsResult {
 export interface EmitArtifactsResult {
   artifactsPerFile: ReadonlyMap<string, string[]>;
   buildInfoPath: string;
-  typeFilePaths: Map<string, string>;
+  typeFilePaths: ReadonlyMap<string, string>;
   buildInfoOutputPath: string;
 }