NomicFoundation / hardhat-vscode

Solidity and Hardhat support for Visual Studio Code
https://hardhat.org
MIT License
166 stars 36 forks source link

Does not work with the "src/=src/" remappings #369

Closed PaulRBerg closed 1 year ago

PaulRBerg commented 1 year ago

Description

The using for ... global directive is explained in the release announcement for Solidity v0.8.13, and my particular situation is explained here.

I have a bunch of imports like this:

import "./Casting.sol" as C;
import "./Helpers.sol" as H;
import "./Math.sol" as M;

Which I am trying to apply to the UD60x18 value type, like this:

type UD60x18 is uint256;

using { C.intoSD1x18, C.intoUD2x18, C.intoSD59x18, C.intoUint128, C.intoUint40 } for UD60x18 global;

The Hardhat VSCode extension complains:

hardhat-using-for-global

But I can happily compile my contracts with forge build.

Source Code

See the src/ud60x18/ValueType.sol file in PRBMath.

kanej commented 1 year ago

Thanks for the report @PaulRBerg. I have been able to reproduce locally.

It looks like we are running the wrong solc version in this case for the diagnostics. We will have to investigate how that is happening.

antico5 commented 1 year ago

@PaulRBerg I found the issue, there's one line in the remappings.txt file that's making the extension misbehave in unsuspected ways, and it's the src/=src/ line.

Our current handling of remappings makes it so the solc input has the remappings setting to use absolute paths (we may need to change that), so the entry ends up being src/=/absolute/path/prb-math/src/ on the solc input. When sent it that way, solc returns the errors that you see displayed, that don't seem related at all.

After removing the src/=src/ line from remappings, our extension seems to work well with your project. Is this good enough of a solution for you? otherwise we may need to look into changing our remapping handling a bit on our side.

PaulRBerg commented 1 year ago

Thanks a lot for digging this, @antico5.

Removing src/=src/ is not a great solution (unfortunately) . As discussed at length in this thread on Twitter, particularly in this reply, one has to apply the src/=src/ remapping for the "Go to Definition" feature of VSCode to keep working when importing contracts from src.

So it looks like users have to choose between some VSCode features and Hardhat when importing directly from src. Of course, the reason why we do this in the first place is nicer import paths - to avoid the ../../../../ relative path hell.

antico5 commented 1 year ago

@PaulRBerg Just did a little testing and our extension can deal with "go to definition" requests through src/* imports, without having the src=src remappings entry.

Althought a small caveat, jump to definition doesn't work if the definition is present on a file that we cannot parse, and we use solidity-parser for this. I just found out that we need to update the version of solidity-parser that we use in order to support the latest syntax that you are using on this project. Will submit a PR for this.

In any case, I don't see why we'd need to have the src/=src/ entry on remappings. Maybe it's for the JuanBlanco's extension to work?

PaulRBerg commented 1 year ago

Oh, I think you're right. I was using Juan Blanco's extension when I added src/=src/. If we don't need it for the Hardhat extension, then that's all for the better!

I'll try it out and follow up here to report my findings.

PaulRBerg commented 1 year ago

Unfortunately, removing src/=src/ produces a bunch of errors when using the Hardhat VSCode extension. This is from a private repository:

Screenshot 2023-01-18 at 3 05 19 PM

Also, Forge is not able to compile the code without the src/=src/ remapping:

Failed to resolve file: "/Users/paulrberg/workspace/sablier/v2-core/lib/prb-math/src/libraries/Errors.sol": No such file or directory (os error 2).
 Check configured remappings..
    --> "/Users/paulrberg/workspace/sablier/v2-core/test/unit/comptroller/get-protocol-fee/getProtocolFee.t.sol"
        "src/libraries/Errors.sol"

I have tried to change src/ to ./src - to no avail.

PaulRBerg commented 1 year ago

Wait, on second thoughts, I now realize that the issue above was caused by PRBMath itself having a src/=src/ remapping.

So the issue is that if just of a user's dependencies have this remapping, then the user itself is forced to add it, to disambiguate the src/ path.

Therefore, I think that a remedy to this is sort of needed, because users can't control what remappings get set by third-parties.

antico5 commented 1 year ago

@PaulRBerg thanks for giving us more information on this. I'll try now to change the way remappings are built when sent to solc, to use relative paths instead of absolute, to allow having the src/=src/ remapping that some libraries may have (althought that feels hackish).

About your second-to-last comment, where you show the import error, to me it seems that it's just a legitimate import error, since relative imports shouldn't fail as they are not affected by remappings (at least on the extension side). I may be missing something so if it's the case please help me reproduce with a minimum example.

PaulRBerg commented 1 year ago

Thanks for your quick reply, @antico5.

The scenario was this - the private repo has PRBMath as a git submodule dependency, and PRBMath has src/=src/ in its remappings.txt file. Removing src/=src/ from the private repo's remappings triggers the error shared above.

peersky commented 1 year ago

Hi Everyone. Not sure how much is this relevant but I'm running in issue described here:

import {ERC1155Burnable} from "@openzeppelin/contracts/token/ERC1155/extensions/ERC1155Burnable.sol";

Compiles fine, but VScode underlines with red this import saying:

Source "@openzeppelin/contracts/token/ERC1155/extensions/ERC1155Burnable.sol" not found: File not found. Searched the following locations: "".solidity(6275)

Apperently it tries to look in / directory and does not take path in respect.

Right now will try to roll back extension version and see if this helps. Please let me know if there is any way to fix this. Thx!

Upd: Downgrade to 0.5.5 made error go away

antico5 commented 1 year ago

@peersky by any chance are you working on a public project that we can check? otherwise please let us know if you are using hardhat, foundry, or both? in the past we've found this issue one projects using hardhat+foundry with the hardhat-preprocessor plugin, but please give us all the information available for us to pinpoint the problem.