ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.35k stars 5.77k forks source link

Overhaul Inheritance and Override Logic #12665

Open ekpyron opened 2 years ago

ekpyron commented 2 years ago

https://github.com/ethereum/solidity/issues/12615 reported a serious error in our override logic. While https://github.com/ethereum/solidity/pull/12616 fixes the issue, we realized that our strict override logic has some counterintuitive implications, namely https://github.com/ethereum/solidity/pull/12616#discussion_r797992050 and https://github.com/ethereum/solidity/pull/12616#discussion_r797985635.

So we intend to relax our override requirements to avoid cases like that.

In particular, we want to have a specification with the following property:

In particular consider the following situation:

interface I { function f() external; }
contract A is I { function f() external {} }
contract B is I { }
contract C is A, B {
  // no override is required here!
}
contract D is C {
  // The following *should* be valid, but currently fails and requires "override(I, A)".
  function f() external override // more specifically "override(A)"
  {
  }
}

Our current plan is to relax the override requirements, s.t. D in the example becomes valid. However, this is breaking (since currently specifying more bases than required is an error). So more specifically, we want to do this change while allowing additional bases (with a warning) in 0.8, while those additional bases will become an error only in 0.8.

So this issue has two parts:

cameel commented 2 years ago

Perhaps #13136 should be a part or this too.

ZumZoom commented 2 years ago

Here is a bit simpler example of the case when explicit interface overriding is unreasonably required:

interface IERC20 {
    function balanceOf() external view returns(uint256);
}

contract ERC20 is IERC20 {
    function balanceOf() external view virtual returns(uint256) {
        return 0;
    }
}

interface IERC20Mintable is IERC20{
    function mint() external;
}

contract MintableERC20 is ERC20, IERC20Mintable {
    function mint() external {
        this;
    }

    function balanceOf() external view override returns(uint256) {
        return 1;
    }
}

balanceOf override fails with Function needs to specify overridden contracts "ERC20" and "IERC20"

naddison36 commented 1 year ago

Here's another example using an abstract contract

// SPDX-License-Identifier: AGPL-3.0-or-later
pragma solidity ^0.8.0;

abstract contract Base {
    function genericStuff() public {
        // does some common stuff

        // and some specific stuff
        specificStuff();
    }

    function specificStuff() public virtual;
}

abstract contract ImplOne {
    function specificStuff() public virtual {
        // does something specific
    }
}

abstract contract ImplTwo {
    function specificStuff() public virtual {
        // does something else
    }
}

contract Concrete1 is Base, ImplOne {
    // why is this necessary when only ImplOne.specificStuff is implemented
    // Base.specificStuff() is abstract
    function specificStuff() public override(Base, ImplOne) {
        ImplOne.specificStuff();
    }
}

The above example is not that bad when you only have one abstract function. But once you have more than a handful of abstract functions it becomes very verbose doing trivial overrides that the compiler could work out.

naddison36 commented 1 year ago

One way to avoid implementing overrides for every abstract function is to use a struct of function types. For example

// SPDX-License-Identifier: AGPL-3.0-or-later
pragma solidity ^0.8.0;

struct BaseFunctions {
    function() returns (uint256) specificStuff;
    function(uint256) someMoreStuff;
}

abstract contract Base {

    // Abstract function that must be implemented later
    function _getFunctions() internal virtual returns (BaseFunctions memory);

    function genericStuff() public {
        // does some common stuff

        // Get the implemented functions
        BaseFunctions memory baseFunctions = _getFunctions();
        // do some implementation specific stuff
        uint256 someAnswer = baseFunctions.specificStuff();
        // do more implementation specific stuff
        baseFunctions.someMoreStuff(someAnswer);
    }
}

abstract contract ImplOne {
    function _getFunctions() internal virtual returns (BaseFunctions memory) {
        return BaseFunctions({
            specificStuff: specificStuff,
            someMoreStuff: someMoreStuff
        });
    }

    function specificStuff() public returns (uint256) {
        // does something specific
        return 1;
    }

    function someMoreStuff(uint256) public {
        // does more implementation specific stuff
    }
}

abstract contract ImplTwo {
    function _getFunctions() internal virtual returns (BaseFunctions memory) {
        return BaseFunctions({
            specificStuff: specificStuff,
            someMoreStuff: someMoreStuff
        });
    }

    function specificStuff() public returns (uint256) {
        // does something specific
        return 2;
    }

    function someMoreStuff(uint256) public {
        // does more implementation specific stuff
    }
}

contract Concrete1 is Base, ImplOne {
    // Only requires one override function, not two
    function _getFunctions() internal override(Base, ImplOne) returns (BaseFunctions memory) {
        return ImplOne._getFunctions();
    }
}