An array or mapping being omitted from a struct returned by a getter should generate a hint/warning/error

[0xmichalis]

Description

Nested arrays inside structs are not included in the generated ABI. Original issue reported in the Hardhat repo at NomicFoundation/hardhat#2433

Environment

• Compiler version: 0.8.0
• Framework/IDE: Hardhat 2.8.4

Steps to Reproduce

After compiling the following contract, I don't see the integer array ids as part of the generated ABI in artifacts/contracts/Example.sol/Example.json, neither as part of the mapping mappedData nor in the top-level struct field data.

//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

contract Example {
    struct Data {
        uint256[] ids;
        uint256 createdAt;
        address user;
    }
    mapping (uint256 => Data) public mappedData;
    Data public data;
}

FWIW I have also tried with a different type of nested array ( address[]) and it also has the same issue so it doesn't look type-specific.

cc @alcuadrado

[alcuadrado]

The reason I think this is worth opening an issue is that it can be confusing for users. Maybe a warning can be printed when a struct element isn't included in the abi.

[0xmichalis]

@alcuadrado why wouldn't this get fixed at its source in the first place though? The generated ABI should include all public storage fields.

[alcuadrado]

Hey @Kargakis,

You are confusing what an ABI is. It's just a description of the binary encoding of inputs and outputs and other things that a smart contract understands/generate. Adding fields to the ABI will not make the contract start returning them, but it will make things worse instead.

[bfondevila]

@alcuadrado I see an inconsistent behaviour with return types on functions vs mappings, though.

That solidity code above generates the following ABI for the mappedData property, where the field "ids" is not considered a part of the output:

{
   "inputs": [
     {
       "internalType": "uint256",
       "name": "",
       "type": "uint256"
     }
   ],
   "name": "mappedData",
   "outputs": [
     {
       "internalType": "uint256",
       "name": "createdAt",
       "type": "uint256"
     },
     {
       "internalType": "address",
       "name": "user",
       "type": "address"
     }
   ],
   "stateMutability": "view",
   "type": "function"
}

I would expect to also be present the ABI definition this field:

{
   "internalType": "uint256[]",
   "name": "ids",
   "type": "uint256[]"
},

Basically, I would expect these two lines of code to produce a very similar ABI:

mapping(uint256 => Data) public mappedData;

function getData(uint256 index) external view returns (Data memory) {
    return mappedData[index];
}

Yet the first form does not include the field with an array type "ids" in the ABI, where the second does.

[bfondevila]

That being said, I just read the documentation stating that this is intentionally excluded:

https://docs.soliditylang.org/en/v0.8.17/contracts.html#getter-functions

The mapping and arrays (with the exception of byte arrays) in the struct are omitted because there is no good way to select individual struct members or provide a key for the mapping

If we care so much about having the arrays in structs on the ABI, we need to create a specific function in the contract to retrieve the struct like I mentioned before; I use hardhat and typechain and this way I can see that the EVM returns all the data in the array as expected:

function getData(uint256 index) external view returns (Data memory) {
    return mappedData[index];
}

Edit: I see that this is explicitly excluded in this piece of code in Types.cpp; I understand it is returned by the EVM but ignored by solidity, and explains why it is returned correctly in the struct return type in a function call but not as a mapping member type?

[bfondevila]

I wrote a small app using hardhat & typechain to show that the EVM does return the array inside the struct.
With a few modifications in the solidity compiler, I was able to compile this sample contract, produce the expected ABI, and confirm the value is returned correctly in this example:

https://github.com/bfondevila/solidity-fix-12863-test

I'd submit a merge request for the changes required in solidity, but I broke a few existing tests (and I'm sure functionality) with my changes; @alcuadrado if I can get confirmation that this issue is indeed within solidity, I'm happy to work on a more appropriate fix for this.

[alcuadrado]

@bfondevila, yes, this is a solidity/evm thing, and has little to do with Hardhat. We just save the abi as created by solc.

[pedrommaiaa]

As mentioned above, the compiler will omit the array:

[bfondevila]

I'll share here a commit with some of the areas that I identified as needing changes, but I am concerned about the implications it may have on performance, and not understanding the reason why they were initially left behind in the first place.

Seeing this code is many years old makes me think it just wasn't possible to implement back in the day, but the natural evolution of the compiler makes it possible now.

https://github.com/bfondevila/solidity/pull/1/files

[cameel]

Related issues: #12792, #11067.

This is not a bug, just an unfortunate "feature". It was motivated by the fact that returning a whole array is usually not what you want and may unintentionally lead to DoS. A getter would work at first but as your contract gathers more data and the array grows, it would at some point start running out of gas. In many cases you'll want to instead provide a function to access items at a specific index instead and this is what the compiler does when you declare a getter for an array. When the array is nested in a struct though, that's not as straightforward so the compiler omits it. And for mappings it's not even possible to return the whole mapping.

Note that it's like this just for getters, and those are just syntactic sugar over a state variable with an accessor function. If you have a better idea of how it should work in your contract, you define the function yourself and make it work however you want. There the compiler does not prevent you from returning the whole array if that's really what you want.

Having said that, I'm not a fan of this happening implicitly without any kind of warning or hint from the compiler. I think it's very confusing and unexpected. I think we should at the minimum issue an INFO message saying what is happening. Even better a warning. I'd even go as far as to make it an error, though that would be a breaking change. That would be in line with the general direction to remove such implicit behavior, similar to how we for example disallowed deleting structs with mappings (#8535), where originally the compiler would just silently ignore them.

So any opinions on making this a hint/warning/error?

[murderteeth]

Related issues: #12792, #11067.

This is not a bug, just an unfortunate "feature". It was motivated by the fact that returning a whole array is usually not what you want and may unintentionally lead to DoS. A getter would work at first but as your contract gathers more data and the array grows, it would at some point start running out of gas. In many cases you'll want to instead provide a function to access items at a specific index instead and this is what the compiler does when you declare a getter for an array. When the array is nested in a struct though, that's not as straightforward so the compiler omits it. And for mappings it's not even possible to return the whole mapping.

Note that it's like this just for getters, and those are just syntactic sugar over a state variable with an accessor function. If you have a better idea of how it should work in your contract, you define the function yourself and make it work however you want. There the compiler does not prevent you from returning the whole array if that's really what you want.

Having said that, I'm not a fan of this happening implicitly without any kind of warning or hint from the compiler. I think it's very confusing and unexpected. I think we should at the minimum issue an INFO message saying what is happening. Even better a warning. I'd even go as far as to make it an error, though that would be a breaking change. That would be in line with the general direction to remove such implicit behavior, similar to how we for example disallowed deleting structs with mappings (#8535), where originally the compiler would just silently ignore them.

So any opinions on making this a hint/warning/error?

Thanks for explaining! A warning makes sense to me here

[bfondevila]

Beautiful explanation, thanks! Perfectly understand the technical reasons behind the decision.

I am however against the idea of making this a warning or an error. Errors and warnings are prompts for me to go and fix my code, and this is not the solution here. Based on the explanation, this is unlikely to change in the future, so it would become a semi permanent warning that everyone will see.

Agreed that a hint at least is necessary to highlight the implicit behaviour - probably even linking to a more detailed explanation of why.

This is a good solution for now, and I'd love to even discuss the possibility of adding a descriptor of "ignored fields" in the ABI, to make it even more explicit and allow tools like hardhat to use that information to provide another touch point to see that behaviour. What do you think @alcuadrado ?

[murderteeth]

good point, ...so it would become a semi permanent warning that everyone will see

[cameel]

With the warning/error the idea is that such getters are inherently misleading and you should not have them. I.e. when you get the warning, you're better off switching from a getter to a function that either ignores the warning and returns the nested data as is (if you think it's safe in your case) or explicitly filters it out. This way the reader can see that there's more going on. The downside is of course that you'd probably need to define a separate type for the filtered version of the struct instead of the compiler doing that for you under the hood.

If you think that the current implicit behavior has its uses, then yeah, a hint is the most we can do. Personally, I think we would be better off with a warning or error, but curious to hear if there are people who do like this mechanism.

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

[benluelo]

hit this and spent way too long debugging just to find this issue, there should absolutely be a warning/ error as this is incredibly unexpected behaviour.