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: Syntax tests update #15661

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rodiazet
Copy link
Contributor

  • Compile syntax test via IR by default when EOF enabled.
  • Disable syntax tests which won't work with EOF when compiling to flag EOF enabled.

This comment was marked as resolved.

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I think that in some cases here just disabling the test on EOF is not the right solution. Some of them should have EOF version, some can be adjusted to work on both. I even found some that shouldn't need to be disabled because they seem work on both already.

There are also cases which indicate that we're missing some codegen asserts (and analysis checks). And even one case which is a result of a previously undiscovered unimplemented part of the IR codegen.

Copy link
Member

Choose a reason for hiding this comment

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

This one (and nested_calldata_storage2.sol) should still be tested via IR, just with different expectations (no error). Please add a copy with compileViaYul: true and that will pass on EOF just fine.

Or, even better, add them as semantic tests, since if they pass, we should also make sure they produce correct results.

@@ -7,5 +7,7 @@ library L2 {
contract A {
function f() public pure { type(L2).creationCode; }
Copy link
Member

Choose a reason for hiding this comment

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

Looks like creationCode and runtimeCode are producing broken IR on EOF. We need an assert against that in the codegen.

In the future it will also need an analysis check and an EOF-only copy of this test to test 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.

Same situation with:

  • gasleft() in 497_gasleft.sol.
  • selfdestruct() in address_payable_selfdestruct.sol.
  • <address>.codehash in codehash.sol.
  • <address>.code in address_members.sol.
    • In this case we should also split the EOF version of the test into part that does not use code (it should pass) and the one for code (that should report an error and will only be possible after we add the analysis check).

Copy link
Member

Choose a reason for hiding this comment

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

What's the problem with this one? Looks like it should work the same way on EOF and in legacy (i.e. still unimplemented).

Same for inline_array_fixed_types.sol, inline_array_rationals.sol, lexer_numbers_with_underscores_fixed.sol, rational_number_literal_to_fixed_implicit.sol and all those fixed-point type tests in nameAndTypeResolution/.

Copy link
Member

Choose a reason for hiding this comment

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

And viewPureChecker/builtin_functions_restrict_warning.sol. It also does not seem to have anything legacy-specific and gives me identical warnings on EOF when I compile it by hand.

Copy link
Member

Choose a reason for hiding this comment

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

I think this, calloptions_on_staticcall.sol and call_gas_on_function.sol are pretty relevant to EOF, because different set of available call options is one of the syntax differences. We should have a copy of these tests for EOF, just with different expectations.

Copy link
Member

Choose a reason for hiding this comment

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

We should have an EOF version of this, without gas.

Same for viewPureChecker/gas_value_without_call.sol and viewPureChecker/gas_with_call_nonpayable.sol.

Copy link
Member

Choose a reason for hiding this comment

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

And these should be rewritten to use value instead:

  • comparison_operator_for_external_functions_with_extra_gas_slots.sol (rename to comparison_operator_for_external_functions_with_call_options.sol)
  • external_functions_with_variable_number_of_stack_slots.sol
  • types_with_unspecified_encoding_internal_functions.sol

The use of gas there seems incidental. Any other call option would work just as well.

Copy link
Member

Choose a reason for hiding this comment

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

Needs a TODO that this should eventually work the same way on EOF. It's just that the immutable implementation is not yet in it's final state so we're not tracking immutables properly.

Copy link
Member

Choose a reason for hiding this comment

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

This one will still work if you replace codesize() with something else, e.g. calldataload(0). It's meant to test that hex literals are usable with switch and the use of codesize() is incidental.

Same for string_literal_switch_case.sol.

Copy link
Member

Choose a reason for hiding this comment

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

This one should have an EOF version. EOF is still subject to the size limit and that should be tested.

Copy link
Member

Choose a reason for hiding this comment

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

viewPureChecker/assembly.sol, viewPureChecker/builtin_functions.sol and all the viewPureChecker/inline_assembly_instructions_*.sol tests should have EOF versions, with legacy instructions removed (EOF equivalents used where possible) and some new EOF instructions added.

Copy link
Member

Choose a reason for hiding this comment

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

This one is a bug, though unrelated to EOF. Looks like it just triggers this unimplemented check when run via IR:

solUnimplementedAssert(varDecl);

It's probably a leftover from the IR codegen implementation. I think it fails because the translation of library name to its address was never implemented and we never hit this case in our tests.

We should fix that separately (I'll report an issue). For this PR it's fine to disable the test, but I'd use compileViaYul: true instead since it's not just EOF.

Copy link
Member

Choose a reason for hiding this comment

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

Reported as #15669.

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