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

feat: track and cache context of each compiler invocation #140

Merged
merged 9 commits into from
Jun 11, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jun 10, 2024

ref foundry-rs/foundry#7379
ref foundry-rs/foundry#4981
ref foundry-rs/foundry#2704

The way Solidity assigns source_ids is simply by order in which sources are passed in compiler input. Thus, if we have two sources A.sol and B.sol, then on the first (non-cached) compiler run, A.sol will get assigned ID 1 and B.sol will have ID 2.

Then, if we change B.sol slightly and recompile, it will be the only source in the input, so it will have ID 1.

The same ID collisions are more often appearing on multi-version and multi-compiler builds. After many cached runs such discrepancies result in debugger basically displaying random sources.

This PR adds a way to link an artifact to the build info for input that produced it, thus allowing us to track correct source_id -> source mapping for cached artifacts.

I've added BuildContext which is the foundry context we are tracking for each compiler invocation. It is getting inlined into BuildInfo, and read along with cached artifacts. BuildContexts are indexed by the same IDs BuildInfos are, and each ArtifactId now has a reference to build_id which produced it.

Build info now produced on every compiler run, however, it does not include complete input and output unless project.build_info is true, to avoid overhead while keeping our internal logic working correctly.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes a lot of sense and this approach works afaict

only have a pedantic nit,

can we do a foundry companion before merging?

src/buildinfo.rs Outdated Show resolved Hide resolved
src/cache.rs Show resolved Hide resolved
Comment on lines 30 to 31
/// A mapping from build_id to [BuildContext].
pub type Builds<L> = BTreeMap<String, BuildContext<L>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a wrapper type for this, just because aliases for maps are not great imo

@mattsse
Copy link
Member

mattsse commented Jun 11, 2024

error[rejected]: failed to satisfy license requirements
┌─ icu_locid 1.5.0 (registry+https://github.com/rust-lang/crates.io-index):4:12

4 │ license = "Unicode-3.0"

we can add "Unicode-3.0" to allowed in deny.toml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants