Fix ICE for user-defined value type in external library function

[chfast]

UserDefinedValueType::signatureInExternalFunction() had an unconditional solAssert(false), assuming the type would always be unwrapped before signature generation. This fails for library external functions with storage array/mapping parameters of UDVT, since ArrayType::interfaceType(inLibrary=true) returns the storage array without unwrapping the base type.

Delegate to underlyingType().signatureInExternalFunction() instead, consistent with how encodingType() and interfaceType() already behave.

This is the same class of bug that was previously fixed for canonicalName() in de01822 ("UserDefinedValueType: from simple name to canonical name") — both methods were originally stubbed with solAssert(false) when UDVT was introduced in 1545237, and signatureInExternalFunction was missed.

Fixes #16225

[github-actions]

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.

[r0qs]

Thanks @chfast. LGTM, the external tests failures are unrelated ;)

[cameel]

Thanks! The PR still needs a few things though.

[cameel]

Since this passes without errors, it should really be a semantic test. A syntax test only verifies that this code can pass through analysis without errors. It does not ensure that we generate sane bytecode for it.

[cameel]

Also, we're clearly missing coverage for things like this. We should at least have tests using other underlying types and other storage parameter types (e.g. mappings mentioned in the description or structs).

[cameel]

I finally managed to do proper review of this. Unfortunately it looks like the fix is incomplete. It does suppress the ICE, but the signature we get for mappings is inconsistent with how UDVTs are treated in library function signatures.

I also discovered that our treatment of UDVTs in libraries is already inconsistent - probably no one bothered to check when we implemented them or in one of the fixes you mentioned. We need to update the docs to reflect that.

[cameel]

In addition to a semantic test we also need one that shows what the new signatures look like so that we can evaluate how the change impacts the ABI. Especially how they differ between libraries and contracts and between memory and storage arguments. ABI has very strong backwards-compatibility guarantees, so if we get it wrong, we'll be stuck with mess that we won't be able to fix easily.

Here's one I just used to investigate this manually. It should be added to cmdlineTest/:

contract C {}
interface I {}
type U is uint;
struct S { U x; E y; C c; I i; }
enum E {A, B, C}

library LMap {
    function getUDVTMapping(mapping(U => U) storage) external {}
    function getEnumMapping(mapping(E => E) storage) external {}
    function getStructMapping(mapping(uint => S) storage) external {}
    function getContractMapping(mapping(C => I) storage) external {}
}

library LStor {
    function getUDVT(U[] storage) external {}
    function getBytes(bytes[] storage) external {}
    function getStruct(S[] storage) external {}
    function getEnum(E[] storage) external {}
    function getContract(C[] storage) external {}
    function getInterface(I[] storage) external {}
    function getFunction(function(U, S memory, E) external[] storage) external view {}
}

library LMem {
    function getUDVT(U[] memory) external {}
    function getBytes(bytes[] memory) external {}
    function getStruct(S[] memory) external {}
    function getEnum(E[] memory) external {}
    function getContract(C[] memory) external {}
    function getInterface(I[] memory) external {}
    function getFunction(function(U, S memory, E) external[] memory) external view {}
}

contract CMem {
    function getUDVT(U[] memory) external {}
    function getBytes(bytes[] memory) external {}
    function getStruct(S[] memory) external {}
    function getEnum(E[] memory) external {}
    function getContract(C[] memory) external {}
    function getInterface(I[] memory) external {}
    function getFunction(function(U, S memory, E) external[] memory) external view {}
}

Here's the relevant part of the output of solc --hashes from your branch:

======= test.sol:LMap =======
Function signatures:
027b4c0e: getContractMapping(mapping(C => I) storage)
d02c5bc5: getEnumMapping(mapping(E => E) storage)
88246425: getStructMapping(mapping(uint256 => S) storage)
5f175033: getUDVTMapping(mapping(U => U) storage)

======= test.sol:LStor =======
Function signatures:
c8a96f7b: getBytes(bytes[] storage)
4b8c90be: getContract(C[] storage)
7652387c: getEnum(E[] storage)
c738c8fb: getFunction(function[] storage)
5b6d96b0: getInterface(I[] storage)
93bb8e52: getStruct(S[] storage)
89fd932b: getUDVT(uint256[] storage)

======= test.sol:LMem =======
Function signatures:
e4b4deb7: getBytes(bytes[])
5ef1bc6a: getContract(C[])
d2c247c9: getEnum(E[])
afd7a10c: getFunction(function[])
392067e7: getInterface(I[])
be8c0fb1: getStruct(S[])
ed352282: getUDVT(uint256[])

======= test.sol:CMem =======
Function signatures:
e4b4deb7: getBytes(bytes[])
ca3cbc19: getContract(address[])
d23989a5: getEnum(uint8[])
afd7a10c: getFunction(function[])
df8e7709: getInterface(address[])
bbabad85: getStruct((uint256,uint8,address,address)[])
ed352282: getUDVT(uint256[])

Would be good to also add a similar one for scalar types (without mappings/arrays).

[cameel]

Looking at the output, there are some irregularities. It's odd that we decided to use the underlying type for UDVTs in libraries. We do not do this with any other user-defined type. Enums, contracts, interfaces, structs all use the user-provided name.

I wonder if this was even intentional. It contradicts what the documentation says about value types (Function Signatures and Selectors in Libraries):

Value types, non-storage string and non-storage bytes use the same identifiers as in the contract ABI.

It looks like a bug to me, but it's too late to fix it now. We have to live with it. Could you correct that in the documentation? After you do that, it would be best to explicitly mention that UDVTs behave differently.

[cameel]

The output also shows that your fix is incomplete. The types are not being unwrapped for mappings. That's probably because the assumption that user-provided name is used is hard-coded in multiple places. I suspect we'll need to special-case it for mappings. We should also think if there are cases other than mappings where it could happen.

[cameel]

Return parameters were unaffected since they are not a part of the signature, right?

Suggested change

	* Type System: Fix internal compiler error when using user-defined value types in storage arrays or mappings in external library functions.
	* Type System: Fix internal compiler error when defining external library functions accepting storage pointers to arrays/mappings of user-defined value types.

[cameel]

Test names should be more specific so that you can find what you're looking for without having to look inside. We have so many of them that it's becoming unmanageable otherwise.

storage_array_library.sol -> external_library_function_with_user_defined_storage_parameter.sol

For completeness, the test should also cover enums, contracts and interfaces. Or it could be a separate test, but then please call this one external_library_function_with_udvt_or_struct_storage_parameter.sol.

[cameel]

Thanks @chfast. LGTM, the external tests failures are unrelated ;)

@r0qs Please be careful when approving PRs that touch the ABI. The consequences of getting it wrong are extremely annoying due to our backwards-compatibility guarantees.