foundry-rs / compilers

Utilities for working with native solc and compiling projects.
Apache License 2.0
73 stars 41 forks source link

New flattening impl #52

Closed klkvr closed 8 months ago

klkvr commented 8 months ago

Ref https://github.com/foundry-rs/foundry/issues/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.

klkvr commented 8 months ago

This should also fix issues with aliases (like https://github.com/foundry-rs/foundry/issues/2545, https://github.com/foundry-rs/foundry/issues/3265).

Current impl handles aliases renaming via regex'es and may often be inaccurate.

klkvr commented 8 months ago

I've refactored code and added logic for correct processing of references to types declared in scope of contracts.

It requires such workaround because for some reason solc does not give any information about the parent contract for UserDefinedTypeName objects representing types like ParentContract.EnumName. Such node will only have AST ID and location of EnumName definition included.

That way, if we want to rename contract ParentContract, we need to be able to determine that AST ID of EnumName is related to that contract and replace it with ParentContract_i.EnumName

mattsse commented 8 months ago

@sakulstra I believe you had some issues with flattening in the past, do you know any projects we should test this against?

sakulstra commented 8 months ago

@mattsse there were a few issues/things i commented in the past:

For #4034 specifically we also had some issues with that, but can't recall right now in which contracts. Will check if i can find on monday.

klkvr commented 8 months ago

Added test and fix for https://github.com/foundry-rs/foundry/issues/2378

If any of files contains any experimental pragma, it's getting added to the target

klkvr commented 8 months ago

Pushed solution for https://github.com/foundry-rs/foundry/issues/6539.

https://github.com/foundry-rs/compilers/blob/ce8e42746599694bb884f06e69d5014e5d7a10e8/src/flatten.rs#L586-L624

I've also shared it's logic with current flattening implementation and rewritten it a bit as it not longer needs to be recursive.

klkvr commented 8 months ago

@mattsse pushed fixes

let's merge a bit later, I will now write integration of it into foundry and may decide to change the API of Flattener a little bit