solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Add InlineArrayType to support literals conversion to statically and dynamically allocated arrays.

Open wechman opened this issue 3 years ago • 12 comments

fixes #11879 fixes #13085

This is an early version of the changes needed to support literals conversion to dynamic arrays. Definitely it is not ready to review yet.

Things we should not forget to test:

  • assigning to storage
  • x = [x[1], x[0]], where x is a multi-dimensional storage array

wechman avatar Mar 31 '22 10:03 wechman

Interestingly, at a quick glance, the optimizer seems to be handling this rather well even for larger-than-16-elements-arrays (without having to use stack-to-memory).

ekpyron avatar Apr 06 '22 09:04 ekpyron

Random idea for a syntax test on this:

function f(uint256[] memory x) {}
function f(uint256[3] memory x) {}
function g() {
  f([1]); // should work
  f([1,2,3]); // should fail (no unique function)
  f([1,2,3,4]); // should work
}

This should work already, but good to have a test for it.

ekpyron avatar Apr 06 '22 10:04 ekpyron

Please note that I think it makes sense to re-unify this with tuple type again. For now, it helps to find places where it is used as an inline array, but in the end, they types are very similar, especially when we keep just putting things on the stack.

chriseth avatar Apr 11 '22 11:04 chriseth

There was an error when running chk_coding_style for commit 54094d5cbfd1145c175c030f1a11296570e3015d:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/array/indexAccess/inline_array_with_utf8_sequence.sol:1:contract C { 
test/libsolidity/syntaxTests/inline_array_with_invalid_utf8_sequence.sol:1:contract C { 

Please check that your changes are working as intended.

stackenbotten avatar May 04 '22 11:05 stackenbotten

There was an error when running chk_coding_style for commit dd1de0952203e1ae1004589250e449be36826cfb:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/array/indexAccess/inline_array_with_utf8_sequence.sol:1:contract C { 
test/libsolidity/syntaxTests/inline_array_with_invalid_utf8_sequence.sol:1:contract C { 

Please check that your changes are working as intended.

stackenbotten avatar May 06 '22 13:05 stackenbotten

There was an error when running chk_coding_style for commit 7bea6a97cba902a3ba57cfab66f808b2927e7f04:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/array/inline_array_to_storage_assignment.sol:3:    uint8[] public dt = [4, 5, 6]; 

Please check that your changes are working as intended.

stackenbotten avatar May 16 '22 08:05 stackenbotten

There was an error when running chk_coding_style for commit b0be0e7e33efa679173c68b5f6dc7be829dcd30c:

Error: Trailing whitespace found:
test/libsolidity/semanticTests/array/inline_array_to_storage_assignment.sol:3:    uint8[] public dt = [4, 5, 6]; 

Please check that your changes are working as intended.

stackenbotten avatar May 18 '22 07:05 stackenbotten

There was an error when running chk_coding_style for commit a9fd1feb2759d4a8610be913f129ba4c1ca1a874:

Error: Trailing whitespace found:
test/libsolidity/syntaxTests/conversion/implicit_conversion_from_array_of_string_literals_to_calldata_string.sol:9:// TypeError 6359: (122-147): Return argument type inline_array(literal_string "h", literal_string "e", literal_string "l", literal_string "l", literal_string "o") is not implicitly convertible to expected type (type of first return variable) string calldata[5] calldata. Invalid conversion from literal_string "h" to string calldata. 
test/libsolidity/syntaxTests/inline_arrays/invalid_types_in_inline_array.sol:7:// TypeError 9574: (47-83): Type inline_array(int_const 45, literal_string "foo", bool) is not implicitly convertible to expected type uint256[3] memory. Invalid conversion from literal_string "foo" to uint256. 

Please check that your changes are working as intended.

stackenbotten avatar May 27 '22 06:05 stackenbotten

There was an error when running chk_coding_style for commit a9fd1feb2759d4a8610be913f129ba4c1ca1a874:

Error: Trailing whitespace found:
test/libsolidity/syntaxTests/conversion/implicit_conversion_from_array_of_string_literals_to_calldata_string.sol:9:// TypeError 6359: (122-147): Return argument type inline_array(literal_string "h", literal_string "e", literal_string "l", literal_string "l", literal_string "o") is not implicitly convertible to expected type (type of first return variable) string calldata[5] calldata. Invalid conversion from literal_string "h" to string calldata. 
test/libsolidity/syntaxTests/inline_arrays/invalid_types_in_inline_array.sol:7:// TypeError 9574: (47-83): Type inline_array(int_const 45, literal_string "foo", bool) is not implicitly convertible to expected type uint256[3] memory. Invalid conversion from literal_string "foo" to uint256. 

Please check that your changes are working as intended.

stackenbotten avatar May 31 '22 11:05 stackenbotten

We should also have test cases for the following:

contract C {
        function f() public pure {
                uint8[3][] memory x = new uint8[3][](1);
                x[0] = [1,2,3];
        }
}

and

contract C {
        function f() public pure {
                uint8[][] memory x = new uint8[][](1);
                x[0] = [1,2,3];
        }
}

The former used to work before this PR, but I think the IR codegen LValue code still needs to be adjusted for this to work again.

ekpyron avatar Jun 08 '22 08:06 ekpyron

We should also have test cases for the following:

contract C {
        function f() public pure {
                uint8[3][] memory x = new uint8[3][](1);
                x[0] = [1,2,3];
        }
}

and

contract C {
        function f() public pure {
                uint8[][] memory x = new uint8[][](1);
                x[0] = [1,2,3];
        }
}

The former used to work before this PR, but I think the IR codegen LValue code still needs to be adjusted for this to work again.

Good point! Test cases added and code updated.

wechman avatar Jun 10 '22 06:06 wechman

Summary of the optimizer problems: There should be an issue about it but I currently cannot find it. The problem is the memory tracking inside DataFlowAnalyzer. For every memory write, it has to check if the memory write would overwrite any other memory state it has stored inside m_state.memory. If the memory mapping stays large, this can be quite expensive. Two ways to overcome this come to my mind right now:

  • only make use of the memory mapping if the optimizer step deriving from DataFlowAnalyzer actually uses it
  • limit the size of the memory mapping

chriseth avatar Jul 05 '22 08:07 chriseth

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

github-actions[bot] avatar Nov 17 '22 12:11 github-actions[bot]

This pull request was closed due to a lack of activity for 7 days after it was stale.

github-actions[bot] avatar Nov 24 '22 12:11 github-actions[bot]

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.

github-actions[bot] avatar Nov 24 '22 16:11 github-actions[bot]

Closing for now.

NunoFilipeSantos avatar Feb 06 '23 15:02 NunoFilipeSantos