Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
PaulRBerg opened this issue Jan 13, 2023 · 11 comments · Fixed by #377
Closed

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

PaulRBerg opened this issue Jan 13, 2023 · 11 comments · Fixed by #377
Labels
status:ready This issue is ready to be worked on type:bug Something isn't working

Comments

@PaulRBerg
Copy link

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 kanej added type:bug Something isn't working status:ready This issue is ready to be worked on and removed status:triaging labels Jan 13, 2023
@kanej kanej moved this to Todo in hardhat-vscode Jan 13, 2023
@kanej
Copy link
Member

kanej commented Jan 13, 2023

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
Copy link
Collaborator

antico5 commented Jan 17, 2023

@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
Copy link
Author

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
Copy link
Collaborator

antico5 commented Jan 17, 2023

@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
Copy link
Author

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
Copy link
Author

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.

@antico5 antico5 moved this from Todo to Done in hardhat-vscode Jan 18, 2023
@PaulRBerg PaulRBerg changed the title Does not work with the "using for ... global" directive introduced in Solidity v0.8.13 Does not work with the "src/=src/" remappings Jan 19, 2023
@PaulRBerg
Copy link
Author

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.

@kanej kanej moved this from Done to In Progress in hardhat-vscode Jan 19, 2023
@antico5
Copy link
Collaborator

antico5 commented Jan 19, 2023

@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
Copy link
Author

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
Copy link

peersky commented Jan 30, 2023

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
Copy link
Collaborator

antico5 commented Feb 1, 2023

@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.

@kanej kanej moved this from In Progress to In Review in hardhat-vscode Feb 7, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in hardhat-vscode Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready This issue is ready to be worked on type:bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants