Solidity: warn about usage of `this` in constructor

[dwhjames]

This was originally a bug report that turned into a feature request:

The compiler should warn about calling functions on this in the constructor and any function called from the constructor. I think we will not be able to catch all cases (you also have to include modifiers, base classes and other things), but we could at least try.

---

This seems like a compiler bug to me, unless I have a misunderstanding here…

Consider the contract

contract PingPong {
    function PingPong() {
        this.foo(msg.sender, msg.value);
    }
    function () {
        this.foo(msg.sender, msg.value);
    }
    function foo(address originalSender, uint originalValue) {
        originalSender.send(originalValue);
    }
}

You can see this deployed on Morden here: 0x1689e1992bc3690383d74d439e4055c34a2640fe

Upon contract creation, I was expecting the value sent with the tx to be bounced back to the sender. Instead, the contract ends up sending the ETH to itself in an internal tx: msg.sender was resolved to the address of the newly created contract _not_ the true sender.

(The default method works as expected: two internal txs, the second returning the ETH to the original sender.)

Lifting msg.sender to a local name makes no difference:

contract PingPong {
    function PingPong() {
        address originalSender = msg.sender;
        uint originalValue = msg.value;
        this.foo(originalSender, originalValue);
    }
…

Only an internal method call works as expected:

contract PingPong {
    function PingPong() {
        foo(msg.value, msg.sender);
    }
…

Is this a bug? Have I misunderstood something special about the constructor?

[chriseth]

Just tried the following snippet in browser-solidity and it shows different addresses for the contract and the variables x:

contract c {
    address public x;
    function c() { x = msg.sender; }
}

Perhaps you are confusing the contract address with the contract creator address?

[dwhjames]

Hi @chriseth,
Thanks for the reply, but I think you’ve misunderstood my bug report.

In your example c, yes I fully expect the value of address x to be the contract creator address.

Instead, try:

contract c {
    address public x;
    function c() { this.setX(msg.sender); }
    function setX(address _x) { x = _x; }
}

[axic]

@dwhjames I think there might different issues going on here.

For this I get the proper sender stored in x:

contract c {
    address public x;
    function c() { x = msg.sender; }
    function setX(address _x) { x = _x; }
}

For your last example I get 0 stored in x.

[axic]

Update: by removing the this keyword in the constructor from your example the proper value is stored. x = this; works properly, so perhaps it is a bug in the compiler?

[dwhjames]

Indeed, which is exactly the nature of the bug that I’m trying to point out.

Case 1:

contract c {
    address public x;
    function c() {
        address tmp = msg.sender;
        this.setX(tmp);
    }
    function setX(address _x) { x = _x; }
}

Case 2:

contract c {
    address public x;
    function c() {
        address tmp = msg.sender;
        setX(tmp);
    }
    function setX(address _x) { x = _x; }
}

My impression: Case 1 and Case 2 should be observably equivalent (with respect to the final value of x); however, Case 1 is ‘broken’.

It seems _exceedingly_ suspicious to me that the evaluation of msg.sender, stored in the variable tmp, depends on whether the next statement is this.setX(tmp) or setX(tmp)!

[chriseth]

@dwhjames there is a big difference between this.setX and setX. The first is a message-call, i.e. it creates a new call context inside ethereum ("internal transaction") and thus has an effect on msg. The second is a contract-internal call which does not create such an internal transaction.

[dwhjames]

@chriseth, I am aware that there is a big difference this.setX and setX.

You appear to be claiming that the evaluation of:

line 1: address tmp = msg.sender;

_should_ be different, depending on if this next line is

line 2: this.setX(tmp);

or

line 2: setX(tmp);

i.e. rules of lexical scope are broken? And the semantics of the statement on line 1 are affected by the semantics of the statement on a subsequent line?

Can we agree that, _no matter what_, the evaluation of msg.sender, in the scope of a contract constructor, _must_ be the address of the contract creator?

[dwhjames]

And to further drive the point home, my original example was of the following pattern:

contract c {
    address public x;
    function c() {
        address tmp = msg.sender;
        this.setX(tmp);
    }
    function () {
        address tmp = msg.sender;
        this.setX(tmp);
    }
    function setX(address _x) { x = _x; }
}

Could you explain the discrepancy between the constructor and the default function?

[chriseth]

Ok, I'm sorry, I did not read your comment correctly. This is what happens: When the contract is still in construction, its code is not yet deployed to its address (that is a "feature" / limitation of the EVM). This means if you call this.setX(tmp), you call a function on a non-existing contract and thus setX is not really called. When the fallback function is called, the contract does exist and thus setX is correctly called.

[dwhjames]

@chriseth, sorry, but I’m still not convinced by this answer…

Here’s why:

contract c {
    address public x;
    address public y;
    function c() {
        address tmp = msg.sender;
        this.setX(tmp);
        setY(tmp);
    }
    function setX(address _x) { x = _x; }
    function setY(address _y) { y = _y; }
}

• x == 0
• y == creator

For this to be the case, that means that solc has mangled

address tmp = msg.sender;
this.setX(tmp);
setY(tmp);

in such a way that tmp is effective evaluated _twice_, not the once, as it appears lexically.
So it becomes the 0 address in the context of the second line, and the actual creator address on the third line.

(Btw, I am compiling without the optimizer turned on.)

So it would appear that

address tmp = msg.sender;

Is actually getting inlined!? and thus the value of the variable tmp takes on two different values.

[dwhjames]

Ok, I’m starting to see where you’re coming from here…

contract c {
    uint public x;
    uint public y;
    function c() {
        uint tmp = msg.value + 1;
        this.setX(tmp);
        setY(tmp);
    }
    function setX(uint _x) { x = _x + 1; }
    function setY(uint _y) { y = _y + 1; }
}

Send 1 Wei:

• x == 0
• y == 3

So any call to this.someFunction turns into a no-op?

[chriseth]

No, it is not about the variable tmp. The function call this.setX(tmp) just fails, it does not have any effect. Your code is identical to the one where this.setX(tmp) is commented out (minus gas costs of course).

[chriseth]

yep :-)

[dwhjames]

Ok… yikes.

But it’s a no-op that does at least get registered as an internal transaction?
Back to my original example:

contract PingPong {
    function PingPong() {
        this.foo(msg.sender, msg.value);
    }
…

This internal call shows up:
http://testnet.etherscan.io/tx/0x97c23603b226c1001b14eb4d9d70f245e290749e2f09008823e601c9b31a4db0#internal

[chriseth]

Yes, it is an internal transaction that does nothing.

[axic]

Maybe it would be a good enhancement to warn/error if this is used as part of a method call from the constructor? Probably hard to do it as it wouldn't check if any other method called from constructor does the same.

[dwhjames]

A compiler warning for at least this simple case would be great. And some documentation somewhere about this issue; did I miss existing docs that cover this?

I can understand not wanting to do a global static analysis to find violations of using this transitively, but even a warning to cover direct usage is worth something.

[chriseth]

De-prioritizing because it is not a breaking change.

[VoR0220]

first step to this issue appears to be building an "expect warning" system...building

[daira]

Wait, so this serious language design flaw was reported in May 2016 and only got "fixed" –with a compiler warning, not by actually fixing the semantics– just yesterday?

[chriseth]

@daira I would be very happy to welcome you in the weekly solidity dev call to discuss this. It takes place every Wednesday at 17.00 CEST in https://meet.jit.si/SolidityWeekly