-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 require override for functions from interfaces #8281
Comments
I think this is a delicate question. It is true that override may sound misleading, as it suggests an existing implementation is changed. This makes sense for inherited functions. It is confusing for implementing functions from abstract contracts, too. However, if we would remove the keyword, then it would not be possible to easily tell apart functions specific to |
Yes, I agree. The way I see it, Implementing an abstract function is more of a normal situation. If |
As @axic pointed out, one of the benefits of However, given that we also have I'm with @frangio on this one: if we require |
@frangio @nventuro so you wouldn't want to know if the function actually refers to the declaration in the interface or not? What if you have: interface I and a function |
If More importantly though, if Overrides should be used carefully, with knowledge of the expected behavior of the overridden function in the base contract. Allowing contracts to unknowingly override a base function is IMO dangerous. |
Note that this is only true if |
I'm wondering: Would anyone have a good suggestion for a second keyword for indicating that a function implements a previously unimplemented function? |
Interfaces require that inheriting contracts implement all the methods declared in the interface. There is, by definition, nothing to override. This was a clean intuitive means to guard against incomplete implementations in related contracts. Now, with 0.6, having made that declaration, we aditionally need to tag all the implementations as 'override' - and yet they are not overriding anything. Interfaces were great as they were and now they are, if not broken, certainly diminished. Is there no way to restore interfaces without compromising the goals for this keyword ? |
It feels like removing the requirement for From this discussion, it seems like moving in this direction would be desirable. Does anyone feel strongly about keeping |
Just to strengthen the case against removing the requirement without any replacement:
Now consider two parties - one providing a contract library, and the other using it.
Now you have an auditor confirm that this always does the right stuff. It's a perfectly conformant implementation of I for an implementation of J the user provides. Every conformant implementation of J will get a conformant implementation of I, nothing can go wrong. Based on that the second party writes:
And an auditor confirms: this is fine - But now there is a fancy third party, providing a cheaper, safer and more feature-rich implementation of
And their contracts are audited. It gives you a perfectly conformant implementation of I, if you provide a conformant implementation of J. It is even rumored to be especially safe, since it enforces additional validation. Now
Everything compiles, no reason to doubt this... but you potentially have an utterly broken implementation of the important stuff. That's of course highly artificial and a bit stupid, but I still maintain that allowing to silently implement unimplemented functions is dangerous. And there would be no justification to allow it for interfaces, but not for abstract contracts, which increases the problem. |
Yes, the scenario you've described isn't too different from Solidity v0.5, where you unexpectedly may end up overriding an already defined function. Given the focus on readability and explicitness, somehow signalling that a definition corresponds to a previous declaration makes sense. I like the |
I see the point that implementing an interface function and overriding a previously implemented functions are two different things, but I'm not sure we should add yet another keyword for that. Furthermore, note that you can have a function that, at the same time, implements an interface and changes the functionality of a parallel base class. |
I also don't think we should add another keyword for this. |
If doImportantStuff is actually 'important' to the implementor of B they would not have switched to A2. If it is important to consumers of B presumably the author of B would advertise the important change. I think attempting to solve this scenario (a social problem) with a keyword breaks what inheritiance is for. If as a consumer of B i am very sensitive to (or untrusting of) how doImportantStuff is implemented, what I actually want is contract C is B[using A for doImportantStuff] { I am explicitly opting out of the normal mechanics of inheritance, which permit the provider of B full discretion on how 'doImportantStuff' is implemented. This is more explicit than having to carpet bomb my implementations with a keyword that is, mostly, redundant. Or, rather than overrides everywhere, instead have 'overrides a in b' only in those places where this kind of thing is a problem. |
To be honest, I'm not in favor of introducing a new keyword either. I was hoping that we could establish that it makes sense to somehow indicate that a function implements a previously unimplemented function, because otherwise we give way to implicit unexpected changes in behavior (the otherwise rather silly example was mainly meant to demonstrate a case where this in fact happens). Based on that I'd personally argue that the best means to indicate this is to keep it at requiring Or put differently: you said: "Or, rather than overrides everywhere, instead have 'overrides a in b' only in those places where this kind of thing is a problem." - I agree with that, but I would argue that implementing unimplemented functions is a place where this kind of thing is a problem. |
Thats a fair response. I quible over 'implementing unimplemented' in the case of interfaces. Do you think this situation is an inevitable consequence of mixing 'abstract' with 'interfaces' ? Ie, what interfaces led me to expect is just simply not the right way to think about smart contracts ? |
Does it help to read |
Hah! I think that is quite nice mental judo but in my case I read interfaces as saying "I don't refer to anything yet and you must fill that in". But that is a very special case of what is generally going on in significant inheritance graphs. I'm content to adapt. |
I also feel strongly for removing the |
@elenadimitrova what about an abstract contract that adds some functionality to an interface but not everything? |
What exactly bothers you there? |
"we could enforce all interface methods are implemented in a contract" - we already do enforce that unless the contract is abstract. |
Am I correct to understand that I have to do // SPDX-License-Identifier: MIT
pragma solidity ^0.6.10;
/**
* @title Owner
* @dev Set & change owner
*/
interface Foo {
function foo1() external view returns (uint);
function bar1() external view returns (uint);
}
abstract contract Bar is Foo {
function bar1() external virtual override view returns (uint) {}
function foo1() external view override returns (uint){
return this.bar1();
}
}
contract Baz is Bar {
function bar1() external override view returns (uint){
return 1;
}
} I was currently trying to upgrade my smartcontract but I end up adding |
All of the arguments in favour of override seem to miss the point of ineheritance all together. They are based on largely synthetic examples. And those examples really only illustrate well understood 'gotchas' with Inheritance in general. Really if there is a compeling case for override then inheritance is too dangerous to have in the language all together (somewhat toungue in cheek). Developers who prioritize explicitness over the established conventions for inheritance are free not to inherit. The burden is then theirs. With respect, override seems to be little more than an opinionated burden on the rest of us. And it is certainly not helping us in any way I recognise. Remove override or remove inheritance ;-) |
This is IMO the key issue: |
@makoto is your intention that |
Another data point on this.... As I was reviewing code, I saw a function definition with an |
Seeing this issue for the first time, wanted to add my support. I assume that at least some of the motivation for the
|
We discussed this on the last call. There seems to be no consensus here so we need more feedback. To make discussion a bit more organized and bring more attention to this issue I have created a forum post laying out the available options and summarizing the most important points from this discussion: Should overriding function declarations require the |
Since we're discussing this again: The example case in which not requiring So while I personally still think it conceptually makes sense to mark functions overriding interface functions, I'd not object to drop that requirement based on popular opinion :-). |
imo |
The override keyword is required when implementing a function from a parent interface.
This doesn't sound right to me. The function is not overriding anything, it's simply filling in a missing abstract function.
The text was updated successfully, but these errors were encountered: