Merge pull request #2720 from ethereum/visibility-parser

Show previous visibility specifier in parser error

7 files changed, 41 insertions, 26 deletions

diff --git a/Changelog.md b/Changelog.md
index a61ac0af..0381d434 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,6 +1,7 @@
 ### 0.4.16 (unreleased)
 
 Features:
+ * Parser: Display previous visibility specifier in error if multiple are found.
 
 Bugfixes:
 
diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h
index 3e97286b..bdf20e81 100644
--- a/libsolidity/ast/AST.h
+++ b/libsolidity/ast/AST.h
@@ -152,6 +152,24 @@ public:
 	/// Visibility ordered from restricted to unrestricted.
 	enum class Visibility { Default, Private, Internal, Public, External };
 
+	static std::string visibilityToString(Declaration::Visibility _visibility)
+	{
+		switch(_visibility)
+		{
+		case Declaration::Visibility::Public:
+			return "public";
+		case Declaration::Visibility::Internal:
+			return "internal";
+		case Declaration::Visibility::Private:
+			return "private";
+		case Declaration::Visibility::External:
+			return "external";
+		default:
+			solAssert(false, "Invalid visibility specifier.");
+		}
+		return std::string();
+	}
+
 	Declaration(
 		SourceLocation const& _location,
 		ASTPointer<ASTString> const& _name,
diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp
index e4a602cb..abee55ee 100644
--- a/libsolidity/ast/ASTJsonConverter.cpp
+++ b/libsolidity/ast/ASTJsonConverter.cpp
@@ -285,7 +285,7 @@ bool ASTJsonConverter::visit(StructDefinition const& _node)
 {
 	setJsonNode(_node, "StructDefinition", {
 		make_pair("name", _node.name()),
-		make_pair("visibility", visibility(_node.visibility())),
+		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
 		make_pair("canonicalName", _node.annotation().canonicalName),
 		make_pair("members", toJson(_node.members())),
 		make_pair("scope", idOrNull(_node.scope()))
@@ -325,7 +325,7 @@ bool ASTJsonConverter::visit(FunctionDefinition const& _node)
 		make_pair("name", _node.name()),
 		make_pair(m_legacy ? "constant" : "isDeclaredConst", _node.isDeclaredConst()),
 		make_pair("payable", _node.isPayable()),
-		make_pair("visibility", visibility(_node.visibility())),
+		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
 		make_pair("parameters", toJson(_node.parameterList())),
 		make_pair("isConstructor", _node.isConstructor()),
 		make_pair("returnParameters", toJson(*_node.returnParameterList())),
@@ -346,7 +346,7 @@ bool ASTJsonConverter::visit(VariableDeclaration const& _node)
 		make_pair("constant", _node.isConstant()),
 		make_pair("stateVariable", _node.isStateVariable()),
 		make_pair("storageLocation", location(_node.referenceLocation())),
-		make_pair("visibility", visibility(_node.visibility())),
+		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
 		make_pair("value", _node.value() ? toJson(*_node.value()) : Json::nullValue),
 		make_pair("scope", idOrNull(_node.scope())),
 		make_pair("typeDescriptions", typePointerToJson(_node.annotation().type))
@@ -361,7 +361,7 @@ bool ASTJsonConverter::visit(ModifierDefinition const& _node)
 {
 	setJsonNode(_node, "ModifierDefinition", {
 		make_pair("name", _node.name()),
-		make_pair("visibility", visibility(_node.visibility())),
+		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
 		make_pair("parameters", toJson(_node.parameterList())),
 		make_pair("body", toJson(_node.body()))
 	});
@@ -418,7 +418,7 @@ bool ASTJsonConverter::visit(FunctionTypeName const& _node)
 {
 	setJsonNode(_node, "FunctionTypeName", {
 		make_pair("payable", _node.isPayable()),
-		make_pair("visibility", visibility(_node.visibility())),
+		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
 		make_pair(m_legacy ? "constant" : "isDeclaredConst", _node.isDeclaredConst()),
 		make_pair("parameterTypes", toJson(*_node.parameterTypeList())),
 		make_pair("returnParameterTypes", toJson(*_node.returnParameterTypeList())),
@@ -730,23 +730,6 @@ void ASTJsonConverter::endVisit(EventDefinition const&)
 	m_inEvent = false;
 }
 
-string ASTJsonConverter::visibility(Declaration::Visibility const& _visibility)
-{
-	switch (_visibility)
-	{
-	case Declaration::Visibility::Private:
-		return "private";
-	case Declaration::Visibility::Internal:
-		return "internal";
-	case Declaration::Visibility::Public:
-		return "public";
-	case Declaration::Visibility::External:
-		return "external";
-	default:
-		solAssert(false, "Unknown declaration visibility.");
-	}
-}
-
 string ASTJsonConverter::location(VariableDeclaration::Location _location)
 {
 	switch (_location)
diff --git a/libsolidity/ast/ASTJsonConverter.h b/libsolidity/ast/ASTJsonConverter.h
index 27114c2a..70e260db 100644
--- a/libsolidity/ast/ASTJsonConverter.h
+++ b/libsolidity/ast/ASTJsonConverter.h
@@ -128,7 +128,6 @@ private:
 		return _node ? toJson(*_node) : Json::nullValue;
 	}
 	Json::Value inlineAssemblyIdentifierToJson(std::pair<assembly::Identifier const* , InlineAssemblyAnnotation::ExternalIdentifierInfo> _info);
-	std::string visibility(Declaration::Visibility const& _visibility);
 	std::string location(VariableDeclaration::Location _location);
 	std::string contractKind(ContractDefinition::ContractKind _kind);
 	std::string functionCallKind(FunctionCallKind _kind);
diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp
index bc004d14..a6d6e025 100644
--- a/libsolidity/parsing/Parser.cpp
+++ b/libsolidity/parsing/Parser.cpp
@@ -354,7 +354,11 @@ Parser::FunctionHeaderParserResult Parser::parseFunctionHeader(bool _forceEmptyN
 		{
 			if (result.visibility != Declaration::Visibility::Default)
 			{
-				parserError(string("Multiple visibility specifiers."));
+				parserError(string(
+					"Visibility already specified as \"" +
+					Declaration::visibilityToString(result.visibility) +
+					"\"."
+				));
 				m_scanner->next();
 			}
 			else
@@ -512,7 +516,11 @@ ASTPointer<VariableDeclaration> Parser::parseVariableDeclaration(
 		{
 			if (visibility != Declaration::Visibility::Default)
 			{
-				parserError(string("Visibility already specified."));
+				parserError(string(
+					"Visibility already specified as \"" +
+					Declaration::visibilityToString(visibility) +
+					"\"."
+				));
 				m_scanner->next();
 			}
 			else
diff --git a/libsolidity/parsing/Parser.h b/libsolidity/parsing/Parser.h
index 19631c58..82ab91a6 100644
--- a/libsolidity/parsing/Parser.h
+++ b/libsolidity/parsing/Parser.h
@@ -73,6 +73,7 @@ private:
 	ASTPointer<ContractDefinition> parseContractDefinition(Token::Value _expectedKind);
 	ASTPointer<InheritanceSpecifier> parseInheritanceSpecifier();
 	Declaration::Visibility parseVisibilitySpecifier(Token::Value _token);
+	std::string visibilitySpecifierName(Declaration::Visibility _visibility);
 	FunctionHeaderParserResult parseFunctionHeader(bool _forceEmptyName, bool _allowModifiers);
 	ASTPointer<ASTNode> parseFunctionDefinitionOrFunctionTypeStateVariable(ASTString const* _contractName);
 	ASTPointer<FunctionDefinition> parseFunctionDefinition(ASTString const* _contractName);
diff --git a/test/libsolidity/SolidityParser.cpp b/test/libsolidity/SolidityParser.cpp
index 78edd4d1..2be824d6 100644
--- a/test/libsolidity/SolidityParser.cpp
+++ b/test/libsolidity/SolidityParser.cpp
@@ -898,7 +898,12 @@ BOOST_AUTO_TEST_CASE(multiple_visibility_specifiers)
 		contract c {
 			uint private internal a;
 		})";
-	CHECK_PARSE_ERROR(text, "Visibility already specified");
+	CHECK_PARSE_ERROR(text, "Visibility already specified as \"private\".");
+	text = R"(
+		contract c {
+			function f() private external {}
+		})";
+	CHECK_PARSE_ERROR(text, "Visibility already specified as \"private\".");
 }
 
 BOOST_AUTO_TEST_CASE(multiple_payable_specifiers)