feat(solc): create compilation unit for files that share solc version/ settings

[0xalpharush]

Motivation

Currently, the cache file stores the compiler version and its settings individually for each file, and it can be ambiguous what compilation artifacts are a "unit"; that is, they were determined to be compatible and/or necessary e.g. imports and then compiled with the same solc version and compiler settings. This is mainly a concern when multiple compiler versions are used and the artifacts can then contain duplicate id's in the source mappings (see crytic/slither#1213).

Ref foundry-rs/foundry#3450

Solution

This PR changes the data model of the artifacts to instead store a unique identifier representing that the source files were compiled together (CompilationUnitId). This identifier maps back to a CompilationUnit which stores the version, configuration, and source files' paths. Rather than redundantly store the SolcConfig for each CacheEntry, it is now looked up by its CompilationUnitId. Now, the each compilation unit can be parsed individually and lookup where the artifact is in the cache entry's files. This diff in Foundry is pretty small fwiw.

I'm new to Rust and also may have overlooked some design decision that my changes are incompatible with so I'm very open to feedback!

PR Checklist

• Added Tests
• Added Documentation
• Updated the changelog
• Breaking changes

[plotchy]

The attached slither issue shows one of the most ubiquitous, difficult to diagnose bugs in slither. very nice job pursuing a fix

[mattsse]

thanks for this.

Given that this will improve slither integration, I'm supportive of this @gakonst.

on thing I'm not certain of is, how this should/will affect partial recompilation on changes.
Currently, we only include the necessary files needed by a modified file. would the compileIdunit still be valid if the settings did not change?

compiling the entire unit on a single change would not be feasible imo

[gakonst]

Given that this will improve slither integration, I'm supportive of this @gakonst.

Same. Great change, thanks for taking this on @0xalpharush.

on thing I'm not certain of is, how this should/will affect partial recompilation on changes.
Currently, we only include the necessary files needed by a modified file. would the compileIdunit still be valid if the settings did not change?
compiling the entire unit on a single change would not be feasible imo

Agreed. Let's add a test for this to check what happens to partial recompilation?

[gakonst]

Would like an extra test for the partial recompilation case that Matt mentioned. Appreciate you tackling this.

[0xalpharush]

Currently, we only include the necessary files needed by a modified file. would the compileIdunit still be valid if the settings did not change?

@mattsse AFAICT I didn't modify the detection of "clean" and "dirty" sources (there is an existing test for this). The only time an entire compilation unit should be recompiled is if the settings are changed for the entire unit e.g. an incompatible solidity version, creating another compilation unit derived from the incompatible settings. I added a test case to show that the compilation unit id remains valid if only a file's contents are changed and that only the dirty source is recompiled.

[gakonst]

@mattsse PTAL I think this is starting to look good

[mattsse]

thanks for clarifying @0xalpharush

changes and test look good, let's rebase this PR then the foundry companion, good to merge when the companion passes.

[0xalpharush]

Rebased. I think the ethereum test failure is unrelated:

---- nonce_manager stdout ----
thread 'nonce_manager' panicked at 'called `Result::unwrap()` on an `Err` value: MiddlewareError(MiddlewareError(JsonRpcClientError(JsonRpcError(JsonRpcError { code: -32000, message: "replacement transaction underpriced", data: None }))))', ethers-middleware/tests/nonce_manager.rs:42:14

Updated the Foundry PR foundry-rs/foundry#3939

[gakonst]

LGTM pending @mattsse approval @0xalpharush do you want to merge this and fix #1995 separately or together?

[0xalpharush]

@gakonst One thing I realized chasing down #1995 is that each cache entry should have multiple compilation units instead of just one so maybe the compilation unit should be stored at the artifact level. This would allow the cache to handle having the same version being compiled with different compiler settings e.g. someone doesn't want to use via-IR while testing. I explored this in a separate branch but not sure it's a great solution (0xalpharush@c378b49).

[mattsse]

this is a reasonable consideration and would give us a bit more flexibility I think.

can't think of any downsides to moving this to the artifact level

[gakonst]

SGTM.

[gakonst]

@0xalpharush friendly ping here, no pressure again just dont want this to get stale

[0xalpharush]

Will get back to this when I have the time. I started over holiday break and have been busy recently... If anyone finds a solution to #1995, that would be great to have for the time being.

[gakonst]

ACK. -thx! #1995 up for grabs.

[gakonst]

Closing as stale.