No warnings emitted when auto-detection corrects incorrect import paths

[samooyo]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (e028b92 2024-11-11T00:22:42.074556087Z)

What command(s) is the bug in?

forge build

Operating System

Linux

Describe the bug

When Foundry encounters an incorrect import path in a contract, it appears to automatically correct it without emitting any warnings or notifications. This silent correction prevents developers from being aware that there was an issue with the original import path, which could lead to confusion or unintended behavior.

In my case, there was a double slash in the path of an import (//), which made it difficult to debug. I was using the OpenZeppelin Upgrades plugin to deploy a proxy with my contract, and during the validation step, I encountered an unexpected error: Cannot read properties of undefined (reading 'ast'). This error was caused by OpenZeppelin not finding the specified import, even though everything was compiling correctly.

I've created a minimal example to demonstrate this error here.
You can see that in src/TestContract.sol, line 4, the import is: import "@openzeppelin/contracts-upgradeable//proxy/utils/Initializable.sol"; but still compile.

Expected Behavior

If an import path is detected as incorrect, a warning should be emitted to inform the user of the original issue before auto-correcting it. This would allow developers to address the root cause of the path issue directly.

Steps to Reproduce

1. Clone the repo: git clone https://github.com/samooyo/foundry-bug-report.git
2. Run the script: forge script script/Deploy.s.sol

[grandizzy]

I think command should just fail in such cases, forge build is passing even with an import like import "@openzeppelin/contracts-upgradeable//././proxy/////utils////././//Initializable.sol";

@klkvr could you please share insights? Thanks!

[klkvr]

such paths are valid from both filesystem and solc pov. changing the way this is handled would be a breaking change

imo it doesn't make much sense to change this to comply with external tool expectations, however I am wondering on which stage the actual error actually happens? could we do better in e.g writing artifacts? will take a look at upgrades typescript library

[klkvr]

looks like the root issue is here
https://github.com/OpenZeppelin/openzeppelin-upgrades/blob/b41336b157fb944ddd1e8743a6eaea314b6676f9/packages/core/src/validate/run.ts#L166
https://github.com/OpenZeppelin/openzeppelin-upgrades/blob/b41336b157fb944ddd1e8743a6eaea314b6676f9/packages/core/src/validate/run.ts#L269

this logic assumes that all keys present in output.contracts are also present in output.sources. however, for some reason in this case both input.sources and output.contracts contain normalized path but output.sources contains path with 2 slashes

[klkvr]

alright, so this is actually an issue related to differences in our vs solc representation of compiler output.

in actual solc output.sources there are both normal path and path with 2 slashes. however, when deserializing, we end up with a single entry with 2 slashes.

this is happening because from PathBuf pov both paths are identical, thus when deserializing into BTreeMap<PathBuf, SourceFile>, we can only have a single entry for both paths

this behavior can be reproduced like this:

fn main() {
    let data: BTreeMap<PathBuf, String> = serde_json::from_str(
        r#"{
            "src/file.sol": "a",
            "src//file.sol": "b"
        }"#,
    )
    .unwrap();

    assert_eq!(data, BTreeMap::from([(PathBuf::from("src/file.sol"), String::from("b"))]));
    assert_eq!(data, BTreeMap::from([(PathBuf::from("src//file.sol"), String::from("b"))]));
}

unsure what's the best approach to address this would be. probably we should prefer deserializing only keys which match the input.sources values

[grandizzy]

@klkvr that's interesting and I suspect #9272 is also related (was marked as fixed but actually is not, would be great to get a confirmation). in morpho-org/morpho-blue-bundlers@331b310 there's a wrong path in tests (that is an additional ../ in imports after refactoring), so when building, in the first phase bad imports are shown

[⠒] Compiling...
[⠢] Unable to resolve imports:
      "../../../../src/chain-agnostic/ChainAgnosticBundlerV2.sol" in "/home/george/work/foundry-issues/morpho-blue-bundlers/test/fork/helpers/ForkTest.sol"
      "../../../lib/morpho-blue/src/interfaces/IMorpho.sol" in "/home/george/work/foundry-issues/morpho-blue-bundlers/test/helpers/SigUtils.sol"
      "../../../src/ethereum/EthereumStEthBundler.sol" in "/home/george/work/foundry-issues/morpho-blue-bundlers/test/fork/StEthBundlerForkTest.sol"
      "../../../../src/interfaces/IStEth.sol" in "/home/george/work/foundry-issues/morpho-blue-bundlers/test/fork/helpers/ForkTest.sol"
...

however build continues and completes successfully after

[⠰] Compiling 163 files with Solc 0.8.24
[⠔] Compiling 42 files with Solc 0.8.21
[⠔] Compiling 40 files with Solc 0.8.19
[⠰] Solc 0.8.21 finished in 4.87s
[⠔] Solc 0.8.19 finished in 6.69s

Could you pls confirm if it is due to the same behavior? thank you!

[klkvr]

I think it's a different issue. the errors reported during path resoluting is pretty much just our internal logic which doesn't touch solc at all, so probably it's just that we fail to resolve some of the actually valid imports, and then solc figures it out

[klkvr]

unsure what's actually going on there haha, the imports are indeed invalid and looks like you can add arbitrary number of ../ to it without solc complaining

[grandizzy]

yeah, I thought is something cache related but I wiped project several times and rebuilt with same weirdness :)