More strict override check for data locations.

Author: chriseth

Changelog.md

@@ -1,8 +1,5 @@
 ### 0.8.14 (unreleased)
 
-Important Bugfixes:
- * Type Checker: Disallow data location change between ``memory`` and ``calldata`` when overriding, except for external functions.
-
 Language Features:
 
 

libsolidity/analysis/ContractLevelChecker.cpp

@@ -39,7 +39,7 @@ namespace
 {
 
 template <class T, class B>
-bool hasEqualParameters(T const& _a, B const& _b)
+bool hasEqualExternalCallableParameters(T const& _a, B const& _b)
 {
 	return FunctionType(_a).asExternallyCallableFunction(false)->hasEqualParameterTypes(
 		*FunctionType(_b).asExternallyCallableFunction(false)
@@ -204,7 +204,7 @@ void ContractLevelChecker::findDuplicateDefinitions(map<string, vector<T>> const
 			SecondarySourceLocation ssl;
 
 			for (size_t j = i + 1; j < overloads.size(); ++j)
-				if (hasEqualParameters(*overloads[i], *overloads[j]))
+				if (hasEqualExternalCallableParameters(*overloads[i], *overloads[j]))
 				{
 					solAssert(
 						(

libsolidity/analysis/OverrideChecker.cpp

@@ -313,7 +313,7 @@ Token OverrideProxy::functionKind() const
 	}, m_item);
 }
 
-FunctionType const* OverrideProxy::functionType() const
+FunctionType const* OverrideProxy::externalFunctionType() const
 {
 	return std::visit(GenericVisitor{
 		[&](FunctionDefinition const* _item) { return FunctionType(*_item).asExternallyCallableFunction(false); },
@@ -322,6 +322,15 @@ FunctionType const* OverrideProxy::functionType() const
 	}, m_item);
 }
 
+FunctionType const* OverrideProxy::specificFunctionType() const
+{
+	return std::visit(GenericVisitor{
+		[&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); },
+		[&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specifig function type of variable."); return nullptr; },
+		[&](ModifierDefinition const*) -> FunctionType const* { solAssert(false, "Requested specific function type of modifier."); return nullptr; }
+	}, m_item);
+}
+
 ModifierType const* OverrideProxy::modifierType() const
 {
 	return std::visit(GenericVisitor{
@@ -413,7 +422,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
 			[&](FunctionDefinition const* _function)
 			{
 				vector<string> paramTypes;
-				for (Type const* t: functionType()->parameterTypes())
+				for (Type const* t: externalFunctionType()->parameterTypes())
 					paramTypes.emplace_back(t->richIdentifier());
 				return OverrideComparator{
 					_function->name(),
@@ -424,7 +433,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con
 			[&](VariableDeclaration const* _var)
 			{
 				vector<string> paramTypes;
-				for (Type const* t: functionType()->parameterTypes())
+				for (Type const* t: externalFunctionType()->parameterTypes())
 					paramTypes.emplace_back(t->richIdentifier());
 				return OverrideComparator{
 					_var->name(),
@@ -589,21 +598,54 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 
 	if (_super.isFunction())
 	{
-		FunctionType const* functionType = _overriding.functionType();
-		FunctionType const* superType = _super.functionType();
+		FunctionType const* functionType = _overriding.externalFunctionType();
+		FunctionType const* superType = _super.externalFunctionType();
 
+		bool returnTypesDifferAlready = false;
 		if (_overriding.functionKind() != Token::Fallback)
 		{
 			solAssert(functionType->hasEqualParameterTypes(*superType), "Override doesn't have equal parameters!");
 
 			if (!functionType->hasEqualReturnTypes(*superType))
+			{
+				returnTypesDifferAlready = true;
 				overrideError(
 					_overriding,
 					_super,
 					4822_error,
 					"Overriding " + _overriding.astNodeName() + " return types differ.",
 					"Overridden " + _overriding.astNodeName() + " is here:"
 				);
+			}
+		}
+
+		// The override proxy considers calldata and memory the same data location.
+		// Here we do a more specific check:
+		// Data locations of parameters and return variables have to match
+		// unless we have a public function overriding an external one.
+		if (
+			_overriding.isFunction() &&
+			!returnTypesDifferAlready &&
+			_super.visibility() != Visibility::External &&
+			_overriding.functionKind() != Token::Fallback
+		)
+		{
+			if (!_overriding.specificFunctionType()->hasEqualParameterTypes(*_super.specificFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					7723_error,
+					"Data locations of parameters have to be the same when overriding, but they differ.",
+					"Overridden " + _overriding.astNodeName() + " is here:"
+				);
+			if (!_overriding.specificFunctionType()->hasEqualReturnTypes(*_super.specificFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					1443_error,
+					"Data locations of return variables have to be the same when overriding, but they differ.",
+					"Overridden " + _overriding.astNodeName() + " is here:"
+				);
 		}
 
 		// Stricter mutability is always okay except when super is Payable

libsolidity/analysis/OverrideChecker.h

@@ -84,7 +84,10 @@ class OverrideProxy
 	/// @returns receive / fallback / function (only the latter for modifiers and variables);
 	langutil::Token functionKind() const;
 
-	FunctionType const* functionType() const;
+	/// @returns the externally callable function type
+	FunctionType const* externalFunctionType() const;
+	/// @returns the (unmodified) function type
+	FunctionType const* specificFunctionType() const;
 	ModifierType const* modifierType() const;
 
 	Declaration const* declaration() const;
@@ -101,6 +104,7 @@ class OverrideProxy
 
 	/**
 	 * Struct to help comparing override items about whether they override each other.
+	 * Compares functions based on their "externally callable" type.
 	 * Does not produce a total order.
 	 */
 	struct OverrideComparator

libsolidity/ast/AST.cpp

@@ -480,7 +480,9 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 	solAssert(isOrdinary(), "");
 	solAssert(!libraryFunction(), "");
 
-	FunctionType const* functionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
+	// We actually do not want the externally callable function here.
+	// This is just to add an assertion since the comparison used to be less strict.
+	FunctionType const* externalFunctionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
 
 	bool foundSearchStart = (_searchStart == nullptr);
 	for (ContractDefinition const* c: _mostDerivedContract.annotation().linearizedBaseContracts)
@@ -495,9 +497,12 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 				// With super lookup analysis guarantees that there is an implemented function in the chain.
 				// With virtual lookup there are valid cases where returning an unimplemented one is fine.
 				(function->isImplemented() || _searchStart == nullptr) &&
-				FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*functionType)
+				FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*externalFunctionType)
 			)
+			{
+				solAssert(FunctionType(*function).hasEqualParameterTypes(*TypeProvider::function(*this)));
 				return *function;
+			}
 	}
 
 	solAssert(false, "Virtual function " + name() + " not found.");

test/libsolidity/semanticTests/inheritance/dataLocation/external_public_calldata.sol

@@ -14,5 +14,5 @@ contract B is A {
 // ====
 // compileViaYul: also
 // ----
-// f(uint256[]): 0x20, 1, 9 -> 23
-// g(uint256[]): 0x20, 1, 9 -> 23
+// f(uint256[]): 0x20, 1, 9 -> 9
+// g(uint256[]): 0x20, 1, 9 -> 9

test/libsolidity/syntaxTests/inheritance/dataLocation/memory_calldata_internal.sol

@@ -13,5 +13,4 @@ contract B is A {
     }
 }
 // ----
-// TypeError 7792: (279-287): Function has override specified but does not override anything.
-// TypeError 3656: (214-336): Contract "B" should be marked as abstract.
+// TypeError 7723: (236-334): Data locations of parameters have to be the same when overriding, but they differ.

test/libsolidity/syntaxTests/inheritance/dataLocation/memory_calldata_public.sol

@@ -13,5 +13,4 @@ contract B is A {
     }
 }
 // ----
-// TypeError 7792: (275-283): Function has override specified but does not override anything.
-// TypeError 3656: (212-332): Contract "B" should be marked as abstract.
+// TypeError 7723: (234-330): Data locations of parameters have to be the same when overriding, but they differ.

tools/solidityUpgrade/Upgrade060.cpp

@@ -72,7 +72,7 @@ void OverridingFunction::endVisit(ContractDefinition const& _contract)
 			{
 				auto& super = (*begin);
 				auto functionType = FunctionType(*function).asExternallyCallableFunction(false);
-				auto superType = super.functionType()->asExternallyCallableFunction(false);
+				auto superType = super.externalFunctionType();
 
 				if (functionType && functionType->hasEqualParameterTypes(*superType))
 				{