preprocessor to skip recompiling tests on non-interface source file changes #252

grandizzy · 2025-02-19T07:58:53Z

supersedes #198
closes #197

rebased with main
uses solar instead solang to compute interface representation

Implements preprocessor allowing us to skip recompiling tests on non-interface source file changes. Still testing, but looks like it's working correctly, decreasing single-change build times from ~1 minute to 5s in some cases.

TODOs

use solar HIR (deps to review and include feat(hir): expose hir from parsing context paradigmxyz/solar#210)
unit tests
opt-in caching changes
autodetect mocks
figure out how to parse when building from different dir
additional testing (tested with uniswap v4-core and simpler projects)

modified: crates/compilers/src/preprocessor.rs

crates/compilers/src/preprocessor.rs

crates/compilers/src/flatten.rs

crates/compilers/src/lib.rs

crates/compilers/src/flatten.rs

crates/compilers/src/preprocessor/data.rs

crates/compilers/src/preprocessor/deps.rs

crates/compilers/src/preprocessor/data.rs

crates/compilers/src/lib.rs

crates/compilers/src/preprocessor/mod.rs

klkvr · 2025-03-03T15:31:56Z

crates/compilers/src/cache.rs

+        if self.cache.preprocessed {
+            return self.missing_extra_files();
+        }


the missing_extra_files logic should always run, it's unrelated to preprocessor, we should always run it

gotcha, will then call it in is_missing_artifacts() only, previously was in is_dirty_impl(), is this OK?

changed in 2d1e59f

klkvr · 2025-03-03T15:40:13Z

crates/compilers/src/cache.rs

+    }
+
+    // Walks over all cache entries, detects dirty files and removes them from preprocessed cache.
+    fn find_and_remove_dirty_preprocessed(&mut self) {


given that we already have the self.cache.preprocessed flag I don't think we need two separate methods? those share most of the logic and only different in a way we traverse imports so I'd prefer to only have a single find_and_remove_dirty to avoid redundancy

changed in 2d1e59f

klkvr · 2025-03-03T17:13:42Z

crates/compilers/src/cache.rs

+                if !self.cache.preprocessed {
+                    // Purge all sources on graph resolution error.
+                    self.dirty_sources.extend(files);
+                } else {
+                    // Purge all sources on graph resolution error and return.
+                    self.cache.files.clear();
+                    return;
+                }


I don't think this should be handled differently depending on this flag

crates/compilers/src/cache.rs

klkvr · 2025-03-04T16:01:40Z

crates/compilers/src/compile/project.rs

                jobs.push((input, profile, actually_dirty));
            }
        }

+        cache.add_mocks(mocks);


wouldn't this only take effect on the next compiler run? I guess ideally we should detect mocks before searching for dirty files? this means that we need to somehow integrate parsing into caching logic though which might get complex quickly

yep that's correct, but the first compiler run does compile and run all tests anyways so don't think should be a problem to take effect from next run. Do I miss any scenario?

- pass the set of cached mocks to preprocessor - preprocessor updates the set accordingly (add new mocks and cleanup old mocks - handles case when a mock is refactored to non mock impl) - replace set of mocks in cache

zerosnacks · 2025-03-05T11:51:01Z

crates/compilers/src/cache.rs

+    /// hash of the interface representation of the file, if it's a source file
+    pub interface_repr_hash: Option<String>,


nit: noticed interfaceReprHash is sometimes null in the cache, would it make sense to skip this field when it is None during serialization w/ #[serde(skip_serializing_if = "Option::is_none")]?

yep, that's null for all files under test / script dirs, will check if any side effect if we don't serialize them but shouldn't

DaniPopes · 2025-03-10T07:51:31Z

crates/compilers/src/compile/project.rs


        // convert paths on windows to ensure consistency with the `CompilerOutput` `solc` emits,
        // which is unix style `/`
        sources.slash_paths();

-        let mut cache = ArtifactsCache::new(project, edges)?;
+        let mut cache = ArtifactsCache::new(project, edges, preprocessor.is_some())?;


do the cache changes apply to any preprocessor that modifies sources or specifically to the one implemented in this PR? If the latter, this should be .as_ref().is_some_and(|p| p.some_method())

DaniPopes · 2025-03-10T07:52:02Z

crates/compilers/src/compile/project.rs

@@ -481,10 +514,18 @@ impl<L: Language, S: CompilerSettings> CompilerSources<'_, L, S> {

                input.strip_prefix(project.paths.root.as_path());

+                if let Some(preprocessor) = preprocessor.as_ref() {
+                    input =
+                        preprocessor.preprocess(&project.compiler, input, &project.paths, mocks)?;


take by input reference?

klkvr and others added 12 commits September 13, 2024 14:46

wip

abd521f

wip

fb64ca7

add preprocessor

0876b01

wip

314f284

modified: crates/compilers/src/preprocessor.rs

wip

86f0562

modified: crates/compilers/src/preprocessor.rs

fixes

af6550f

helper libs

276bc94

some docs

fa5d7bf

clean up

15ce120

fixes

a379db3

Merge branch 'main' into klkvr/pp-test-rebase

25e5ef9

Port to solar

3faaa02

grandizzy force-pushed the klkvr/pp-test-rebase branch from 1c02f7f to 325a627 Compare February 19, 2025 08:03

Clippy

0f78197

grandizzy force-pushed the klkvr/pp-test-rebase branch from 325a627 to 0f78197 Compare February 19, 2025 08:16

DaniPopes requested changes Feb 19, 2025

View reviewed changes

grandizzy added 2 commits February 19, 2025 14:29

Changes after review

594c09b

Patch solar to get HIR visitor impl

3105272

DaniPopes reviewed Feb 19, 2025

View reviewed changes

crates/compilers/src/flatten.rs Outdated Show resolved Hide resolved

Review changes

7d0edb8

grandizzy force-pushed the klkvr/pp-test-rebase branch from b4c0746 to 7d0edb8 Compare February 19, 2025 14:37

reuse replace source fn

4025e5a

grandizzy force-pushed the klkvr/pp-test-rebase branch from b5ebece to 4025e5a Compare February 19, 2025 16:56

grandizzy added 2 commits February 19, 2025 19:04

Clippy

0c940d1

Cleanup, move Update type in lib

79d0a0f

DaniPopes reviewed Feb 20, 2025

View reviewed changes

crates/compilers/src/lib.rs Outdated Show resolved Hide resolved

crates/compilers/src/lib.rs Outdated Show resolved Hide resolved

crates/compilers/src/flatten.rs Outdated Show resolved Hide resolved

crates/compilers/src/flatten.rs Outdated Show resolved Hide resolved

Change replace_source_content sig, move apply_updates

d37cafb

grandizzy force-pushed the klkvr/pp-test-rebase branch from be6c378 to d37cafb Compare February 20, 2025 12:56

grandizzy added 2 commits February 26, 2025 12:29

Port to solar HIR, refactor

c526df6

add preprocessing parse constructors

e691a96

grandizzy commented Feb 26, 2025

View reviewed changes

crates/compilers/src/preprocessor/data.rs Outdated Show resolved Hide resolved

grandizzy added 2 commits February 26, 2025 19:26

Contract id cleanup

4241d2b

Cleanup, filter collected dependencies to be source contracts

c86c4aa

grandizzy force-pushed the klkvr/pp-test-rebase branch from 4efbf23 to c86c4aa Compare February 27, 2025 11:03

DaniPopes reviewed Feb 28, 2025

View reviewed changes

grandizzy added 2 commits February 28, 2025 21:00

Cleanup, remove Hir:: usage, use ContractIds

f4a109d

Optional preprocessed cache

f0818ea

klkvr reviewed Mar 3, 2025

View reviewed changes

Review cleanup

2d1e59f

klkvr reviewed Mar 3, 2025

View reviewed changes

crates/compilers/src/cache.rs Outdated Show resolved Hide resolved

klkvr reviewed Mar 3, 2025

View reviewed changes

crates/compilers/src/cache.rs Outdated Show resolved Hide resolved

Simplify find and remove branches

c354a7c

grandizzy force-pushed the klkvr/pp-test-rebase branch 2 times, most recently from a967188 to 99b5a06 Compare March 4, 2025 08:37

Autodetect and recompile mocks

719d4e8

grandizzy force-pushed the klkvr/pp-test-rebase branch from 99b5a06 to 719d4e8 Compare March 4, 2025 08:38

Fix description

4342f82

klkvr reviewed Mar 4, 2025

View reviewed changes

grandizzy added 2 commits March 5, 2025 09:03

Cleanup autodetect and update cached mocks:

d8ece14

- pass the set of cached mocks to preprocessor - preprocessor updates the set accordingly (add new mocks and cleanup old mocks - handles case when a mock is refactored to non mock impl) - replace set of mocks in cache

Invalidate cache on preprocess option toggle

c4ec52c

zerosnacks reviewed Mar 5, 2025

View reviewed changes

grandizzy mentioned this pull request Mar 5, 2025

feat(forge): optimize compilation through preprocessing and caching foundry-rs/foundry#10010

Draft

7 tasks

DaniPopes reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preprocessor to skip recompiling tests on non-interface source file changes #252

preprocessor to skip recompiling tests on non-interface source file changes #252

grandizzy commented Feb 19, 2025 •

edited

Loading

klkvr Mar 3, 2025

grandizzy Mar 3, 2025

grandizzy Mar 3, 2025

klkvr Mar 3, 2025

grandizzy Mar 3, 2025

klkvr Mar 3, 2025

klkvr Mar 4, 2025

grandizzy Mar 4, 2025 •

edited

Loading

zerosnacks Mar 5, 2025

grandizzy Mar 5, 2025

DaniPopes Mar 10, 2025

DaniPopes Mar 10, 2025

		/// hash of the interface representation of the file, if it's a source file
		pub interface_repr_hash: Option<String>,

preprocessor to skip recompiling tests on non-interface source file changes #252

Are you sure you want to change the base?

preprocessor to skip recompiling tests on non-interface source file changes #252

Conversation

grandizzy commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented Feb 19, 2025 •

edited

Loading

grandizzy Mar 4, 2025 •

edited

Loading