Refactor data location check.

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -626,6 +626,17 @@ void DeclarationRegistrationHelper::endVisit(ModifierDefinition&)
 	closeCurrentScope();
 }
 
+bool DeclarationRegistrationHelper::visit(FunctionTypeName& _funTypeName)
+{
+	enterNewSubScope(_funTypeName);
+	return true;
+}
+
+void DeclarationRegistrationHelper::endVisit(FunctionTypeName&)
+{
+	closeCurrentScope();
+}
+
 bool DeclarationRegistrationHelper::visit(Block& _block)
 {
 	_block.setScope(m_currentScope);

libsolidity/analysis/NameAndTypeResolver.h

@@ -171,6 +171,8 @@ private:
 	void endVisit(FunctionDefinition& _function) override;
 	bool visit(ModifierDefinition& _modifier) override;
 	void endVisit(ModifierDefinition& _modifier) override;
+	bool visit(FunctionTypeName& _funTypeName) override;
+	void endVisit(FunctionTypeName& _funTypeName) override;
 	bool visit(Block& _block) override;
 	void endVisit(Block& _block) override;
 	bool visit(ForStatement& _forLoop) override;

libsolidity/analysis/ReferencesResolver.cpp

@@ -30,7 +30,10 @@
 #include <libsolidity/inlineasm/AsmData.h>
 #include <libsolidity/interface/ErrorReporter.h>
 
+#include <libdevcore/StringUtils.h>
+
 #include <boost/algorithm/string.hpp>
+#include <boost/range/adaptor/transformed.hpp>
 
 using namespace std;
 using namespace dev;
@@ -155,7 +158,10 @@ void ReferencesResolver::endVisit(UserDefinedTypeName const& _typeName)
 	else if (ContractDefinition const* contract = dynamic_cast<ContractDefinition const*>(declaration))
 		_typeName.annotation().type = make_shared<ContractType>(*contract);
 	else
+	{
+		_typeName.annotation().type = make_shared<TupleType>();
 		typeError(_typeName.location(), "Name has to refer to a struct, enum or contract.");
+	}
 }
 
 void ReferencesResolver::endVisit(FunctionTypeName const& _typeName)
@@ -166,13 +172,13 @@ void ReferencesResolver::endVisit(FunctionTypeName const& _typeName)
 	case VariableDeclaration::Visibility::External:
 		break;
 	default:
-		typeError(_typeName.location(), "Invalid visibility, can only be \"external\" or \"internal\".");
+		fatalTypeError(_typeName.location(), "Invalid visibility, can only be \"external\" or \"internal\".");
 		return;
 	}
 
 	if (_typeName.isPayable() && _typeName.visibility() != VariableDeclaration::Visibility::External)
 	{
-		typeError(_typeName.location(), "Only external function types can be payable.");
+		fatalTypeError(_typeName.location(), "Only external function types can be payable.");
 		return;
 	}
 
@@ -182,7 +188,7 @@ void ReferencesResolver::endVisit(FunctionTypeName const& _typeName)
 			solAssert(t->annotation().type, "Type not set for parameter.");
 			if (!t->annotation().type->canBeUsedExternally(false))
 			{
-				typeError(t->location(), "Internal type cannot be used for external function type.");
+				fatalTypeError(t->location(), "Internal type cannot be used for external function type.");
 				return;
 			}
 		}
@@ -300,6 +306,9 @@ void ReferencesResolver::endVisit(VariableDeclaration const& _variable)
 	if (_variable.annotation().type)
 		return;
 
+	if (_variable.isConstant() && !_variable.isStateVariable())
+		m_errorReporter.declarationError(_variable.location(), "The \"constant\" keyword can only be used for state variables.");
+
 	if (!_variable.typeName())
 	{
 		// This can still happen in very unusual cases where a developer uses constructs, such as
@@ -309,96 +318,71 @@ void ReferencesResolver::endVisit(VariableDeclaration const& _variable)
 		// after this step.
 		return;
 	}
-
-	TypePointer type;
-	type = _variable.typeName()->annotation().type;
 	using Location = VariableDeclaration::Location;
 	Location varLoc = _variable.referenceLocation();
 	DataLocation typeLoc = DataLocation::Memory;
-	// References are forced to calldata for external function parameters (not return)
-	// and memory for parameters (also return) of publicly visible functions.
-	// They default to memory for function parameters and storage for local variables.
-	// As an exception, "storage" is allowed for library functions.
-	if (auto ref = dynamic_cast<ReferenceType const*>(type.get()))
+
+	set<Location> allowedDataLocations = _variable.allowedDataLocations();
+	if (!allowedDataLocations.count(varLoc))
 	{
-		bool isPointer = true;
-		if (_variable.isExternalCallableParameter())
+		auto locationToString = [](VariableDeclaration::Location _location) -> string
 		{
-			auto const& contract = dynamic_cast<ContractDefinition const&>(
-				*dynamic_cast<Declaration const&>(*_variable.scope()).scope()
-			);
-			if (contract.isLibrary())
+			switch (_location)
 			{
-				if (varLoc == Location::Memory)
-					fatalTypeError(_variable.location(),
-						"Location has to be calldata or storage for external "
-						"library functions (remove the \"memory\" keyword)."
-					);
+			case Location::Memory: return "\"memory\"";
+			case Location::Storage: return "\"storage\"";
+			case Location::CallData: return "\"calldata\"";
+			case Location::Default: return "none";
 			}
+			return {};
+		};
+
+		string errorString;
+		if (!_variable.hasReferenceOrMappingType())
+			errorString = "Data location can only be specified for array, struct or mapping types";
 		else
 		{
-				// force location of external function parameters (not return) to calldata
-				if (varLoc != Location::CallData && varLoc != Location::Default)
-					fatalTypeError(_variable.location(),
-						"Location has to be calldata for external functions "
-						"(remove the \"memory\" or \"storage\" keyword)."
+			errorString = "Data location must be " +
+			joinHumanReadable(
+				allowedDataLocations | boost::adaptors::transformed(locationToString),
+				", ",
+				" or "
 			);
-			}
-			if (varLoc == Location::Default || varLoc == Location::CallData)
-				typeLoc = DataLocation::CallData;
-			else
-				typeLoc = varLoc == Location::Memory ? DataLocation::Memory : DataLocation::Storage;
-		}
-		else if (_variable.isCallableParameter() && dynamic_cast<Declaration const&>(*_variable.scope()).isPublic())
-		{
-			auto const& contract = dynamic_cast<ContractDefinition const&>(
-				*dynamic_cast<Declaration const&>(*_variable.scope()).scope()
-			);
-			// force locations of public or external function (return) parameters to memory
-			if (varLoc != Location::Memory && varLoc != Location::Default && !contract.isLibrary())
-				fatalTypeError(_variable.location(),
-					"Location has to be memory for publicly visible functions "
-					"(remove the \"storage\" or \"calldata\" keyword)."
-				);
-			if (varLoc == Location::Default || !contract.isLibrary())
-				typeLoc = DataLocation::Memory;
-			else
-			{
-				if (varLoc == Location::CallData)
-					fatalTypeError(_variable.location(),
-						"Location cannot be calldata for non-external functions "
-						"(remove the \"calldata\" keyword)."
-					);
-				typeLoc = varLoc == Location::Memory ? DataLocation::Memory : DataLocation::Storage;
-			}
-		}
-		else
-		{
-			if (_variable.isConstant())
-			{
-				if (varLoc != Location::Default && varLoc != Location::Memory)
-					fatalTypeError(
-						_variable.location(),
-						"Data location has to be \"memory\" (or unspecified) for constants."
-					);
-				typeLoc = DataLocation::Memory;
-			}
-			else if (varLoc == Location::Default)
-			{
 			if (_variable.isCallableParameter())
+				errorString +=
+					" for " +
+					string(_variable.isReturnParameter() ? "return " : "") +
+					"parameter in" +
+					string(_variable.isExternalCallableParameter() ? " external" : "") +
+					" function";
+			else
+				errorString += " for variable";
+		}
+		errorString += ", but " + locationToString(varLoc) + " was given.";
+		typeError(_variable.location(), errorString);
+
+		solAssert(!allowedDataLocations.empty(), "");
+		varLoc = *allowedDataLocations.begin();
+	}
+
+	// Find correct data location.
+	if (_variable.isEventParameter())
+	{
+		solAssert(varLoc == Location::Default, "");
 		typeLoc = DataLocation::Memory;
-				else
+	}
+	else if (_variable.isStateVariable())
 	{
+		solAssert(varLoc == Location::Default, "");
+		typeLoc = _variable.isConstant() ? DataLocation::Memory : DataLocation::Storage;
+	}
+	else if (
+		dynamic_cast<StructDefinition const*>(_variable.scope()) ||
+		dynamic_cast<EnumDefinition const*>(_variable.scope())
+	)
+		// The actual location will later be changed depending on how the type is used.
 		typeLoc = DataLocation::Storage;
-					if (_variable.isLocalVariable())
-						typeError(
-							_variable.location(),
-							"Data location must be specified as either \"memory\" or \"storage\"."
-						);
-				}
-			}
 	else
-			{
 		switch (varLoc)
 		{
 		case Location::Memory:
@@ -408,28 +392,18 @@ void ReferencesResolver::endVisit(VariableDeclaration const& _variable)
 			typeLoc = DataLocation::Storage;
 			break;
 		case Location::CallData:
-					fatalTypeError(_variable.location(),
-						"Variable cannot be declared as \"calldata\" (remove the \"calldata\" keyword)."
-					);
+			typeLoc = DataLocation::CallData;
 			break;
-				default:
-					solAssert(false, "Unknown data location");
-				}
-			}
-			isPointer = !_variable.isStateVariable();
+		case Location::Default:
+			solAssert(!_variable.hasReferenceOrMappingType(), "Data location not properly set.");
 		}
+
+	TypePointer type = _variable.typeName()->annotation().type;
+	if (auto ref = dynamic_cast<ReferenceType const*>(type.get()))
+	{
+		bool isPointer = !_variable.isStateVariable();
 		type = ref->copyForLocation(typeLoc, isPointer);
 	}
-	else if (dynamic_cast<MappingType const*>(type.get()))
-	{
-		if (_variable.isLocalVariable() && varLoc != Location::Storage)
-			typeError(
-				_variable.location(),
-				"Data location for mappings must be specified as \"storage\"."
-			);
-	}
-	else if (varLoc != Location::Default && !ref)
-		typeError(_variable.location(), "Data location can only be given for array or struct types.");
 
 	_variable.annotation().type = type;
 }

libsolidity/analysis/TypeChecker.cpp

@@ -630,8 +630,13 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
 	for (ASTPointer<VariableDeclaration> const& var: _function.parameters() + _function.returnParameters())
 	{
 		if (
+			type(*var)->category() == Type::Category::Mapping &&
+			!type(*var)->dataStoredIn(DataLocation::Storage)
+		)
+			m_errorReporter.typeError(var->location(), "Mapping types can only have a data location of \"storage\".");
+		else if (
 			!type(*var)->canLiveOutsideStorage() &&
-			!(_function.visibility() <= FunctionDefinition::Visibility::Internal)
+			_function.visibility() > FunctionDefinition::Visibility::Internal
 		)
 			m_errorReporter.typeError(var->location(), "Type is required to live outside storage.");
 		if (_function.visibility() >= FunctionDefinition::Visibility::Public && !(type(*var)->interfaceType(isLibraryFunction)))
@@ -716,8 +721,6 @@ bool TypeChecker::visit(VariableDeclaration const& _variable)
 		expectType(*_variable.value(), *varType);
 	if (_variable.isConstant())
 	{
-		if (!_variable.isStateVariable())
-			m_errorReporter.typeError(_variable.location(), "Illegal use of \"constant\" specifier.");
 		if (!_variable.type()->isValueType())
 		{
 			bool allowed = false;

libsolidity/ast/AST.cpp

@@ -418,6 +418,7 @@ bool VariableDeclaration::isLocalVariable() const
 {
 	auto s = scope();
 	return
+		dynamic_cast<FunctionTypeName const*>(s) ||
 		dynamic_cast<CallableDeclaration const*>(s) ||
 		dynamic_cast<Block const*>(s) ||
 		dynamic_cast<ForStatement const*>(s);
@@ -425,14 +426,18 @@ bool VariableDeclaration::isLocalVariable() const
 
 bool VariableDeclaration::isCallableParameter() const
 {
-	auto const* callable = dynamic_cast<CallableDeclaration const*>(scope());
-	if (!callable)
-		return false;
-	for (auto const& variable: callable->parameters())
-		if (variable.get() == this)
+	if (isReturnParameter())
 		return true;
-	if (callable->returnParameterList())
-		for (auto const& variable: callable->returnParameterList()->parameters())
+
+	vector<ASTPointer<VariableDeclaration>> const* parameters = nullptr;
+
+	if (auto const* funTypeName = dynamic_cast<FunctionTypeName const*>(scope()))
+		parameters = &funTypeName->parameterTypes();
+	else if (auto const* callable = dynamic_cast<CallableDeclaration const*>(scope()))
+		parameters = &callable->parameters();
+
+	if (parameters)
+		for (auto const& variable: *parameters)
 			if (variable.get() == this)
 				return true;
 	return false;
@@ -445,11 +450,16 @@ bool VariableDeclaration::isLocalOrReturn() const
 
 bool VariableDeclaration::isReturnParameter() const
 {
-	auto const* callable = dynamic_cast<CallableDeclaration const*>(scope());
-	if (!callable)
-		return false;
+	vector<ASTPointer<VariableDeclaration>> const* returnParameters = nullptr;
+
+	if (auto const* funTypeName = dynamic_cast<FunctionTypeName const*>(scope()))
+		returnParameters = &funTypeName->returnParameterTypes();
+	else if (auto const* callable = dynamic_cast<CallableDeclaration const*>(scope()))
 		if (callable->returnParameterList())
-		for (auto const& variable: callable->returnParameterList()->parameters())
+			returnParameters = &callable->returnParameterList()->parameters();
+
+	if (returnParameters)
+		for (auto const& variable: *returnParameters)
 			if (variable.get() == this)
 				return true;
 	return false;
@@ -457,15 +467,88 @@ bool VariableDeclaration::isReturnParameter() const
 
 bool VariableDeclaration::isExternalCallableParameter() const
 {
-	auto const* callable = dynamic_cast<CallableDeclaration const*>(scope());
-	if (!callable || callable->visibility() != Declaration::Visibility::External)
+	if (!isCallableParameter())
 		return false;
-	for (auto const& variable: callable->parameters())
-		if (variable.get() == this)
-			return true;
+
+	if (auto const* callable = dynamic_cast<CallableDeclaration const*>(scope()))
+		if (callable->visibility() == Declaration::Visibility::External)
+			return !isReturnParameter();
+
 	return false;
 }
 
+bool VariableDeclaration::isInternalCallableParameter() const
+{
+	if (!isCallableParameter())
+		return false;
+
+	if (auto const* funTypeName = dynamic_cast<FunctionTypeName const*>(scope()))
+		return funTypeName->visibility() == Declaration::Visibility::Internal;
+	else if (auto const* callable = dynamic_cast<CallableDeclaration const*>(scope()))
+		return callable->visibility() <= Declaration::Visibility::Internal;
+	return false;
+}
+
+bool VariableDeclaration::isLibraryFunctionParameter() const
+{
+	if (!isCallableParameter())
+		return false;
+	if (auto const* funDef = dynamic_cast<FunctionDefinition const*>(scope()))
+		return dynamic_cast<ContractDefinition const&>(*funDef->scope()).isLibrary();
+	else
+		return false;
+}
+
+bool VariableDeclaration::isEventParameter() const
+{
+	return dynamic_cast<EventDefinition const*>(scope()) != nullptr;
+}
+
+bool VariableDeclaration::hasReferenceOrMappingType() const
+{
+	solAssert(typeName(), "");
+	solAssert(typeName()->annotation().type, "Can only be called after reference resolution");
+	TypePointer const& type = typeName()->annotation().type;
+	return type->category() == Type::Category::Mapping || dynamic_cast<ReferenceType const*>(type.get());
+}
+
+set<VariableDeclaration::Location> VariableDeclaration::allowedDataLocations() const
+{
+	using Location = VariableDeclaration::Location;
+
+	if (!hasReferenceOrMappingType() || isStateVariable() || isEventParameter())
+		return set<Location>{ Location::Default };
+	else if (isStateVariable() && isConstant())
+		return set<Location>{ Location::Memory };
+	else if (isExternalCallableParameter())
+	{
+		set<Location> locations{ Location::CallData };
+		if (isLibraryFunctionParameter())
+			locations.insert(Location::Storage);
+		return locations;
+	}
+	else if (isCallableParameter())
+	{
+		set<Location> locations{ Location::Memory };
+		if (isInternalCallableParameter() || isLibraryFunctionParameter())
+			locations.insert(Location::Storage);
+		return locations;
+	}
+	else if (isLocalVariable())
+	{
+		solAssert(typeName(), "");
+		solAssert(typeName()->annotation().type, "Can only be called after reference resolution");
+		if (typeName()->annotation().type->category() == Type::Category::Mapping)
+			return set<Location>{ Location::Storage };
+		else
+			//  TODO: add Location::Calldata once implemented for local variables.
+			return set<Location>{ Location::Memory, Location::Storage };
+	}
+	else
+		// Struct members etc.
+		return set<Location>{ Location::Default };
+}
+
 TypePointer VariableDeclaration::type() const
 {
 	return annotation().type;
@@ -580,7 +663,7 @@ bool Literal::passesAddressChecksum() const
 	return dev::passesAddressChecksum(value(), true);
 }
 
-std::string Literal::getChecksummedAddress() const
+string Literal::getChecksummedAddress() const
 {
 	solAssert(isHexNumber(), "Expected hex number");
 	/// Pad literal to be a proper hex address.

libsolidity/ast/AST.h

@@ -685,6 +685,8 @@ public:
 	virtual bool isLValue() const override;
 	virtual bool isPartOfExternalInterface() const override { return isPublic(); }
 
+	/// @returns true iff this variable is the parameter (or return parameter) of a function
+	/// (or function type name or event) or declared inside a function body.
 	bool isLocalVariable() const;
 	/// @returns true if this variable is a parameter or return parameter of a function.
 	bool isCallableParameter() const;
@@ -693,13 +695,27 @@ public:
 	/// @returns true if this variable is a local variable or return parameter.
 	bool isLocalOrReturn() const;
 	/// @returns true if this variable is a parameter (not return parameter) of an external function.
+	/// This excludes parameters of external function type names.
 	bool isExternalCallableParameter() const;
+	/// @returns true if this variable is a parameter or return parameter of an internal function
+	/// or a function type of internal visibility.
+	bool isInternalCallableParameter() const;
+	/// @returns true iff this variable is a parameter(or return parameter of a library function
+	bool isLibraryFunctionParameter() const;
 	/// @returns true if the type of the variable does not need to be specified, i.e. it is declared
 	/// in the body of a function or modifier.
+	/// @returns true if this variable is a parameter of an event.
+	bool isEventParameter() const;
+	/// @returns true if the type of the variable is a reference or mapping type, i.e.
+	/// array, struct or mapping. These types can take a data location (and often require it).
+	/// Can only be called after reference resolution.
+	bool hasReferenceOrMappingType() const;
 	bool isStateVariable() const { return m_isStateVariable; }
 	bool isIndexed() const { return m_isIndexed; }
 	bool isConstant() const { return m_isConstant; }
 	Location referenceLocation() const { return m_location; }
+	/// @returns a set of allowed storage locations for the variable.
+	std::set<Location> allowedDataLocations() const;
 
 	virtual TypePointer type() const override;