-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Conversation
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? |
This is confusing
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:
Please consider more explicit override documentation. Perhaps use these tables. Allowed function overrides:
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. |
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, Perhaps this can be simplified:
or...
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() ...
} |
Recommend:
|
@fulldecent can you please comment on the specific lines in the files? |
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; |
There was a problem hiding this comment.
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
.
docs/contracts.rst
Outdated
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: |
There was a problem hiding this comment.
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.
docs/contracts.rst
Outdated
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, |
There was a problem hiding this comment.
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)
Idea from weekly discussion: Overriding and overloading cannot be combined unless all overloads are overridden in the derived class. |
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 |
Proposal by @axic: Separate inlining and visibility aspect of libraries: #2739 (comment) |
@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`` | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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`` | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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`` | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`` | |
There was a problem hiding this comment.
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.
@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:
The specification also states that an ERC-721 contract must support ERC-165 (and support the interface 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 |
#. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must have
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. |
@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 |
@chriseth, in regards to your last reference
instead of..
|
May I please see a specific example where allowing override of just one part of an overloaded function is bad? |
@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. |
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 |
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 systemBorrowed heavily from Swift -- https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html Access control levels:
ABI modifier:
UsageThis allows six combinations of functions. Examples are shown for a base contracts have each of the six types:
Interfaces: This delineation of access control and ABI modifiers has the benefit that interfaces work as-is. Because a Discussion
|
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? |
For calldata types: so far I only know about:
Any more? Proposal updated for section |
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? |
@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. |
🗺➡🌎🌍🌏 ~~ Inheritance Manifesto ~~
Inheritance and access control systemA 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 Borrowed heavily from Swift -- https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html ContractsSynopsis:
Access control levels: These are regarding internal function calls from an inheriting contract, i.e. not using EVM message calls.
Note: a contract which defines a function without an implementation must mark that function as ABI modifier:
Override modifier:
InterfacesInterfaces must mark each function as 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 overridesAn 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
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 Alternatively, the contract Discussion
|
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. |
In the above example, I believe it should be |
@maurelian Thank you, updated |
My latest version updated at #3729 (comment). This is a complete version, and I a request your review. |
One side effect of the inheritance manifesto above is that the 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.
This compiles just the same on current 0.6.0 branch. |
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. |
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? |
What is the next concrete action to move this PR forward? |
@Marenz The current status is we have
And we have the following resources:
The next concrete actions could be:
|
@fulldecent part of what you're writing is already implemented, namely |
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. |
@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. |
Thank you yes I will join will be a few minutes late |
Summary from the call: @fulldecent plans to open a new manifesto issue after reading up on 0.6.0 |
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. |
Thank you for the update @fulldecent |
This is a pull request to discuss the changed inheritance specification for version 0.5.0.