Cleaner shift handling and type conversion for binary operations.

4 files changed, 122 insertions, 78 deletions

diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp
index 896d51fa..03ff8471 100644
--- a/libsolidity/ast/Types.cpp
+++ b/libsolidity/ast/Types.cpp
@@ -251,6 +251,19 @@ MemberList::MemberMap Type::boundFunctions(Type const& _type, ContractDefinition
 	return members;
 }
 
+bool isValidShiftAndAmountType(Token::Value _operator, Type const& _shiftAmountType)
+{
+	// Disable >>> here.
+	if (_operator == Token::SHR)
+		return false;
+	else if (IntegerType const* otherInt = dynamic_cast<decltype(otherInt)>(&_shiftAmountType))
+		return !otherInt->isAddress();
+	else if (RationalNumberType const* otherRat = dynamic_cast<decltype(otherRat)>(&_shiftAmountType))
+		return otherRat->integerType() && !otherRat->integerType()->isSigned();
+	else
+		return false;
+}
+
 IntegerType::IntegerType(int _bits, IntegerType::Modifier _modifier):
 	m_bits(_bits), m_modifier(_modifier)
 {
@@ -342,24 +355,13 @@ TypePointer IntegerType::binaryOperatorResult(Token::Value _operator, TypePointe
 		return TypePointer();
 	if (Token::isShiftOp(_operator))
 	{
-		// Disable >>> here.
-		if (_operator == Token::SHR)
-			return TypePointer();
-
 		// Shifts are not symmetric with respect to the type
 		if (isAddress())
 			return TypePointer();
-		if (IntegerType const* otherInt = dynamic_cast<decltype(otherInt)>(_other.get()))
-		{
-			if (!otherInt->isAddress())
-				return shared_from_this();
-		}
-		else if (RationalNumberType const* otherRat = dynamic_cast<decltype(otherRat)>(_other.get()))
-		{
-			if (!otherRat->isFractional())
-				return shared_from_this();
-		}
-		return TypePointer();
+		if (isValidShiftAndAmountType(_operator, *_other))
+			return shared_from_this();
+		else
+			return TypePointer();
 	}
 
 	auto commonType = Type::commonType(shared_from_this(), _other); //might be a integer or fixed point
@@ -978,22 +980,10 @@ TypePointer FixedBytesType::binaryOperatorResult(Token::Value _operator, TypePoi
 {
 	if (Token::isShiftOp(_operator))
 	{
-		// Disable >>> here.
-		if (_operator == Token::SHR)
+		if (isValidShiftAndAmountType(_operator, *_other))
+			return shared_from_this();
+		else
 			return TypePointer();
-
-		// Shifts are not symmetric with respect to the type
-		if (IntegerType const* otherInt = dynamic_cast<decltype(otherInt)>(_other.get()))
-		{
-			if (!otherInt->isAddress())
-				return shared_from_this();
-		}
-		else if (RationalNumberType const* otherRat = dynamic_cast<decltype(otherRat)>(_other.get()))
-		{
-			if (!otherRat->isFractional())
-				return shared_from_this();
-		}
-		return TypePointer();
 	}
 
 	auto commonType = dynamic_pointer_cast<FixedBytesType const>(Type::commonType(shared_from_this(), _other));
diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp
index d28ffed8..afe91c5c 100644
--- a/libsolidity/codegen/ExpressionCompiler.cpp
+++ b/libsolidity/codegen/ExpressionCompiler.cpp
@@ -197,22 +197,37 @@ bool ExpressionCompiler::visit(Conditional const& _condition)
 bool ExpressionCompiler::visit(Assignment const& _assignment)
 {
 	CompilerContext::LocationSetter locationSetter(m_context, _assignment);
+	Token::Value op = _assignment.assignmentOperator();
+	Token::Value binOp = op == Token::Assign ? op : Token::AssignmentToBinaryOp(op);
+	Type const& leftType = *_assignment.leftHandSide().annotation().type;
+	if (leftType.category() == Type::Category::Tuple)
+	{
+		solAssert(*_assignment.annotation().type == TupleType(), "");
+		solAssert(op == Token::Assign, "");
+	}
+	else
+		solAssert(*_assignment.annotation().type == leftType, "");
+	bool cleanupNeeded = false;
+	if (op != Token::Assign)
+		cleanupNeeded = cleanupNeededForOp(leftType.category(), binOp);
 	_assignment.rightHandSide().accept(*this);
 	// Perform some conversion already. This will convert storage types to memory and literals
 	// to their actual type, but will not convert e.g. memory to storage.
-	TypePointer type = _assignment.rightHandSide().annotation().type->closestTemporaryType(
-		_assignment.leftHandSide().annotation().type
-	);
-	utils().convertType(*_assignment.rightHandSide().annotation().type, *type);
+	TypePointer rightIntermediateType;
+	if (op != Token::Assign && Token::isShiftOp(binOp))
+		rightIntermediateType = _assignment.rightHandSide().annotation().type->mobileType();
+	else
+		rightIntermediateType = _assignment.rightHandSide().annotation().type->closestTemporaryType(
+			_assignment.leftHandSide().annotation().type
+		);
+	utils().convertType(*_assignment.rightHandSide().annotation().type, *rightIntermediateType, cleanupNeeded);
 
 	_assignment.leftHandSide().accept(*this);
 	solAssert(!!m_currentLValue, "LValue not retrieved.");
 
-	Token::Value op = _assignment.assignmentOperator();
 	if (op != Token::Assign) // compound assignment
 	{
-		Token::Value target_op = Token::AssignmentToBinaryOp(op);
-		solUnimplementedAssert(_assignment.annotation().type->isValueType(), "Compound operators not implemented for non-value types.");
+		solAssert(leftType.isValueType(), "Compound operators only available for value types.");
 		unsigned lvalueSize = m_currentLValue->sizeOnStack();
 		unsigned itemSize = _assignment.annotation().type->sizeOnStack();
 		if (lvalueSize > 0)
@@ -222,11 +237,17 @@ bool ExpressionCompiler::visit(Assignment const& _assignment)
 			// value lvalue_ref value lvalue_ref
 		}
 		m_currentLValue->retrieveValue(_assignment.location(), true);
-		if (Token::isShiftOp(target_op))
-			// shift only cares about the signedness of both sides
-			appendShiftOperatorCode(target_op, *_assignment.leftHandSide().annotation().type, *_assignment.rightHandSide().annotation().type);
+		utils().convertType(leftType, leftType, cleanupNeeded);
+
+		Token::Value targetOp = Token::AssignmentToBinaryOp(op);
+
+		if (Token::isShiftOp(targetOp))
+			appendShiftOperatorCode(targetOp, leftType, *rightIntermediateType);
 		else
-			appendOrdinaryBinaryOperatorCode(target_op, *_assignment.annotation().type);
+		{
+			solAssert(leftType == *rightIntermediateType, "");
+			appendOrdinaryBinaryOperatorCode(targetOp, leftType);
+		}
 		if (lvalueSize > 0)
 		{
 			solAssert(itemSize + lvalueSize <= 16, "Stack too deep, try removing local variables.");
@@ -235,7 +256,7 @@ bool ExpressionCompiler::visit(Assignment const& _assignment)
 				m_context << swapInstruction(itemSize + lvalueSize) << Instruction::POP;
 		}
 	}
-	m_currentLValue->storeValue(*type, _assignment.location());
+	m_currentLValue->storeValue(*rightIntermediateType, _assignment.location());
 	m_currentLValue.reset();
 	return false;
 }
@@ -356,20 +377,19 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
 	Expression const& leftExpression = _binaryOperation.leftExpression();
 	Expression const& rightExpression = _binaryOperation.rightExpression();
 	solAssert(!!_binaryOperation.annotation().commonType, "");
-	Type const& commonType = *_binaryOperation.annotation().commonType;
+	TypePointer const& commonType = _binaryOperation.annotation().commonType;
 	Token::Value const c_op = _binaryOperation.getOperator();
 
 	if (c_op == Token::And || c_op == Token::Or) // special case: short-circuiting
 		appendAndOrOperatorCode(_binaryOperation);
-	else if (commonType.category() == Type::Category::RationalNumber)
-		m_context << commonType.literalValue(nullptr);
+	else if (commonType->category() == Type::Category::RationalNumber)
+		m_context << commonType->literalValue(nullptr);
 	else
 	{
-		bool cleanupNeeded = false;
-		if (Token::isCompareOp(c_op) || Token::isShiftOp(c_op))
-			cleanupNeeded = true;
-		if (commonType.category() == Type::Category::Integer && (c_op == Token::Div || c_op == Token::Mod))
-			cleanupNeeded = true;
+		bool cleanupNeeded = cleanupNeededForOp(commonType->category(), c_op);
+
+		TypePointer leftTargetType = commonType;
+		TypePointer rightTargetType = Token::isShiftOp(c_op) ? rightExpression.annotation().type->mobileType() : commonType;
 
 		// for commutative operators, push the literal as late as possible to allow improved optimization
 		auto isLiteral = [](Expression const& _e)
@@ -380,24 +400,24 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
 		if (swap)
 		{
 			leftExpression.accept(*this);
-			utils().convertType(*leftExpression.annotation().type, commonType, cleanupNeeded);
+			utils().convertType(*leftExpression.annotation().type, *leftTargetType, cleanupNeeded);
 			rightExpression.accept(*this);
-			utils().convertType(*rightExpression.annotation().type, commonType, cleanupNeeded);
+			utils().convertType(*rightExpression.annotation().type, *rightTargetType, cleanupNeeded);
 		}
 		else
 		{
 			rightExpression.accept(*this);
-			utils().convertType(*rightExpression.annotation().type, commonType, cleanupNeeded);
+			utils().convertType(*rightExpression.annotation().type, *rightTargetType, cleanupNeeded);
 			leftExpression.accept(*this);
-			utils().convertType(*leftExpression.annotation().type, commonType, cleanupNeeded);
+			utils().convertType(*leftExpression.annotation().type, *leftTargetType, cleanupNeeded);
 		}
 		if (Token::isShiftOp(c_op))
 			// shift only cares about the signedness of both sides
-			appendShiftOperatorCode(c_op, *leftExpression.annotation().type, *rightExpression.annotation().type);
+			appendShiftOperatorCode(c_op, *leftTargetType, *rightTargetType);
 		else if (Token::isCompareOp(c_op))
-			appendCompareOperatorCode(c_op, commonType);
+			appendCompareOperatorCode(c_op, *commonType);
 		else
-			appendOrdinaryBinaryOperatorCode(c_op, commonType);
+			appendOrdinaryBinaryOperatorCode(c_op, *commonType);
 	}
 
 	// do not visit the child nodes, we already did that explicitly
@@ -1396,30 +1416,31 @@ void ExpressionCompiler::appendBitOperatorCode(Token::Value _operator)
 	}
 }
 
-void ExpressionCompiler::appendShiftOperatorCode(Token::Value _operator, Type const& _leftType, Type const& _rightType)
+void ExpressionCompiler::appendShiftOperatorCode(Token::Value _operator, Type const& _valueType, Type const& _shiftAmountType)
 {
-	// stack: rvalue lvalue
+	// stack: shift_amount value_to_shift
 
-	bool c_leftSigned = false;
-	if (auto leftType = dynamic_cast<IntegerType const*>(&_leftType))
-		c_leftSigned = leftType->isSigned();
+	bool c_valueSigned = false;
+	if (auto valueType = dynamic_cast<IntegerType const*>(&_valueType))
+		c_valueSigned = valueType->isSigned();
 	else
-		solUnimplemented("Only IntegerType can be shifted.");
+		solAssert(dynamic_cast<FixedBytesType const*>(&_valueType), "Only integer and fixed bytes type supported for shifts.");
 
-	// The RValue can be a RationalNumberType too.
-	bool c_rightSigned = false;
-	if (auto rightType = dynamic_cast<RationalNumberType const*>(&_rightType))
+	// The amount can be a RationalNumberType too.
+	bool c_amountSigned = false;
+	if (auto amountType = dynamic_cast<RationalNumberType const*>(&_shiftAmountType))
 	{
-		solAssert(rightType->integerType(), "integerType() called for fractional number.");
-		c_rightSigned = rightType->integerType()->isSigned();
+		// This should be handled by the type checker.
+		solAssert(amountType->integerType(), "");
+		solAssert(!amountType->integerType()->isSigned(), "");
 	}
-	else if (auto rightType = dynamic_cast<IntegerType const*>(&_rightType))
-		c_rightSigned = rightType->isSigned();
+	else if (auto amountType = dynamic_cast<IntegerType const*>(&_shiftAmountType))
+		c_amountSigned = amountType->isSigned();
 	else
-		solUnimplemented("Not implemented yet - FixedPointType.");
+		solAssert(false, "Invalid shift amount type.");
 
-	// shift with negative rvalue throws exception
-	if (c_rightSigned)
+	// shift by negative amount throws exception
+	if (c_amountSigned)
 	{
 		m_context << u256(0) << Instruction::DUP3 << Instruction::SLT;
 		m_context.appendConditionalJumpTo(m_context.errorTag());
@@ -1431,7 +1452,7 @@ void ExpressionCompiler::appendShiftOperatorCode(Token::Value _operator, Type co
 		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::MUL;
 		break;
 	case Token::SAR:
-		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::SWAP1 << (c_leftSigned ? Instruction::SDIV : Instruction::DIV);
+		m_context << Instruction::SWAP1 << u256(2) << Instruction::EXP << Instruction::SWAP1 << (c_valueSigned ? Instruction::SDIV : Instruction::DIV);
 		break;
 	case Token::SHR:
 	default:
@@ -1721,6 +1742,16 @@ void ExpressionCompiler::setLValueToStorageItem(Expression const& _expression)
 	setLValue<StorageItem>(_expression, *_expression.annotation().type);
 }
 
+bool ExpressionCompiler::cleanupNeededForOp(Type::Category _type, Token::Value _op)
+{
+	if (Token::isCompareOp(_op) || Token::isShiftOp(_op))
+		return true;
+	else if (_type == Type::Category::Integer && (_op == Token::Div || _op == Token::Mod))
+		return true;
+	else
+		return false;
+}
+
 CompilerUtils ExpressionCompiler::utils()
 {
 	return CompilerUtils(m_context);
diff --git a/libsolidity/codegen/ExpressionCompiler.h b/libsolidity/codegen/ExpressionCompiler.h
index e6cf382c..d0a8ac15 100644
--- a/libsolidity/codegen/ExpressionCompiler.h
+++ b/libsolidity/codegen/ExpressionCompiler.h
@@ -91,7 +91,7 @@ private:
 
 	void appendArithmeticOperatorCode(Token::Value _operator, Type const& _type);
 	void appendBitOperatorCode(Token::Value _operator);
-	void appendShiftOperatorCode(Token::Value _operator, Type const& _leftType, Type const& _rightType);
+	void appendShiftOperatorCode(Token::Value _operator, Type const& _valueType, Type const& _shiftAmountType);
 	/// @}
 
 	/// Appends code to call a function of the given type with the given arguments.
@@ -117,6 +117,10 @@ private:
 	template <class _LValueType, class... _Arguments>
 	void setLValue(Expression const& _expression, _Arguments const&... _arguments);
 
+	/// @returns true if the operator applied to the given type requires a cleanup prior to the
+	/// operation.
+	bool cleanupNeededForOp(Type::Category _type, Token::Value _op);
+
 	/// @returns the CompilerUtils object containing the current context.
 	CompilerUtils utils();
 
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 0ac88a81..3d38516d 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -8507,17 +8507,18 @@ BOOST_AUTO_TEST_CASE(shift_left_uint8)
 
 BOOST_AUTO_TEST_CASE(shift_left_larger_type)
 {
+	// This basically tests proper cleanup and conversion. It should not convert x to int8.
 	char const* sourceCode = R"(
 		contract C {
 			function f() returns (int8) {
-				uint8 x = 255;
+				uint8 x = 254;
 				int8 y = 1;
-				return a << b;
+				return y << x;
 			}
 		}
 	)";
 	compileAndRun(sourceCode, 0, "C");
-	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(1) << 255));
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
 }
 
 BOOST_AUTO_TEST_CASE(shift_left_assignment)
@@ -8570,6 +8571,7 @@ BOOST_AUTO_TEST_CASE(shift_right_garbled)
 	)";
 	compileAndRun(sourceCode, 0, "C");
 	BOOST_CHECK(callContractFunction("f(uint256,uint256)", u256(0x0), u256(4)) == encodeArgs(u256(0xf)));
+	BOOST_CHECK(callContractFunction("f(uint256,uint256)", u256(0x0), u256(0x1004)) == encodeArgs(u256(0xf)));
 }
 
 BOOST_AUTO_TEST_CASE(shift_right_uint32)
@@ -8769,6 +8771,23 @@ BOOST_AUTO_TEST_CASE(shift_overflow)
 	BOOST_CHECK(callContractFunction("leftS(int8,int8)", 1, 6) == encodeArgs(u256(64)));
 }
 
+BOOST_AUTO_TEST_CASE(cleanup_in_compound_assign)
+{
+	char const* sourceCode = R"(
+		contract C {
+			function test() returns (uint, uint) {
+				uint32 a = 0xffffffff;
+				uint16 x = uint16(a);
+				uint16 y = x;
+				x /= 0x100;
+				return (x, y);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("test()") == encodeArgs(u256(0xff), u256(0xff)));
+}
+
 BOOST_AUTO_TEST_CASE(inline_assembly_in_modifiers)
 {
 	char const* sourceCode = R"(