[Deprecation feedback] Disallowing internal function pointers in storage

A recently reported issue brought our attention to the fact that compiler’s guarantees about the stability of internal function pointers are pretty weak. Such pointers are only valid in the context of a single contract and even then only in a very strict sense - any bytecode change may invalidate them. It will now be documented (Document internal function pointer stability guarantees by cameel · Pull Request #15721 · ethereum/solidity · GitHub) and users should be aware of it, but maybe this is not enough.

My suggestion would be to go further and disallow declaring storage variables of internal function types. The question is whether anyone thinks this is a bad idea or would be negatively impacted by this? If there are no objections, we’re likely to go ahead with this idea for 0.9.0.

Such types are already considered internal and cannot be passed across contracts (unless masked as something else). Putting them in storage can be safe if done carefully but it also seems like the fact that it has to be considered at all is not obvious to most users and a pretty big gotcha. It will still be possible via inline assembly of course but that will at least make it clearer that something unusual is happening.

2 Likes

Definitely agree with deprecating and removing this. It shouldn’t cause any issue, it can be recovered manually if someone needs it, and that’s the way it should’ve been originally.

2 Likes

Would this impact immutable function pointers that are set during contract construction? I’ve found those to be handy when you different ways that something could be handled depending on how a contract is initialized.

Immutables seem perfectly safe because they are stored in the contract code so they would be updated along with it. We’d let those be and only disallow putting internal pointers in storage.

Does anyone think we should disallow them in transient storage as well? On the one hand it’s technically possible to update a contract during a transaction, so it has the same problem as storage. On the other, the same could be said about memory and disallowing such pointers in memory seems to me like it would be going too far. So maybe transient storage is fine.

In any case, thanks for pointing out immutables. I actually didn’t think about that use case.

I think transient storage would be fine. Memory would be going too far, we use them in memory because of stack too deep.

1 Like

We decided to schedule the change for 0.9.0. The compiler will also start issuing a deprecation warning about it in one of the upcoming releases.