Reorganize semantic tests for calldata and abicoder by cameel · Pull Request #16483 · argotorg/solidity

cameel · 2026-02-19T19:34:08Z

This is just a refactor in preparation for fixing a bug in calldata validation. I needed to figure out what we have coverage for and since our tests for this are all over the place, I did some renaming and reorganization of the test dirs.

Summary of changes:

abiEncodeDecode/, abiEncoderV1/, abiEncoderV2/, calldata/ under semanticTests/ all had random subsets of ABI encoding and calldata tests. I merged them all into a single abicoder/ dir with subdirs for validation, cleanup, structs, arrays, etc. Also moved some tests from arrays/ and structs/ there.
This revealed that some of the tests are almost exact duplicates. I removed these.
- Note that tests without the abicoder pragma cover both v1 and v2 since isoltest inserts a pragma depending on whether it runs with or without --abiencoderv1. Those with v2 pragma cover only v2. Those with v1 pragma cover v1 via evmasm and v2 via Yul (since the compiler ignores the pragma via Yul).
- There are also quite a few non-identical tests covering the same thing. I intentionally left those, as I did not want to get into figuring out which version is better and should be left in.
I enabled debug revert strings in tests covering validations. Otherwise it's not clear which validation failed in the test. Sometimes that required creating separate v1 and v2 versions.
Other minor corrections: one test no longer needs compileViaYul: false. Another mixed tabs and spaces. Etc.

cameel · 2026-02-19T19:39:56Z

test/libsolidity/semanticTests/abicoder/validation/static_struct_v2.sol

+// revertStrings: debug
 // ----
 // f((int16,uint8,bytes2)): 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01, 0xff, "ab" -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01, 0xff, "ab"
 // f((int16,uint8,bytes2)): 0xff010, 0xff, "ab" -> FAILURE


Looks like we actually have no message for this validation, so revertStrings: debug does nothing here.

solidity/libsolidity/codegen/YulUtilFunctions.cpp

Line 4088 in 80d5c53

templ("failure", "revert(0, 0)");

This is in validatorFunction(), used by ABI utilities. It was likely missed when error messages were added, because it sits in YulUtilFunctions. I think we should give it a message.

I agree. There is probably more case which should have a message. Maybe it would be good to scan the code looking for revert(0, 0) pattern.

I would add an issue to not forget to implement this in the future.

cameel · 2026-02-19T19:50:38Z

copy_from_calldata_removes_bytes_data.sol is the only test I left under calldata/. It does not really test decoding, but I'm also not sure what it really tests. It comes from #12289, which extracted tests from SolidityEndToEndTest.cpp, but this does not seem to be one of them. It's as if it appeared out of nowhere. It's even weirder that I reviewed that PR and apparently did see the original back then. I suspect that something got botched when the PR was rebased.

In any case, this test could be covering some ancient, long fixed bug, but in isolation it looks a bit worthless and weird (why unused set() function?) so I'm considering just removing it.

cameel · 2026-02-19T19:52:22Z

...olidity/semanticTests/abicoder/validation/array_exceeds_size_limit_for_calldata_types_v2.sol

    }
 }
+// ====
+// revertStrings: debug


Maybe we should make it the default in tests? It's not enabled normally due to gas reasons, but I don't see a strong reason not to always have these messages in tests.

I agree. It would be much more clear what is the exact failure.

rodiazet

LGTM. Some comments or rather questions added and I proposed to move one test.

rodiazet · 2026-02-26T14:53:58Z

test/libsolidity/semanticTests/array/calldata_array_bounds_check_nested_bytes2.sol

This test is a copy of test/libsolidity/semanticTests/array/calldata_array_bounds_check_nested_bytes.sol Shouldn't this be moved to test/libsolidity/semanticTests/abicoder/calldataDecoding/array

This test is a copy of test/libsolidity/semanticTests/array/calldata_array_bounds_check_nested_bytes.sol

Yes, that's why I kept only one of them.

Shouldn't this be moved to test/libsolidity/semanticTests/abicoder/calldataDecoding/array

Well, we have many tests that would fit in more than one place. When it comes to calldata arrays, for example, there is a big overlap between array behavior and calldata decoding. This case IMO is more about the former. The test checks that accessing an element past the end of the array reverts. This is not specific to calldata and happens with all kinds of arrays (note that it's Panic, not Error).

...ibsolidity/semanticTests/abicoder/validation/array_exceeds_calldatasize_no_revert_string.sol

rodiazet · 2026-02-26T15:01:26Z

...olidity/semanticTests/abicoder/validation/array_exceeds_size_limit_for_calldata_types_v2.sol

    }
 }
+// ====
+// revertStrings: debug


I agree. It would be much more clear what is the exact failure.

rodiazet · 2026-02-26T15:03:48Z

test/libsolidity/semanticTests/abicoder/validation/external_function_type_inside_struct_v2.sol

+// revertStrings: debug
 // ----
 // f((function)): "01234567890123456789abcd" -> 1
 // f((function)): "01234567890123456789abcdX" -> FAILURE


Why this failure has not message?

Same reason as in https://github.com/argotorg/solidity/pull/16483/files#r2829800249. It fails in validatorFunction().

rodiazet · 2026-02-26T15:05:04Z

test/libsolidity/semanticTests/abicoder/validation/static_struct_v2.sol

+// revertStrings: debug
 // ----
 // f((int16,uint8,bytes2)): 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01, 0xff, "ab" -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01, 0xff, "ab"
 // f((int16,uint8,bytes2)): 0xff010, 0xff, "ab" -> FAILURE


I agree. There is probably more case which should have a message. Maybe it would be good to scan the code looking for revert(0, 0) pattern.

cameel · 2026-03-12T18:18:37Z

copy_from_calldata_removes_bytes_data.sol is the only test I left under calldata/.

Still not sure what to do about it, but for now I decided to move it to abicoder/calldataDecoding/ to avoid having an almost empty calldata/ dir. If we keep it, things will get messy again, because people will start adding new tests there.

- The test does seem to pass now via both pipelines.

… tests

rodiazet

LGTM

cameel requested review from nikola-matic and rodiazet February 19, 2026 19:34

cameel self-assigned this Feb 19, 2026

cameel added the refactor label Feb 19, 2026

cameel force-pushed the reorganize-calldata-semantic-tests branch from 6095512 to f20af71 Compare February 19, 2026 19:35

cameel commented Feb 19, 2026

View reviewed changes

rodiazet previously approved these changes Feb 26, 2026

View reviewed changes

cameel force-pushed the reorganize-calldata-semantic-tests branch from f20af71 to d45a45e Compare March 12, 2026 16:39

cameel mentioned this pull request Mar 12, 2026

Fix negative calldata offset validations #16522

Draft

cameel dismissed rodiazet’s stale review via 0df51f7 March 12, 2026 18:16

cameel force-pushed the reorganize-calldata-semantic-tests branch from d45a45e to 0df51f7 Compare March 12, 2026 18:16

cameel added 5 commits March 12, 2026 19:19

Reorganize calldata and abicoder semantic tests

ad60fd4

calldata_array_dynamic_bytes.sol: Enable testing via Yul

a11bebd

- The test does seem to pass now via both pipelines.

Fix whitespace in some abiEncodeDecode tests

5bab519

Remove redundant ABI coder tests

06b36cc

Enable debug revert strings in some of the existing calldata/encoding…

8b17183

… tests

cameel force-pushed the reorganize-calldata-semantic-tests branch from 0df51f7 to 8b17183 Compare March 12, 2026 18:20

cameel requested a review from rodiazet March 12, 2026 18:21

rodiazet approved these changes Mar 17, 2026

View reviewed changes

Conversation

cameel commented Feb 19, 2026

Uh oh!

cameel Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel commented Feb 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel commented Mar 12, 2026

Uh oh!

rodiazet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cameel Feb 19, 2026 •

edited

Loading