Code Generator: treat abi.encodeCall as no-op when referenced as a bare member#16726
Code Generator: treat abi.encodeCall as no-op when referenced as a bare member#16726genisis0x wants to merge 1 commit into
abi.encodeCall as no-op when referenced as a bare member#16726Conversation
…bare member The set of `abi.*` members that are silently dropped by ExpressionCompiler when referenced without a call (`abi.encode;`, `abi.decode;`, etc.) omitted `encodeCall`. Bare references like `abi.encodeCall;` reached the fallthrough `solAssert(false, "Unknown magic member.")` and triggered an internal compiler error during code generation. The other members in the same family already no-op without bytecode side effects, matching the expected behavior described in the issue. Add `encodeCall` to that no-op set, alongside the existing `encode`/`encodePacked`/`encodeWithSelector`/`encodeWithSignature`/`decode` entries. Test coverage: - New `syntaxTests/specialFunctions/abi_encodeCall_member_access.sol` exercises the bare `abi.encodeCall;` reference and asserts the expected `Warning 6133` (statement has no effect) instead of the prior ICE. - Existing `semanticTests/specialFunctions/abi_functions_member_access.sol` extended to include `abi.encodeCall;` so codegen executes the new branch as part of the existing semantic round-trip. Fixes argotorg#16612
|
You are opening a lot of new pull requests while previous ones are still pending (you have 10 now). In some cases (#16710) even for issues that already had a pending fix (#16520). Unfortunately we have limited bandwidth for reviewing contributions and as you can see there are a lot of PRs pending. ICEs specifically are also quite low priority compared to other things we need to focus on (security issues, SSA CFG, ethdebug, optimizer). Even when the fix seems straightforward, there is a lot of complexity in the compiler and the review effort is not an insignificant portion of the work that goes into it (#16509 is a good example of that). Some of them just have to wait for their turn and flooding us with PRs that will get stale is not helpful. Can I ask you to limit yourself to one PR at a time? We are not going to be reviewing them all at once anyway. Also, I encourage you to first post about your proposed solution in the issue before you start working on a fix. Very often there are nuances that need to be taken into account and the most straightforward solution may not be the right one. Working them out through a high-level discussion is easier for us than than being forced to deal with all the details in the code immediately. |
|
Fair feedback, and apologies — the volume was on me. I got into a habit of opening a PR per ICE / typechecker edge as I worked through them, without weighing the review cost on your side. That's not the right shape for this codebase, especially with the priorities you listed (security, SSA CFG, ethdebug, optimizer) needing the real bandwidth. I'll do three things:
Sorry for the noise on your queue. |
cameel
left a comment
There was a problem hiding this comment.
The fix is fine, but the PR is missing a changelog entry.
It would also be good to check why we're not taking the same path for calls. That's suspicious.
There's also an optional refactor you could do to not only fix it but also prevent the bug from reappearing if new members are added in the future. I'd still be fine with merging it without it though since at this point it's not very likely that we'll add any.
|
|
||
| } | ||
| else if ((std::set<std::string>{"encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode"}).count(member)) | ||
| else if ((std::set<std::string>{"encode", "encodePacked", "encodeCall", "encodeWithSelector", "encodeWithSignature", "decode"}).count(member)) |
There was a problem hiding this comment.
This fixes the issue at hand, but it would be good to also prevent it from happening in the future. We do have the list already under Kind::ABI in MagicType::nativeMembers(), so it should be reused here instead of being hard-coded.
We could refactor nativeMembers() to have a static overload taking just MagicType::Kind. Then initialize this set using that helper, filtering out non-function members (those may not be no-ops).
Alternatively, we could assert that it produces the same set. This would fail when we add a new member, but the advantage would be that we'd force them to have a look at this place and decide what to do. This would also make things more fool-proof by not excluding non-function members.
There was a problem hiding this comment.
Note that we have the same code in IRGeneratorForStatements.cpp (just without the bug):
So that refactor should be done also there.
|
|
||
| } | ||
| else if ((std::set<std::string>{"encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode"}).count(member)) | ||
| else if ((std::set<std::string>{"encode", "encodePacked", "encodeCall", "encodeWithSelector", "encodeWithSignature", "decode"}).count(member)) |
There was a problem hiding this comment.
Can you check why this assert is not triggered when the function is actually called? I'd expect that to enter here too.
There was a problem hiding this comment.
The PR needs a changelog entry. Something like:
* Evmasm Code Generator: Fix internal compiler error when `abi.encodeCall` is referenced but not called.
Thanks! Judging from this one, the contributions seem good and we do need to fix them eventually. But we'll generally need to introduce some rules to limit the PRs because recently that has skyrocketed and a long backlog of unreviewed PRs is quite discouraging to everyone involved. |
Description
Fixes #16612.
solc --binpanics with an internal compiler error whenabi.encodeCallis referenced as a bare member expression (not called):The
abi.*member access branch inExpressionCompiler::visit(MemberAccess const&)carries a hard-coded set of members that are silently dropped when referenced without a call:encode,encodePacked,encodeWithSelector,encodeWithSignature,decode.encodeCallis missing from this set, so its bare reference falls through to thesolAssert(false, "Unknown magic member.")guard. The other members in the same family already no-op without bytecode side effects; this change addsencodeCallto match.The TypeChecker continues to flag the bare reference with
Warning 6133("Statement has no effect"), preserving the existing user-visible diagnostic surface.Tests
test/libsolidity/syntaxTests/specialFunctions/abi_encodeCall_member_access.solexercises the bareabi.encodeCall;reference and asserts the expectedWarning 6133instead of the prior ICE.test/libsolidity/semanticTests/specialFunctions/abi_functions_member_access.solextended to includeabi.encodeCall;so the codegen no-op branch is exercised as part of the same semantic round-trip that already covers the rest of the family.Checklist
AI Disclosure