From fe12f05c080bd143cfb656f71e54d586d0810c4f Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 3 May 2018 19:21:42 +0200 Subject: Deprecate wildcard assignments. --- Changelog.md | 1 + libsolidity/analysis/TypeChecker.cpp | 57 +++++++++++++++++++++++++++++++----- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/Changelog.md b/Changelog.md index 7c4ac925..37e0b0a1 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,7 @@ Features: * Optimizer: Remove unnecessary masking of the result of known short instructions (``ADDRESS``, ``CALLER``, ``ORIGIN`` and ``COINBASE``). * Type Checker: Deprecate the ``years`` unit denomination and raise a warning for it (or an error as experimental 0.5.0 feature). * Type Checker: Make literals (without explicit type casting) an error for tight packing as experimental 0.5.0 feature. + * Type Checker: Warn about wildcard tuple assignments (this will turn into an error with version 0.5.0). Bugfixes: * Type Checker: Show proper error when trying to ``emit`` a non-event. diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 32cf1b18..a222bdf0 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -1076,6 +1076,7 @@ void TypeChecker::endVisit(EmitStatement const& _emit) bool TypeChecker::visit(VariableDeclarationStatement const& _statement) { + bool const v050 = m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050); if (!_statement.initialValue()) { // No initial value is only permitted for single variables with specified type. @@ -1092,7 +1093,7 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement) if (varDecl.referenceLocation() == VariableDeclaration::Location::Default) errorText += " Did you mean ' memory " + varDecl.name() + "'?"; solAssert(m_scope, ""); - if (m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050)) + if (v050) m_errorReporter.declarationError(varDecl.location(), errorText); else m_errorReporter.warning(varDecl.location(), errorText); @@ -1132,12 +1133,33 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement) ") in value for variable assignment (0) needed" ); } - else if (valueTypes.size() != variables.size() && !variables.front() && !variables.back()) - m_errorReporter.fatalTypeError( - _statement.location(), - "Wildcard both at beginning and end of variable declaration list is only allowed " - "if the number of components is equal." - ); + else if (valueTypes.size() != variables.size()) + { + if (v050) + m_errorReporter.fatalTypeError( + _statement.location(), + "Different number of components on the left hand side (" + + toString(variables.size()) + + ") than on the right hand side (" + + toString(valueTypes.size()) + + ")." + ); + else if (!variables.front() && !variables.back()) + m_errorReporter.fatalTypeError( + _statement.location(), + "Wildcard both at beginning and end of variable declaration list is only allowed " + "if the number of components is equal." + ); + else + m_errorReporter.warning( + _statement.location(), + "Different number of components on the left hand side (" + + toString(variables.size()) + + ") than on the right hand side (" + + toString(valueTypes.size()) + + ")." + ); + } size_t minNumValues = variables.size(); if (!variables.empty() && (!variables.back() || !variables.front())) --minNumValues; @@ -1335,6 +1357,7 @@ bool TypeChecker::visit(Conditional const& _conditional) bool TypeChecker::visit(Assignment const& _assignment) { + bool const v050 = m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050); requireLValue(_assignment.leftHandSide()); TypePointer t = type(_assignment.leftHandSide()); _assignment.annotation().type = t; @@ -1347,11 +1370,29 @@ bool TypeChecker::visit(Assignment const& _assignment) ); // Sequenced assignments of tuples is not valid, make the result a "void" type. _assignment.annotation().type = make_shared(); + expectType(_assignment.rightHandSide(), *tupleType); // expectType does not cause fatal errors, so we have to check again here. - if (dynamic_cast(type(_assignment.rightHandSide()).get())) + if (TupleType const* rhsType = dynamic_cast(type(_assignment.rightHandSide()).get())) + { checkDoubleStorageAssignment(_assignment); + // @todo For 0.5.0, this code shoud move to TupleType::isImplicitlyConvertibleTo, + // but we cannot do it right now. + if (rhsType->components().size() != tupleType->components().size()) + { + string message = + "Different number of components on the left hand side (" + + toString(tupleType->components().size()) + + ") than on the right hand side (" + + toString(rhsType->components().size()) + + ")."; + if (v050) + m_errorReporter.typeError(_assignment.location(), message); + else + m_errorReporter.warning(_assignment.location(), message); + } + } } else if (t->category() == Type::Category::Mapping) { -- cgit From 07e862a145c1b900c3bd0bf5d193c3bd8acc6c75 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 4 May 2018 16:10:25 +0200 Subject: Extract tests. --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 74 ---------------------- .../tupleAssignments/nowarn_swap_memory.sol | 8 +++ .../nowarn_swap_storage_pointers.sol | 10 +++ .../warn_multiple_storage_storage_copies.sol | 9 +++ ...n_multiple_storage_storage_copies_fill_left.sol | 10 +++ ..._multiple_storage_storage_copies_fill_right.sol | 10 +++ 6 files changed, 47 insertions(+), 74 deletions(-) create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index a2540302..5a8be8b8 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -5833,80 +5833,6 @@ BOOST_AUTO_TEST_CASE(pure_statement_check_for_regular_for_loop) CHECK_SUCCESS(text); } -BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies) -{ - char const* text = R"( - contract C { - struct S { uint a; uint b; } - S x; S y; - function f() public { - (x, y) = (y, x); - } - } - )"; - CHECK_WARNING(text, "This assignment performs two copies to storage."); -} - -BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_right) -{ - char const* text = R"( - contract C { - struct S { uint a; uint b; } - S x; S y; - function f() public { - (x, y, ) = (y, x, 1, 2); - } - } - )"; - CHECK_WARNING(text, "This assignment performs two copies to storage."); -} - -BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_left) -{ - char const* text = R"( - contract C { - struct S { uint a; uint b; } - S x; S y; - function f() public { - (,x, y) = (1, 2, y, x); - } - } - )"; - CHECK_WARNING(text, "This assignment performs two copies to storage."); -} - -BOOST_AUTO_TEST_CASE(nowarn_swap_memory) -{ - char const* text = R"( - contract C { - struct S { uint a; uint b; } - function f() pure public { - S memory x; - S memory y; - (x, y) = (y, x); - } - } - )"; - CHECK_SUCCESS_NO_WARNINGS(text); -} - -BOOST_AUTO_TEST_CASE(nowarn_swap_storage_pointers) -{ - char const* text = R"( - contract C { - struct S { uint a; uint b; } - S x; S y; - function f() public { - S storage x_local = x; - S storage y_local = y; - S storage z_local = x; - (x, y_local, x_local, z_local) = (y, x_local, y_local, y); - } - } - )"; - CHECK_SUCCESS_NO_WARNINGS(text); -} - BOOST_AUTO_TEST_CASE(warn_unused_local) { char const* text = R"( diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol new file mode 100644 index 00000000..b20bbf90 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol @@ -0,0 +1,8 @@ +contract C { + struct S { uint a; uint b; } + function f() pure public { + S memory x; + S memory y; + (x, y) = (y, x); + } +} diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol new file mode 100644 index 00000000..5f7a18f7 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol @@ -0,0 +1,10 @@ + contract C { + struct S { uint a; uint b; } + S x; S y; + function f() public { + S storage x_local = x; + S storage y_local = y; + S storage z_local = x; + (x, y_local, x_local, z_local) = (y, x_local, y_local, y); + } + } diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol new file mode 100644 index 00000000..e4c3e694 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol @@ -0,0 +1,9 @@ +contract C { + struct S { uint a; uint b; } + S x; S y; + function f() public { + (x, y) = (y, x); + } +} +// ---- +// Warning: (79-94): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first. diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol new file mode 100644 index 00000000..b2979804 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol @@ -0,0 +1,10 @@ +contract C { + struct S { uint a; uint b; } + S x; S y; + function f() public { + (,x, y) = (1, 2, y, x); + } +} +// ---- +// Warning: (79-101): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first. +// Warning: (79-101): Different number of components on the left hand side (3) than on the right hand side (4). diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol new file mode 100644 index 00000000..aa35d7d4 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol @@ -0,0 +1,10 @@ +contract C { + struct S { uint a; uint b; } + S x; S y; + function f() public { + (x, y, ) = (y, x, 1, 2); + } +} +// ---- +// Warning: (79-102): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first. +// Warning: (79-102): Different number of components on the left hand side (3) than on the right hand side (4). -- cgit From 43ec1699ba688336891065e45c2d02402619189a Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 4 May 2018 16:20:03 +0200 Subject: Remove deprecated syntax from a test --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 5a8be8b8..91fd1fff 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -2894,7 +2894,7 @@ BOOST_AUTO_TEST_CASE(dynamic_return_types_not_possible) contract C { function f(uint) public returns (string); function g() public { - var (x,) = this.f(2); + var x = this.f(2); // we can assign to x but it is not usable. bytes(x).length; } -- cgit From 8ee5d3b274112d064328aa673496dfb484b03531 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 4 May 2018 16:20:38 +0200 Subject: New tests for wildcard assignments. --- .../syntaxTests/tupleAssignments/error_fill.sol | 12 +++++++++++ .../tupleAssignments/large_component_count.sol | 24 ++++++++++++++++++++++ .../nowarn_explicit_singleton_token_expression.sol | 8 ++++++++ .../tupleAssignments/warn_fill_assignment.sol | 11 ++++++++++ .../tupleAssignments/warn_fill_vardecl.sol | 11 ++++++++++ 5 files changed, 66 insertions(+) create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol create mode 100644 test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol diff --git a/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol b/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol new file mode 100644 index 00000000..5b7f870b --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol @@ -0,0 +1,12 @@ +pragma experimental "v0.5.0"; +contract C { + function f() public pure returns (uint, uint, bytes32) { + uint a; + bytes32 b; + (a,) = f(); + (,b) = f(); + } +} +// ---- +// TypeError: (126-136): Different number of components on the left hand side (2) than on the right hand side (3). +// TypeError: (140-150): Different number of components on the left hand side (2) than on the right hand side (3). diff --git a/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol b/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol new file mode 100644 index 00000000..bbf21d7e --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol @@ -0,0 +1,24 @@ +pragma experimental "v0.5.0"; +contract C { + function g() public pure returns ( + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint, + uint + ) { } + function f() public pure returns (uint, uint, bytes32) { + uint a; + uint b; + (,,,,a,,,,b,,,,) = g(); + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol new file mode 100644 index 00000000..3262781b --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol @@ -0,0 +1,8 @@ +contract C { + function f() public pure { + uint a; + (a,) = (uint(1),); + } +} +// ---- +// Warning: (53-70): Different number of components on the left hand side (2) than on the right hand side (1). diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol new file mode 100644 index 00000000..a079a509 --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol @@ -0,0 +1,11 @@ +contract C { + function f() public pure returns (uint, uint, bytes32) { + uint a; + bytes32 b; + (a,) = f(); + (,b) = f(); + } +} +// ---- +// Warning: (96-106): Different number of components on the left hand side (2) than on the right hand side (3). +// Warning: (110-120): Different number of components on the left hand side (2) than on the right hand side (3). diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol new file mode 100644 index 00000000..1d243c7c --- /dev/null +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol @@ -0,0 +1,11 @@ +contract C { + function f() public pure returns (uint, uint, uint, uint) { + // Can later be replaced by (uint a, uint b,) = f(); + var (a,b,) = f(); + a; b; + } +} +// ---- +// Warning: (136-137): Use of the "var" keyword is deprecated. +// Warning: (138-139): Use of the "var" keyword is deprecated. +// Warning: (131-147): Different number of components on the left hand side (3) than on the right hand side (4). -- cgit From 741ada79f9fd58a3b652cf42de2a69cad54070b0 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 4 May 2018 16:27:21 +0200 Subject: Update documentation. --- docs/control-structures.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/control-structures.rst b/docs/control-structures.rst index f3c351dd..f18e1e10 100644 --- a/docs/control-structures.rst +++ b/docs/control-structures.rst @@ -293,12 +293,7 @@ Solidity internally allows tuple types, i.e. a list of objects of potentially di // Common trick to swap values -- does not work for non-value storage types. (x, y) = (y, x); // Components can be left out (also for variable declarations). - // If the tuple ends in an empty component, - // the rest of the values are discarded. - (data.length,) = f(); // Sets the length to 7 - // The same can be done on the left side. - // If the tuple begins in an empty component, the beginning values are discarded. - (,data[3]) = f(); // Sets data[3] to 2 + (data.length,,) = f(); // Sets the length to 7 // Components can only be left out at the left-hand-side of assignments, with // one exception: (x,) = (1,); @@ -307,6 +302,11 @@ Solidity internally allows tuple types, i.e. a list of objects of potentially di } } +.. note:: + Prior to version 0.4.24 it was possible to assign to tuples of smaller size, either + filling up on the left or on the right side (which ever was empty). This is + now deprecated, both sides have to have the same number of components. + Complications for Arrays and Structs ------------------------------------ -- cgit