The problem this is trying to address is that if you’re overriding more than one function, your new function must take into account all of what the base functions do because you’re effectively disabling them unless your implementation calls them explicily. If you’re overriding
B.foo(), for each of them you make a decision of whether to call it, ignore it, replicate its code or do something completely different. Even if you do nothing, it’s a choice.
The thing is that overrides like that are really brittle and can break with non-local changes. If another of your base classes, say
foo(), your function suddenly is overriding three functions even though nothing in the function itself changed. With
override(A, B) the compiler can tell that you never considered
C and force you to revisit your override. Hopefully you then update it to take the extra implementation into account.
See OpenZeppelin/openzeppelin-contracts#2936 3 as an example.
I think the syntax directly causes both of these misunderstandings by giving a false impression that A and B are all that’s relevant at that point (and in that order).
I can see how it may confusing and it’s unfortunate that it can be understood that way but this really has nothing to do with
override(A, B) says that functions
B.foo() are being replaced by your new definition. True,
super.foo() may end up making a call to
C.foo() from some other contract
C that you are not directly inheriting from but
C.foo() is not being replaced by your definition of
C.foo() is not really relevant to the check because it’s not your definition of
foo() that has to worry about it. Instead thare must exist some other contract
Y that inherits both from your contract (let’s call it
X) and from
Y.foo() then needs
override (X, C) and that’s where the compiler will want you to consider
It requires knowledge of implementation details of dependencies, since A, B can be grandparents.
At the very least could
override(A, B) be changed so that A and B can be the direct parents, as opposed the grandparents that originate the conflict?
You have a point here. I agree that specifying direct parents should be allowed. We could actually allow either, making such a change backwards-compatible.
I don’t know the motivation behind this particular design choice but it might have been that we wanted the programmer to look at the definition of
foo() being overridden rather than to just mechanically add the name of the contract to
override(). The definition you should look at is not necessarily in the parent and can be buried deeper in the hierarchy.
Personally, I think that we can’t really force anyone to handle this the right way and the job of the compiler here is just to provide enough information to bring attention to the problem and make fixing it possible. The error message already points at the location of the function being overridden and that should be enough.
override(A, B) gives the false impression of a locally determined inheritance ordering.
This is definitely a problem and we need a solution for it. The idea we had was to enforce a particular order #8373 with a warning but ultimately we wasn’t sure it really solves the problem and #8396 implementing it was closed. The discussion on that is still open.
There’s one more point to consider here. I think that a lot of pain related to
override(A, B) comes unnecessarily repeating contracts in the inheritance hierarchy. Let’s consider this structure that you get when you use the OZ contract wizard:
contract ERC20Permit is ERC20
contract ERC20Votes is ERC20Permit
contract MyToken is ERC20, ERC20Permit, ERC20Votes
It might seem that including
ERC20 here should not be a problem because
ERC20Votes already inherits from it. Unfortunately
ERC20Votes overrides some functions from
ERC20 and by inheriting directly from
ERC20 you bring back the old, non-overriden versions of these functions. Then you need to use
override(ERC20 ERC20Votes) to declare that you are aware that you’re overriding both of them.
This pattern seems pretty common overall and eliminating it would remove a lot of cases where you need to list overridden contracts. In the compiler we could silence it and make a decision for you by treating the version from
ERC20 as if it wasn’t there (#12554) but it would be making a decision for the programmer and I think it might be better instead to issue an INFO note hinting that this pattern is superfluous to nudge people towards dropping it.