Stricter check for member access to "this" in constructor.

ethereum · Apr 13, 2018 · 96cabbe · 96cabbe
1 parent c3dc67d
commit 96cabbe
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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();
+  }
+}