Require 'override' and 'virtual' keyword

[chriseth]

Related to #2563

Steps:

• private functions cannot be overridden at all (virtual and private cannot be combined, and private functions are "visible" for overriding detection) Disallow use of virtual and private together #7807
• override for public state variables, but they can't be virtual Public State Variable Overriding #7839
• override Implement override keyword #7563
• virtual
• common base / cut vertex - override exception

[chriseth]

draft of specification:

• interface functions are always virtual implicitly
• functions coming in from different bases are disallowed, unless they override the same function in a shared base
• public state variables can override external virtual functions, but need to be marked "override", but cannot be marked virtual themselves. (can they be overridden again?)
• overriding an "override" function is fine
• private functions cannot be overridden at all (virtual and private cannot be combined, and private functions are "visible" for overriding detection)
• functions without implementation still need virtual

[chriseth]

How to handle combinations of overloading and overriding?

[ekpyron]

I tend to make functions that are marked override not overridable themselves unless they are again explicitly marked virtual again. Otherwise we need to think about whether we need an additional final keyword and I think just sticking to "only things explicitly marked virtual can be overriden" is easier and better than doing that... if overriding state variables are allowed to be overwritten again, that may be an issue though - but maybe that shouldn't be possible anyways (not sure about that)...

[ekpyron]

And I think it makes sense to introduce the keywords themselves in 0.6.0, but to only deprecate omitting them. So in 0.6.0 omitting the keywords would cause warnings, unless you specify e.g. pragma legacy; or pragma ignore deprecated; or sth like that and these warnings turn into errors with 0.7.0. That may be a good way to get around the adoption and real-world testing problem and if it works well we could re-use the concept for future changes.
EDIT: maybe even pragma compat 0.5;

[nventuro]

Will this info (e.g. a function being virtual) be available in the contract artifacts? It'd interesting if we were able to e.g. have tests asserting a certain function is virtual to prevent regression errors.

[axic]

contract artifacts

What do you exactly mean by that?

[nventuro]

I guess the most appropriate location for such a thing would be the JSON AST (which is included in the .json file that results from compilation when using e.g. truffle).

[Marenz]

To recap my understanding of the goals:

• override to explicitly show that a function is overriding another function from a base
• virtual to explicitly allow overriding in a derived function

I would argue that users want to do the virtual use case a lot and thus virtual would need to be used a lot. If that kind of usage is the default, wouldn't it be smarter to make the other way explicit, e.g. having a final keyword to mark a function explicitly as not overridable?

[Marenz]

How to handle combinations of overloading and overriding?

I'd say the overriding function function signature must match the signature exactly, that should include any overloads, too.
Or did I miss a scenario where this isn't enough?

[chriseth]

I'd say the overriding function function signature must match the signature exactly, that should include any overloads, too.

Yes, I think this is how it is handled currently.

[chriseth]

About the default: I'm actually not sure if virtual is really that common. In any case, if we do not default to non-virtual, this feature is useless.

[Marenz]

About the default: I'm actually not sure if virtual is really that common. In any case, if we do not default to non-virtual, this feature is useless.

Well, yes, my suggestion was to basically use final instead of virtual with virtual as the default. But if it is less common then my main argument doesn't apply anymore. We should gather some data.

[chriseth]

Discussion resulted in relaxation of the requirements for the case of a function being inherited from multiple base classes:

A function declaration D1 is called a "base function" of a function declaration D2 if they declare the same function, D1 is in a (direct or indirect) base contract of the contract of D2 and no contract in the path through the inheritance graph contains the function.

If a function has a base function, it needs the "override" specifier and the base function needs the "virtual" specifier (which is implied in interfaces).
If a function has more than one base functions, the "override" specifier needs to list all contract types of all the base functions.

[chriseth]

In addition to that: It is illegal, if a function that is not declared in a contract would have multiple base functions.

[ekpyron]

I'm still not entirely sure about this case and fine with both options, but I wanted to bring it up once more:

contract A { function f() virtual {...} }
contract B is A {}
contract C is B { function f() override {...} }

Should this be valid or not? We said ǹot having f in B indicates "do everything like the base", but does that mean "always use A's implementation of f" or "inherit the fact that A's f isvirtual"?

There's two cases: either B wants to allow overriding f or wants to disallow it, the question is what's the default.

Explicit allowing would mean you need to write
contract B is A { function f() virtual override { A.f(); } }
if overriding f in C should be possible.

Explicit disallowing would mean you need to write
contract B is A { function f() override { A.f(); } }
if overriding f in C should not be possible.

@chriseth Your specification would mean defaulting to allowing. Do we want to do it that way? I'm fine with it, just reconfirming.

[ekpyron]

I was just looking over the draft PR(s) for implementing this - and I'm realizing: requiring to redeclare functions of the base "virtual" again to allow them to be overridden would make the implementation much simpler, since we'd only need to check one level at a time :-). And by the same logic I could imagine that this would make auditing code easier, so I'm actually still leaning towards that option...

[chriseth]

Can you ask the list?

[ekpyron]

One option that will get us around discussing which should be the default is to allow both, i.e. to have "virtual" and "final" inheritance, but I'm not sure, if we should seriously consider that :-).

[fasteater]

if B is A , B should inherit everything from A, which means B.f should also be virtual

[ericDeCourcy]

public state variables can override external virtual functions, but need to be marked "override", but cannot be marked virtual themselves. (can they be overridden again?)

How would this work in the case of an overloaded functions? Is this portion of the spec still being considered?

[chriseth]

@ericDeCourcy currently, overloading and overriding are considered to be unrelated. We might want to restrict this, but probably not for 0.6.0.

[ekpyron]

Our current specification has one hole:

contract A {
  function f() external {...}
}
contract B {
  function f() external {...}
}
contract C is A,B {
  function f() external override(A,B) { /* wants to call A.f, but can't */ }
}

[ekpyron]

Which we could solve be allowing
function f() external override(A,B) = A.f;
Which would also be nice to have for non-external functions.

[chriseth]

Sounds good! I would not allow anything like A.g for now, but we might in the future.

[Marenz]

for reference, the current PR for override is #7320

[ekpyron]

Which we could solve be allowing
function f() external override(A,B) = A.f;
Which would also be nice to have for non-external functions.

Do we want this for 0.6.0 as well?

[chriseth]

Maybe better to introduce calldata variables for public functions?