solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Allow covariant return type implementation

Open jO-Osko opened this issue 4 years ago • 9 comments

Abstract

Languages usually allow covariant return type implementation of interfaces (C++, Java), but solidity seems to enforce type equality in return values.

Motivation

Languages usually allow covariant return type implementation of interfaces (C++, Java), but solidity seems to enforce type equality in return values. Take an example code (fails both in 8.+ and 7.6):

interface A {}

interface B is A {}

interface X {
    function temp() external pure returns (A);
}

contract Y is X {
// This is allowed since B is implicitly cast to A on return
/* 
    function some_fun() pure external override returns(A){
        return B(address(0));
    }
*/
    // This gets rejected by the compiler since `B` is not equal to `A`
    function some_fun() pure external override returns(B){
        return B(address(0));
    }
}

This gets especially problematic with automatic getters for public contract data.

interface A {}

interface B is A {}

interface X {
    function data() external view returns (A);
}

contract Y is X {
    B public override data; // Fails since type is not correct
    B public data; // Missing override specifier
}

To get the second contract to type check (and use the B interface instead of A), one has to add an internal field for data and manually write the getter function.

Specification

Implementation type checking should be implemented using is subtype instead of using strict type equality.

Backwards Compatibility

I am unsure of backward incompatibilities, but it might pose some unexpected problems with differently sized uint types and unexpected implicit overloads.

jO-Osko avatar Jul 06 '21 15:07 jO-Osko

Thanks for creating this issue, @jO-Osko . Do you have some real-world usage examples of that feature? It would allow us to better judge the need for it.

chriseth avatar Jul 07 '21 08:07 chriseth

One is to make it more in sync with type systems of other languages. This is quite important both for newcomers and for general development where people perceive subtyping to follow the same principles as C++, java... Any inconsistencies can be a great source of frustration.

Second is when dealing with two levels of interfaces for easier separation of concerns. Think user-facing simple and developer-facing more detailed version, which extends the user-facing interface.

interface UserFacingInterface {
    function readSomeData() external view returns (uint256);
}

interface DeveloperInterface is UserFacingInterface {
    // setData implementation has some internal checks to only allow specific accounts to interact with it, 
    // but we also want to exclude it general public interface, 
    // as they won't be able to call it anyway
   function setData(uint256 data) external view;
}

Assume that you have a supervisor contract, that interacts with and uses the UserFacingInterface(the one with simple methods), and would also like to expose the user-facing interface (for normal users to consume).

If you were to write an interface for such a contract you would do something like this, with a quite simple implementation (due to automatically generated getters):

interface UserFacingSupervisor {
   function mySpecialInterface() external view returns(UserFacingInterface);
}
// An implementation
contract Supervisor is UserFacingSupervisor {

    UserFacingInterface public mySpecialInterface; 
    constructor(UserFacingInterface _intParam){
        mySpecialInterface = _intParam;
    }

   function a() private view {
       // code is irrelevant, just use mySpecialInterface as UserFacingInterface
       require(mySpecialInterface.readSomeData() == 0);
   }

}

I want to add new methods to Supervisor, but they require DeveloperInterface (eg. I want to also use setData).

Naively (and that also works in C++), I just change the declaration in Supervisor from UserFacingInterface public mySpecialInterface; to DeveloperInterface public mySpecialInterface;. Since DeveloperInterface is (a subtype of) UserFacingInterface, method a works as intended, and it would be nice (and arguably more correct) to also extend this also automatically generated method mySpecialInterface() returns (UserFacingInterface) as before. But the type of this generated method is now DeveloperInterface which is (a subtype of) UserFacingInterface but it is not the same type so solidity rejects it.

For now, you have to change the contract to have a field that holds mySpecialInterface as before and manually create a method that returns the correct type to conform to the interface.

// Just part of the code
DeveloperInterface internal _mySpecialInterface;

function mySpecialInterface() external view returns(UserFacingInterface){
   return mySpecialInterface // This has a type DeveloperInterface but gets implicitly converted to UserFacingInterface
}

It would also nicely correspond to documentation:

Every contract defines its own type. You can implicitly convert contracts to contracts they inherit from. Contracts can be explicitly converted to and from the address type.

I think that the main gist is, that overriding should allow overriding with a more specific type not just with the same type as specified. If B is A, then every function function f() returns (B) is also function f() returns (A), since every contract of type B is implicitly convertible to A.

Take an example from the called side:

// B is A
// f() returns(A);
// g() returns(B);

A contractA = f(); // f returns something of type A
A contractA = g(); // g returns something of type B, that is implicitly convertible to A

jO-Osko avatar Jul 07 '21 15:07 jO-Osko

I understand the general concept and think that it can be useful. Still, we have so many planned features that are useful. In order to prioritize this feature, it would be really nice to get some real world usages of that pattern, as in smart contracts that are actually being developed or already deployed that would profit from it.

chriseth avatar Jul 07 '21 16:07 chriseth

I ran into this too.

interface IExchange { … } 
interface IExchangeInternal is IExchange { … }
interface IStrategy { function exchange() external view returns (IExchange); }

contract Strategy is IStrategy {

   // TypeError: Overriding public state variable return types differ.
   IExchangeInternal public immutable override exchange;

   // `Strategy` uses `IExchangeInternal` internally with additional functions
   // but exposes it as `IExchange` externally through `IStrategy`
}

fluidsonic avatar Jul 11 '21 00:07 fluidsonic

@fluidsonic do you have a link to a full code example?

chriseth avatar Jul 26 '21 12:07 chriseth

Hi Team,

Huge thanks to @cameel for helping me out about this.

I come with my own use case here.

There's a DAO contract and 3 other ones(C1, C2, C3).

Requirements: 1. C1, C2, C3 need to be upgradable and DAO as well should be upgradable. 2. ACL management should only be on the DAO and C1,C2,C3's ACL managements will also go through DAO contract as well. This way, ACL code is only deployed once when DAO is deployed. Using ACL for each C1,C2,C3 would be costly.

Because of the 2nd requirement, each C1, C2, C3 need to have DAO instances. So I design something like this..

abstract contract Component  {
    IDAO internal dao;
    
    function initialize(IDAO _dao) public virtual  {
        dao = _dao;
    }

    modifier authP(bytes32 role)  {
        require(dao.hasPermission(address(this), msg.sender, role, msg.data), "auth: check");
        _;
    }
   
}


interface IDAO {
    // ACL handling permission
    function hasPermission(address _where, address _who, bytes32 _role, bytes memory data) external returns(bool);
}

contract DAO is IDAO {

}

contract C1 is Component {
    
    function initialize(DAO dao) public override {
         Component.initialize(dao);
    }

}

Now, the above code fails because o override, as it thinks DAO that I have in initialize of C1 is not the same as IDAO instance that Component's initialize waits for. You might ask me to change DAO with IDAO in C1, but I can explain why sometimes it's not a good idea.

in Component, I don't want my IDAO instance or DAO instance(whatever it will be to have all the insights(what DAO currently allows to be seen from outside. I want component to just know one function hasPermission on the DAO and that's it.)

If override didn't scream above, I'd achieve the following:

in C1, since it receives DAO instance, I'd be able to use DAO's other functions(for example: dao.checkSomething), but in Component, I'd only be able to check dao.hasPermission as in IDAO, I'd only put hasPermission virtual function. This way, each part knows exactly what it needs.

The way I solved it now(override problem) is that whatever C1 also needs from dao, I put it in IDAO as well and changed C1's Initialize function to accept IDAO. now, Component also knows everything about dao as well.

The gain of having 2 parts only know the things they need, mightn't be too beneficial, it's just the language should be still allowing what this issue is about.

Hope that makes sense.

novaknole avatar Dec 10 '21 14:12 novaknole

The PR with my proposed solution was closed, but if there is a consensus/idea on what should be done to allow this, I'm happy to help and participate :)

jO-Osko avatar Dec 10 '21 14:12 jO-Osko

The issue itself is still open and from what I see the objections in the PR were mainly to the way it was implemented so maybe we can find an alternative way to provide it. I personally think that the feature does make sense and the new use case makes it somewhat more compelling.

cameel avatar Dec 10 '21 15:12 cameel

I'm sorry, but I still think this is much more complex than it looks from the outside, and I would prefer not to do it for now.

chriseth avatar Dec 15 '21 11:12 chriseth

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] avatar Mar 20 '23 12:03 github-actions[bot]

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

github-actions[bot] avatar Mar 28 '23 12:03 github-actions[bot]

@chriseth, @cameel @leonardoalt I just ran into this myself. This is a pretty standard pattern in other languages. See https://en.wikipedia.org/wiki/Covariant_return_type .

My usecase is pretty simple, and currently it's incredibly annoying because it introduces an insane amount of casts in user code and tests without covariant return types. IMO it makes the code more brittle/dangerous without this feature.

To illustrate the need, consider a contract Market , a factory to create instances of the contract, and someone who uses a factory. When all the types are concrete, things are ok:

interface IMarket {
  function buy(uint256 quantity) external returns (uint256);
}

contract Market is IMarket {
  function buy(uint256 quantity) external returns (uint256);

  function otherFunction() external returns (uint256); // function of interest that is not in interface
}

contract MarketFactory {
  function createMarket() external returns (Market);
}

contract MarketFactoryUser {
  function useFactory(MarketFactory factory) external returns () {
    Market market = factory.createMarket();
    market.otherFunction(); // call the concrete function
  }
}

As soon as an interface is introduced to the MarketFactory, all users who deal with the concrete Factory must now cast everything it returns. These casts are completely unnecessary, and could "desensitize" a codebase to casting interfaces to their concrete types, and allow an incorrect cast to slip through elsewhere.

// the same
interface IMarket {
  function buy(uint256 quantity) external returns (uint256);
}

// the same
contract Market is IMarket {
  function buy(uint256 quantity) external returns (uint256);

  function otherFunction() external returns (uint256);
}

// new interface for factory
interface IMarketFactory {
  function createMarket() external returns (IMarket);
}

contract MarketFactory is IMarketFactory {
  // now forced to return the IMarket interface.
  function createMarket() external returns (IMarket);
}

contract MarketFactoryUser {
  function useFactory(MarketFactory factory) external returns () {
    // Even though we are dealing with the concrete factory and KNOW that
    // it returns the concrete Market, we are forced to cast  from the interface.
    Market market = Market(address(factory.createMarket()));
    market.otherFunction(); // call the concrete function
  }
}

My solution for now is to introduce a separate function in the factory that returns the concrete market:

contract MarketFactory is IMarketFactory {
  function createMarket() external returns (IMarket);
  function createMarketConcrete() external returns (Market) {
    return Market(address(createMarket())); // isolate the cast here
  }
}

contract MarketFactoryUser {
  function useFactory(MarketFactory factory) external returns () {
    Market market = factory.createMarketConcrete();
    market.otherFunction(); // call the concrete function
  }
}

KholdStare avatar Nov 15 '23 21:11 KholdStare