Fix #11889 - private functions should not be overridden by other contracts (rebase)

libsolidity/analysis/OverrideChecker.cpp

@@ -933,7 +933,7 @@ OverrideChecker::OverrideProxyBySignatureMultiSet const& OverrideChecker::inheri
 		{
 			set<OverrideProxy, OverrideProxy::CompareBySignature> functionsInBase;
 			for (FunctionDefinition const* fun: base->definedFunctions())
-				if (!fun->isConstructor())
+				if (!fun->isConstructor() && fun->visibility() != Visibility::Private)
 					functionsInBase.emplace(OverrideProxy{fun});
 			for (VariableDeclaration const* var: base->stateVariables())
 				if (var->isPublic())

libsolidity/analysis/TypeChecker.cpp

@@ -2791,6 +2791,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 	case Type::Category::Function:
 		functionType = dynamic_cast<FunctionType const*>(expressionType);
 		funcCallAnno.kind = FunctionCallKind::FunctionCall;
+		funcCallAnno.origCallingContract = dynamic_cast<ContractDefinition const*>(currentDefinitionScope());
 
 		if (auto memberAccess = dynamic_cast<MemberAccess const*>(&_functionCall.expression()))
 		{

libsolidity/ast/ASTAnnotations.h

@@ -339,6 +339,9 @@ struct FunctionCallAnnotation: ExpressionAnnotation
 	util::SetOnce<FunctionCallKind> kind;
 	/// If true, this is the external call of a try statement.
 	bool tryCall = false;
+	/// The original contract calling the function - actual calling contract may be different,
+	/// e.g. in case is inlining Internal library function call.
+	ContractDefinition const* origCallingContract = nullptr;
 };
 
 }

libsolidity/codegen/ExpressionCompiler.cpp

@@ -696,6 +696,17 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall)
 			// Calling convention: Caller pushes return address and arguments
 			// Callee removes them and pushes return values
 
+			FunctionDefinition const* functionDef = ASTNode::resolveFunctionCall(_functionCall, &m_context.mostDerivedContract());
+			if (functionDef && !functionDef->isVisibleInDerivedContracts())
+			{
+				auto origCallingContract = _functionCall.annotation().origCallingContract;
+				solAssert (origCallingContract, "Original calling contract is not set");
+				solAssert(
+					functionDef->annotation().contract == origCallingContract,
+					"Attempt to call private function from other contract."
+				);
+			}
+
 			evmasm::AssemblyItem returnLabel = m_context.pushNewTag();
 			for (unsigned i = 0; i < arguments.size(); ++i)
 				acceptAndConvert(*arguments[i], *function.parameterTypes()[i]);

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

@@ -1012,6 +1012,16 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall)
 		{
 			solAssert(functionDef->isImplemented());
 
+			if (!functionDef->isVisibleInDerivedContracts())
+			{
+				auto origCallingContract = _functionCall.annotation().origCallingContract;
+				solAssert (origCallingContract, "Original calling contract is not set");
+				solAssert(
+					functionDef->annotation().contract == origCallingContract,
+					"Attempt to call private function from other contract"
+				);
+			}
+
 			define(_functionCall) <<
 				m_context.enqueueFunctionForCodeGeneration(*functionDef) <<
 				"(" <<

test/libsolidity/semanticTests/functionScopting/private_function_vs_free_function.sol

@@ -0,0 +1,16 @@
+// Tests that private contract function does not shadow a free function,
+// but that internal contract function does.
+function foo(uint256 value) pure returns (uint256) { return 1; }
+function bar(uint256 value) pure returns (uint256) { return 2; }
+contract A {
+    function foo(uint256 value) private pure returns (uint256) { return 3; }
+    function bar(uint256 value) internal pure returns (uint256) { return 4; }
+}
+contract B is A {
+    using {foo} for uint256;
+    function test1(uint256 value) public pure returns (uint256) { return value.foo(); }
+    function test2(uint256 value) public pure returns (uint256) { return bar(value); }
+}
+// ----
+// test1(uint256): 0 -> 1
+// test2(uint256): 0 -> 4

test/libsolidity/semanticTests/functionScopting/private_function_vs_inheritance.sol

@@ -0,0 +1,18 @@
+// Tests that private contract functions are not overridden or visible by inheriting contracts,
+// and that that functions functions do.
+contract A {
+    function foo() private pure returns (uint256) { return 1; }
+    function bar() internal pure returns (uint256) { return foo(); }
+}
+contract B is A {
+    function foo() private pure returns (uint256) { return 2; }
+    function test1() public pure returns (uint256) { return bar(); }
+    function test2() public pure returns (uint256) { return foo(); }
+}
+contract C is B {
+    function foo() public pure returns (uint256) { return 3; }
+}
+// ----
+// test1() -> 1
+// test2() -> 2
+// foo() -> 3

test/libsolidity/semanticTests/functionScopting/private_function_vs_internal_contract.sol

@@ -0,0 +1,13 @@
+// Tests that private contract function is not inherited, but that internal function does.
+contract A {
+    function f() private pure returns (uint) { return 7; }
+    function g() internal pure returns (uint) { return f(); }
+}
+contract B is A {
+    function f() private pure returns (uint) { return 42; }
+    function test() external pure returns (uint, uint) {
+        return (g(), f());
+  }
+}
+// ----
+// test() -> 7, 0x2a

test/libsolidity/semanticTests/functionScopting/private_function_vs_library.sol

@@ -0,0 +1,18 @@
+// Tests that private library functions are not overridden or visible contracts,
+// and that that internal library functions do.
+library L {
+    function foo(uint256 value) private pure returns (uint256) { return 1; }
+    function bar(uint256 value) internal pure returns (uint256) { return foo(value); }
+}
+contract A {
+    function foo(uint256 value) private pure returns (uint256) { return 2; }
+}
+contract B is A {
+    using L for uint256;
+    function foo(uint256 value) private pure returns (uint256) { return 3; }
+    function test1(uint256 value) public pure returns (uint256) { return value.bar(); }
+    function test2(uint256 value) public pure returns (uint256) { return foo(0); }
+}
+// ----
+// library: L
+// test1(uint256): 0 -> 1

test/libsolidity/semanticTests/functionScopting/private_function_vs_overloading.sol

@@ -0,0 +1,18 @@
+// Tests that private functions are not overridden by inheriting contract's functions.
+contract A {
+    function foo() private pure returns (uint256) { return 1; }
+    function foo(uint256 value) private pure returns (uint256) { return 2; }
+    function test1() public pure returns (uint256) { return foo(); }
+    function test2() public pure returns (uint256) { return foo(0); }
+}
+contract B is A {
+    function foo() private pure returns (uint256) { return 3; }
+    function foo(uint256 value) private pure returns (uint256) { return 4; }
+    function test3() public pure returns (uint256) { return foo(); }
+    function test4() public pure returns (uint256) { return foo(0); }
+}
+// ----
+// test1() -> 1
+// test2() -> 2
+// test3() -> 3
+// test4() -> 4

test/libsolidity/syntaxTests/inheritance/overloaded_private_function.sol

@@ -0,0 +1,13 @@
+// Tests that private functions are not overridden by inheriting contracts, but that public functions does.
+contract A {
+    function foo() private {}
+    function foo(uint128 value) private {}
+    function foo(uint256 value) public {}
+}
+contract B is A {
+    function foo(uint128 value) private {}
+    function foo(uint256 value) public {}
+}
+// ----
+// TypeError 9456: (303-340): Overriding function is missing "override" specifier.
+// TypeError 4334: (198-235): Trying to override non-virtual function. Did you forget to add "virtual"?

test/libsolidity/syntaxTests/inheritance/override/implement_private_function_by_public_variable.sol

@@ -6,5 +6,5 @@ contract T {
     constructor() { new Y(); }
 }
 // ----
-// TypeError 5225: (97-130): Public state variables can only override functions with external visibility.
+// TypeError 7792: (112-120): Public state variable has override specified but does not override anything.
 // TypeError 3942: (22-72): "virtual" and "private" cannot be used together.

test/libsolidity/syntaxTests/inheritance/override/override_private_function.sol

@@ -0,0 +1,9 @@
+// Tests that private functions are not overridden by inheriting contracts.
+contract A {
+	function foo() private {}
+}
+contract B is A {
+	function foo() private override {}
+}
+// ----
+// TypeError 7792: (160-168): Function has override specified but does not override anything.

test/libsolidity/syntaxTests/inheritance/override/virtual_private.sol

@@ -5,4 +5,5 @@ abstract contract X is A {
 	function test() private override returns (uint256) {}
 }
 // ----
+// TypeError 7792: (128-136): Function has override specified but does not override anything.
 // TypeError 3942: (23-73): "virtual" and "private" cannot be used together.

test/libsolidity/syntaxTests/inheritance/shadowing_private_base_function.sol

@@ -0,0 +1,11 @@
+// Tests that private functions do not shadow, and are not shadowed by, inheriting contract's functions.
+contract A {
+	function foo() private {}
+}
+contract B is A {
+	function foo() private {}
+}
+contract C is B {
+	function foo() public {}
+}
+// ----

test/libsolidity/syntaxTests/scoping/double_private_function_declaration.sol

@@ -0,0 +1,7 @@
+// Tests that two private functions cannot be defined with the same name.
+contract A {
+	function foo() private {}
+	function foo() private {}
+}
+// ----
+// DeclarationError 1686: (88-113): Function with same name and parameter types defined twice.