Merge pull request #549 from chriseth/sol_saveScope

Disallow assignments to structs and mappings

libsolidity/AST.cpp

@@ -378,6 +378,9 @@ void Assignment::checkTypeRequirements()
 {
 	m_leftHandSide->checkTypeRequirements();
 	m_leftHandSide->requireLValue();
+	//@todo later, assignments to structs might be possible, but not to mappings
+	if (!m_leftHandSide->getType()->isValueType() && !m_leftHandSide->isLocalLValue())
+		BOOST_THROW_EXCEPTION(createTypeError("Assignment to non-local non-value lvalue."));
 	m_rightHandSide->expectType(*m_leftHandSide->getType());
 	m_type = m_leftHandSide->getType();
 	if (m_assigmentOperator != Token::ASSIGN)
@@ -403,7 +406,7 @@ void Expression::expectType(Type const& _expectedType)
 
 void Expression::requireLValue()
 {
-	if (!isLvalue())
+	if (!isLValue())
 		BOOST_THROW_EXCEPTION(createTypeError("Expression has to be an lvalue."));
 	m_lvalueRequested = true;
 }
@@ -495,7 +498,8 @@ void MemberAccess::checkTypeRequirements()
 	m_type = type.getMemberType(*m_memberName);
 	if (!m_type)
 		BOOST_THROW_EXCEPTION(createTypeError("Member \"" + *m_memberName + "\" not found in " + type.toString()));
-	m_isLvalue = (type.getCategory() == Type::Category::STRUCT && m_type->getCategory() != Type::Category::MAPPING);
+	//@todo later, this will not always be STORAGE
+	m_lvalue = type.getCategory() == Type::Category::STRUCT ? LValueType::STORAGE : LValueType::NONE;
 }
 
 void IndexAccess::checkTypeRequirements()
@@ -507,7 +511,7 @@ void IndexAccess::checkTypeRequirements()
 	MappingType const& type = dynamic_cast<MappingType const&>(*m_base->getType());
 	m_index->expectType(*type.getKeyType());
 	m_type = type.getValueType();
-	m_isLvalue = m_type->getCategory() != Type::Category::MAPPING;
+	m_lvalue = LValueType::STORAGE;
 }
 
 void Identifier::checkTypeRequirements()
@@ -521,7 +525,7 @@ void Identifier::checkTypeRequirements()
 		if (!variable->getType())
 			BOOST_THROW_EXCEPTION(createTypeError("Variable referenced before type could be determined."));
 		m_type = variable->getType();
-		m_isLvalue = true;
+		m_lvalue = variable->isLocalVariable() ? LValueType::LOCAL : LValueType::STORAGE;
 		return;
 	}
 	//@todo can we unify these with TypeName::toType()?

libsolidity/AST.h

@@ -88,11 +88,16 @@ public:
 	Declaration(Location const& _location, ASTPointer<ASTString> const& _name):
 		ASTNode(_location), m_name(_name) {}
 
-	/// Returns the declared name.
+	/// @returns the declared name.
 	ASTString const& getName() const { return *m_name; }
+	/// @returns the scope this declaration resides in. Can be nullptr if it is the global scope.
+	/// Available only after name and type resolution step.
+	Declaration* getScope() const { return m_scope; }
+	void setScope(Declaration* const& _scope) { m_scope = _scope; }
 
 private:
 	ASTPointer<ASTString> m_name;
+	Declaration* m_scope;
 };
 
 /**
@@ -237,6 +242,8 @@ public:
 	std::shared_ptr<Type const> const& getType() const { return m_type; }
 	void setType(std::shared_ptr<Type const> const& _type) { m_type = _type; }
 
+	bool isLocalVariable() const { return !!dynamic_cast<FunctionDefinition*>(getScope()); }
+
 private:
 	ASTPointer<TypeName> m_typeName; ///< can be empty ("var")
 
@@ -521,12 +528,16 @@ private:
  */
 class Expression: public ASTNode
 {
+protected:
+	enum class LValueType { NONE, LOCAL, STORAGE };
+
 public:
-	Expression(Location const& _location): ASTNode(_location), m_isLvalue(false), m_lvalueRequested(false) {}
+	Expression(Location const& _location): ASTNode(_location), m_lvalue(LValueType::NONE), m_lvalueRequested(false) {}
 	virtual void checkTypeRequirements() = 0;
 
 	std::shared_ptr<Type const> const& getType() const { return m_type; }
-	bool isLvalue() const { return m_isLvalue; }
+	bool isLValue() const { return m_lvalue != LValueType::NONE; }
+	bool isLocalLValue() const { return m_lvalue == LValueType::LOCAL; }
 
 	/// Helper function, infer the type via @ref checkTypeRequirements and then check that it
 	/// is implicitly convertible to @a _expectedType. If not, throw exception.
@@ -541,9 +552,9 @@ public:
 protected:
 	//! Inferred type of the expression, only filled after a call to checkTypeRequirements().
 	std::shared_ptr<Type const> m_type;
-	//! Whether or not this expression is an lvalue, i.e. something that can be assigned to.
+	//! If this expression is an lvalue (i.e. something that can be assigned to) and is stored
-	//! This is set during calls to @a checkTypeRequirements()
+	//! locally or in storage. This is set during calls to @a checkTypeRequirements()
-	bool m_isLvalue;
+	LValueType m_lvalue;
 	//! Whether the outer expression requested the address (true) or the value (false) of this expression.
 	bool m_lvalueRequested;
 };

libsolidity/Scope.cpp → libsolidity/DeclarationContainer.cpp

@@ -20,7 +20,7 @@
  * Scope - object that holds declaration of names.
  */
 
-#include <libsolidity/Scope.h>
+#include <libsolidity/DeclarationContainer.h>
 #include <libsolidity/AST.h>
 
 namespace dev
@@ -28,7 +28,7 @@ namespace dev
 namespace solidity
 {
 
-bool Scope::registerDeclaration(Declaration& _declaration)
+bool DeclarationContainer::registerDeclaration(Declaration& _declaration)
 {
 	if (m_declarations.find(_declaration.getName()) != m_declarations.end())
 		return false;
@@ -36,13 +36,13 @@ bool Scope::registerDeclaration(Declaration& _declaration)
 	return true;
 }
 
-Declaration* Scope::resolveName(ASTString const& _name, bool _recursive) const
+Declaration* DeclarationContainer::resolveName(ASTString const& _name, bool _recursive) const
 {
 	auto result = m_declarations.find(_name);
 	if (result != m_declarations.end())
 		return result->second;
-	if (_recursive && m_enclosingScope)
+	if (_recursive && m_enclosingContainer)
-		return m_enclosingScope->resolveName(_name, true);
+		return m_enclosingContainer->resolveName(_name, true);
 	return nullptr;
 }
 

libsolidity/Scope.h → libsolidity/DeclarationContainer.h

@@ -36,18 +36,20 @@ namespace solidity
  * Container that stores mappings betwee names and declarations. It also contains a link to the
  * enclosing scope.
  */
-class Scope
+class DeclarationContainer
 {
 public:
-	explicit Scope(Scope* _enclosingScope = nullptr): m_enclosingScope(_enclosingScope) {}
+	explicit DeclarationContainer(Declaration* _enclosingDeclaration = nullptr, DeclarationContainer* _enclosingContainer = nullptr):
+		m_enclosingDeclaration(_enclosingDeclaration), m_enclosingContainer(_enclosingContainer) {}
 	/// Registers the declaration in the scope unless its name is already declared. Returns true iff
 	/// it was not yet declared.
 	bool registerDeclaration(Declaration& _declaration);
 	Declaration* resolveName(ASTString const& _name, bool _recursive = false) const;
-	Scope* getEnclosingScope() const { return m_enclosingScope; }
+	Declaration* getEnclosingDeclaration() const { return m_enclosingDeclaration; }
 
 private:
-	Scope* m_enclosingScope;
+	Declaration* m_enclosingDeclaration;
+	DeclarationContainer* m_enclosingContainer;
 	std::map<ASTString, Declaration*> m_declarations;
 };
 

libsolidity/NameAndTypeResolver.cpp

@@ -78,9 +78,9 @@ Declaration* NameAndTypeResolver::getNameFromCurrentScope(ASTString const& _name
 	return m_currentScope->resolveName(_name, _recursive);
 }
 
-DeclarationRegistrationHelper::DeclarationRegistrationHelper(map<ASTNode const*, Scope>& _scopes,
+DeclarationRegistrationHelper::DeclarationRegistrationHelper(map<ASTNode const*, DeclarationContainer>& _scopes,
 															 ASTNode& _astRoot):
-	m_scopes(_scopes), m_currentScope(&m_scopes[nullptr])
+	m_scopes(_scopes), m_currentScope(nullptr)
 {
 	_astRoot.accept(*this);
 }
@@ -135,31 +135,30 @@ bool DeclarationRegistrationHelper::visit(VariableDeclaration& _declaration)
 	return true;
 }
 
-void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _node)
+void DeclarationRegistrationHelper::enterNewSubScope(Declaration& _declaration)
 {
-	map<ASTNode const*, Scope>::iterator iter;
+	map<ASTNode const*, DeclarationContainer>::iterator iter;
 	bool newlyAdded;
-	tie(iter, newlyAdded) = m_scopes.emplace(&_node, Scope(m_currentScope));
+	tie(iter, newlyAdded) = m_scopes.emplace(&_declaration, DeclarationContainer(m_currentScope, &m_scopes[m_currentScope]));
 	if (asserts(newlyAdded))
 		BOOST_THROW_EXCEPTION(InternalCompilerError() << errinfo_comment("Unable to add new scope."));
-	m_currentScope = &iter->second;
+	m_currentScope = &_declaration;
 }
 
 void DeclarationRegistrationHelper::closeCurrentScope()
 {
 	if (asserts(m_currentScope))
 		BOOST_THROW_EXCEPTION(InternalCompilerError() << errinfo_comment("Closed non-existing scope."));
-	m_currentScope = m_currentScope->getEnclosingScope();
+	m_currentScope = m_scopes[m_currentScope].getEnclosingDeclaration();
 }
 
 void DeclarationRegistrationHelper::registerDeclaration(Declaration& _declaration, bool _opensScope)
 {
-	if (asserts(m_currentScope))
+	if (!m_scopes[m_currentScope].registerDeclaration(_declaration))
-		BOOST_THROW_EXCEPTION(InternalCompilerError() << errinfo_comment("Declaration registered without scope."));
-	if (!m_currentScope->registerDeclaration(_declaration))
 		BOOST_THROW_EXCEPTION(DeclarationError() << errinfo_sourceLocation(_declaration.getLocation())
 												 << errinfo_comment("Identifier already declared."));
 	//@todo the exception should also contain the location of the first declaration
+	_declaration.setScope(m_currentScope);
 	if (_opensScope)
 		enterNewSubScope(_declaration);
 }

libsolidity/NameAndTypeResolver.h

@@ -25,7 +25,7 @@
 #include <map>
 #include <boost/noncopyable.hpp>
 
-#include <libsolidity/Scope.h>
+#include <libsolidity/DeclarationContainer.h>
 #include <libsolidity/ASTVisitor.h>
 
 namespace dev
@@ -61,9 +61,9 @@ private:
 	/// Maps nodes declaring a scope to scopes, i.e. ContractDefinition and FunctionDeclaration,
 	/// where nullptr denotes the global scope. Note that structs are not scope since they do
 	/// not contain code.
-	std::map<ASTNode const*, Scope> m_scopes;
+	std::map<ASTNode const*, DeclarationContainer> m_scopes;
 
-	Scope* m_currentScope;
+	DeclarationContainer* m_currentScope;
 };
 
 /**
@@ -73,7 +73,7 @@ private:
 class DeclarationRegistrationHelper: private ASTVisitor
 {
 public:
-	DeclarationRegistrationHelper(std::map<ASTNode const*, Scope>& _scopes, ASTNode& _astRoot);
+	DeclarationRegistrationHelper(std::map<ASTNode const*, DeclarationContainer>& _scopes, ASTNode& _astRoot);
 
 private:
 	bool visit(ContractDefinition& _contract);
@@ -85,12 +85,12 @@ private:
 	void endVisit(VariableDefinition& _variableDefinition);
 	bool visit(VariableDeclaration& _declaration);
 
-	void enterNewSubScope(ASTNode& _node);
+	void enterNewSubScope(Declaration& _declaration);
 	void closeCurrentScope();
 	void registerDeclaration(Declaration& _declaration, bool _opensScope);
 
-	std::map<ASTNode const*, Scope>& m_scopes;
+	std::map<ASTNode const*, DeclarationContainer>& m_scopes;
-	Scope* m_currentScope;
+	Declaration* m_currentScope;
 	FunctionDefinition* m_currentFunction;
 };
 

test/solidityNameAndTypeResolution.cpp

@@ -244,6 +244,36 @@ BOOST_AUTO_TEST_CASE(balance_invalid)
 	BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError);
 }
 
+BOOST_AUTO_TEST_CASE(assignment_to_mapping)
+{
+	char const* text = "contract test {\n"
+					   "  struct str {\n"
+					   "    mapping(uint=>uint) map;\n"
+					   "  }\n"
+					   "  str data;"
+					   "  function fun() {\n"
+					   "    var a = data.map;\n"
+					   "    data.map = a;\n"
+					   "  }\n"
+					   "}\n";
+	BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError);
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_struct)
+{
+	char const* text = "contract test {\n"
+					   "  struct str {\n"
+					   "    mapping(uint=>uint) map;\n"
+					   "  }\n"
+					   "  str data;"
+					   "  function fun() {\n"
+					   "    var a = data;\n"
+					   "    data = a;\n"
+					   "  }\n"
+					   "}\n";
+	BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }