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

Bug: forge flatten does not account for as keyword in imports #1440

Closed
2 tasks done
transmissions11 opened this issue Apr 28, 2022 · 8 comments
Closed
2 tasks done

Bug: forge flatten does not account for as keyword in imports #1440

transmissions11 opened this issue Apr 28, 2022 · 8 comments
Labels
C-forge Command: forge Cmd-forge-create Command: forge create P-normal Priority: normal T-bug Type: bug

Comments

@transmissions11
Copy link
Contributor

transmissions11 commented Apr 28, 2022

Component

Forge

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

  • Foundry
  • Foundryup

What version of Foundry are you on?

latest

What command(s) is the bug in?

forge flatten

Operating System

macOS (M1)

Describe the bug

the as keyword, used to remap imports like

import {FixedPointMath as Math} from "..."

Math.mul()

is ignored by forge flatten, which just indiscriminately pastes FixedPointMath in and fails to rename/alias it to Math

this results in the output of forge flatten failing to compile because the compiler does not know what Math is if FixedPointMath is not alias'd/renamed, while the input could be a perfectly valid contract like above

@transmissions11 transmissions11 added the T-bug Type: bug label Apr 28, 2022
@onbjerg onbjerg added this to Foundry Apr 28, 2022
@onbjerg onbjerg moved this to Todo in Foundry Apr 28, 2022
@mattsse
Copy link
Member

mattsse commented Apr 28, 2022

@rkrasiuk perhaps we're using the offset of the wrong identifier?

@onbjerg onbjerg added C-forge Command: forge P-normal Priority: normal Cmd-forge-create Command: forge create labels Apr 28, 2022
@rkrasiuk
Copy link
Collaborator

rkrasiuk commented May 2, 2022

@mattsse so i've fixed parsing aliases using solang and our regex fallback. now the only question is how do we want to proceed with fixing the aliases in the flattened file. both renaming import target to the alias and renaming the alias back to the original import name have the edge cases that break the flattened file.
i.e. if we chose to rename the import target

// FixedPointMath.sol
library FixedPointMath { }

// Calculator1.sol
import {FixedPointMath as Math} from "./FixedPointMath.sol";

Math.mul();

// Calculator2.sol
import {FixedPointMath} from "./FixedPointMath.sol";

FixedPointMath.mul(); // this is where it would break

@mattsse
Copy link
Member

mattsse commented May 2, 2022

hmm, this not ideal but not really solvable until we have proper formatting.
But I think this limitation is negligible and would even highlight inconsistent import pattern.
so perhaps the right approach for now is to simply resolve the alias. and replace Math with FixedPointMath in the node's content?
wdyt?

@rkrasiuk
Copy link
Collaborator

rkrasiuk commented May 2, 2022

sgtm, just want to make sure that these edge cases are acknowledged. so, you suggest replacing the alias with the original import name, not the other way around, right?

@mattsse
Copy link
Member

mattsse commented May 2, 2022

sgtm, just want to make sure that these edge cases are acknowledged. so, you suggest replacing the alias with the original import name, not the other way around, right?

can we replace per node or only once we've flattened everything?

the other way around would include renaming the library as well, right?

I'd think replacing the more verbose lib name with the alias could be more accurate but I don't think we should rename the library.

so, I'd say let's replace alias Math with actual name FixedPointMath.

@rkrasiuk
Copy link
Collaborator

rkrasiuk commented May 2, 2022

we can replace per node, the only caveat is identifying correctly the token to replace. we either regex it on flattening or traverse the solang result and store information about various tokens used inside every file

@mattsse
Copy link
Member

mattsse commented May 2, 2022

I think regexing should be sufficient :)

@mattsse
Copy link
Member

mattsse commented May 5, 2022

closed by gakonst/ethers-rs#1192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-create Command: forge create P-normal Priority: normal T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

4 participants