UDVT: Allow internal function types as underlying type#16604
UDVT: Allow internal function types as underlying type#16604k06a wants to merge 1 commit intoargotorg:developfrom
Conversation
|
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
left a comment
There was a problem hiding this comment.
Function type UDVTs sound fine to me in principle, but I don't like the potential implementation/review complexity. Off the top of my head I can list several corner cases that are likely not being handled here and I'm sure there's more. See the comment below. If the implementation really stays this simple after accounting for all of it, we could add this to the language, but it definitely needs way more test coverage to prove nothing was missed.
But overall this feature is not really something we'd prioritize right now - as always we're bottlenecked on reviewing and making sure this does no open holes in the codegen. If I had to prioritize one thing, it would be #14167. That's really useful and highly requested. Especially now that it ranked quite high in Solidity Survey maybe we can agree internally that the cost/benefit ratio is high enough to spare some bandwidth without impacting SSA CFG or ethdebug.
There was a problem hiding this comment.
I doubt that implementation for this will be this short. There are many corner cases around function types, so if you allow them to masquerade as UDVTs, we have to basically replicate all their analysis checks in all of the UDVT cases. Otherwise some safeguards could be bypassed.
- Internal functions are not allowed in the ABI. UDVTs are.
- Same for immutables.
- Storing internal function pointers is complicated. It differs between the IR and evmasm codegen and in case of evmasm requires combining two versions (creation and deployed) into a single slot. Not sure if this PR handles that properly or not, but I at least don't see any coverage for it.
- There are some limitations and design question around constants of function types that we're currently working to resolve (Function references are not considered constant #16339). It's still not certain that we'll allow constant internal function pointers. This PR likely enables them, because UDVTs can be constant.
- Even if we decide that we want them (which is likely), the fact that they weren't allowed so far means they have zero test coverage for internal function constants. The PR will have to add it.
- Codegen relies on detecting function pointers to build the internal dispatch and assign them IDs. It may work as is since you have to assign the function already, but needs some coverage.
- In library ABI user-defined types are handled specially (reported
Motivation
User-defined value types (UDVTs) currently only accept elementary types (
uint256,address,bool, etc.) as their underlying type. This prevents creating type-safe abstractions over internal function pointers – a pattern that is especially useful for hook/callback architectures (e.g. Uniswap v4 hooks, SwapVM instructions, plugin systems, strategy patterns).Without this feature, developers resort to raw
function(...) internaltypes everywhere, losing the ability to distinguish semantically different callbacks at the type level. For example, aBeforeSwapHookand anAfterSwapHookmight have identical signatures but represent fundamentally different roles – UDVTs solve exactly this problem for integers and addresses, and should do the same for function pointers.Change
Relax the
ElementaryTypeNamerestriction inDeclarationTypeCheckerto also acceptFunctionTypeName, limited to internal function types only. External function types are explicitly rejected since they occupy two stack slots and would require additional codegen support.No codegen changes are needed: internal function pointers are single-slot values (like
uint256), sowrap/unwrap, storage, and conversions work as-is through the existing UDVT delegation tounderlyingType().Example
Test plan