foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.33k stars 1.76k forks source link

implicit trailing `/` in remappings causes file remappings to not resolve #7527

Closed BlazeWasHere closed 4 months ago

BlazeWasHere commented 7 months ago

Component

Forge

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

What version of Foundry are you on?

forge 0.2.0 (d1ab09d 2024-03-30T00:17:25.901579435Z)

What command(s) is the bug in?

No response

Operating System

Linux

Describe the bug

Using remappings.txt

ds-test/=lib/ds-test/src/
src/test.sol=src/testPatched.sol

when ran with forge (notice how its src/test.sol/ instead of src/test.sol)

~> forge remappings 
ds-test/=lib/ds-test/src/
src/test.sol/=src/testPatched.sol/

expected behavior is similar to dapptools -- no trailing / is added to the remapping.

simple testcase:

A.t.sol ```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.6; import "ds-test/test.sol"; import {Test} from "src/test.sol"; contract ATest is DSTest { function test_basic_sanity() public { Test.log(); assertTrue(true); } } ```
test.sol ```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.6; library Test { event log_string(string); function log() internal { emit log_string("test"); } } ```
testPatched.sol ```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.6; library Test { event log_string(string); function log() internal { emit log_string("patched"); } } ```

now we run forge:

~> forge test -vv
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for src/A.t.sol:ATest
[PASS] test_basic_sanity() (gas: 1821)
Logs:
  test

with dapptools:

~> DAPP_REMAPPINGS=$(cat remappings.txt) dapp test --verbosity 2 
 dapp-build: building with linked libraries
dapp: Predeploying test library src/testPatched.sol:Test at 0x0DdeDfFe2789df61F5f76479464F568334414389
dapp: Predeploying test library src/test.sol:Test at 0x5Fe5dAc04fe3527Cf7bDba8798F5A8b5edDfE4Af
Running 1 tests for src/A.t.sol:ATest
[PASS] test_basic_sanity() (gas: 1994)

Success: test_basic_sanity

  "patched"

it does seem like this will be a breaking change if implemented due to some people who may be relying on this unexpected behavior, wdyt?

BlazeWasHere commented 7 months ago

seems like its caused by this https://github.com/foundry-rs/foundry/blob/a16714ed40f733013d7a80f4f969564175c3318e/crates/config/src/utils.rs#L172-L180 https://github.com/foundry-rs/foundry/blob/d94e3c631e2da7756af46c70f8f58b75563b7013/crates/config/src/providers/remappings.rs#L216-L226

this looks related https://github.com/foundry-rs/compilers/issues/47

zerosnacks commented 4 months ago

This seems related to https://github.com/foundry-rs/foundry/issues/6706

you cannot remap individual files, only directories

BlazeWasHere commented 4 months ago

@zerosnacks i believe this issue was fixed in https://github.com/foundry-rs/compilers/pull/49 and all tests passed in foundry CI https://github.com/foundry-rs/foundry/pull/8377 so im guessing we are waiting for foundry main to update to the latest version of foundry-compilers

zerosnacks commented 4 months ago

Thanks @BlazeWasHere, looks like that should fix it!

I'll follow up on this once a new release of foundry-compilers is cut (should be relatively soon) and we have bumped the version in Foundry

zerosnacks commented 4 months ago

Marking as resolved as foundry-compilers has been updated: https://github.com/foundry-rs/compilers/commit/ea346377deaf18dc1f972a06fad76df3d9aed8d9 + https://github.com/foundry-rs/foundry/blob/4e4f35c1d536b4a371ddc5a666258c260fd31b53/Cargo.toml#L161

BlazeWasHere commented 3 months ago

@zerosnacks doesnt look like original poc has been fixed with latest nightly (00ff367) + (37ea1e9). should i create a test case for this?

$ DAPP_REMAPPINGS=$(cat remappings.txt) dapp test --verbosity 2  
 dapp-build: building with linked libraries
[...]

  src/A.t.sol:ATest
   ├╴constructor
   └╴test_basic_sanity()
      ├╴log_string("patched") (src/testPatched.sol:8)
[...]

  "patched" # <--- this is good :)

$ forge test -vvv 
[...]
Logs:
  test # <--- this is bad :(
zerosnacks commented 3 months ago

Hi @BlazeWasHere, that would be appreciated. However I'm still not sure what the intended behavior here would be because Foundry does not support remapping individual files to my knowledge, only directories.

BlazeWasHere commented 3 months ago

@zerosnacks Correct that Foundry does not support individual file remappings, ideally it should (foundry-compilers does) but i think that's related to #6706