Make super skip unimplemented functions
#11472
Conversation
| 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). |
There was a problem hiding this comment.
This test ensures that we'll not hit the final assert in resolveVirtual() when super skips all the unimplemented functions and there are no implemented ones to use.
The error is misleading though. Should just say that the function is not implemented.
| 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. |
There was a problem hiding this comment.
This shows that the same problem does not exist without multiple inheritance because we just do not allow unimplemented functions between implemented ones in the resolution chain.
| contract C { | ||
| modifier m() { _; } | ||
| } | ||
| contract D is C { | ||
| function f() super.m public { | ||
| } | ||
| } | ||
| // ---- | ||
| // DeclarationError 7920: (74-81): Identifier not found or not unique. |
There was a problem hiding this comment.
It's also not possible with modifiers because they do not have super (though I found a different problem: #11468).
cameel
force-pushed
the
make-super-skip-unimplemented-functions
branch
from
d5ca844 to
3f7cb20
Compare
|
Comment fixed. I have also added a changelog entry. |
cameel
force-pushed
the
make-super-skip-unimplemented-functions
branch
from
3f7cb20 to
b7b7e2f
Compare
libsolidity/ast/AST.cpp
Outdated
| !function->isConstructor() && | ||
| // Lookup via super should skip an unimplemented function because there must be | ||
| // another, implemented one behind it (otherwise we would fail at analysis stage). | ||
| // For ordinary virtual lookup, on the other hand, the only case in which we might |
There was a problem hiding this comment.
So if this cannot happen for virtual functions, why not also skip unimplemented functions in that case?
There was a problem hiding this comment.
It can happen - in the one situation the comment mentions. I.e. when the call is in a function that will never run because it has been overridden. See for example: call_unimplemented_base.sol.
My first implementation was actually what you suggested but it felt a bit like a hack. To cover this single case I had to remember the last seen unimplemented function and go back to it if it turned out there were no implemented ones at all. Doing it only for super makes it simpler.
There was a problem hiding this comment.
Ok, I see it now: super essentially searches upwards while virtual searches downwards. Can you still shorten this comment to at most two lines? Maybe the second part is not really relevant.
There was a problem hiding this comment.
super essentially searches upwards while virtual searches downwards
I do not understand what you mean by this part. They both walk the linearized hierarchy in the same direction. Can you elaborate a bit more on how you see this situation?
There was a problem hiding this comment.
Comment changed.
Was:
// Lookup via super should skip an unimplemented function because there must be
// another, implemented one behind it (otherwise we would fail at analysis stage).
// For ordinary virtual lookup, on the other hand, the only case in which we might
// encounter an unimplemented function is when there is no implementation at all
// and that only happens if the code passed analysis due to the call never actually
// happening (i.e. it's inside a function that gets overridden).is:
// 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.
|
@chriseth About cases where we want different behavior between
|
cameel
force-pushed
the
make-super-skip-unimplemented-functions
branch
from
b7b7e2f to
d96cc34
Compare
Fixes #11445.