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

eof: Enable compilation to EOF for all EVMVersionRestrictedTestCase tests by default #15666

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Dec 19, 2024

Depends on:
#15818 Merged
#15657 Merged
#15658 Merged
#15659 Merged
#15660 Merged
#15661 Merged
#15662 Merged
#15663 Merged
#15665 Merged

Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

@cameel cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Dec 20, 2024
@rodiazet rodiazet force-pushed the eof-enable-eof-testing branch 8 times, most recently from 4c506e9 to ccfa0f9 Compare January 9, 2025 11:40
@rodiazet rodiazet force-pushed the eof-enable-eof-testing branch 3 times, most recently from b62dc92 to 0caa233 Compare February 1, 2025 09:15
Comment on lines 153 to 155
auto const compileViaYul = m_reader.stringSetting("compileViaYul", "unset");
std::string bytecodeFormatString
= m_reader.stringSetting("bytecodeFormat", compileViaYul == "false" ? "legacy" : "legacy,>=EOFv1");
Copy link
Member

Choose a reason for hiding this comment

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

Duplicating compileViaYul here may be a bad idea. This makes the setting available even in tests where it does not make sense. And, even worse, in some where it would make sense but they don't check it so it has no effect. And in those that define it later a conflicting definition here might have some weird side-effects. On top of that it will be just confusing - here EOF depends on compileViaYul, but in SyntaxTest we already have a default for compileViaYul that depends on EOF.

You need this change only because of semantic tests, right? It's only there that it defaults to also. If that's the case then a better solution would be to have the default in SemanticTest be dependent on EOF, just like we already did in SyntaxTest. Make it also in non-EOF runs and true in EOF runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix. I also not run a test if compileViaYul flag is set to false and command option enables EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But doing this I found one problem with test configuration check. We don't verify that a test settings are correct. In case where compileViaYul: fasle and 'bytecodeFormat: >=EOFv1' the test never runs and we don't get any error.

Copy link
Member

@cameel cameel Feb 3, 2025

Choose a reason for hiding this comment

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

We don't verify that a test settings are correct.

True. We should have checks against this. Or use TestCaseReader::enumSetting() instead of stringSetting().

Can you refactor then into enum settings (in a separate PR)?

Copy link
Member

@cameel cameel Feb 3, 2025

Choose a reason for hiding this comment

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

Wait, but both semantic and syntax test have compileViaYulAllowedValues and check the value against it. Doesn't it catch the error?

Copy link
Member

Choose a reason for hiding this comment

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

I also not run a test if compileViaYul flag is set to false and command option enables EOF

Sounds fine, but please put this in a separate commit. It's a separate fix.

Copy link
Contributor Author

@rodiazet rodiazet Feb 3, 2025

Choose a reason for hiding this comment

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

I mean something different by verifying a test settings. We do verify the values separately but we should also check that i.e. compileViaYul: false then bytecodeFormat cannot be >=EOFv1. Every test like this never runs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, we don't verify that they conflict. True, we should add a check against that.

Do you want to do it here, of should I merge already?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see #15819. I guess you can do it there.

@rodiazet rodiazet force-pushed the eof-enable-eof-testing branch 4 times, most recently from e4dff1c to 047043c Compare February 3, 2025 12:07
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Feb 3, 2025
@cameel
Copy link
Member

cameel commented Feb 3, 2025

All dependencies merged. Please rebase for a final check and we're done!

@cameel cameel merged commit eb3b721 into ethereum:develop Feb 3, 2025
74 checks passed
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