-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: optimize compilation by reading AST #7599
Conversation
There was a problem hiding this comment.
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
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()))?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
b17a4c6
to
c2cfc04
Compare
df83444
to
ca60334
Compare
There was a problem hiding this comment.
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
Motivation
Currently when running
forge script <contract_name>
orforge script <contract_path>
we will recompile entire project, this is true forforge create
andforge 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.