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

Warn for Inheritance same signature functions and variables #2116

Open
rolandkofler opened this Issue Apr 11, 2017 · 3 comments

Comments

4 participants
@rolandkofler

rolandkofler commented Apr 11, 2017

contract B {
    int public x=10;
    function b() returns (int){return x--;}
}
contract C {
    int public y =1;
    function b() returns (int) {return y++;}
}
contract A is B,C{
    int public z;
    function a(){
        z=b()+b();
    }

}

executing a() yields z=3;

reverting is B,C and a yields z=19;

But this is too confusing and helps to introduce bugs. Better warn if same signatures are detected

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 11, 2017

Contributor

To be more precise: I assume this issue is about a warning in situations where functions are overwritten through multiple inheritance where there is no common base contract that also has the function.

I think this is a good idea for an external static analyzer, but it should not be part of the compiler. The reason is that if you want to use two base contracts that lie outside of your own project, you cannot modify their function names and name clashes might be unavoidable. Note that you can still access specific "instances" of b by using A.b() and B.b().

Contributor

chriseth commented Apr 11, 2017

To be more precise: I assume this issue is about a warning in situations where functions are overwritten through multiple inheritance where there is no common base contract that also has the function.

I think this is a good idea for an external static analyzer, but it should not be part of the compiler. The reason is that if you want to use two base contracts that lie outside of your own project, you cannot modify their function names and name clashes might be unavoidable. Note that you can still access specific "instances" of b by using A.b() and B.b().

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Sep 18, 2017

Member

As agreed in #2563 we should require qualified names when referring to such "conflicting" functions.

Member

axic commented Sep 18, 2017

As agreed in #2563 we should require qualified names when referring to such "conflicting" functions.

@pdaian

This comment has been minimized.

Show comment
Hide comment
@pdaian

pdaian Sep 24, 2017

I've uploaded my thoughts on this issue as a blog post. Specifically, I definitely support either the addition of warnings for multiple overrides at the same level of the developer specified inheritance graph. Requiring unambiguous references to each potentially ambiguous method/field is also not a bad step, but it should be paired with explicit access to any superclass functions when, because of linearization, those functions will not be the ones in a class's explicitly stated superclass (in my blog post this is the case with the WhitelistedCrowdsale call; this class directly extends Crowdsale but its super reference will be to CappedCrowdsale because of linearization. this should be required to be made explicit).

pdaian commented Sep 24, 2017

I've uploaded my thoughts on this issue as a blog post. Specifically, I definitely support either the addition of warnings for multiple overrides at the same level of the developer specified inheritance graph. Requiring unambiguous references to each potentially ambiguous method/field is also not a bad step, but it should be paired with explicit access to any superclass functions when, because of linearization, those functions will not be the ones in a class's explicitly stated superclass (in my blog post this is the case with the WhitelistedCrowdsale call; this class directly extends Crowdsale but its super reference will be to CappedCrowdsale because of linearization. this should be required to be made explicit).

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