Merge pull request #3589 from ethereum/identifiers

Clean up type identifiers

3 files changed, 78 insertions, 42 deletions

diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp
index 41f95f30..771ae643 100644
--- a/libsolidity/ast/Types.cpp
+++ b/libsolidity/ast/Types.cpp
@@ -131,28 +131,28 @@ namespace
 
 string parenthesizeIdentifier(string const& _internal)
 {
-	return "$_" + _internal + "_$";
+	return "(" + _internal + ")";
 }
 
 template <class Range>
 string identifierList(Range const&& _list)
 {
-	return parenthesizeIdentifier(boost::algorithm::join(_list, "_$_"));
+	return parenthesizeIdentifier(boost::algorithm::join(_list, ","));
 }
 
-string identifier(TypePointer const& _type)
+string richIdentifier(TypePointer const& _type)
 {
-	return _type ? _type->identifier() : "";
+	return _type ? _type->richIdentifier() : "";
 }
 
 string identifierList(vector<TypePointer> const& _list)
 {
-	return identifierList(_list | boost::adaptors::transformed(identifier));
+	return identifierList(_list | boost::adaptors::transformed(richIdentifier));
 }
 
 string identifierList(TypePointer const& _type)
 {
-	return parenthesizeIdentifier(identifier(_type));
+	return parenthesizeIdentifier(richIdentifier(_type));
 }
 
 string identifierList(TypePointer const& _type1, TypePointer const& _type2)
@@ -165,11 +165,22 @@ string identifierList(TypePointer const& _type1, TypePointer const& _type2)
 
 string parenthesizeUserIdentifier(string const& _internal)
 {
-	return parenthesizeIdentifier(boost::algorithm::replace_all_copy(_internal, "$", "$$$"));
+	return parenthesizeIdentifier(_internal);
 }
 
 }
 
+string Type::escapeIdentifier(string const& _identifier)
+{
+	string ret = _identifier;
+	// FIXME: should be _$$$_
+	boost::algorithm::replace_all(ret, "$", "$$$");
+	boost::algorithm::replace_all(ret, ",", "_$_");
+	boost::algorithm::replace_all(ret, "(", "$_");
+	boost::algorithm::replace_all(ret, ")", "_$");
+	return ret;
+}
+
 TypePointer Type::fromElementaryTypeName(ElementaryTypeNameToken const& _type)
 {
 	solAssert(Token::isElementaryTypeName(_type.token()),
@@ -334,7 +345,7 @@ IntegerType::IntegerType(int _bits, IntegerType::Modifier _modifier):
 	);
 }
 
-string IntegerType::identifier() const
+string IntegerType::richIdentifier() const
 {
 	if (isAddress())
 		return "t_address";
@@ -504,7 +515,7 @@ FixedPointType::FixedPointType(int _totalBits, int _fractionalDigits, FixedPoint
 	);
 }
 
-string FixedPointType::identifier() const
+string FixedPointType::richIdentifier() const
 {
 	return "t_" + string(isSigned() ? "" : "u") + "fixed" + std::to_string(m_totalBits) + "x" + std::to_string(m_fractionalDigits);
 }
@@ -936,7 +947,7 @@ TypePointer RationalNumberType::binaryOperatorResult(Token::Value _operator, Typ
 	}
 }
 
-string RationalNumberType::identifier() const
+string RationalNumberType::richIdentifier() const
 {
 	return "t_rational_" + m_value.numerator().str() + "_by_" + m_value.denominator().str();
 }
@@ -1077,7 +1088,7 @@ bool StringLiteralType::isImplicitlyConvertibleTo(Type const& _convertTo) const
 		return false;
 }
 
-string StringLiteralType::identifier() const
+string StringLiteralType::richIdentifier() const
 {
 	// Since we have to return a valid identifier and the string itself may contain
 	// anything, we hash it.
@@ -1177,7 +1188,7 @@ MemberList::MemberMap FixedBytesType::nativeMembers(const ContractDefinition*) c
 	return MemberList::MemberMap{MemberList::Member{"length", make_shared<IntegerType>(8)}};
 }
 
-string FixedBytesType::identifier() const
+string FixedBytesType::richIdentifier() const
 {
 	return "t_bytes" + std::to_string(m_bytes);
 }
@@ -1370,7 +1381,7 @@ bool ArrayType::isExplicitlyConvertibleTo(const Type& _convertTo) const
 	return true;
 }
 
-string ArrayType::identifier() const
+string ArrayType::richIdentifier() const
 {
 	string id;
 	if (isString())
@@ -1604,7 +1615,7 @@ TypePointer ArrayType::copyForLocation(DataLocation _location, bool _isPointer)
 	return copy;
 }
 
-string ContractType::identifier() const
+string ContractType::richIdentifier() const
 {
 	return (m_super ? "t_super" : "t_contract") + parenthesizeUserIdentifier(m_contract.name()) + std::to_string(m_contract.id());
 }
@@ -1756,7 +1767,7 @@ bool StructType::isImplicitlyConvertibleTo(const Type& _convertTo) const
 	return this->m_struct == convertTo.m_struct;
 }
 
-string StructType::identifier() const
+string StructType::richIdentifier() const
 {
 	return "t_struct" + parenthesizeUserIdentifier(m_struct.name()) + std::to_string(m_struct.id()) + identifierLocationSuffix();
 }
@@ -1988,7 +1999,7 @@ TypePointer EnumType::unaryOperatorResult(Token::Value _operator) const
 	return _operator == Token::Delete ? make_shared<TupleType>() : TypePointer();
 }
 
-string EnumType::identifier() const
+string EnumType::richIdentifier() const
 {
 	return "t_enum" + parenthesizeUserIdentifier(m_enum.name()) + std::to_string(m_enum.id());
 }
@@ -2074,7 +2085,7 @@ bool TupleType::isImplicitlyConvertibleTo(Type const& _other) const
 		return false;
 }
 
-string TupleType::identifier() const
+string TupleType::richIdentifier() const
 {
 	return "t_tuple" + identifierList(components());
 }
@@ -2309,7 +2320,7 @@ TypePointers FunctionType::parameterTypes() const
 	return TypePointers(m_parameterTypes.cbegin() + 1, m_parameterTypes.cend());
 }
 
-string FunctionType::identifier() const
+string FunctionType::richIdentifier() const
 {
 	string id = "t_function_";
 	switch (m_kind)
@@ -2802,7 +2813,7 @@ ASTPointer<ASTString> FunctionType::documentation() const
 	return ASTPointer<ASTString>();
 }
 
-string MappingType::identifier() const
+string MappingType::richIdentifier() const
 {
 	return "t_mapping" + identifierList(m_keyType, m_valueType);
 }
@@ -2825,7 +2836,7 @@ string MappingType::canonicalName() const
 	return "mapping(" + keyType()->canonicalName() + " => " + valueType()->canonicalName() + ")";
 }
 
-string TypeType::identifier() const
+string TypeType::richIdentifier() const
 {
 	return "t_type" + identifierList(actualType());
 }
@@ -2910,7 +2921,7 @@ u256 ModifierType::storageSize() const
 	solAssert(false, "Storage size of non-storable type type requested.");
 }
 
-string ModifierType::identifier() const
+string ModifierType::richIdentifier() const
 {
 	return "t_modifier" + identifierList(m_parameterTypes);
 }
@@ -2939,7 +2950,7 @@ string ModifierType::toString(bool _short) const
 	return name + ")";
 }
 
-string ModuleType::identifier() const
+string ModuleType::richIdentifier() const
 {
 	return "t_module_" + std::to_string(m_sourceUnit.id());
 }
@@ -2965,7 +2976,7 @@ string ModuleType::toString(bool) const
 	return string("module \"") + m_sourceUnit.annotation().path + string("\"");
 }
 
-string MagicType::identifier() const
+string MagicType::richIdentifier() const
 {
 	switch (m_kind)
 	{
diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h
index 2e7d05ba..7985521e 100644
--- a/libsolidity/ast/Types.h
+++ b/libsolidity/ast/Types.h
@@ -163,10 +163,20 @@ public:
 	/// @returns a valid solidity identifier such that two types should compare equal if and
 	/// only if they have the same identifier.
 	/// The identifier should start with "t_".
+	/// Can contain characters which are invalid in identifiers.
+	virtual std::string richIdentifier() const = 0;
+	/// @returns a valid solidity identifier such that two types should compare equal if and
+	/// only if they have the same identifier.
+	/// The identifier should start with "t_".
+	/// Will not contain any character which would be invalid as an identifier.
+	std::string identifier() const { return escapeIdentifier(richIdentifier()); }
+
 	/// More complex identifier strings use "parentheses", where $_ is interpreted as as
 	/// "opening parenthesis", _$ as "closing parenthesis", _$_ as "comma" and any $ that
 	/// appears as part of a user-supplied identifier is escaped as _$$$_.
-	virtual std::string identifier() const = 0;
+	/// @returns an escaped identifier (will not contain any parenthesis or commas)
+	static std::string escapeIdentifier(std::string const& _identifier);
+
 	virtual bool isImplicitlyConvertibleTo(Type const& _other) const { return *this == _other; }
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const
 	{
@@ -306,7 +316,7 @@ public:
 
 	explicit IntegerType(int _bits, Modifier _modifier = Modifier::Unsigned);
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool isImplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
@@ -353,7 +363,7 @@ public:
 
 	explicit FixedPointType(int _totalBits, int _fractionalDigits, Modifier _modifier = Modifier::Unsigned);
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool isImplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
@@ -410,7 +420,7 @@ public:
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
 	virtual TypePointer binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const override;
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 
 	virtual bool canBeStored() const override { return false; }
@@ -459,7 +469,7 @@ public:
 		return TypePointer();
 	}
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 
 	virtual bool canBeStored() const override { return false; }
@@ -493,7 +503,7 @@ public:
 
 	virtual bool isImplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
 	virtual TypePointer binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const override;
@@ -521,7 +531,7 @@ class BoolType: public Type
 public:
 	BoolType() {}
 	virtual Category category() const override { return Category::Bool; }
-	virtual std::string identifier() const override { return "t_bool"; }
+	virtual std::string richIdentifier() const override { return "t_bool"; }
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
 	virtual TypePointer binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const override;
 
@@ -621,7 +631,7 @@ public:
 
 	virtual bool isImplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(const Type& _other) const override;
 	virtual unsigned calldataEncodedSize(bool _padded) const override;
 	virtual bool isDynamicallySized() const override { return m_hasDynamicLength; }
@@ -678,7 +688,7 @@ public:
 	/// Contracts can be converted to themselves and to integers.
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual unsigned calldataEncodedSize(bool _padded ) const override
 	{
@@ -736,7 +746,7 @@ public:
 	explicit StructType(StructDefinition const& _struct, DataLocation _location = DataLocation::Storage):
 		ReferenceType(_location), m_struct(_struct) {}
 	virtual bool isImplicitlyConvertibleTo(const Type& _convertTo) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual unsigned calldataEncodedSize(bool _padded) const override;
 	virtual bool isDynamicallyEncoded() const override;
@@ -791,7 +801,7 @@ public:
 	virtual Category category() const override { return Category::Enum; }
 	explicit EnumType(EnumDefinition const& _enum): m_enum(_enum) {}
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual unsigned calldataEncodedSize(bool _padded) const override
 	{
@@ -832,7 +842,7 @@ public:
 	virtual Category category() const override { return Category::Tuple; }
 	explicit TupleType(std::vector<TypePointer> const& _types = std::vector<TypePointer>()): m_components(_types) {}
 	virtual bool isImplicitlyConvertibleTo(Type const& _other) const override;
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); }
 	virtual std::string toString(bool) const override;
@@ -966,7 +976,7 @@ public:
 	/// @returns the "self" parameter type for a bound function
 	TypePointer const& selfType() const;
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override;
 	virtual TypePointer unaryOperatorResult(Token::Value _operator) const override;
@@ -1070,7 +1080,7 @@ public:
 	MappingType(TypePointer const& _keyType, TypePointer const& _valueType):
 		m_keyType(_keyType), m_valueType(_valueType) {}
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual std::string toString(bool _short) const override;
 	virtual std::string canonicalName() const override;
@@ -1107,7 +1117,7 @@ public:
 	TypePointer const& actualType() const { return m_actualType; }
 
 	virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); }
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual bool canBeStored() const override { return false; }
 	virtual u256 storageSize() const override;
@@ -1135,7 +1145,7 @@ public:
 	virtual u256 storageSize() const override;
 	virtual bool canLiveOutsideStorage() const override { return false; }
 	virtual unsigned sizeOnStack() const override { return 0; }
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual std::string toString(bool _short) const override;
 
@@ -1156,7 +1166,7 @@ public:
 	explicit ModuleType(SourceUnit const& _source): m_sourceUnit(_source) {}
 
 	virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); }
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual bool canBeStored() const override { return false; }
 	virtual bool canLiveOutsideStorage() const override { return true; }
@@ -1186,7 +1196,7 @@ public:
 		return TypePointer();
 	}
 
-	virtual std::string identifier() const override;
+	virtual std::string richIdentifier() const override;
 	virtual bool operator==(Type const& _other) const override;
 	virtual bool canBeStored() const override { return false; }
 	virtual bool canLiveOutsideStorage() const override { return true; }
@@ -1210,7 +1220,7 @@ class InaccessibleDynamicType: public Type
 public:
 	virtual Category category() const override { return Category::InaccessibleDynamic; }
 
-	virtual std::string identifier() const override { return "t_inaccessible"; }
+	virtual std::string richIdentifier() const override { return "t_inaccessible"; }
 	virtual bool isImplicitlyConvertibleTo(Type const&) const override { return false; }
 	virtual bool isExplicitlyConvertibleTo(Type const&) const override { return false; }
 	virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); }
diff --git a/test/libsolidity/SolidityTypes.cpp b/test/libsolidity/SolidityTypes.cpp
index 9f385a04..bc9f2fe1 100644
--- a/test/libsolidity/SolidityTypes.cpp
+++ b/test/libsolidity/SolidityTypes.cpp
@@ -88,6 +88,21 @@ BOOST_AUTO_TEST_CASE(storage_layout_arrays)
 	BOOST_CHECK(ArrayType(DataLocation::Storage, make_shared<FixedBytesType>(32), 9).storageSize() == 9);
 }
 
+BOOST_AUTO_TEST_CASE(type_escaping)
+{
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier("("), "$_");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier(")"), "_$");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier(","), "_$_");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier("$"), "$$$");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier("()"), "$__$");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier("(,)"), "$__$__$");
+	BOOST_CHECK_EQUAL(Type::escapeIdentifier("(,$,)"), "$__$_$$$_$__$");
+	BOOST_CHECK_EQUAL(
+		Type::escapeIdentifier("((__(_$_$$,__($$,,,$$),$,,,)))$$,$$"),
+		"$_$___$__$$$_$$$$$$_$___$_$$$$$$_$__$__$_$$$$$$_$_$_$$$_$__$__$__$_$_$$$$$$$_$_$$$$$$"
+	);
+}
+
 BOOST_AUTO_TEST_CASE(type_identifiers)
 {
 	ASTNode::resetID();