From 02348cf94ff21d585ca43c22be69433af9cd3b98 Mon Sep 17 00:00:00 2001 From: Alex Gherghisan Date: Fri, 17 Nov 2023 17:45:20 +0000 Subject: [PATCH] fix: noir-compiler breadth-first resolver (#3307) This PR fixes an issue hit by @spypsy and @catmcgee last week while working on a sample contract. They were not able to compile their contract because the compiler failed to find all of the needed dependencies. I've identified the issue with `NoirDependencyManager` resolving dependencies in a depth-first manner. This caused issues with libraries that had dependencies of their own linked to by relative paths. This was a problem because the new pipeline using wasm only unpacks the required folder from a github archive (while Nargo gets the whole thing). In the contract's case, it had dependencies on easy_private_state and value_note (in this order) but easy_private_state needed value_note to exist before the manager resolved it. This PR replaces the algorithm with a breadth-first resolver which fixes this. It also adds updates the existing unit test for the manager to check this edge case. --- .../noir-compiler/src/cli/contract.ts | 2 - .../dependencies/dependency-manager.test.ts | 7 ++- .../noir/dependencies/dependency-manager.ts | 56 +++++++++++++------ 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/yarn-project/noir-compiler/src/cli/contract.ts b/yarn-project/noir-compiler/src/cli/contract.ts index 20b61de076a..c01ea134626 100644 --- a/yarn-project/noir-compiler/src/cli/contract.ts +++ b/yarn-project/noir-compiler/src/cli/contract.ts @@ -69,13 +69,11 @@ export function compileContract(program: Command, name = 'contract', log: LogFn const tsPath = resolve(projectPath, typescript, `${contract.name}.ts`); log(`Writing ${contract.name} typescript interface to ${path.relative(currentDir, tsPath)}`); let relativeArtifactPath = path.relative(path.dirname(tsPath), artifactPath); - log(`Relative path: ${relativeArtifactPath}`); if (relativeArtifactPath === `${contract.name}.json`) { // relative path edge case, prepending ./ for local import - the above logic just does // `${contract.name}.json`, which is not a valid import for a file in the same directory relativeArtifactPath = `./${contract.name}.json`; } - log(`Relative path after correction: ${relativeArtifactPath}`); const tsWrapper = generateTypescriptContractInterface(contract, relativeArtifactPath); mkdirpSync(path.dirname(tsPath)); writeFileSync(tsPath, tsWrapper); diff --git a/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.test.ts b/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.test.ts index 94996a54dd2..5f9ba570d4d 100644 --- a/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.test.ts +++ b/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.test.ts @@ -18,6 +18,9 @@ describe('DependencyManager', () => { lib2: { path: '/lib2', }, + lib3: { + path: '/lib3', + }, }, package: { name: 'test_contract', @@ -38,7 +41,7 @@ describe('DependencyManager', () => { it('resolves root dependencies', async () => { await manager.resolveDependencies(); - expect(manager.getEntrypointDependencies()).toEqual(['lib1', 'lib2']); + expect(manager.getEntrypointDependencies()).toEqual(['lib1', 'lib2', 'lib3']); }); it('resolves library dependencies', async () => { @@ -75,7 +78,7 @@ class TestDependencyResolver implements NoirDependencyResolver { package: new NoirPackage('/lib2', '/lib2/src', { dependencies: { lib3: { - path: '/lib3', + path: '../lib3', }, }, package: { diff --git a/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.ts b/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.ts index 0581917f556..44bdd25743a 100644 --- a/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.ts +++ b/yarn-project/noir-compiler/src/compile/noir/dependencies/dependency-manager.ts @@ -46,7 +46,7 @@ export class NoirDependencyManager { * Resolves dependencies for a package. */ public async resolveDependencies(): Promise { - await this.#recursivelyResolveDependencies('', this.#entryPoint); + await this.#breadthFirstResolveDependencies(); } /** @@ -59,26 +59,46 @@ export class NoirDependencyManager { return dep?.version; } - async #recursivelyResolveDependencies(packageName: string, noirPackage: NoirPackage): Promise { - for (const [name, config] of Object.entries(noirPackage.getDependencies())) { - // TODO what happens if more than one package has the same name but different versions? - if (this.#libraries.has(name)) { - this.#log(`skipping already resolved dependency ${name}`); + async #breadthFirstResolveDependencies(): Promise { + /** Represents a package to resolve dependencies for */ + type Job = { + /** Package name */ + packageName: string; + /** The package location */ + noirPackage: NoirPackage; + }; + + const queue: Job[] = [ + { + packageName: '', + noirPackage: this.#entryPoint, + }, + ]; + + while (queue.length > 0) { + const { packageName, noirPackage } = queue.shift()!; + for (const [name, config] of Object.entries(noirPackage.getDependencies())) { + // TODO what happens if more than one package has the same name but different versions? + if (this.#libraries.has(name)) { + this.#log(`skipping already resolved dependency ${name}`); + this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]); + + continue; + } + const dependency = await this.#resolveDependency(noirPackage, config); + if (dependency.package.getType() !== 'lib') { + this.#log(`Non-library package ${name}`, config); + throw new Error(`Dependency ${name} is not a library`); + } + + this.#libraries.set(name, dependency); this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]); - continue; + queue.push({ + noirPackage: dependency.package, + packageName: name, + }); } - - const dependency = await this.#resolveDependency(noirPackage, config); - if (dependency.package.getType() !== 'lib') { - this.#log(`Non-library package ${name}`, config); - throw new Error(`Dependency ${name} is not a library`); - } - - this.#libraries.set(name, dependency); - this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]); - - await this.#recursivelyResolveDependencies(name, dependency.package); } }