Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Bugfixes:
* Type Checker: Fix internal compiler error when attempting to use an invalid external function type on pre-byzantium EVMs.
* Type Checker: Make errors about (nested) mapping type in event or error parameter into fatal type errors.
* Type Checker: Fix internal compiler error when overriding receive ether function with one having different parameters during inheritance.
* Type Checker: Fix internal compiler error when overriding an implemented modifier with an unimplemented one.


AST Changes:
Expand Down
21 changes: 13 additions & 8 deletions libsolidity/analysis/OverrideChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,19 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
);
}

if (_overriding.unimplemented() && !_super.unimplemented())
{
solAssert(!_overriding.isVariable() || !_overriding.unimplemented(), "");
overrideError(
_overriding,
_super,
4593_error,
"Overriding an implemented " + _super.astNodeName() +
" with an unimplemented " + _overriding.astNodeName() +
" is not allowed."
);
}

if (_super.isFunction())
{
FunctionType const* functionType = _overriding.functionType();
Expand Down Expand Up @@ -613,14 +626,6 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
stateMutabilityToString(_overriding.stateMutability()) +
"\"."
);

if (_overriding.unimplemented() && !_super.unimplemented())
overrideError(
_overriding,
_super,
4593_error,
"Overriding an implemented function with an unimplemented function is not allowed."
);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
contract A {
modifier m() virtual { _; }
}
abstract contract B is A {
modifier m() virtual override;
}
contract C is B {
function f() m public {}
}
Comment on lines +1 to +9
Copy link
Collaborator

@cameel cameel Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use some positive test cases too (unless they already exist somewhere):

Overriding unimplemented with unimplemented:

abstract contract A {
    modifier m() virtual;
}
abstract contract B is A {
    modifier m() virtual override;
}
abstract contract C is B {
    modifier m() virtual override;
    function f() m public {}
}

Leaving a modifier somewhere in the chain unimplemented through multiple inheritance:

contract A {
    modifier m() virtual { _; }
}
abstract contract B {
    modifier m() virtual;
}
contract C is A, B {
    modifier m() override(A, B) { _; }
    function f() m public {}
}

Use of an unimplemented modifier in an abstract contract:
(EDIT: Replaced A.m with m. Also, this should not be an error after all)

abstract contract A {
    modifier m() virtual;
    function f() m public {}
}
contract B is A {
    modifier m() virtual override { _; }
}

Use of an unimplemented modifier on a method that gets overridden:
(EDIT: Replaced A.m with m, changed B to abstract and removed m override from B)

abstract contract A {
    modifier m() virtual;
    function f() m public virtual {}
}
abstract contract B is A {
    function f() public override {}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about these? Could you add them as tests?

All but the second one are currently errors (Cannot call unimplemented modifier.). I think they should all work without errors because equivalent examples for virtual functions do pass.

// ----
// TypeError 4593: (78-108): Overriding an implemented modifier with an unimplemented modifier is not allowed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
contract A {
modifier m() virtual { _; }
}
abstract contract B {
modifier m() virtual;
}
contract C is A, B {
modifier m() override(A, B) { _; }
function f() m public {}
}
// ----
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
abstract contract A {
modifier m() virtual;
}
abstract contract B is A {
modifier m() virtual override;
}
abstract contract C is B {
modifier m() virtual override;
function f() m public {}
}
// ----
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
abstract contract A {
modifier m() virtual;
function f() m public {}
}
contract B is A {
modifier m() virtual override { _; }
}
// ----
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
abstract contract A {
modifier m() virtual;
function f() m public virtual {}
}
abstract contract B is A {
function f() public override {}
}
// ----