Conversation
🦋 Changeset detectedLatest commit: f344087 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
9ecf615 to
43bb131
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces compiler-agnostic Solidity compiler config base interfaces (SolidityCompilerConfig / SolidityCompilerUserConfig) with a type discriminator to support multiple compiler implementations (e.g. future solx integration) while keeping SolcConfig backwards compatible.
Changes:
- Add
SolidityCompilerConfig/SolidityCompilerUserConfigbase interfaces with optionaltype, and widen relevant hook/config types to use the base interface. - Export
isSolcConfig()type guard andspawnCompile()for plugin reuse via a newhardhat/internal/solidityexport path. - Relax custom compiler version parsing to accept plain semver, add tests, and update config resolution tests to cover
typebehavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| v-next/hardhat/src/internal/builtin-plugins/solidity/type-extensions.ts | Introduces base compiler config/user config interfaces and widens hook/config types to compiler-agnostic ones. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/config.ts | Adds type runtime validation and resolves configs via the new base compiler config shape. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts | Adds isSolcConfig guard and adjusts preferWasm/version validation logic for generalized compiler configs. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/solc-config-selection.ts | Widens selector return type from SolcConfig to SolidityCompilerConfig. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/compiler/index.ts | Extends custom compiler --version parsing to accept plain semver and synthesize a long version. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/compiler/compiler.ts | Exports spawnCompile for reuse. |
| v-next/hardhat/src/internal/builtin-plugins/solidity/exports.ts | Adds internal re-exports (isSolcConfig, spawnCompile) for plugin consumption. |
| v-next/hardhat/src/types/solidity/compilation-job.ts | Updates compilation job typing/docs to use the compiler-agnostic config type. |
| v-next/hardhat/package.json | Adds the ./internal/solidity package export. |
| v-next/hardhat/test/internal/builtin-plugins/solidity/config.ts | Adds tests for type validation/resolution and isSolcConfig. |
| v-next/hardhat/test/internal/builtin-plugins/solidity/hooks.ts | Updates hook typing in tests to accept SolidityCompilerConfig. |
| v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compilation-job.ts | Updates test typing to SolidityCompilerConfig. |
| v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compiler/version-parsing.ts | Adds unit tests for the updated version parsing behavior. |
| .changeset/cold-deer-buy.md | Adds a patch changeset entry for the new abstraction/export surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts
Outdated
Show resolved
Hide resolved
| // Resolve solc-specific preferWasm if this is a SolcUserConfig | ||
| if (isSolcUserConfig(compilerConfig)) { | ||
| // Resolve per-compiler preferWasm: | ||
| // If explicitly set, use that value. | ||
| // Otherwise, for ARM64 Linux in production, default to true only for |
There was a problem hiding this comment.
resolveSolidityCompilerConfig drops solc-only fields like preferWasm when type is non-solc, but the current zod schema still allows preferWasm on any compiler entry. This can silently ignore user config mistakes (e.g. type: "solx" with preferWasm: true). Consider making the per-compiler schema conditional: only allow preferWasm when type is undefined/"solc", and reject it for other type values.
62da1a7 to
1cd6812
Compare
1cd6812 to
0f7deb7
Compare
d004fe7 to
7cade07
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "./internal/coverage": "./dist/src/internal/builtin-plugins/coverage/exports.js", | ||
| "./internal/gas-analytics": "./dist/src/internal/builtin-plugins/gas-analytics/exports.js", | ||
| "./internal/solidity": "./dist/src/internal/builtin-plugins/solidity/exports.js", |
There was a problem hiding this comment.
The package.json exports map now includes "./internal/coverage" and "./internal/gas-analytics" pointing at dist/src/internal/builtin-plugins/{coverage,gas-analytics}/exports.js, but there are no corresponding source entrypoints (exports.ts) in those directories. This will produce broken package exports at publish/build time unless those files are added or the export targets are updated to existing built artifacts (e.g. index.js).
| "./internal/coverage": "./dist/src/internal/builtin-plugins/coverage/exports.js", | |
| "./internal/gas-analytics": "./dist/src/internal/builtin-plugins/gas-analytics/exports.js", | |
| "./internal/solidity": "./dist/src/internal/builtin-plugins/solidity/exports.js", | |
| "./internal/coverage": "./dist/src/internal/builtin-plugins/coverage/index.js", | |
| "./internal/gas-analytics": "./dist/src/internal/builtin-plugins/gas-analytics/index.js", | |
| "./internal/solidity": "./dist/src/internal/builtin-plugins/solidity/index.js", |
There was a problem hiding this comment.
I wouldn't pay attention to this comment, LGTM
| type: compilerConfig.type, | ||
| version: compilerConfig.version, | ||
| settings: resolvedSettings, | ||
| path: compilerConfig.path, |
There was a problem hiding this comment.
For non-"solc" compiler types the zod schema allows arbitrary extra fields via .passthrough(), but resolveSolidityCompilerConfig reconstructs a new object with only { type, version, settings, path }, dropping any plugin-defined fields from the resolved config. If plugins are expected to add compiler-specific options (as implied by passthrough), those options won’t be available later in the build pipeline. Consider preserving unknown fields (e.g. by spreading the original config and overriding settings with the merged value) for non-solc types.
| type: compilerConfig.type, | |
| version: compilerConfig.version, | |
| settings: resolvedSettings, | |
| path: compilerConfig.path, | |
| ...compilerConfig, | |
| settings: resolvedSettings, |
There was a problem hiding this comment.
This comment is irrelevant, because we have our own resolution. This change looks good to me.
| public selectBestSolcConfigForSingleRootGraph( | ||
| subgraph: DependencyGraph, | ||
| ): { success: true; config: SolcConfig } | CompilationJobCreationError { | ||
| ): | ||
| | { success: true; config: SolidityCompilerConfig } | ||
| | CompilationJobCreationError { |
There was a problem hiding this comment.
SolcConfigSelector now returns a generic SolidityCompilerConfig, but selection is still based only on version and doesn’t consider type. If a build profile contains multiple compiler types (or two configs share the same version but different types), the selected config can be ambiguous/non-deterministic. Consider either validating that each build profile is single-compiler-type for now, or including type as part of the selection/identity logic (and any tie-breaking).
There was a problem hiding this comment.
This is an interesting point. But the whole selector class works with a single build profile. It will return the right SolidityCompilerConfig based either on the version matching logic, or overrides.
Copilot isn't aware that we only have a single compiler config per version, so the situation of matching the same version but different types is not possible.
However, we should make sure that we are considering the type when merging compilation jobs.
There was a problem hiding this comment.
However, we should make sure that we are considering the type when merging compilation jobs.
Self-answer: we merge by SolidityCompilerConfig identity, not even value, so the type is taken into account.
| let subgraphsWithConfig: Array< | ||
| [SolcConfig, DependencyGraphImplementation] | ||
| [SolidityCompilerConfig, DependencyGraphImplementation] | ||
| > = []; |
There was a problem hiding this comment.
subgraphsWithConfig now carries SolidityCompilerConfig, but the build-system logic downstream still treats compiler configs as solc-only (e.g. compiler metadata caches keyed by version, and getCompiler(version, { preferWasm, compilerPath })). With multiple compiler types this can conflate different compilers that share a version string and makes it easy to accidentally run the solc pipeline for non-solc configs. Consider incorporating type into cache keys / compiler selection, or rejecting mixed/non-solc compiler types until the build-system paths are properly split by type.
There was a problem hiding this comment.
This is a very good point tbh
There was a problem hiding this comment.
Yes, we would need to add type to CompileCacheEntry.
5a48ee9 to
2fe6cb9
Compare
2fe6cb9 to
5b313d0
Compare
…e for the new type field
…gisteredCompilerTypes
5544e68 to
5d68d88
Compare
5d68d88 to
1163e75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/cache.ts:26
compilerTypeis required inCompileCacheEntry, butloadCache()can return entries from older cache files where this field is missing. Consider makingcompilerTypeoptional in the type (and/or normalizing/migrating loaded entries) so the TypeScript type matches the runtime data shape and avoids unsafe assumptions elsewhere.
export interface CompileCacheEntry {
jobHash: string;
isolated: boolean;
compilerType: string;
buildInfoPath: string;
buildInfoOutputPath: string;
artifactPaths: string[];
typeFilePath?: string;
wasm: boolean;
}
You can also share your feedback on Copilot code review. Take the survey.
| ...incompatibleCompilerFields, | ||
| }); | ||
|
|
||
| // This defintion needs to be aligned with solidityCompilerUserConfigType. |
| const versionPart = this.solcConfig.version.replaceAll(".", "_"); | ||
|
|
||
| // For non-solc compiler types, include the compiler type in the build ID. | ||
| // We keep the `solc-` prefix for all types to avoid breaking codepaths | ||
| // that look for it. | ||
| if (compilerType !== undefined && compilerType !== "solc") { | ||
| /* eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- | ||
| compilerType is `never` in the base type system (only "solc" is registered), | ||
| but plugins can extend SolidityCompilerConfigPerType to add new compiler types. */ | ||
| return `solc-${versionPart}-${compilerType}-${jobHash}`; | ||
| } |
| const BUILD_INFO_FORMAT = | ||
| /^solc-(?<major>\d+)_(?<minor>\d+)_(?<patch>\d+)-[0-9a-fA-F]*$/; | ||
| export const BUILD_INFO_FORMAT: RegExp = | ||
| /^solc-(?<major>\d+)_(?<minor>\d+)_(?<patch>\d+)(?:-(?<compilerType>[a-zA-Z][a-zA-Z0-9]*))?-[0-9a-fA-F]*$/; |
.changeset/five-experts-beg.md
Outdated
| "hardhat": patch | ||
| --- | ||
|
|
||
| Add an compilerType field to the `SolidityBuildInfo` and their ids. Where undefined/not-present means "solc". ([#8008](https://github.com/NomicFoundation/hardhat/pull/8008)) |
| "Expected a string or an array of strings", | ||
| ); | ||
| /** | ||
| * The top-level type SolidityUserConfig is a unition type too complex for |
| * `compilers` field. | ||
| */ | ||
| const incompatibleCompilerFields = { | ||
| type: incompatibleFieldType("This field is incompatible with `profiles`"), |
| const commonSolidityCompilerUserConfigFields = { | ||
| type: z.string().optional(), | ||
| version: z.string(), | ||
| settings: z.any().optional(), | ||
| path: z.string().optional(), | ||
| preferWasm: z.boolean().optional(), | ||
| compilers: incompatibleFieldType("This field is incompatible with `version`"), | ||
| overrides: incompatibleFieldType("This field is incompatible with `version`"), | ||
| profiles: incompatibleFieldType("This field is incompatible with `version`"), | ||
| }); | ||
| }; |
127601c to
4238ea7
Compare
| /* eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- | ||
| compilerType is `never` in the base type system (only "solc" is registered), | ||
| but plugins can extend SolidityCompilerConfigPerType to add new compiler types. */ | ||
| return `solc-${versionPart}-${compilerType}-${jobHash}`; | ||
| } | ||
|
|
||
| return `solc-${versionPart}-${jobHash}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
compilerType is interpolated verbatim into the build id (solc-<version>-<compilerType>-<hash>). Because build ids are later used as filenames (e.g. artifacts/build-info/<buildId>.json), a compiler type containing path separators or extra dashes can create invalid/ambiguous ids and potentially unsafe paths. Consider validating/sanitizing compilerType before using it in the id (e.g. enforce an allowed pattern and throw a config validation error if it doesn’t match), or encode it so that it can’t contain /, .., or --delimiters.
| /* eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- | |
| compilerType is `never` in the base type system (only "solc" is registered), | |
| but plugins can extend SolidityCompilerConfigPerType to add new compiler types. */ | |
| return `solc-${versionPart}-${compilerType}-${jobHash}`; | |
| } | |
| return `solc-${versionPart}-${jobHash}`; | |
| } | |
| const compilerTypeForId = this.#sanitizeCompilerTypeForBuildId(compilerType); | |
| /* eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- | |
| compilerType is `never` in the base type system (only "solc" is registered), | |
| but plugins can extend SolidityCompilerConfigPerType to add new compiler types. */ | |
| return `solc-${versionPart}-${compilerTypeForId}-${jobHash}`; | |
| } | |
| return `solc-${versionPart}-${jobHash}`; | |
| } | |
| #sanitizeCompilerTypeForBuildId(compilerType: string): string { | |
| // Restrict the compiler type used in build IDs to a safe subset of characters | |
| // to avoid introducing path separators or ambiguous delimiters into filenames. | |
| // This keeps behavior unchanged for "normal" identifiers while safely | |
| // normalizing any unexpected input. | |
| return compilerType.replace(/[^A-Za-z0-9_]/g, "_"); | |
| } |
| const commonSolidityCompilerUserConfigFields = { | ||
| type: z.string().optional(), | ||
| version: z.string(), | ||
| settings: z.any().optional(), | ||
| path: z.string().optional(), | ||
| preferWasm: z.boolean().optional(), | ||
| compilers: incompatibleFieldType("This field is incompatible with `version`"), | ||
| overrides: incompatibleFieldType("This field is incompatible with `version`"), | ||
| profiles: incompatibleFieldType("This field is incompatible with `version`"), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The type discriminator is currently validated as an arbitrary string (type: z.string().optional()), but downstream the build id format and EDR parsing regex assume a restricted token-like compiler type. Without restricting/validating this field, users/plugins can create compiler types that yield build ids that can’t be parsed and/or aren’t safe as filenames (e.g. containing /, .., or -). Consider tightening the schema (and/or adding a resolved-config validation) to enforce a safe pattern for compiler types that matches the build id format.
Summary
This change introduces compiler-agnostic base interfaces:
SolidityCompilerConfig,SolidityCompilerUserConfigandSolidityCompilerType, with a newtypeproperty to distinguish between compilers e.g. when downloading/injecting settings/getting the compiler/invoking compilation.Note: This PR also introduces a new pattern to add post-validation hooks after a config has been resolved. We need this because we need to validate that all compiler entries in the config have been 'claimed' by an existing plugin (either the built-in solc, or other plugins) and that no remaining compiler types are 'orphaned' (e.g. due to typos).
This is the first change in a series of PRs to introduce support for solx in Hardhat 3. In later PRs, the
typeproperty will be used to split handling of downloads, settings/extra args, and compilation invocation between the current built-in solc pipeline, and a new solx compilation path (which will be made available via a plugin).Details
This is a backward compatible change for Hardhat users, and a mostly backward compatible change for plugin authors - it is strictly backward compatible at runtime, although at build time there is a less likely minor type-level breaking change: invokeSolc hook parameter widens from SolcConfig to SolidityCompilerConfig. Existing handlers compile and run without changes. Plugins accessing preferWasm should use the isSolcConfig() type guard.