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

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

Open
frangio opened this Issue Jul 12, 2017 · 19 comments

Comments

@frangio
Contributor

frangio commented Jul 12, 2017

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

This comment has been minimized.

Member

axic commented Jul 12, 2017

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

This comment has been minimized.

Contributor

frangio commented Jul 12, 2017

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

@federicobond

This comment has been minimized.

Contributor

federicobond commented Jul 12, 2017

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

This comment has been minimized.

Contributor

chriseth commented Jul 13, 2017

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

This comment has been minimized.

Contributor

frangio commented Jul 13, 2017

@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

This comment has been minimized.

SCBuergel commented Aug 8, 2017

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

This comment has been minimized.

Contributor

frangio commented Sep 7, 2017

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

@chriseth

This comment has been minimized.

Contributor

chriseth commented Sep 7, 2017

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

@axic

This comment has been minimized.

Member

axic commented Sep 7, 2017

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 axic added the Focus label Sep 14, 2017

@axic

This comment has been minimized.

Member

axic commented Sep 15, 2017

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

This comment has been minimized.

Member

axic commented Sep 18, 2017

Also cover #2116.

alex-kampa added a commit to GizerInc/crowdsale that referenced this issue Nov 2, 2017

Changes to GizerTokenTest (variable overriding did not work)
Overriding variables in a child contract does not work, this is for example discussed here: ethereum/solidity#2563
@alex-kampa

This comment has been minimized.

alex-kampa commented Nov 3, 2017

Re "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" : different languages handle this differently, so this is about understanding Solidity inheritance, not inheritance in general. Most languages do allow different forms of overriding, some even by default.

Personally I would very much welcome the "override" keyword, recently had a concrete need to change a few constants and "override" would have made the process simpler and more elegant.

@7flash

This comment has been minimized.

7flash commented Nov 11, 2017

Is there any way to override variable now?


contract Tokensale {
    uint hardcap = 10000 ether;

    function Tokensale() {}

    function fetchCap() public constant returns(uint) {
        return hardcap;
    }

}

contract Presale is Tokensale {
    uint hardcap = 1000 ether;

    function Presale() Tokensale() {}
}

To my disappointment, fetchCap returns 10000 ether. How to deal with this when I have a lot of common code in parent contract?

@sessai

This comment has been minimized.

sessai commented Nov 11, 2017

@7flash - add the fetchCap() function into the Presale contract, which should override the parent's fetchCap

contract Presale is Tokensale {
  
   ...

    function fetchCap() public constant returns(uint) {
        return hardcap;
    }
   ...
}

This is a pretty boring limitation of Solidity - in your case not a big deal as the function affected is short. Much more annoying if dealing with a long/complex function.

@7flash

This comment has been minimized.

7flash commented Nov 12, 2017

@sessai Exactly, that's the problem. Tokensale contract has complex functions related to hardcap. Should I duplicate these functions in presale? There is any better solution?

@7flash

This comment has been minimized.

7flash commented Nov 12, 2017

Here is valid solution, validPayment returns false as expected.

pragma solidity ^0.4.11;

contract Tokensale {
    function Tokensale(){
    }

    function validPayment() public constant returns(bool) {
        bool confirmed = true; // some complex calculations
        uint weiRaised = 1001 ether;
        return confirmed && weiRaised <= getHardcap();
    }
    
    function getHardcap() public constant returns(uint) {
        return 10000 ether;
    }
}

contract Presale is Tokensale {
    function Presale() Tokensale() {}
    
    function getHardcap() public constant returns(uint) {
        return 1000 ether;
    }
}
@7flash

This comment has been minimized.

7flash commented Nov 12, 2017

But there is a way to override generated getter function to write something like this?

// doesn't compile
pragma solidity ^0.4.11;

contract Tokensale {
    uint hardcap = 10000 ether;
    
    function Tokensale(){
    }

    function validPayment() public constant returns(bool) {
        bool confirmed = true; // some complex calculations
        uint weiRaised = 1001 ether;
        return confirmed && weiRaised <= hardcap;
    }
}

contract Presale is Tokensale {
    function Presale() Tokensale() {}
    
    function hardcap() public constant returns(uint) {
        return 1000 ether;
    }
}
@sessai

This comment has been minimized.

sessai commented Nov 12, 2017

@7flash - as far as I know you can't override a variable with a function.

The only solution I can imagine is to foresee this in the parent contract already, and instead of declaring variables write functions. So instead of:

contract Tokensale {
    uint hardcap = 10000 ether;
 }

write:

contract Tokensale {
    function hardcap() public constant returns(uint) { return 10000 ether; }
 }

And then override the function in the child contract.

Yes this is VERY BORING! If anyone here has another solution, please let us know.

@o0ragman0o

This comment has been minimized.

o0ragman0o commented Nov 12, 2017

The safer option is to assign TokenSale.hardcap in Presale constructor.

contract Presale is Tokensale {
    function Presale() Tokensale() {
        hardcap = 1000;
    }
}

@chriseth chriseth added this to To discuss in Backlog (breaking) via automation May 14, 2018

@chriseth chriseth removed this from To do in 0.5.0 May 14, 2018

@chriseth chriseth added this to Under discussion in Zeppelin Audit via automation Jul 5, 2018

@axic axic changed the title from Shadowing of inherited state variables should be an error to Shadowing of inherited state variables should be an error (override keyword) Jul 28, 2018

@axic axic removed the Focus label Jul 28, 2018

@chriseth chriseth added this to To do in 0.6.0 via automation Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment