Don't require `override(A, B)` when inherited contracts already inherit from each other

[cameel]

Description

Let's consider a situation where we have a contract C that inherits from A and B where B already inherits from A. A provides a function that B overrides:

contract A {
    function foo() public virtual {}
}
contract B is A {
    function foo() public virtual override {}
}
contract C is A, B {}

Currently the compiler reports an error if C does not define function foo() public virtual override(A, B):

Error: Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.
 --> test.sol:7:1:
  |
7 | contract C is A, B {}
  | ^^^^^^^^^^^^^^^^^^^^^
Note: Definition in "A":
 --> test.sol:2:5:
  |
2 |     function foo() public virtual {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: Definition in "B":
 --> test.sol:5:5:
  |
5 |     function foo() public virtual override {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Technically, the compiler is right because both A and B provide conflicting definitions for foo() (the original one and an overridden one). In practice though, due to how multiple inheritance works in Solidity, the user will expect the explicitly inherited A and the one inside B to be the same thing so it would make sense to spare the user the error and just choose B.foo() automatically.

Context

This originally came up in this thread on OZ forum: // The following functions are overrides required by Solidity.

While the case I'm showing above may look like an obscure corner case, such code is actually something users encounter often because OpenZeppelin's Contract Wizard generates code where contracts inherit from other contracts that are already inherited by one of base contracts. I have also often seen this pattern in real-life contracts - users often inherit ERC20 explicitly even if one of the other contracts they inherit already does.

Here's an example you get from the wizard when you just select the Votes checkbox:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.2;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";

contract MyToken is ERC20, ERC20Permit, ERC20Votes {
    constructor() ERC20("MyToken", "MTK") ERC20Permit("MyToken") {}

    // The following functions are overrides required by Solidity.

    function _afterTokenTransfer(address from, address to, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._afterTokenTransfer(from, to, amount);
    }

    function _mint(address to, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._mint(to, amount);
    }

    function _burn(address account, uint256 amount)
        internal
        override(ERC20, ERC20Votes)
    {
        super._burn(account, amount);
    }
}

The only reason for including the overridden functions is the problem described in this issue. Without it the contract would have been as short as

contract MyToken is ERC20, ERC20Permit, ERC20Votes {
    constructor() ERC20("MyToken", "MTK") ERC20Permit("MyToken") {}
}

The same effect could be achieved by removing ERC20 and ERC20Permit from the inheritance list but, like I already mentioned, that's not something people do in practice.

[cameel]

After discussing this with @ekpyron, we think that instead of relaxing the check it might be better to just discourage this inheritance pattern. After all, the fact that it may not be entirely clear to users that despite B inheriting from A there still exist two different versions of foo() in the hierarchy is a good reason to clarify the behavior with an explicit override.

Instead we could issue an INFO message saying that this kind of inheritance is entirely superfluous. I.e.

• if you have something like A is X, B is X and C is A, B then virtual inheritance is justified and can't be avoided without redesigning the contract (no message).
• In B is A, C is A, B it can be avoided easily by having C not inherit from A (we should inform about it).

[chriseth]

Sounds good to warn about the weird pattern!

[ekpyron]

We actually use that pattern ourselves in the inheritance documentation: https://docs.soliditylang.org/en/v0.8.11/contracts.html#inheritance - we should probably change that...

[cameel]

Yeah, adjusting the docs should be relatively easy though given that the pattern is really superfluous.

I'm adding this to implementation backlog in that case.

[ekpyron]

The problem is that if we remove the superfluous bases in the docs example, then it's no longer an example that actually shows of override(...) :-). So we'll need another example in which that's actually required...

[cameel]

We can introduce some additional contract so that PriceFeed.destroy() still needs override(...). For example contract Pausable is Destructible that overrides both get() and destroy() so that they revert if you try to use them when it's paused.