More strict override check for data locations.

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::originalFunctionType() const
+{
+	return std::visit(GenericVisitor{
+		[&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); },
+		[&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specific 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,14 +598,17 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 
 	if (_super.isFunction())
 	{
-		FunctionType const* functionType = _overriding.functionType();
+		FunctionType const* functionType = _overriding.externalFunctionType();
-		FunctionType const* superType = _super.functionType();
+		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,
@@ -604,6 +616,36 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 					"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.originalFunctionType()->hasEqualParameterTypes(*_super.originalFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					7723_error,
+					"Data locations of parameters have to be the same when overriding non-external functions, but they differ.",
+					"Overridden " + _overriding.astNodeName() + " is here:"
+				);
+			if (!_overriding.originalFunctionType()->hasEqualReturnTypes(*_super.originalFunctionType()))
+				overrideError(
+					_overriding,
+					_super,
+					1443_error,
+					"Data locations of return variables have to be the same when overriding non-external functions, 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 @@ public:
 	/// @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* originalFunctionType() const;
 	ModifierType const* modifierType() const;
 
 	Declaration const* declaration() const;
@@ -101,6 +104,7 @@ public:
 
 	/**
 	 * 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

@@ -0,0 +1,18 @@
+abstract contract A {
+    function f(uint256[] calldata a) external virtual returns (uint256[] calldata);
+}
+
+contract B is A {
+    function f(uint256[] memory a) public override returns (uint256[] memory) {
+        return a;
+    }
+
+    function g(uint[] calldata x) public returns (uint256[] memory) {
+        return f(x);
+    }
+}
+// ====
+// compileViaYul: also
+// ----
+// f(uint256[]): 0x20, 2, 9, 8 -> 0x20, 2, 9, 8
+// g(uint256[]): 0x20, 2, 9, 8 -> 0x20, 2, 9, 8

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

@@ -0,0 +1,14 @@
+abstract contract A {
+    function f(uint256[1] memory a) external virtual returns (uint256);
+}
+contract B is A {
+    function f(uint256[1] calldata a) external pure virtual override returns (uint256) {
+        return a[0];
+    }
+}
+contract C is A, B {
+    function f(uint256[1] memory a) external pure override(B, A) returns (uint256) {
+        return a[0];
+    }
+}
+// ----

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

@@ -0,0 +1,13 @@
+abstract contract A {
+    modifier m(uint256[1] memory a) virtual;
+    function test(uint256[1] memory a) m(a) external {
+    }
+}
+
+contract B is A {
+    modifier m(uint256[1] calldata a) override {
+        _;
+    }
+}
+// ----
+// TypeError 1078: (153-214): Override changes modifier signature.

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

@@ -0,0 +1,11 @@
+abstract contract A {
+    function f(uint256[1] calldata a) public virtual returns (uint256);
+}
+
+contract B is A {
+    function f(uint256[1] memory a) public override returns (uint256) {
+        return a[0];
+    }
+}
+// ----
+// TypeError 7723: (119-213): Data locations of parameters have to be the same when overriding non-external functions, but they differ.

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

@@ -0,0 +1,16 @@
+abstract contract A {
+    function f(uint256[1] memory a) internal virtual returns (uint256);
+    function test() external returns (uint) {
+        uint[1] memory t;
+        t[0] = 7;
+        return f(t);
+    }
+}
+
+contract B is A {
+    function f(uint256[1] calldata a) internal override returns (uint256) {
+        return a[0];
+    }
+}
+// ----
+// TypeError 7723: (236-334): Data locations of parameters have to be the same when overriding non-external functions, but they differ.

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

@@ -0,0 +1,16 @@
+abstract contract A {
+    function f(uint256[1] memory a) public virtual returns (uint256);
+    function test() external returns (uint) {
+        uint[1] memory t;
+        t[0] = 7;
+        return f(t);
+    }
+}
+
+contract B is A {
+    function f(uint256[1] calldata a) public override returns (uint256) {
+        return a[0];
+    }
+}
+// ----
+// TypeError 7723: (234-330): Data locations of parameters have to be the same when overriding non-external functions, but they differ.

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

@@ -0,0 +1,7 @@
+abstract contract A {
+	function foo() external virtual view returns(uint[] calldata);
+}
+contract X is A {
+	function foo() public view override returns(uint[] memory) {  }
+}
+// ----

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

@@ -0,0 +1,8 @@
+abstract contract A {
+	function foo() public virtual view returns(uint[] calldata);
+}
+contract X is A {
+	function foo() public view override returns(uint[] memory) {  }
+}
+// ----
+// TypeError 1443: (105-168): Data locations of return variables have to be the same when overriding non-external functions, 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))
 				{