From 96cabbea21fbf1109be85db15f3afdc9daaa69d7 Mon Sep 17 00:00:00 2001
From: Daniel Kirchner <daniel@ekpyron.org>
Date: Thu, 12 Apr 2018 18:14:48 +0200
Subject: [PATCH] Stricter check for member access to "this" in constructor.

---
 libsolidity/analysis/StaticAnalyzer.cpp       | 16 +++++++++++-----
 libsolidity/analysis/StaticAnalyzer.h         |  4 ++++
 .../syntaxTests/constructor_this.sol          | 12 ++++++++++++
 .../parsing/constructor_self_ref.sol          |  8 ++++++++
 .../syntaxTests/parsing/constructor_super.sol | 19 +++++++++++++++++++
 5 files changed, 54 insertions(+), 5 deletions(-)
 create mode 100644 test/libsolidity/syntaxTests/constructor_this.sol
 create mode 100644 test/libsolidity/syntaxTests/parsing/constructor_self_ref.sol
 create mode 100644 test/libsolidity/syntaxTests/parsing/constructor_super.sol

diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp
index 51aa0b286e34..d00f7a8a5fa2 100644
--- a/libsolidity/analysis/StaticAnalyzer.cpp
+++ b/libsolidity/analysis/StaticAnalyzer.cpp
@@ -100,6 +100,10 @@ bool StaticAnalyzer::visit(Identifier const& _identifier)
 			if (var->isLocalVariable())
 				m_localVarUseCount[make_pair(var->id(), var)] += 1;
 		}
+
+	if (m_constructor && m_memberAccess && _identifier.name() == "this")
+		m_errorReporter.warning(_identifier.location(), "\"this\" used in constructor.");
+
 	return true;
 }
 
@@ -152,6 +156,8 @@ bool StaticAnalyzer::visit(MemberAccess const& _memberAccess)
 {
 	bool const v050 = m_currentContract->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
 
+	m_memberAccess = true;
+
 	if (MagicType const* type = dynamic_cast<MagicType const*>(_memberAccess.expression().annotation().type.get()))
 	{
 		if (type->kind() == MagicType::Kind::Message && _memberAccess.memberName() == "gas")
@@ -206,14 +212,14 @@ bool StaticAnalyzer::visit(MemberAccess const& _memberAccess)
 					);
 			}
 
-	if (m_constructor && m_currentContract)
-		if (ContractType const* type = dynamic_cast<ContractType const*>(_memberAccess.expression().annotation().type.get()))
-			if (type->contractDefinition() == *m_currentContract)
-				m_errorReporter.warning(_memberAccess.location(), "\"this\" used in constructor.");
-
 	return true;
 }
 
+void StaticAnalyzer::endVisit(MemberAccess const&)
+{
+	m_memberAccess = false;
+}
+
 bool StaticAnalyzer::visit(InlineAssembly const& _inlineAssembly)
 {
 	if (!m_currentFunction)
diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h
index 2a62e39113e9..65c3fd3c4323 100644
--- a/libsolidity/analysis/StaticAnalyzer.h
+++ b/libsolidity/analysis/StaticAnalyzer.h
@@ -63,6 +63,7 @@ class StaticAnalyzer: private ASTConstVisitor
 	virtual bool visit(Identifier const& _identifier) override;
 	virtual bool visit(Return const& _return) override;
 	virtual bool visit(MemberAccess const& _memberAccess) override;
+	virtual void endVisit(MemberAccess const& _memberAccess) override;
 	virtual bool visit(InlineAssembly const& _inlineAssembly) override;
 	virtual bool visit(BinaryOperation const& _operation) override;
 	virtual bool visit(FunctionCall const& _functionCall) override;
@@ -88,6 +89,9 @@ class StaticAnalyzer: private ASTConstVisitor
 	/// Flag that indicates a constructor.
 	bool m_constructor = false;
 
+	/// Flag that indicates a member access.
+	bool m_memberAccess = false;
+
 	/// Current contract.
 	ContractDefinition const* m_currentContract = nullptr;
 };
diff --git a/test/libsolidity/syntaxTests/constructor_this.sol b/test/libsolidity/syntaxTests/constructor_this.sol
new file mode 100644
index 000000000000..6f9111ae5b99
--- /dev/null
+++ b/test/libsolidity/syntaxTests/constructor_this.sol
@@ -0,0 +1,12 @@
+contract C {
+    function f() public pure {}
+    constructor() public {
+        C c = this;
+        c.f(); // this does not warn now, but should warn in the future
+        this.f();
+        (this).f();
+    }
+}
+// ----
+// Warning: (172-176): "this" used in constructor.
+// Warning: (191-195): "this" used in constructor.
diff --git a/test/libsolidity/syntaxTests/parsing/constructor_self_ref.sol b/test/libsolidity/syntaxTests/parsing/constructor_self_ref.sol
new file mode 100644
index 000000000000..4dd212dedd88
--- /dev/null
+++ b/test/libsolidity/syntaxTests/parsing/constructor_self_ref.sol
@@ -0,0 +1,8 @@
+contract Foo {
+    bool public bar;
+
+    constructor(Foo other) public {
+        require(other.bar());
+    }
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/parsing/constructor_super.sol b/test/libsolidity/syntaxTests/parsing/constructor_super.sol
new file mode 100644
index 000000000000..0d576d17cc55
--- /dev/null
+++ b/test/libsolidity/syntaxTests/parsing/constructor_super.sol
@@ -0,0 +1,19 @@
+contract A {
+  function x() pure internal {}
+}
+
+contract B is A {
+  constructor() public {
+    super.x();
+  }
+
+  function x() pure internal {
+    require(false);
+  }
+}
+
+contract C is A {
+  constructor() public {
+    x();
+  }
+}