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

forge flatten duplication of interfaces/contract names #262 #4034

Closed
1 of 2 tasks
gbalabasquer opened this issue Jan 4, 2023 · 6 comments · Fixed by #6936
Closed
1 of 2 tasks

forge flatten duplication of interfaces/contract names #262 #4034

gbalabasquer opened this issue Jan 4, 2023 · 6 comments · Fixed by #6936
Labels
T-bug Type: bug

Comments

@gbalabasquer
Copy link

gbalabasquer commented Jan 4, 2023

Component

Forge

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

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (a44159a 2023-01-02T00:04:45.597843Z)

What command(s) is the bug in?

forge flatten

Operating System

macOS (Apple Silicon)

Describe the bug

When running forge flatten <file> in a project that the path of files includes multiple interfaces/contracts with the same name, the command can't solve the duplication and just generate the output with the same name repeated.
For example if running forge flatten command in this file: https://github.com/makerdao/spells-goerli/blob/solc-0.8/src/DssSpell.sol will generate the result with two OracleLike interfaces. Hevm would have produced OracleLike_1 and OracleLike_2 instead.

@gbalabasquer gbalabasquer added the T-bug Type: bug label Jan 4, 2023
@gbalabasquer
Copy link
Author

@mattsse @gakonst do you think this is something that could be addressed in the short term? It seems to be the only thing that's pending to do a full migration from dapp to forge in the Makerdao's spells repos.

@The-Arbiter
Copy link

Hi @gakonst @mattsse - our migration to forge is nearly complete, with the exception of this issue.

Foundry needs to be able to rename interfaces in flattened contracts to prevent collisions.

For example, if we have VatLike in an imported contract and it will collide with a defined VatLike in the child contract, dapp will rename both of those to VatLike_1 and VatLike_2 and fix the references.

For example, this contract has the LineMomLike interface twice, so it turns into LineMomLike_1 and LineMomLike_2.

I've also been informed about Foundry flattening sometimes having issues due to duplicate licenses. Someone shared the following code snipped with me as they have said it fixes the issues most of the time:

remove-duplicate-licenses() {
  awk '!(/SPDX/ && seen[$0]++)' $@
}

I understand that this is a separate issue however I wanted to raise it here. I am aiming for us to migrate our spells to foundry before the end of the month - this is the last issue we are having at the moment.

@mds1
Copy link
Collaborator

mds1 commented Jul 26, 2023

@Evalir Just bumping this issue as it still seems to be present. Here's another repro for you:

  1. Clone https://github.com/scopelift/pooltogether-governor-upgrade/, install deps, and run forge flatten src/PoolTogetherGovernor.sol > flat.sol
  2. Run solc flat.sol and it errors due to duplicate definitions, i.e. forge is not flattening correctly

@SidestreamSweatyPumpkin
Copy link

It seems like the flattening breaks on the level of ethers-rs package and not in foundry. So The issue was also raised there: gakonst/ethers-rs#2588

@mattsse
Copy link
Member

mattsse commented Sep 7, 2023

fixing this properly would require resolving all imports/usages and renaming everything accordingly.

just generate the output with the same name repeated.

do you mean it generates the exact same interface twice, or does this happen when the project imports two identical interfaces?

@SidestreamColdMelon
Copy link

do you mean it generates the exact same interface twice, or does this happen when the project imports two identical interfaces?

It happens when project imports two interfaces with the same name (but they don't have to be identical besides the name). I think this external issue gakonst/ethers-rs#2588 clearly outlines what happens.

Here is the except from it:

For example: given two files FileA and FileB

// FileA.sol
pragma solidity ^0.8.10;
import * as B from "./FileB.sol";

interface FooBar {
    function foo() external;
}
contract A {
    function execute() external {
        FooBar(address(0)).foo();
    }
}
// FileB.sol
pragma solidity ^0.8.10;

interface FooBar {
    function bar() external;
}
contract B {
    function execute() external {
        FooBar(address(0)).bar();
    }
}

Flattening will produce

pragma solidity ^0.8.10;

interface FooBar {
    function bar() external;
}
contract B {
    function execute() external {
        FooBar(address(0)).bar();
    }
}

interface FooBar { // here we have a conflict with the interface declared above
    function foo() external;
}
contract A {
    function execute() external {
        FooBar(address(0)).foo();
    }
}

mattsse pushed a commit to foundry-rs/compilers that referenced this issue Jan 24, 2024
Add `Hash` derive for `SourceLocation`, fix schema for `TryCatchClause`
and add test for it.

I am currently working on
foundry-rs/foundry#4034 and it requires usage
of native solc AST, so I might be opening some other PRs around it as it
seems that we are not using current models anywhere and they might be a
bit outdated
mattsse pushed a commit to foundry-rs/compilers that referenced this issue Jan 29, 2024
Ref foundry-rs/foundry#4034.

## Solution
solang-parser AST was not sufficient to rename stuff, because it doesn't
collect IDs of declarations and we can't match specific identifier to
the declaration. Native solc AST gives such ability, so I've implemented
native solc AST visitor.

The implementation of flattening itself consits of several steps:
1. Find all sources that are needed for target (imports of any depth).
2. Expore ASTs for all sources and find all top-level declarations
(contracts, events, structs, etc).
3. Find if any top-level declarations names are same
4. If any duplicates are found, figure out new names for them (e.g. 2
`OracleLike` interfaces become `OracleLike_0` and `OracleLike_1`)
5. Walk AST and find all references to top-level declarations. Replace
every reference with declaration name. We should update names even for
unchanged declarations to deal with aliased imports.
6. Collect and remove all import directives.
7. Collect and remove all pragmas and license identifiers.

This flattener implementation can be a full replacer of the current impl
for all cases when source being flattened can be compiled via solc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants