bug(config): file paths not resolved if submodule specifies root path remappings

[mds1]

Component

Forge

Describe the feature you would like

Steps to reproduce:

1. Clone this repo and run forge install
2. Run forge build, it fails with the below error
3. In lib/forge-test-my-dep, open foundry.toml and comment out the remappings
4. Run forge build, it passes

[⠊] Compiling...
Error: 
Failed to resolve file: "/Users/mds/Documents/projects/forge-test-my-proj/lib/forge-test-my-dep/script/BaseScript.sol": No such file or directory (os error 2).
    --> "/Users/mds/Documents/projects/forge-test-my-proj/script/Counter.s.sol"
        "script/BaseScript.sol"
    Check configured remappings.

Additional context

No response

[dalechyn]

suffering from the same exact issue.
importing @thirdweb contracts and OZ in the project breaks remappings.
I have the latest foundry and foundry-std versions

[mattsse]

this is a general problem with absolute import paths.

you can fix this by using relative paths for contracts you import from folders like script or test, or simply overwrite the remapping.

I'd recommend against remappings for folders in the same project.

[mattsse]

ref #1855

[PaulRBerg]

Just bumped into this issue while installing @mds1's library solidity-generators.

I also have absolute import paths, and I want to counter @mattsse's suggestion from above that we should simply use relative paths.

Relative paths can get really ugly and hard to maintain in complex projects; several Foundry users use absolute paths because of the aforementioned reason.

[dalechyn]

@PaulRBerg 🤝🤝🤝
Absolutely agree on this.

@mattsse, I can't really help atm with the MR, but many devs that come from TypeScript environment are used to path aliases and I cannot find any con in using them in the smart contracts as well.
Huge smart contract repos are seeking for usable remappings inside themselves.

[PaulRBerg]

Don't think this issue should have been closed?

mds1/solidity-generators#2 was related but it didn't fix the issue with Foundry itself.

[mds1]

Huh interesting. I guess because I opened this issue originally, a PR from a repo in my namespace was able to close it? Either way, re-opening it now

[0xGabi]

I started experiencing the same issue when I upgrade foundry to the latest nightly for the submodule https://github.com/0xPhaze/upgrade-scripts.

In my particular case everything was working fine with foundry commit 3d5f038.

[PaulRBerg]

For future reference: when @mattsse said this:

simply overwrite the remapping.

He was referring to set one or more of the following remappings in your project:

script/=script/
src/=src/
test/=test/

You only have to do this for those remappings that are overwritten by your dependencies.

[aathan]

I guess my #4597 is this same issue. I just want to add that relative imports in the dependent project suffer from this problem (which seems to contradict some earlier suggestions in this thread). I believe solc resolves those relative paths into relative paths, relative to the project root. Thus import "./SomeOtherFile.sol"; in the dependent project resolves to src/SomeOtherFile.sol and then this matches src/=src/, which is remapped into the dependency.

It seems to me that relative remappings.txt in lib/... should be prepended with lib/... as they're loaded into foundry's vfs or solc command-line builder?

[onbjerg]

The issue I see is that the former promotes/allows unsafe behaviours because, if i understand correctly what foundry is doing, it seems that the version of libs compiled into contracts is whatever version is in the root lib/* NOT the version in lib/somedep/lib.

This is exactly what #5397 fixes, PTAL. It scopes remappings in e.g. lib/some-lib to that folder, so imports in lib/some-lib will resolve to lib/some-lib/lib.

[thedavidmeister]

@onbjerg that sounds basically perfect to me, does it introduce the issue i saw with relative paths though?

namely that some type can be defined in two commit-identical dependencies and treated by solidity as different types?

[thedavidmeister]

@Evalir @onbjerg actually i'm still getting quite a lot of errors that mean i can't compile based on import resolving

thedavidmeister@davids-iMac rain.orderbook % forge build
[⠊] Compiling...
[⠒] Unable to resolve imports:
      "rain.datacontract/lib/LibDataContract.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/concrete/RainterpreterExpressionDeployerNP.sol"
      "rain.datacontract/lib/LibDataContract.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/concrete/RainterpreterNP.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/evm/LibOpTimestamp.sol"
      "rain.solmem/lib/LibMemCpy.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/LibOp.sol"
      "rain.solmem/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/LibInterpreterState.sol"
      "rain.lib.memkv/lib/LibMemoryKV.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/LibInterpreterState.sol"
      "rain.solmem/lib/LibUint256Array.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/caller/LibContext.sol"
      "rain.solmem/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/integrity/LibIntegrityCheck.sol"
      "rain.solmem/lib/LibMemCpy.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/LibInterpreterStateDataContract.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/integrity/LibIntegrityCheck.sol"
      "rain.solmem/src/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/concrete/RainterpreterExpressionDeployerNP.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/concrete/RainterpreterExpressionDeployerNP.sol"
      "rain.solmem/lib/LibUint256Array.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/LibOp.sol"
      "rain.solmem/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/LibOp.sol"
      "rain.solmem/lib/LibBytes.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/LibInterpreterStateDataContract.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/evm/LibOpBlockNumber.sol"
      "rain.solmem/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/LibInterpreterStateDataContract.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/evm/LibOpChainId.sol"
      "rain.solmem/lib/LibStackPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/LibOp.sol"
      "rain.solmem/lib/LibPointer.sol" in "/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/parse/LibParse.sol"
with remappings:
      caller/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/caller/
      compile/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/compile/
      ds-test/=/Users/thedavidmeister/Code/rain.orderbook/lib/forge-std/lib/ds-test/src/
      erc4626-tests/=/Users/thedavidmeister/Code/rain.orderbook/lib/openzeppelin-contracts/lib/erc4626-tests/
      eval/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/eval/
      extern/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/extern/
      forge-std/=/Users/thedavidmeister/Code/rain.orderbook/lib/forge-std/src/
      integrity/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/integrity/
      ns/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/ns/
      op/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/op/
      openzeppelin-contracts/=/Users/thedavidmeister/Code/rain.orderbook/lib/openzeppelin-contracts/
      parse/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/parse/
      rain.chainlink/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/lib/rain.chainlink/src/
      rain.erc1820/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.erc1820/src/
      rain.extrospection/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.factory/lib/rain.extrospection/
      rain.factory/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.factory/
      rain.interpreter/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/
      rain.lib.hash/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/lib/rain.lib.memkv/lib/rain.lib.hash/src/
      rain.lib.typecast/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/lib/rain.lib.typecast/src/
      rain.math.fixedpoint/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.math.fixedpoint/src/
      rain.math.saturating/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.math.fixedpoint/lib/rain.math.saturating/src/
      rain.metadata/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.metadata/src/
      state/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.interpreter/src/lib/state/
      lib/openzeppelin-contracts:openzeppelin/=/Users/thedavidmeister/Code/rain.orderbook/lib/openzeppelin-contracts/contracts/
      lib/rain.factory:rain.datacontract/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.factory/lib/rain.interpreter/lib/rain.datacontract/src/
      lib/rain.factory:rain.lib.memkv/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.factory/lib/rain.interpreter/lib/rain.lib.memkv/src/
      lib/rain.factory:rain.solmem/=/Users/thedavidmeister/Code/rain.orderbook/lib/rain.factory/lib/rain.extrospection/lib/rain.solmem/src/

The only way I can see to fix these is to install the deps globally in the root of the project, they aren't being picked up by the submodule as the submodule's own deps

I think this issue is still unresolved (i ran foundryup just now)

thedavidmeister@davids-iMac rain.orderbook % forge --version
forge 0.2.0 (8e365be 2023-07-18T00:32:55.929185000Z)

[thedavidmeister]

It also looks like the same issue as relative paths has now been introduced for remapped paths @onbjerg

types from deps on the same git commit are treated as different types

thedavidmeister@davids-iMac rain.orderbook % forge test
[⠊] Compiling...
[⠑] Compiling 115 files with 0.8.19
[⠘] Solc 0.8.19 finished in 2.71s
Error: 
Compiler run failed:
Error (2333): Identifier already declared.
 --> lib/rain.interpreter/src/concrete/RainterpreterExpressionDeployerNP.sol:8:1:
  |
8 | import "rain.datacontract/lib/LibDataContract.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
  --> lib/rain.solmem/src/lib/LibPointer.sol:16:1:
   |
16 | library LibPointer {
   | ^ (Relevant source part starts here and spans across multiple lines).

Error (2333): Identifier already declared.
 --> lib/rain.interpreter/src/concrete/RainterpreterExpressionDeployerNP.sol:8:1:
  |
8 | import "rain.datacontract/lib/LibDataContract.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
 --> lib/rain.solmem/src/lib/LibPointer.sol:8:1:
  |
8 | type Pointer is uint256;
  | ^^^^^^^^^^^^^^^^^^^^^^^^

thedavidmeister@davids-iMac rain.orderbook % cd lib/rain.interpreter/lib/rain.datacontract/lib/rain.solmem/
thedavidmeister@davids-iMac rain.solmem % git log
commit 7b2f04d6e0d7d4573e6b0a17bd9c7b84a7fde05f (grafted, HEAD, origin/main, origin/HEAD, main)
Author: David Meister <thedavidmeister@gmail.com>
Date:   Mon Jul 10 14:26:49 2023 +0400

    Merge pull request #4 from rainprotocol/2023-07-10-index-signed
    
    toIndexSigned
thedavidmeister@davids-iMac rain.solmem % cd ../../../rain.solmem 
thedavidmeister@davids-iMac rain.solmem % git log
commit 7b2f04d6e0d7d4573e6b0a17bd9c7b84a7fde05f (grafted, HEAD, origin/main, origin/HEAD, main)
Author: David Meister <thedavidmeister@gmail.com>
Date:   Mon Jul 10 14:26:49 2023 +0400

    Merge pull request #4 from rainprotocol/2023-07-10-index-signed
    
    toIndexSigned

[thedavidmeister]

@aathan the problem with just saying that types should be different if they are in different folders, is that sometimes they are the same

in the example i'm running into now ^^ i did start going down the road of manually differentiating between Pointer depending on where it comes from, like import { Pointer as DataContractPointer } from ...

then what you run into is "can't automatically convert between types" which involves wrapping and unwrapping every type at every function boundary. If you write some library that wants its type as an input/output then you can't use that library in two places in a single repo.

now, if these Pointer types were from different source code, no matter how "minor" the difference, then sure I agree that the correct thing to do is treat them differently (that's what rust does)

in this case though, the exact same commit of a dependency is being treated as "already declared" simply because two different deps use it

perhaps if we were to use the preprocessing idea that you are suggesting, all the nested deps could be "hoisted" to the root
lib and have a few bytes of their commit appended like openzeppelin-contracts@01234567 which would have the effect of "deduping" while preserving the ability to "mess around" manually for debugging if the need arises.

[thedavidmeister]

kk, so i did some experimenting comparing

forge 0.2.0 (cc5637a 2023-06-02T00:22:41.615504000Z) - some old "stable" commit
forge 0.2.0 (0e72b71 2023-07-19T00:37:22.147807000Z) - today's nightly

what I found was, that i had added auto_detect_remappings = false to the data contract dep and this changed how things behave. Also, depending on whether I have a copy of the rain.solmem dependency in the project root or not changes how things behave (with every version of rain.solmem in the nested deps being on the same commit).

Running forge build from root.

version	auto detect	dep in root	error
cc5637a	Y	Y	Identifier already declared.
cc5637a	N	Y	Identifier already declared.
cc5637a	Y	N	-
cc5637a	N	N	Identifier already declared.
0e72b71	Y	Y	Identifier already declared.
0e72b71	N	Y	Identifier already declared.
0e72b71	Y	N	Unable to resolve imports
0e72b71	N	N	Unable to resolve imports

8e365be = same as 0e72b71

All the other recent versions of forge seem to have corrupt archives so i can't test them.

[PaulRBerg]

Related: #5447

[thedavidmeister]

@PaulRBerg in my case some of what i'm seeing could potentially be #5447 as i do have multiple instances of the same dep in my tree