Merge pull request #11472 from ethereum/make-super-skip-unimplemented-functions

Make `super` skip unimplemented functions

Changelog.md

@@ -16,6 +16,7 @@ Compiler Features:
 
 Bugfixes:
  * AST: Do not output value of Yul literal if it is not a valid UTF-8 string.
+ * Code Generator: Fix internal error when super would have to skip an unimplemented function in the virtual resolution order.
  * Control Flow Graph: Take internal calls to functions that always revert into account for reporting unused or unassigned variables.
  * SMTChecker: Fix internal error on struct constructor with fixed bytes member initialized with string literal.
  * SMTChecker: Fix internal error on external calls from the constructor.

libsolidity/ast/AST.cpp

@@ -397,6 +397,7 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 ) const
 {
 	solAssert(!isConstructor(), "");
+
 	// If we are not doing super-lookup and the function is not virtual, we can stop here.
 	if (_searchStart == nullptr && !virtualSemantics())
 		return *this;
@@ -407,19 +408,26 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual(
 
 	FunctionType const* functionType = TypeProvider::function(*this)->asExternallyCallableFunction(false);
 
+	bool foundSearchStart = (_searchStart == nullptr);
 	for (ContractDefinition const* c: _mostDerivedContract.annotation().linearizedBaseContracts)
 	{
-		if (_searchStart != nullptr && c != _searchStart)
+		if (!foundSearchStart && c != _searchStart)
 			continue;
-		_searchStart = nullptr;
+		else
+			foundSearchStart = true;
+
 		for (FunctionDefinition const* function: c->definedFunctions())
 			if (
 				function->name() == name() &&
 				!function->isConstructor() &&
+				// 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)
 			)
 				return *function;
 	}
+
 	solAssert(false, "Virtual function " + name() + " not found.");
 	return *this; // not reached
 }

libsolidity/codegen/CompilerContext.cpp

@@ -286,7 +286,11 @@ FunctionDefinition const& CompilerContext::superFunction(FunctionDefinition cons
 	solAssert(m_mostDerivedContract, "No most derived contract set.");
 	ContractDefinition const* super = _base.superContract(mostDerivedContract());
 	solAssert(super, "Super contract not available.");
-	return _function.resolveVirtual(mostDerivedContract(), super);
+
+	FunctionDefinition const& resolvedFunction = _function.resolveVirtual(mostDerivedContract(), super);
+	solAssert(resolvedFunction.isImplemented(), "");
+
+	return resolvedFunction;
 }
 
 ContractDefinition const& CompilerContext::mostDerivedContract() const

libsolidity/codegen/ContractCompiler.cpp

@@ -604,6 +604,8 @@ bool ContractCompiler::visit(VariableDeclaration const& _variableDeclaration)
 
 bool ContractCompiler::visit(FunctionDefinition const& _function)
 {
+	solAssert(_function.isImplemented(), "");
+
 	CompilerContext::LocationSetter locationSetter(m_context, _function);
 
 	m_context.startFunction(_function);

test/libsolidity/semanticTests/functionCall/inheritance/call_unimplemented_base.sol

@@ -2,13 +2,17 @@ abstract contract I
 {
     function a() internal view virtual returns(uint256);
 }
-abstract contract V is I
+abstract contract J is I
+{
+    function a() internal view virtual override returns(uint256);
+}
+abstract contract V is J
 {
     function b() public view returns(uint256) { return a(); }
 }
 contract C is V
 {
-    function a() internal view override returns (uint256) { return 42;}
+    function a() internal view override returns (uint256) { return 42; }
 }
 // ====
 // compileToEwasm: also

test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_abstract_contract.sol

@@ -0,0 +1,22 @@
+contract A {
+    function f() public virtual returns (uint) {
+        return 42;
+    }
+}
+
+abstract contract I {
+    function f() external virtual returns (uint);
+}
+
+contract B is A, I {
+    function f() override(A, I) public returns (uint) {
+        // I.f() is before A.f() in the C3 linearized order
+        // but it has no implementation.
+        return super.f();
+    }
+}
+// ====
+// compileToEwasm: also
+// compileViaYul: also
+// ----
+// f() -> 42

test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_interface.sol

@@ -0,0 +1,22 @@
+contract A {
+    function f() public virtual returns (uint) {
+        return 42;
+    }
+}
+
+interface I {
+    function f() external returns (uint);
+}
+
+contract B is A, I {
+    function f() override(A, I) public returns (uint) {
+        // I.f() is before A.f() in the C3 linearized order
+        // but it has no implementation.
+        return super.f();
+    }
+}
+// ====
+// compileToEwasm: also
+// compileViaYul: also
+// ----
+// f() -> 42

test/libsolidity/syntaxTests/functionCalls/call_unimplemented_base_via_super.sol

@@ -0,0 +1,18 @@
+abstract contract I {
+    function a() internal view virtual returns(uint256);
+}
+
+abstract contract C is I {
+    function f() public view returns(uint256) {
+        return I.a();
+    }
+}
+
+abstract contract D is I {
+    function f() public view returns(uint256) {
+        return super.a();
+    }
+}
+// ----
+// TypeError 7501: (172-177): Cannot call unimplemented base function.
+// TypeError 9582: (278-285): Member "a" not found or not visible after argument-dependent lookup in type(contract super D).

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

@@ -0,0 +1,13 @@
+contract A {
+    function f() public virtual {}
+}
+abstract contract B {
+    function f() public virtual;
+}
+contract C is A, B {
+    function f() public virtual override(A, B) {
+        B.f(); // Should not skip over to A.f() just because B.f() has no implementation.
+    }
+}
+// ----
+// TypeError 7501: (185-190): Cannot call unimplemented base function.

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

@@ -0,0 +1,12 @@
+contract A {
+    function f() public virtual {}
+}
+abstract contract B {
+    function f() public virtual;
+}
+contract C is A, B {
+    function f() public override(A, B) {
+        super.f(); // super should skip the unimplemented B.f() and call A.f() instead.
+    }
+}
+// ----

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

@@ -0,0 +1,12 @@
+contract A {
+    function f() public virtual {}
+}
+abstract contract B {
+    function f() public virtual;
+}
+contract C is A, B {
+    function f() public override(A, B) {
+        // This is fine. The unimplemented B.f() is not used.
+    }
+}
+// ----

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

@@ -0,0 +1,13 @@
+contract A {
+    function f() public virtual {}
+}
+abstract contract B {
+    function f() public virtual;
+}
+abstract contract C is A, B {
+    function g() public {
+        f(); // Would call B.f() if we did not require an override in C.
+    }
+}
+// ----
+// TypeError 6480: (107-243): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.

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

@@ -0,0 +1,11 @@
+contract A {
+    function f() public virtual {}
+}
+abstract contract B is A {
+    function f() public virtual override;
+}
+contract C is B {
+    function f() public virtual override {}
+}
+// ----
+// TypeError 4593: (81-118): Overriding an implemented function with an unimplemented function is not allowed.

test/libsolidity/syntaxTests/modifiers/cross_contract_super.sol

@@ -0,0 +1,9 @@
+contract C {
+    modifier m() { _; }
+}
+contract D is C {
+    function f() super.m public {
+    }
+}
+// ----
+// DeclarationError 7920: (74-81): Identifier not found or not unique.