ICE on push to array with storage mapping

[erak]

Description

The following internal compiler error is thrown in the scenario below:

InternalCompilerError: Invalid non-value type for assignment.
            data.map.push(storageMapping);
            ^---------------------------^

Steps to Reproduce

contract C {
    struct Data {
        mapping(uint=>uint)[] map;
    }
    
    Data data;
    mapping(uint=>uint) storageMapping;
    
    constructor() public {
        for (uint i = 0; i < 6; i++)
            data.map.push(storageMapping);
    }
}

[leonardoalt]

A bit simplified:

contract C {
    mapping(uint=>uint)[] array;
    mapping(uint=>uint) map;
    function f() public {
        array.push(map);
    }
}

[dddejan]

If you manage to push the map to the array, popping it will cause another internal compiler error.

In general, supporting mappings within reference types seems to raise many issues and unexpected results. It would seem more consistent to just disallow it except as a top-level storage member, with no assignment and no delete.

[leonardoalt]

@dddejan the ICE when popping should have been fixed by #7431 (#7378) and released in 0.5.12. Do you still see that behavior?

[dddejan]

@leonardoalt Yes, it seem like no ICE i 0.5.12. I'm still not a big fan. The semantics are bizarre and will eventually get someone in trouble. Like example below (using resize instead for push).

contract C {

    mapping (address => int)[] m;

    function whats_going_on() public {
        // Put something in the array
        m.length = 1;
        m[0][msg.sender] = 1;
        // Get the reference to first elemnet
        mapping (address => int) storage ref = m[0]; 
        // Oh, let's pop the element
        m.pop();
        assert(m.length == 0);
        // Reference still works? 
        assert(ref[msg.sender] == 1); 
        // Seems so, let's put something in
        ref[msg.sender] = 2;
        ref[address(0)] = 3;
        // Let's put something in the array and see what's in the new mapping?
        m.length = 1;
        assert(m[0][msg.sender] == 2);
        assert(m[0][address(0)] == 3);
    }

}

[leonardoalt]

I agree that this is dangerous. The tests added in https://github.com/ethereum/solidity/pull/7431/files make this behavior explicit.
For now we added some security considerations in the documentation (https://solidity.readthedocs.io/en/v0.5.12/security-considerations.html#clearing-mappings) explaining this, and we're considering disallowing it.

[erak]

I'd also tend to re-think this behavior. Assigning to length will be disallowed with #7350 already.

[axic]

Still a problem:

/Users/alex/Projects/solidity/libsolidity/codegen/LValue.cpp(403): Throw in function virtual void solidity::frontend::StorageItem::storeValue(const solidity::frontend::Type &, const solidity::langutil::SourceLocation &, bool) const
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what: Invalid non-value type for assignment.
[solidity::util::tag_comment*] = Invalid non-value type for assignment.
[solidity::langutil::tag_sourceLocation*] = 7410.sol[213,242]

[chriseth]

Maybe related to #8278

[hrkrshnn]

This can be fixed in TypeChecker for ArrayPush operator. (State variable mappings cannot be assigned to.)

But I actually wonder if push makes sense for mapping types?

The following code should be valid:

contract C {
	mapping (uint => uint) map1;
	mapping (uint => mapping(uint => uint)[]) mapToMapArray;

	function f() public {
		mapping (uint => uint)[] storage maps = mapToMapArray[1];
		maps.push(map1);
	}
}

But there is an exception:

/solidity/libsolidity/codegen/LValue.cpp(400): Throw in function virtual void solidity::frontend::StorageItem::storeValue(const solidity::frontend::Type&, const solidity::langutil::SourceLocation&, bool) const
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what: Invalid non-value type for assignment.
[solidity::util::tag_comment*] = Invalid non-value type for assignment.
[solidity::langutil::tag_sourceLocation*] = /tmp/mapping.sol[187,202]

There are two three ways to proceed.

1. TypeChecker disallows when storage mapping types are assigned during a push. Implement push for local storage mapping type.
2. TypeChecker will disallow push to any mapping type.
3. Dissallow arrays with (nested) mapping types. Will need to check if external contracts uses this somehow.

What's a good fix for this? @chriseth @leonardoalt

[leonardoalt]

I thought the idea was to disallow arrays of mappings in general?

[chriseth]

@leonardoalt did we reach a conclusion there yet?

[leonardoalt]

@chriseth I thought so, but maybe it was just me wanting it :p

[cameel]

Related: #8535