Shadowing of inherited state variables should be an error (override keyword)

[frangio]

Code like the following should be an error:

contract Base {
  uint x;
}
contract Derived is Base {
  uint x;
}

Functions defined in Base that use the x state variable will access Base.x, and those defined in Derived will silently access the different slot Derived.x. This is a common problem for beginners that don't understand inheritance very well, and it could happen accidentally to anyone as well. The compiler should fail to compile it to prevent these errors.

This is different (and I believe more serious) than #973. It is about state variables shadowing other (inherited) state variables, possibly defined in different files and so harder to see.

[axic]

We had a short discussion with @federicobond yesterday about the possibility of introducing an override keyword (a la C++) when overriding an inherited variable and issuing an error otherwise.

[frangio]

C++ override applies to virtual functions, though. What would be the semantics here?

[federicobond]

The override keyword lets you be explicit about your intention to override a variable higher up the inheritance graph. Take OpenZeppelin's SimpleToken for example:

contract SimpleToken is StandardToken {
  string public name = "SimpleToken";
  string public symbol = "SIM";
  uint256 public decimals = 18;
  // ...
}

If you were to change the initial value of any of these state variables, you would need to be explicit:

contract MyToken is SimpleToken {
  string override public name = "MyToken";
  string override public symbol = "MYT";
}

It can also help prevent typos such as these:

function trasnfer(address _to, uint256 _value) override {
  require(veryImportantSecurityCheck());
  super.transfer(_to, _value);
}

In the last case, since you are not overriding any function, the compiler will complain.

[chriseth]

I'm not sure override can cleanly apply to state variables, because the only thing that is overridden is the assignment that is done as part of the constructor, but I guess it is intuitive enough.

[frangio]

@federicobond SimpleToken is an example of a concrete token, not meant to be inherited. It's confusing, we should move it to a different directory together with other examples.

If I understand well, the only use of override for state variables would be redefining their initial value. Since this can already be done in the constructor, the only real value it would add on this front (other than syntax) is redefining the initial value of constant variables. I would say it's too much complexity for such small returns.

In any case I think we agree that this kind of shadowing should be an error by default.

[SCBuergel]

Agree with all of the above and want to point out another dangerous example that I just came across:

Libraries like OpenZeppelin implement common tasks in generalised code with a long inheritance graph. That is all well until a developer overwrites a state variable of the base contract that is within the same contract still being used (but should refer to the base's version of it). E.g. the base Crowdsale contract has a token that a user might accidentally silently overwrite and assign another token in some other function.

The issue was luckily not "out there" yet but shows how dangerous this behaviour is.

[frangio]

@axic @chriseth Leaving discussion of override aside, do we agree that shadowing of inherited state variables should at least produce a warning?

[chriseth]

Yes, I agree, and we should probably apply the same logic to functions unless they have an override modifier.

[axic]

I'm not sure override can cleanly apply to state variables, because the only thing that is overridden is the assignment that is done as part of the constructor, but I guess it is intuitive enough.

For state variables currently we do not produce any warning even if the types differ during shadowing. I'd suggest that unless it is marked private in the parent:

• a) show a warning if it has the same type, unless the override keyword is present
• b) throw an error if the types differ

For b) we can only introduce a warning now, but should turn into an error in 0.5.0.

[axic]

From BLWS'17:

• Raise a warning now (an error in the breaking release) for shadowing storage variables
• Require qualification in terms of conflicting variable names from two parent contracts (e.g. refer to them as Parent1.variable and Parent2.variable)
• Require the override keyword for functions

[axic]

Also cover #2116.