Skip to content

feat: optimize compilation by reading AST#7599

Merged
mattsse merged 12 commits into
masterfrom
klkvr/smarter-compilation
Apr 17, 2024
Merged

feat: optimize compilation by reading AST#7599
mattsse merged 12 commits into
masterfrom
klkvr/smarter-compilation

Conversation

@klkvr

@klkvr klkvr commented Apr 8, 2024

Copy link
Copy Markdown
Member

Motivation

Currently when running forge script <contract_name> or forge script <contract_path> we will recompile entire project, this is true for forge create and forge selectors as well.

Solution

Use already existing files filter on ProjectCompiler when path is provided, use solang AST to find source file when only contract name is provided.

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nits and q about error handling

Comment thread crates/common/src/compile.rs Outdated
Comment thread crates/common/src/compile.rs Outdated
for file in graph.files().keys() {
let src = std::fs::read_to_string(file)?;
let (parsed, _) = solang_parser::parse(&src, 0)
.map_err(|_| eyre::eyre!("Failed to parse {}.", file.display()))?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we now depend on solang to parse it successfully.

should we fallback to previous behaviour if this fails?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah I think we can do that

actually thought that we assumed solang being able to parse any valid Solidity

perhaps its better to invoke solc with empty output instead? similar to how we do it for tests now requesting just abi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually thought that we assumed solang being able to parse any valid Solidity

it could be that future lang features are not supported right away, so I think we can fallback to previous behaviour if parsing fails.
if there's any syntax error this should also ensure that the user gets the proper solc error message

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated code, now we fallback to running solc with empty output when solang fails

not really want to fully fallback to old behavior as this is not the most efficient way and will add additional complexity because we'd have to handle potential find_contract_path errors everywhere

Comment thread crates/common/src/compile.rs Outdated
@klkvr klkvr force-pushed the klkvr/smarter-compilation branch from b17a4c6 to c2cfc04 Compare April 14, 2024 14:15
@klkvr klkvr force-pushed the klkvr/smarter-compilation branch from df83444 to ca60334 Compare April 14, 2024 21:54

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggesting to move the new fn f(&Project,..) functions to foundry-compilers directly, we already have solang parsing in there so I think moving them would be appropriate

Comment thread crates/common/src/compile.rs Outdated
mattsse pushed a commit to foundry-rs/compilers that referenced this pull request Apr 17, 2024
@mattsse mattsse merged commit 0a4d246 into master Apr 17, 2024
@mattsse mattsse deleted the klkvr/smarter-compilation branch April 17, 2024 14:36
alexandergrey33 added a commit to alexandergrey33/compilers that referenced this pull request Sep 17, 2025
bug-knightxmiu70 added a commit to bug-knightxmiu70/compilers that referenced this pull request Sep 28, 2025
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.

2 participants