Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] Clean up inheritance specification. #3729

Closed
wants to merge 2 commits into from

Conversation

chriseth
Copy link
Contributor

This is a pull request to discuss the changed inheritance specification for version 0.5.0.

@fulldecent
Copy link
Contributor

Can you please explain "the most derived function is called, except when the contract name is explicitly given."

Does that mean in the future I can do this:

pragma solidity ^0.4.21;

contract Animal {
    function toString() private pure returns (string) {
        return "animal";
    }
    
    function favoriteCrackers() external pure returns (string) {
        return /*concat(*/ Animal.toString() /*," crackers")*/;
    }
}

contract Dog is Animal {
    function toString() private pure returns (string) {
        return "dog";
    }
}

and get animal crackers?

@fulldecent
Copy link
Contributor

fulldecent commented Mar 14, 2018

This is confusing

... this means that visibility and mutability of functions cannot be restricted, but it can be extended.

because visibility can be extended, but mutability can be restricted. Really the thing that is being extended is the immutability. Perhaps a way to say this is:

... this means that visibility and mutability of functions can make stronger guarantees, not weaker.

Please consider more explicit override documentation. Perhaps use these tables.

Allowed function overrides:

If a function is... then an override of that function must be one of...
public public
external public, external
internal public, internal
private (must not override)
If an interface/contract function is... then an inheriting interface/contract that overrides that function must be one of...
pure pure
view pure, view
implicit non-payable pure, view, implicit non-payable
payable pure, view, implicit non-payable, payable
If a contract state variable is... then an inheriting contract that overrides that with a function must be one of...
public pure, view, implicit non-payable

NOTE: that last one adds more prescription than what is currently in the PR. The others are simply reference/clarity of what you have already said.

@fulldecent
Copy link
Contributor

Please bump the solidity version on that example in the PR.


Please move todo items from the commit into your top comment in this PR.


This text is killing me:

+It is an error for two contracts in an inheritance hierarchy to have a member of the same name,
+unless one member (f_B) is defined in a class (B) that derives (directly or indirectly) from the
+class (A) the other member (f_A) is defined in; and: f_B uses the override keyword, f_A is a
+function and f_B is either a function or a public state variable, both are compatible with
+regards to visibility and state mutability as per the above rules and both have exactly the same
+parameter and return types.

Perhaps this can be simplified:

Every function of a contract must be defined once, and may be overridden as per the rules above. Therefore it is an error for a contract's inheritance hierarchy (of contracts and interfaces) to include the same function definition more than once.

or...

Every function of a contract must be defined once, and may be overridden as per the rules above. Therefore it is an error for a contract's inheritance hierarchy of contracts to include the same function definition more than once.

The nuance here is whether or not the following should be legal:

interface ERC20 {
   string name ...
}
interface ERC721Metadata {
   string name...
}
contract TokenToNFTConverter is ERC20, ERC721Metadata {
   function name() ...
}

@fulldecent
Copy link
Contributor

Recommend:

  • It is an error for any function to override using a different return type.

@chriseth
Copy link
Contributor Author

@fulldecent can you please comment on the specific lines in the files?

@fulldecent
Copy link
Contributor

I have reviewed all my open issues on this topic and summarized below. Woah.

THIS PR FIXES:

POTENTIAL BLOCKERS:

OTHER NOTES:

@@ -1136,7 +1178,7 @@ Interfaces are denoted by their own keyword:
pragma solidity ^0.4.11;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bump this version, because of external.

This has to be done explicitly using the ``@inherit`` natspec tag. This tag
will inherit all properties that are not re-defined in the function of the derived contract.

TODO:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move TODO to PR discussion, not the PR commits.

Modifiers are treated in much the same way as functions: They use virtual lookup, require the
``override`` specifier, can use overloading and can have a missing implementation.

It is an error for two contracts in an inheritance hierarchy to have a member of the same name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this can be simplified:

Every function of a contract must be defined once, and may be overridden as per the rules above. Therefore it is an error for a contract's inheritance hierarchy (of contracts and interfaces) to include the same function definition more than once.

or...

Every function of a contract must be defined once, and may be overridden as per the rules above. Therefore it is an error for a contract's inheritance hierarchy of contracts to include the same function definition more than once.

More discussion on this is at: #3729 (comment)

@chriseth
Copy link
Contributor Author

Idea from weekly discussion: Overriding and overloading cannot be combined unless all overloads are overridden in the derived class.

@fulldecent
Copy link
Contributor

A note from the weekly discussion. There may be user complaints about the policy to not allow inheriting from two unrelated interfaces/contracts with the same name.

Example: a contract that implements ERC-20 and ERC-721.

A solution: require ERC-20 and ERC-721 to derive from a common base class.

An issue with that: currently the ERC-165 interface identifier does consider functions from inherited interfaces (also codified in ERC-721, 721 requires 165 but does NOT include supportsInterface in its identifier).

@chriseth
Copy link
Contributor Author

Proposal by @axic: Separate inlining and visibility aspect of libraries: #2739 (comment)

@chriseth
Copy link
Contributor Author

@fulldecent sorry, but I still do not fully understand. If ERC-721 is backwards-compatible to ERC-20, then it should inherit from ERC-20, shouldn't it?

Could you also explain the problem with ERC-165?

| A function | ... can be overridden by a |
| with mutability ... | function with mutability ... |
+=====================+=================================+
| ``payable`` | ``payable`` |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not yet clear about this rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A payable function should be able to be overridden by a function with mutability default, view or pure. You can this already today using boilerplate stubs, see: #3412

Also, this rule is assumed by ERC-721.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's probably right. @axic what do you think?

Modifiers are treated in much the same way as functions: They use virtual lookup, require the
``override`` specifier, can use overloading and can have a missing implementation.

It is an error for multiple contracts in an inheritance hierarchy to have a member of the same name,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this is not formal enough yet.

+---------------------+------------------------------+
| ``public`` | ``public`` |
+---------------------+------------------------------+
| ``internal`` | ``internal`` or ``public`` |
Copy link
Contributor

@fulldecent fulldecent Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IGNORE, MY NOTE IS STUPID.

Why not allow a contract with an internal function to be inherited by a contract that broadens that to public or external?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal functions can be overridden by a public function (this is stated here) but not by an external function, because an external function cannot be called internally.

public = internal and external
private = internal plus not visible in derived contracts

+=====================+==============================+
| ``external`` | ``external`` or ``public`` |
+---------------------+------------------------------+
| ``public`` | ``public`` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public is more general than external, please list first to make a logical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I would find it logical to list it in the external to internal order.

| A function | ... can be overridden by a |
| with mutability ... | function with mutability ... |
+=====================+=================================+
| ``payable`` | ``payable`` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A payable function should be able to be overridden by a function with mutability default, view or pure. You can this already today using boilerplate stubs, see: #3412

Also, this rule is assumed by ERC-721.

@fulldecent
Copy link
Contributor

fulldecent commented Mar 15, 2018

@chriseth ERC-721 is NOT backwards compatible to ERC-20. However, some of the function names are intentionally reused.

Here is how 721 currently works:

interface ERC721 /* is ERC165 */ {
    function A...
    function B...
}

interface ERC721Metadata /* is ERC721 */ {
    function C...
    function D...
}

interface ERC721Enumerable /* is ERC721 */ {
    function E...
    function F...
}

The interface identifiers are explicitly stated in EIP-721 and they are:

  • ERC721_ID = hash(A) ^ hash(B)
  • ERC721Metadata_ID = hash(C) ^ hash(D)
  • ERC721Enumerable_ID = hash(E) ^ hash(F)

The specification also states that an ERC-721 contract must support ERC-165 (and support the interface 0x01ffc9a7). However, the supportsInterface is NOT included in function A, function B above.

Also, please note that EIP-137, the ENS (final status), works the same way where "inherited interfaces" are not included in the interface ID.

Pinging @spalladino, because he is working on an important implementation of 721.

Note: Solidity is considering to add something like ERC721.interfaceID which will calculate the ERC-165 identifier for an interface (issue number?). This would be impacted by the way that inheritance of functions is handled.

#. Interfaces cannot inherit other contracts (but they can inherit interfaces).
#. Interfaces cannot define a constructor.
#. Interfaces cannot define variables.
#. Functions in interfaces have to have ``external`` visibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must have

@spalladino
Copy link

Thanks for the heads-up @fulldecent, but the current implementation does not yet support 165. I'll be sure to keep an eye on this discussion before moving onto it.

@chriseth
Copy link
Contributor Author

@fulldecent I think we should decouple the restrictions concerning functions to be overridden and the way the interface id works. If an interface is seen as a mere extension, then its identifier only contains the "new" functions. If it is seen as including the interface it extends, it includes all inherited functions, too. There should probably be two keywords to access the two ways to view the identifier. Even if not, since the identifier is computed using xor, you can just do extensionIdentifier = fullIdentifier ^ baseIdentifier to remove the old functions from the full identifier.

@OTTTO
Copy link

OTTTO commented Mar 27, 2018

@chriseth, in regards to your last reference
I think we should allow public variable declarations in interfaces.

interface ERC20Basic {
  uint256 public totalSupply;
  function balanceOf(address who) public view returns (uint256);
  function transfer(address to, uint256 value) public returns (bool);
  event Transfer(address indexed from, address indexed to, uint256 value);
}

instead of..

contract ERC20Basic {
  function totalSupply() public view returns (uint256);
  function balanceOf(address who) public view returns (uint256);
  function transfer(address to, uint256 value) public returns (bool);
  event Transfer(address indexed from, address indexed to, uint256 value);
}

@fulldecent
Copy link
Contributor

May I please see a specific example where allowing override of just one part of an overloaded function is bad?

@chriseth
Copy link
Contributor Author

chriseth commented Apr 4, 2018

@OTTTO interfaces are not meant to enforce storage layout, and if you use a public state variable in an interface, you do that. You will be able to "implement" an external interface function with a public state variable, though.

@fulldecent it is just a complicated case, and I think it is better to be explicit about those. Readers might not think about the fact that the function in the base contract is used for the other overload.

@chriseth
Copy link
Contributor Author

Perhaps I'm using slightly different definitions. I'm using overloading as: two functions of the same name but different parameter types. Overriding: Two functions of the same name and same parameter types. Out of two overridden functions, we can select one specifically by prefixing with super or a contract name. Out of two overloaded functions, we always select a specific one just by the types of the arguments we provide.

Note that if we disallow super, then there is no (simple) way to prevent the base function to be called twice in the above example.

I think that the introduction of virtual forces you to edit the source of the inherited contract is a good idea, because you will take a look at it again and overriding dramatically changes the semantics of a contract. Take owned for example that defines a modifier onlyOwner (the same argument can be used for functions). If you know its semantics cannot be changed, you don't have to look at it again. If you know the semantics can be changed, you constantly have to check which semantics are currently in place. Note that overriding applies to every internal call in the base class, too.

@fulldecent
Copy link
Contributor

fulldecent commented Sep 24, 2018

Editing the base class every time you want to change something is bad. That leads to copy-paste programming and violates DRY.

Designing the base class to understand potential extensions is good and that is resilience.


Right now we have two access levels: private and internal, and one modifier but it has two different names (external and public).


Proposal for new access control system

Borrowed heavily from Swift -- https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html

Access control levels:

  • private -- no mangling, child contracts cannot see this function, children can reuse this name
  • internal (maybe public, see below) -- child contracts can use these functions, children may not override these
  • open -- child contracts can see and override these functions

ABI modifier:

  • external -- makes a function accessible from the external Ethereum ABI

Usage

This allows six combinations of functions. Examples are shown for a base contracts have each of the six types:

  • private (not external) -- children can reuse the name but it has zero impact on the parent
  • private external -- to avoid confusion, we should probably disallow child overriding when it is private external. Children can access this through call using a new EVM message-call context. Uses calldata types.
  • internal (not external) -- children cannot override. Children can access from within the contract but not using call
  • internal external -- children cannot override. Children can access from within the contract or they can access through call.
  • open (not external) -- children can override the function. The base class and children will use the overriding function (compile-time lookup table) unless the base implementation is specified using super. or ParentClassName.
  • open external -- same as the not-external version except this can also be accessed externally with call.

Interfaces:

This delineation of access control and ABI modifiers has the benefit that interfaces work as-is. Because a private external, internal external or open external implementation is compatible with an external function in an interface.

Discussion

  • This private usage is inconsistent with Python, see my notes above in the test cases
  • Authors of reusable libraries are encouraged to upgrade to open when feasible. But without upgrade, the current code will be safe and more restrictive. Pull requests will be opened. We went through the same situation in the Swift world.
  • internal external may sound weird. But an alternative is to do s/internal/public/g. This is a backwards-compatibility-breaking change. Previously functions that were public will need to become open external to have the same meaning. I do support making this replacement and using public because the word internal has a much different meaning in other OOP languages. In EVM, contracts will only talk to each other using external interfaces, so it is nice for us to kill one of internal or public.

@chriseth
Copy link
Contributor Author

This sounds like a good plan. The current confusion also stems from the fact that we allow external and internal function calls and that external functions cannot be called internally, which allows them to have 'calldata' types. Could you also take that into account in your proposal?

@fulldecent
Copy link
Contributor

@chriseth

For calldata types: so far I only know about:

Any more?

Proposal updated for section private external.

@chriseth
Copy link
Contributor Author

What i'm mainly aiming at: Currently, you can define functions that can only be accessed externally. If you do, reference types are not copied from calldata to memory and thus it is slightly more efficient, especially if you only process the data and do not store it. Do you propose to remove that feature?

@fulldecent
Copy link
Contributor

@chriseth Yes, I propose to remove that feature. I believe this a compiler detail -- the compiler can decide at compile time if calldata or memory is appropriate for each function based on whether that function is called internally or not.

@fulldecent
Copy link
Contributor

fulldecent commented Oct 15, 2018

🗺➡🌎🌍🌏 ~~ Inheritance Manifesto ~~

Inheritance and access control system

A contract can inherit functions and variables from another contract. Inheritance enables you to organize your implementation into distinct sections so that each addresses a separate concern, also it promotes code reuse. Likewise, a contract can inherit from an interface and an interface can inherit from another interface.

Contracts in Solidity can call and access functions and variables belonging to the contracts they inherit and can provide their own overriding versions of these functions and variables to refine or modify their behavior. Solidity helps to ensure your overrides are correct by checking that the override definition has a matching inherited definition.

Access control restricts access to a contract's functions and variables from inheriting classes and external (EVM CALL) message callers. This feature enables you to hide the implementation details of your code, and to specify a preferred means that functions and variables can be accessed and used.

Borrowed heavily from Swift -- https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html

Contracts

Synopsis:

  • function functionName() private
  • [override] function functionName() [external]
  • [override] function functionName() open [external]

Access control levels:

These are regarding internal function calls from an inheriting contract, i.e. not using EVM message calls.

  • private
    • ✅ Callable from within the current contract
    • ❌ Callable from contracts that inherit the current contract
    • ❌ Overridable by contracts that inherit this current contract
    • Note: an inheriting contract may reuse this function name. When calling the function, each contract will use its own implementation.
  • internal (implicit default)
    • ✅ Callable from within the current contract
    • ✅ Callable from contracts that inherit the current contract
    • ❌ Overridable by contracts that inherit this current contract
  • open
    • ✅ Callable from within the current contract
    • ✅ Callable from contracts that inherit the current contract
    • ✅ Overridable by contracts that inherit this current contract
    • Marking a function as open explicitly indicates that you’ve considered the impact of code from other contracts using that function as an override, and that you’ve designed your contract’s code accordingly.

Note: a contract which defines a function without an implementation must mark that function as open.

ABI modifier:

  • external
    • ✅ Callable from any contract using EVM message calls
    • Note: a private function cannot also be external.

Override modifier:

  • override
    • This keyword is required when a contract will override a function from a parent

Interfaces

Interfaces must mark each function as external and must not use the private or open keywords. The implicit default is that these functions are open and are overridable.

An interface may inherit another interface.

Note that the ERC-165 identifier of an interface includes only the items directly in that interface. For an example, please see the identifiers for ERC-721.

Compatible overrides

An overriding function must accept at least all the inputs that the base function accepts. An overriding function must constrict its outputs at least as strong as the constraints on the base function restricts.

Examples

pragma solidity ^x.x.x;

contract Food {}

contract DogFood is Food {}

contract Animal {
    function eat(Food meal) open;
    function name() open pure returns (string);
}

contract Dog is Animal {
    override function eat(DogFood meal) public; // ERROR: `Dogfood` is more restrictive than `Food`
    override function name() returns (string); // ERROR: base contract has `pure` constraint, this has weaker guarantee
}

Which version of the function is used?:

By default, the most derived version of each function is used. For example:

pragma solidity ^x.x.x;

contract Printer {
    function favoriteMessage() open returns (string) {
        return "PC LOAD LETTER";
    }
    function print() external returns (string) {
        return favoriteMessage();
    }
}

contract ModernPrinter is Printer {
    override function favoriteMessage() returns pure (string) {
        return "Paper Underflow";
    }
}

If you will deploy ModernPrinter and call print() then the result will be Paper Underflow, the modern version!

Alternatively, the contract Printer or any inheriting contract may explicitly choose the Printer class implementation by calling Printer.favoriteMessage(). Lastly, a contract may call its own implementation with this.favoriteMessage() or an implementation it inherits from using super.favoriteMessage().


Discussion

  • This private usage is inconsistent with Python, see my notes above in the test cases
  • Authors of reusable libraries are encouraged to upgrade to open when feasible. But without upgrade, the current code will be safe and more restrictive. Pull requests will be opened. We went through the same situation in the Swift world.
  • abstract is an interesting idea but it is entirely additive, let's not consider that now. Part of the pain point it seeks to address (identifying incomplete contracts) could be addressed downstream in Remix IDE.
  • More historical discussion is at Updated proposal based on discussion at https://gitter.im/ethereum/solidity-dev?at=5bc4c63d435c2a518eafe1f9

@fulldecent
Copy link
Contributor

fulldecent commented Oct 23, 2018

BLOCKER:

PING:

@chriseth @axic Please review the blocker above. It may cause a problem with my latest proposal. That affects on the private storage variables part of this proposal.

Ignoring the private storage variables part of the proposal, would you please advise if I am green-light to proceed with documentation and test case updates?

I do understand that bite-sized updates are preferred. Upon green-light, I will create a quick summary of bite-sized changes for your review. When approved, I will do the bite sized changes.

@maurelian
Copy link
Contributor

In the above example, I believe it should be contract ModernPrinter is Printer {?

@fulldecent
Copy link
Contributor

@maurelian Thank you, updated

@fulldecent
Copy link
Contributor

My latest version updated at #3729 (comment). This is a complete version, and I a request your review.

@fulldecent
Copy link
Contributor

fulldecent commented Mar 21, 2019

One side effect of the inheritance manifesto above is that the calldata keyword gets removed because it becomes a compiler detail.

The current approach in 0.5.6 has problems. Here is a concrete example that illustrates this. It compiles with no errors or warnings. The problem is that the developer specifies the location (calldata AND memory) it is undefined behavior where exactly how the compiler will interpret this.

pragma solidity ^0.5.6;

interface Animal {
    function eat(string calldata animalFood) external;
}

contract Dog is Animal {
    function eat(string memory animalFood) public {
        // eat it
    }
    
    function liveANormalDay() external {
        eat("Dog food");
        // play();
        eat("More dog food");
    }
}

This compiles just the same on current 0.6.0 branch.

@chriseth
Copy link
Contributor Author

I actually don't see the problem in that example.

Note that we are currently leaning towards adding more support for calldata structs and arrays - passing them across internal functions and so on.

@fulldecent
Copy link
Contributor

The problem is that the developer should not care where the memory is stored. The compiler can choose the best option every time.

Is there any situation where the developer providing this extra information will produce a better compiler output?

@Marenz
Copy link
Contributor

Marenz commented Nov 5, 2019

What is the next concrete action to move this PR forward?

@fulldecent
Copy link
Contributor

@Marenz The current status is we have

And we have the following resources:

  • Solidity team for some reason wakes up every day and does great stuff
  • I can help as a volunteer, usually with language design, specification, and test cases
  • The community is generally supportive of languages changes implemented by this project

The next concrete actions could be:

  1. Solidity team and stakeholders (that's you!) must indicate what level of change is in appetite
  2. @chriseth and I could get on the phone (this step saves about 6 months of time)
  3. Now I have a clear and nuanced understanding of appetite
  4. I write up specification / test cases
  5. Consensus is achieved from stakeholders (that's you! and Solidity team)
  6. Solidity team swings bat and hits ball out of park
  7. We all go on Stack Overflow to answers questions for the next year if we broke anybody's code

@Marenz
Copy link
Contributor

Marenz commented Nov 6, 2019

@fulldecent part of what you're writing is already implemented, namely override in our develop_060 branch, see tracking issue #5424 additionally to that virtual is currently being added.

@fulldecent
Copy link
Contributor

Cool, thanks for the reference. I have update the Manifesto above to take that into account. And I can do a lot. Just need project buy-in.

@Marenz
Copy link
Contributor

Marenz commented Nov 19, 2019

@fulldecent would you like to join our next meeting on Wednesday 15:00 CET? See https://solidity.readthedocs.io/en/latest/contributing.html#team-calls for the details.

@fulldecent
Copy link
Contributor

Thank you yes I will join will be a few minutes late

@bshastry
Copy link
Contributor

bshastry commented Dec 2, 2019

Summary from the call: @fulldecent plans to open a new manifesto issue after reading up on 0.6.0

@Marenz Marenz closed this Dec 2, 2019
@fulldecent
Copy link
Contributor

I have reread all of my comments here, reran the test cases on 0.6.0 (at e349973) and compared.

Every language design issue is resolved by 0.6.0.

No further actionable language design items remain in this thread.

Some of the things above could be valuable for documentation. I will check up to see what can be added. Otherwise, all done. The manifesto was a success. No new manifesto is needed now.

@bshastry
Copy link
Contributor

bshastry commented Dec 2, 2019

Thank you for the update @fulldecent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants