Stricter check for "this" in constructor.

[ekpyron]

Fixes #3843.
Fixes #3861.

[axic]

My first hunch was this should be a bug as you did, but after further consideration it may make sense to support this.

There were some examples in the past of splitting/forking contracts, where the reference is to another contract of the same type.

[chriseth]

I agree, this is not the way to fix it :)

I think the condition in StaticAnalyzer.cpp:209 has to be tighter than it currently is.

[chriseth]

I think #3861 and #3843 are basically the same thing.

[ekpyron]

OK, I remembered talking about this with you and concluding that such arguments would be an error in general - that's why I went ahead and did it this way :-).

Since there can only be one constructor per contract, how can a constructor with another contract of the same type ever be called in the first place :-)? I'm just wondering what such an example could look like...

[chriseth]

ok, that's a fair point, but we also have interfaces and stuff...

[chriseth]

I wouldn't restrict it in such a way.

[ekpyron]

Alright!

[axic]

This should be valid:

contract C {
  C parent;

  function hasParent() pure internal returns (bool) {
    return address(parent) != 0;
  }

  function C(C _parent) {
    parent = _parent;
  }

  function fork() returns (C) {
    require(!hasParent(), "multiple level of forks are not allowed");
    return new C(this);
  }
}

And the the first create transaction can just pass an address 0 for the initial contract.

[ekpyron]

Ah OK! I hadn't thought about that!

[axic]

@ekpyron can you just fix it here properly? :)

[ekpyron]

@axic OK - my plan was to open a new PR once it's finished, but I might as well reuse this one :-). I'll be away for an hour now, though, so it'll take until afterwards.

[ekpyron]

I think I need some clarification on this one. I'm not sure this can be properly fixed.

Are there cases in which an occurrence of this is valid, resp. shouldn't be warned about? For example what about the following case?

contract C {
    address m_d;
    constructor(address d) public {
        m_d = d;
    }
}
contract D {
    C m_c;
    constructor() public {
        m_c = new C(this);
    }
}

The discussion in #1646 (comment) seems to indicate that this should be valid.

If this shouldn't trigger a warning, then it might be tricky to warn about cases like this:

contract C {
    address m_d;
    constructor(address d) public {
        m_d = d;
    }
    function d() public view returns (address) {
        return m_d;
    }
}
contract D {
    C m_c;
    constructor() public {
        m_c = new C(this);
        D(m_c.d()).f();
    }
    function f() public pure {
    }
}

Recognizing that D(m_c.d()) will evaluate to this during the analysis phase is non-trivial (and impossible in general, since it may depend on run-time state in more complex examples).

The original discussion in #583 is rather inconclusive as well...

The current state of the PR warns about these cases:

contract C {
  function f() public {
  }
  constructor() {
    this.f();
    (this).f();
  }
}

But will already fail to warn for this case:

contract C {
  function f() public {
  }
  constructor() {
    C c = this;
    c.f();
  }
}

So currently I'm not sure - it doesn't seem that we can warn in all cases that warrant a warning, but I'm not sure which subset of them I should try to catch...

[ekpyron]

Sorry, I think this is still not a good solution. I'll improve it now and comment after I'm done.

[chriseth]

While it is nice that we can catch (this).f() using this mechanism, I think it would also generate an error on f(this).f(), which should in general not be the case. Constructors cannot have nested non-constructor functions, but a member access expression can, so I don't think this can be solved by using stateful AST traversal.

[ekpyron]

This was exactly the problem I was just thinking about. However, "f(this).f()" does not seem to trigger the warning - I just added a test for that.

[ekpyron]

Just trying to think of a case in which this will in fact fail.
Otherwise I could just check for this in the member access again - either while also checking for (this) or while ignoring cases like (this)...

[ekpyron]

Actually I expected test/libsolidity/syntaxTests/parsing/constructor_allowed_this.sol to cause a warning because of this, but as a matter of fact it doesn't...

[ekpyron]

Ah, ok, now I got one that fails - I'll revert back to checking in member access.

[chriseth]

Please reduce the test case to its bare minimum.

[chriseth]

And perhaps also add a comment that this tests a former bug in the detection of this.f().

PR updated

[ekpyron]

Now it should be fine - although IMO it's not particularly nice this way.

I'm really starting to think that we should consider #3878.

[chriseth]

Perhaps we should also extend the message a little: \"this\" used in constructor. Note that external functions of a contract cannot be called while it is being constructed. or something like that.