fix: preserve None tuple components in the left and right liftings of…

… ternary expressions (#2120)

* preserve None tuple components in the left and right liftings of ternary expressions

* added a test for ternary lifting over tuples

---------

Co-authored-by: Kevin Clancy <[email protected]>

slither/utils/expression_manipulations.py

@@ -147,7 +147,7 @@ def convert_expressions(
         for next_expr in expression.expressions:
             # TODO: can we get rid of `NoneType` expressions in `TupleExpression`?
             # montyly: this might happen with unnamed tuple (ex: (,,,) = f()), but it needs to be checked
-            if next_expr:
+            if next_expr is not None:
 
                 if self.conditional_not_ahead(
                     next_expr, true_expression, false_expression, f_expressions
@@ -158,6 +158,9 @@ def convert_expressions(
                         true_expression.expressions[-1],
                         false_expression.expressions[-1],
                     )
+            else:
+                true_expression.expressions.append(None)
+                false_expression.expressions.append(None)
 
     def convert_index_access(
         self, next_expr: IndexAccess, true_expression: Expression, false_expression: Expression

tests/unit/slithir/test_data/ternary_expressions.sol

@@ -1,6 +1,6 @@
 interface Test {
     function test() external payable returns (uint);
-    function testTuple() external payable returns (uint, uint);
+    function testTuple(uint) external payable returns (uint, uint);
 }
 contract C {
     // TODO
@@ -36,21 +36,23 @@ contract C {
     }
 
     // Unused tuple variable
-    function g(address one) public {
-        (, uint x) = Test(one).testTuple();
-    }
-
     uint[] myIntegers;
-    function _h(uint c) internal returns(uint) {
-        return c;
-    }
-    function h(bool cond, uint a, uint b) public {
-        uint d = _h(
-            myIntegers[cond ? a : b]
-        );
+    function g(address one, bool cond, uint a, uint b) public {
+        (, uint x) = Test(one).testTuple(myIntegers[cond ? a : b]);
     }
-    
-    function i(bool cond) public {
+
+    function h(bool cond) public {
         bytes memory a = new bytes(cond ? 1 : 2);
     }
 }
+
+contract D {
+    function values(uint n) internal returns (uint, uint) {
+        return (0, 1);
+    }
+
+    function a(uint n) external {
+        uint a;
+        (a,) = values(n > 0 ? 1 : 0);
+    }
+}

tests/unit/slithir/test_ternary_expressions.py

@@ -16,30 +16,53 @@ def test_ternary_conversions(solc_binary_path) -> None:
     """This tests that true and false sons define the same number of variables that the father node declares"""
     solc_path = solc_binary_path("0.8.0")
     slither = Slither(Path(TEST_DATA_DIR, "ternary_expressions.sol").as_posix(), solc=solc_path)
-    for contract in slither.contracts:
-        if not contract.is_signature_only:
-            for function in contract.functions:
-                vars_declared = 0
-                vars_assigned = 0
-                for node in function.nodes:
-                    if node.type in [NodeType.IF, NodeType.IFLOOP]:
-
-                        # Iterate over true and false son
-                        for inner_node in node.sons:
-                            # Count all variables declared
-                            expression = inner_node.expression
-                            if isinstance(
-                                expression, (AssignmentOperation, NewElementaryType, CallExpression)
-                            ):
-                                var_expr = expression.expression_left
-                                # Only tuples declare more than one var
-                                if isinstance(var_expr, TupleExpression):
-                                    vars_declared += len(var_expr.expressions)
-                                else:
-                                    vars_declared += 1
-
-                            for ir in inner_node.irs:
-                                # Count all variables defined
-                                if isinstance(ir, (Assignment, Unpack)):
-                                    vars_assigned += 1
-                assert vars_declared == vars_assigned and vars_assigned != 0
+    contract = next(c for c in slither.contracts if c.name == "C")
+    for function in contract.functions:
+        vars_declared = 0
+        vars_assigned = 0
+        for node in function.nodes:
+            if node.type in [NodeType.IF, NodeType.IFLOOP]:
+
+                # Iterate over true and false son
+                for inner_node in node.sons:
+                    # Count all variables declared
+                    expression = inner_node.expression
+                    if isinstance(
+                        expression, (AssignmentOperation, NewElementaryType, CallExpression)
+                    ):
+                        var_expr = expression.expression_left
+                        # Only tuples declare more than one var
+                        if isinstance(var_expr, TupleExpression):
+                            vars_declared += len(var_expr.expressions)
+                        else:
+                            vars_declared += 1
+
+                    for ir in inner_node.irs:
+                        # Count all variables defined
+                        if isinstance(ir, (Assignment, Unpack)):
+                            vars_assigned += 1
+        assert vars_declared == vars_assigned and vars_assigned != 0
+
+
+def test_ternary_tuple(solc_binary_path) -> None:
+    """
+    Test that in the ternary liftings of an assignment of the form `(z, ) = ...`,
+    we obtain `z` from an unpack operation in both lifitings
+    """
+    solc_path = solc_binary_path("0.8.0")
+    slither = Slither(Path(TEST_DATA_DIR, "ternary_expressions.sol").as_posix(), solc=solc_path)
+    contract = next(c for c in slither.contracts if c.name == "D")
+    fn = next(f for f in contract.functions if f.name == "a")
+
+    if_nodes = [n for n in fn.nodes if n.type == NodeType.IF]
+    assert len(if_nodes) == 1
+
+    if_node = if_nodes[0]
+    assert isinstance(if_node.son_true.expression, AssignmentOperation)
+    assert (
+        len([ir for ir in if_node.son_true.all_slithir_operations() if isinstance(ir, Unpack)]) == 1
+    )
+    assert (
+        len([ir for ir in if_node.son_false.all_slithir_operations() if isinstance(ir, Unpack)])
+        == 1
+    )