Public State Variable Overriding

[Marenz]

This is a first draft. Please review the general logic if this is done the way we want.
Details like documentation & changelog will be added ones the basics are approved.

refs #5424
fixes #3514

[Marenz]

wrong title, should be overridingName

[ekpyron]

Could you add test cases for overriding multiple base functions in multiple inheritance and all the other stuff, even though it won't "work"? It'd still be good to see what happens.

[chriseth]

I wonder whether we should take the feature recently brought to light by @fulldecent still into 0.6.0: Allowing to soften the mutability requirements when overriding. It might get complicated because mutability is not a linear order and also when taking multiple inheritance into account, but maybe not...

[chriseth]

That looks really good!

[chriseth]

Even if the check itself seems simple, as @ekpyron we need to check all the cases so that nothing slips under the radar. Things like diamond inheritance where on one branch we have a public state variable and on another we have a function overriding.

[Marenz]

So, this is the output for the diamond test case:

  Contract:
    contract A {
        function foo() external virtual pure returns(uint) { return 5; }
    }
    contract B is A {
        function foo() external virtual override pure returns(uint) { return 5; }
    }
    contract C is A {
        function foo() external virtual override pure returns(uint) { return 5; }
    }
    contract X is B, C {
        uint public override foo;
    }

    TypeError: (271-320): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.

And it's the same for normal multiple inheritance..

[Marenz]

    contract A {
        uint public foo;
    }
    contract B {
        function foo() external virtual pure returns(uint) { return 5; }
    }
    contract X is A, B {
        uint public override foo;
    }

This currently works, but I would suspect this will no longer work once @christianparpart variable shadow PR is merged

[Marenz]

This is now ready

[chriseth]

What about

interface ERC20 {
  function balanceOf(address) external view returns (uint);
}
contract C {
  mapping(address => uint) public balanceOf;
}

[Marenz]

That compiles.. should it?

[Marenz]

ah, I didn't add the is IRC20 part in my test

[Marenz]

still works with the fixes

[chriseth]

Then please change the documentation.

[Marenz]

Apparently I am not checking that the parameters are null.. need to fix that

[chriseth]

Please add a test that shows how this works with overloading, i.e. two functions with the same name in the base contract, once the case where one of the functions matches with regards to parameter types and one case where none of the two match.

[Marenz]

Added the cases and extended the logic a bit

[leonardoalt]

Needs to be rebased.
Is it ready for re-review besides rebasing?
@Marenz

[chriseth]

Is it on purpose that this check is not done after the next?

[chriseth]

Rebasing this.

[chriseth]

Still not too happy with this error message, but it might be fine. At least the error is on the state variable.

[Marenz]

Could change it to Public state variable cannot override non-external function.

[Marenz]

or can only override external function

[Marenz]

(functions with external visibility)

[chriseth]

I'm not happy with it because at this point, we do not yet know whether we have the 'override' specifier given or not.

[leonardoalt]

If it is specified, then the message is correct.
If it's not, then when they fix the external function, the new message override required here will appear, so I guess also fine

[Marenz]

looks good to me, can't approve my own PRs though

[Marenz]

Rebased and squashed.

Tests failing

[leonardoalt]

I think some of the test failures are actually correct

[Marenz]

fixed three tests that expected the wrong phrasing