Fix storage layout comparison for function types, disallow internal functions in storage

[ericglau]

This fixes 2 things:

1. Fixes An error due to function defined in struct in library #1031. In storage layout comparisons, type changes involving function pointers caused an error because of an incorrect assertion. The original and updated types could be unrelated during the comparison, but getVisibilityChange was asserting that both are functions. We should only call getVisibilityChange if both the original and updated types are indeed functions.

2. Fixes Disallow internal function types in storage #555. Internal functions are code pointers which will no longer be valid after an upgrade, so they should not be allowed. Added this as a validation error, and allow it to be skipped with the unsafeAllow.internal-function-storage option (if skipping, users need to make sure they always reassign the internal functions in storage variables during each upgrade).

[cairoeth]

mostly looks good, but shouldn't it also check for internal functions as a storage variable? for example:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

contract StorageUpgrade_FunctionType_Visibility_V2_Bad {
    function () internal m;
}

function m will break after an upgrade as it gets moved around storage, no? similar to internal functions in structs

[ernestognw]

Very interesting finding!

I wonder if we should document how to reset the internal function pointers in the docs. It's true we now have a clear instruction within the CLI but we're not providing a concrete example other than the one shared in #1031.

What do you think?

[ernestognw]

mostly looks good, but shouldn't it also check for internal functions as a storage variable? for example:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

contract StorageUpgrade_FunctionType_Visibility_V2_Bad {
    function () internal m;
}

function m will break after an upgrade as it gets moved around storage, no? similar to internal functions in structs

Functions do not live in storage, so m will not change. Instead, its pointer will change.
It's fine that it changes given that an upgrade changes the implementation (i.e. the runtime code and its internal function pointers).

The issue with storing internal function pointers in storage is that after upgrading to another implementation, the pointer in storage is a pointer to code that isn't used anymore. That's why it needs reinitialization.

[ericglau]

Agree we should check storage variables that have function types even outside of structs, and will add a section in the docs.

[cairoeth]

mostly looks good, but shouldn't it also check for internal functions as a storage variable? for example:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

contract StorageUpgrade_FunctionType_Visibility_V2_Bad {
    function () internal m;
}

function m will break after an upgrade as it gets moved around storage, no? similar to internal functions in structs

Functions do not live in storage, so m will not change. Instead, its pointer will change. It's fine that it changes given that an upgrade changes the implementation (i.e. the runtime code and its internal function pointers).

The issue with storing internal function pointers in storage is that after upgrading to another implementation, the pointer in storage is a pointer to code that isn't used anymore. That's why it needs reinitialization.

Yup, meant to refer to the pointer changing when upgrading to a different implementation.

[ericglau]

• Code now handles variables that reference struct definitions outside of contracts, as per comment
• Added new section in documentation
• Added links from errors and docs to the new section
• (To do outside of this PR) Define URL shortened link to the new doc section

[frangio]

LGTM! Minor suggestion.