feat(forge): new flatten implementation

[klkvr]

Motivation

Ref foundry-rs/compilers#52
Full rewrite of flattening utilizing native solc AST.

• Closes Sort before flatten #6539
• Fixes Forge Flatten creates duplicate declarations (unable to compile) #6022
• Fixes forge flatten duplication of interfaces/contract names #262 #4034
• Fixes After run forge flatten command, then I run forge build, it shows: Multiple SPDX license identifiers found in source file.  #3678
• Fixes forge flatten with renamed interface import does not compile #3265
• Fixes forge flatten does not support qualified imports #2545
• Fixes Flatten removes "pragma experimental ABIEncoderV2;" #2378
• Fixes Bug: Forge flatten does handle multiple differing SPDX licenses  #2344

Solution

It seems that we don't have any tests for forge flatten but creating PR with patched dep to run CI just in case.

[DaniPopes]

(You have to write "closes" before each PR, preferably each on a new line)

[avniculae]

@klkvr Great work, congrats! I run your version of flatten on a project that uses prb math (both UD60x18 and SD59x18) and I can see that the resulted file has some issues renamed variables in assembly blocks. For example, in the code below uUNIT was not replaced with the renamed version (uUNIT_1, _2, ...)

assembly ("memory-safe") {
        switch x
        case 1 { result := mul(uUNIT, sub(0, 18)) }
        case 10 { result := mul(uUNIT, sub(1, 18)) }
        case 100 { result := mul(uUNIT, sub(2, 18)) }
        case 1000 { result := mul(uUNIT, sub(3, 18)) }
        case 10000 { result := mul(uUNIT, sub(4, 18)) }
.....

Is this an easy fix for you?

[mattsse]

lgtm!!

nice work

[klkvr]

@avniculae ah, assembly blocks are not in scope of renaming atm

It shouldn't be hard to fix, could you please drop a link to the contract you are flattening or a minimal repro so it's easier to debug?

[avniculae]

@avniculae ah, assembly blocks are not in scope of renaming atm

It shouldn't be hard to fix, could you please drop a link to the contract you are flattening or a minimal repro so it's easier to debug?

Thanks for letting me know! I run it on a private repo, but I think this file should represent a good test.

[avniculae]

@klkvr another point worth mentioning: docs comments are also not updated during flattening, preventing the final file from compiling successfully. For example:

abstract contract UUPSImplementation is IUUPSImplementation_0, ProxyStorage {
    /**
     * @inheritdoc IUUPSImplementation
     */

[avniculae]

@klkvr another point: if the contract renames an import (e.g., import { mul as mulUD } from "..."), the flatten command fails to rename mulUD to the new renaming

[klkvr]

@avniculae thanks for all the points, will try to fix all of them

regarding imports renaming could you please give a bit more context as this case was addressed directly during development and it shouldn't fail

which entity is the mulUD? I mean variable/enum/struct etc

[avniculae]

@klkvr I am experiencing issues with function renaming (mul is a function here)

Later Edit: I am experiencing the same issue with some imported variables as well, which are not renamed in the original import at all. Does the code work for constant variables as well (that are freely declared in a file -- i.e. not within a contract?)

Later Edit 2: I think the bigger problem here might be that identifiers are not replaced when prepended or appended with characters such as . or (: for example a.mul(b) or f(ZERO)

[klkvr]

@avniculae

I can't reproduce this behavior locally, maybe I got something wrong?
So, I am creating 2 files:

mul.sol:

function mul(uint256 a, uint256 b) pure returns(uint256) {
    return a * b;
}

test.sol

pragma solidity 0.8.6;

import {mul as mulUD} from "./mul.sol";

contract Test {
    function foo() public {
        mulMD(1, 2);
    }
}

And test.sol is getting flattened to

pragma solidity 0.8.6;
function mul(uint256 a, uint256 b) pure returns(uint256) {
    return a * b;
}

uint256 constant someVar = 1;

contract Test {
    function foo(uint256 a) public {
        mul(1, 2);
    }
}

which seems correct

[avniculae]

@klkvr Here's a small counterexample:

SFixed.sol

pragma solidity 0.8.19;

type SFixed256x18 is int256;

using {add} for SFixed256x18 global;

function add(SFixed256x18 a, SFixed256x18 b) pure returns (SFixed256x18) {
    return SFixed256x18.wrap(SFixed256x18.unwrap(a) + SFixed256x18.unwrap(b));
}

UFixed.sol

pragma solidity 0.8.19;

type UFixed256x18 is uint256;

using {add} for UFixed256x18 global;

function add(UFixed256x18 a, UFixed256x18 b) pure returns (UFixed256x18) {
    return UFixed256x18.wrap(UFixed256x18.unwrap(a) + UFixed256x18.unwrap(b));
}

Contract.sol

pragma solidity 0.8.19;

import {SFixed256x18} from "./SFixed.sol";
import {UFixed256x18} from "./UFixed.sol";

contract Test {
    function fun() public pure returns (SFixed256x18, UFixed256x18) {
      SFixed256x18 a = SFixed256x18.wrap(10);
      SFixed256x18 b = SFixed256x18.wrap(-90);
      SFixed256x18 c = a.add(b);

      UFixed256x18 d = UFixed256x18.wrap(20);
      UFixed256x18 e = UFixed256x18.wrap(30);
      UFixed256x18 f = d.add(e);

      return (c, f);
    }
}

In the flattened version, a.add(b) is replaced by add_0(b), while d.add(e) is replaced by add_1(e). However, on the project I'm running it (which uses PRB-Math heavily), the command often leaves these functions unchanged ( e.g., a.add(b)).

[mattsse]

🔥

[klkvr]

hey @avniculae thank you once again for all the edge cases

I belive that all of them should be fixed now, could you please pull this PRs version and try once again if they are working for you?

[avniculae]

@klkvr Thanks for pushing these fixes! I re-run the latest version and I now get the following error:

The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: .../foundry-compilers-0.3.0/src/flatten.rs:450

This is a bug. Consider reporting it at https://github.com/foundry-rs/foundry

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 8 frames hidden ⋮                               
   9: core::panicking::panic::h40561ff494e2b577
      at <unknown source file>:<unknown line>
  10: foundry_compilers::flatten::Flattener::flatten::hf4ff1bfccf30ce3e
      at <unknown source file>:<unknown line>
  11: forge::main::h82dc03aed91a2c0a
      at <unknown source file>:<unknown line>
  12: std::sys_common::backtrace::__rust_begin_short_backtrace::hb40b21b17ab48be9
      at <unknown source file>:<unknown line>
  13: std::rt::lang_start::h39e6cad84a7b22b7
      at <unknown source file>:<unknown line>
  14: _main<unknown>
      at <unknown source file>:<unknown line>

However, it's hard to tell what's causing this error since the flattened code has over 11k loc, and I wasn't able to reproduce it with small examples

[mattsse]

@avniculae is this repo public?

[klkvr]

@avniculae this happens while updating @inheritdoc tags, do you have any uncommon patters in those? Like using aliases, qualified imports? Or all values are just straight contract/interface names?

[avniculae]

@klkvr They should all be standard, like this example (I'm less sure about dependencies though)

  /**
   * @inheritdoc InterfaceName
   */

@mattsse Unfortunately it isn't yet :(

[klkvr]

@avniculae any chance you can share names of other dependencies besides prb-math?

[avniculae]

@klkvr I think the main ones are prb-math and @openzeppelin/contracts/

[mattsse]

This includes a ton of improvements, but might still have some limitations which we can fix as soon as they get reported.