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

Unify Yul parsing across test suites #15611

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

cameel
Copy link
Member

@cameel cameel commented Dec 4, 2024

Depends on #15610. Merged.

Just a test refactor in preparation for another PR.

Currently most Yul test suites use the yul::test::parse() helper, which has two overloads which do things in wildly inconsistent ways. One uses YulStack but assumes no errors and reports only the top-level AST. The other manually runs ObjectParser and does return errors and the whole Object but only runs analysis on the top-level AST ignoring the others.

On top of that neither of them really suits my needs, because they don't allow continuing the compilation after parsing. The PR replaces them with a new, more flexible, helper that returns YulStack instead. Then makes all test suites use it.

Also:

  • Most suites do accept complete Yul objects thanks to these helpers but then proceed to silently ignore any sub-objects. I made them report an unimplemented feature instead.
  • Many suites do not actually print Yul compilation errors and just say that compilation failed since they're not testing the parser and assume correctness of input files. I added a helper for easy reporting of errors and made all suites consistently use it (except for those doing Boost tests).
  • I made all the suites assume the EVM dialect. We still technically have a dialect setting but there's only one dialect and passing it to YulStack is a bit of a hassle so not having to do that simplifies things a lot.

@cameel cameel self-assigned this Dec 4, 2024
@cameel cameel marked this pull request as draft December 4, 2024 04:27
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Dec 4, 2024
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 48afef8 to b0efac5 Compare December 4, 2024 04:34
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch 3 times, most recently from 812efab to 609fd43 Compare December 4, 2024 05:52
@cameel cameel force-pushed the fix-code-generation-error-reporting-in-yul branch 4 times, most recently from 7f49fb7 to e3a35e8 Compare December 7, 2024 04:03
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 609fd43 to 4d3140e Compare December 7, 2024 04:26
@cameel cameel force-pushed the fix-code-generation-error-reporting-in-yul branch from e3a35e8 to cbe7782 Compare December 11, 2024 00:05
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch 2 times, most recently from 48fd3d0 to 29c2a39 Compare December 11, 2024 00:37
@cameel cameel force-pushed the fix-code-generation-error-reporting-in-yul branch 2 times, most recently from b051766 to f0656c4 Compare December 11, 2024 18:45
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 29c2a39 to f1e44cf Compare December 11, 2024 18:45
@cameel cameel force-pushed the fix-code-generation-error-reporting-in-yul branch from f0656c4 to ace62cd Compare December 11, 2024 20:30
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch 2 times, most recently from e797b3c to fcbfbf5 Compare December 11, 2024 21:07
@cameel cameel marked this pull request as ready for review December 11, 2024 21:48
@cameel cameel requested review from clonker and r0qs December 11, 2024 21:48
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

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

I didn't find anything wrong with the PR, if you want to expose the dialect in the YulStack I'll reapprove afterwards

@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from fcbfbf5 to 9acd665 Compare December 12, 2024 17:51
clonker
clonker previously approved these changes Dec 13, 2024
@cameel cameel force-pushed the fix-code-generation-error-reporting-in-yul branch from ace62cd to 47545d3 Compare December 13, 2024 12:57
Base automatically changed from fix-code-generation-error-reporting-in-yul to develop December 13, 2024 13:36
@cameel cameel dismissed clonker’s stale review December 13, 2024 13:36

The base branch was changed.

@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Dec 13, 2024
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 9acd665 to 3d8ff6b Compare December 13, 2024 14:26
@cameel cameel requested a review from clonker December 13, 2024 14:34
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 3d8ff6b to 2f4d58f Compare December 13, 2024 20:23
@cameel cameel force-pushed the unify-yul-parsing-across-test-suites branch from 2f4d58f to 46b0616 Compare December 17, 2024 14:55
@cameel cameel merged commit 6c28421 into develop Dec 17, 2024
73 checks passed
@cameel cameel deleted the unify-yul-parsing-across-test-suites branch December 17, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants