fix: double import issue #58
Conversation
| }); | ||
| it('should replace path as instructed by remapping with all import syntaxes', function () { | ||
| const fileContent = ` | ||
| import '../../../interfaces/ITest.sol'; |
There was a problem hiding this comment.
I might be wrong here, but I don't think remappings work this way. If your remapping key is interfaces, ../../../interfaces should not be affected, only when it starts with interfaces/
There was a problem hiding this comment.
forge does a weird thing where it complains in the logs it cant find the files, as if it only used a remapping when the key is at the start of the imported path as you describe, but then replaces the key anywhere and manages to build the project. as a PoC, I:
- ran
forge init - in that repo, added
wherecodeis/=src/ - then edited the
Counterimport toimport {Counter} from "../wherecodeis/Counter.sol";
and got this log:
[N] ~/p/remappingexample (master){2}
> forge test
[⠊] Compiling...
[⠒] Unable to resolve imports:
"../wherecodeis/Counter.sol" in "/home/user/playground/remappingexample/test/Counter.t.sol"
with remappings:
wherecodeis/=/home/user/playground/remappingexample/src/
forge-std/=/home/user/playground/remappingexample/lib/forge-std/src/
[⠰] Compiling 25 files with Solc 0.8.26
[⠔] Solc 0.8.26 finished in 1.20s
Compiler run successful!
Ran 2 tests for test/Counter.t.sol:CounterTest
[PASS] testFuzz_SetNumber(uint256) (runs: 256, μ: 31210, ~: 31288)
[PASS] test_Increment() (gas: 31303)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 7.01ms (6.73ms CPU time)
Ran 1 test suite in 8.85ms (7.01ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)
There was a problem hiding this comment.
as forge considers the remapping valid, I'd say to keep the current behaviour, however I'm open to structuring the tests in a different way to make this edge case more obvious
There was a problem hiding this comment.
structuring the tests in a different way to make this edge case more obvious
Please 🙏🏻
| import '../../../interfaces/ITest.sol'; | ||
| import './someFile.sol'; | ||
| `; | ||
| import './Contract.sol'; |
There was a problem hiding this comment.
what do you think about adding something that should be automatically modified in between these lines? Just to make sure our code is not skipping all the imports after seeing that the first one does not need any modification
No need to add a new test, we could do it in this one
There was a problem hiding this comment.
sure!
now that I think about it (related to your other comment) having multiple imports in a single line will probably break as well (but we could actually make that work without properly parsing the code). Will add a test for that too!
| expect(transformedContent).to.include(`contract C {}`); | ||
| }); | ||
|
|
||
| it.skip('should not have false positives choosing imports', () => { |
There was a problem hiding this comment.
why is this test being skipped? we are not able to know if we are talking about code or comment?
There was a problem hiding this comment.
we are not, since we don't reuse an existing lexer+parser, and only do regular-style modifications to the code. For most cases it is sufficient though, and single-line comments are skipped since we match \s*import to detect imports
Also, its worth noting that the impact of modifying comments when we shouldn't is very low considering multi-line natspec is required to have a leading * which would not match the regex above
There was a problem hiding this comment.
@0xteddybear let's leave a comment to explain why the tests are skipped? I'd also add a warning on the readme.
There was a problem hiding this comment.
Added the warning in 0b2c27b, however I feel leaving the readme as-is and pinning the issue might be better, wdyt?
There was a problem hiding this comment.
@0xteddybear both are fine in my view
| }); | ||
| }); | ||
|
|
||
| it('should apply remapping and then remove node_modules from path', () => { |
There was a problem hiding this comment.
Isn't this test a duplicate of should remove node_modules from import paths?
There was a problem hiding this comment.
not really since the preconditions are different, the testcase defined in line 48 is only concerned with removing node_modules from import paths, with the precondition of no remapping matching the paths with node_modules in it (expressed as with remapping for local file only)
this testcase, however, ensures that when a remapping matches a path inside node_modules, the remapping is applied before removing node_modules from the path
feedback of course welcome on ways to structure this differently so the above is clear from reading the code alone
No description provided.