From 67e87147b4a50809df8725be8838ec19e7fc9c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20=C5=9Aliwak?= Date: Tue, 1 Jun 2021 17:13:50 +0200 Subject: [PATCH 1/2] Extra asserts, test renaming, test for super in modifiers --- libsolidity/codegen/CompilerContext.cpp | 6 +++++- libsolidity/codegen/ContractCompiler.cpp | 2 ++ ...unimplemented_and_implemented_with_unimplemented.sol} | 0 ...implemented_and_unimplemented_with_unimplemented.sol} | 0 .../syntaxTests/modifiers/cross_contract_super.sol | 9 +++++++++ 5 files changed, 16 insertions(+), 1 deletion(-) rename test/libsolidity/syntaxTests/inheritance/override/{override_unimplemented_fail.sol => override_unimplemented_and_implemented_with_unimplemented.sol} (100%) rename test/libsolidity/syntaxTests/inheritance/override/{override_unimplemented_fine.sol => override_unimplemented_and_unimplemented_with_unimplemented.sol} (100%) create mode 100644 test/libsolidity/syntaxTests/modifiers/cross_contract_super.sol diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index 398e362f5..6de51298d 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/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 diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 5afa4d26f..fca076b65 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/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); diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_fail.sol b/test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_and_implemented_with_unimplemented.sol similarity index 100% rename from test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_fail.sol rename to test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_and_implemented_with_unimplemented.sol diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_fine.sol b/test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_and_unimplemented_with_unimplemented.sol similarity index 100% rename from test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_fine.sol rename to test/libsolidity/syntaxTests/inheritance/override/override_unimplemented_and_unimplemented_with_unimplemented.sol diff --git a/test/libsolidity/syntaxTests/modifiers/cross_contract_super.sol b/test/libsolidity/syntaxTests/modifiers/cross_contract_super.sol new file mode 100644 index 000000000..b70ba3092 --- /dev/null +++ b/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. From d96cc3469a456ffa35afec550065a89da2c8a655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20=C5=9Aliwak?= Date: Tue, 1 Jun 2021 17:16:09 +0200 Subject: [PATCH 2/2] FunctionDefinition.resolveVirtual(): Skip unimplemented functions when lookup happens via super --- Changelog.md | 1 + libsolidity/ast/AST.cpp | 12 ++++++++-- .../inheritance/call_unimplemented_base.sol | 8 +++++-- ...kip_unimplemented_in_abstract_contract.sol | 22 +++++++++++++++++++ .../super_skip_unimplemented_in_interface.sol | 22 +++++++++++++++++++ .../call_unimplemented_base_via_super.sol | 18 +++++++++++++++ ...ted_with_implemented_call_via_contract.sol | 13 +++++++++++ ...mented_with_implemented_call_via_super.sol | 12 ++++++++++ ...unimplemented_with_implemented_no_call.sol | 12 ++++++++++ ...mented_virtual_call_into_base_contract.sol | 13 +++++++++++ ...ed_with_unimplemented_then_implemented.sol | 11 ++++++++++ 11 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_abstract_contract.sol create mode 100644 test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_interface.sol create mode 100644 test/libsolidity/syntaxTests/functionCalls/call_unimplemented_base_via_super.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_contract.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_super.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_no_call.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_virtual_call_into_base_contract.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_implemented_with_unimplemented_then_implemented.sol diff --git a/Changelog.md b/Changelog.md index d7c3eec00..26c958076 100644 --- a/Changelog.md +++ b/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. diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 537846007..e217fecfa 100644 --- a/libsolidity/ast/AST.cpp +++ b/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 } diff --git a/test/libsolidity/semanticTests/functionCall/inheritance/call_unimplemented_base.sol b/test/libsolidity/semanticTests/functionCall/inheritance/call_unimplemented_base.sol index 4edfa5cd9..4d84dcd7a 100644 --- a/test/libsolidity/semanticTests/functionCall/inheritance/call_unimplemented_base.sol +++ b/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 diff --git a/test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_abstract_contract.sol b/test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_abstract_contract.sol new file mode 100644 index 000000000..82dc5d513 --- /dev/null +++ b/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 diff --git a/test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_interface.sol b/test/libsolidity/semanticTests/functionCall/inheritance/super_skip_unimplemented_in_interface.sol new file mode 100644 index 000000000..8f7914013 --- /dev/null +++ b/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 diff --git a/test/libsolidity/syntaxTests/functionCalls/call_unimplemented_base_via_super.sol b/test/libsolidity/syntaxTests/functionCalls/call_unimplemented_base_via_super.sol new file mode 100644 index 000000000..4b7d45624 --- /dev/null +++ b/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). diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_contract.sol b/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_contract.sol new file mode 100644 index 000000000..72e0eaa1b --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_super.sol b/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_call_via_super.sol new file mode 100644 index 000000000..61d121f03 --- /dev/null +++ b/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. + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_no_call.sol b/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_no_call.sol new file mode 100644 index 000000000..3a9d94ea1 --- /dev/null +++ b/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. + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_virtual_call_into_base_contract.sol b/test/libsolidity/syntaxTests/inheritance/override/override_implemented_and_unimplemented_with_implemented_virtual_call_into_base_contract.sol new file mode 100644 index 000000000..5321bbdb0 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_implemented_with_unimplemented_then_implemented.sol b/test/libsolidity/syntaxTests/inheritance/override/override_implemented_with_unimplemented_then_implemented.sol new file mode 100644 index 000000000..b1721e164 --- /dev/null +++ b/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.