Fix shadowing struct types by struct member names

a3d4 · cameel · commit 6d8b819e2361 · 2021-04-29T23:24:37.000+02:00
diff --git a/Changelog.md b/Changelog.md
@@ -10,6 +10,7 @@ Compiler Features:
 
 
 Bugfixes:
+ * Name Resolver: Prevent member/parameter names from shadowing type names within struct/enum/error/event declarations.
 
 
 ### 0.8.4 (2021-04-21)
diff --git a/libsolidity/analysis/DeclarationContainer.cpp b/libsolidity/analysis/DeclarationContainer.cpp
@@ -27,6 +27,9 @@
 #include <libsolidity/ast/Types.h>
 #include <libsolutil/StringUtils.h>
 
+#include <range/v3/view/filter.hpp>
+#include <range/v3/range/conversion.hpp>
+
 using namespace std;
 using namespace solidity;
 using namespace solidity::frontend;
@@ -121,11 +124,7 @@ bool DeclarationContainer::registerDeclaration(
 		if (conflictingDeclaration(_declaration, _name))
 			return false;
 
-		// Do not warn about shadowing for structs and enums because their members are
-		// not accessible without prefixes. Also do not warn about event parameters
-		// because they do not participate in any proper scope.
-		bool special = _declaration.scope() && (_declaration.isStructMember() || _declaration.isEnumValue() || _declaration.isEventOrErrorParameter());
-		if (m_enclosingContainer && !special)
+		if (m_enclosingContainer && _declaration.isVisibleAsUnqualifiedName())
 			m_homonymCandidates.emplace_back(*_name, _location ? _location : &_declaration.location());
 	}
 
@@ -144,16 +143,35 @@ bool DeclarationContainer::registerDeclaration(
 	return registerDeclaration(_declaration, nullptr, nullptr, _invisible, _update);
 }
 
-vector<Declaration const*> DeclarationContainer::resolveName(ASTString const& _name, bool _recursive, bool _alsoInvisible) const
+vector<Declaration const*> DeclarationContainer::resolveName(
+	ASTString const& _name,
+	bool _recursive,
+	bool _alsoInvisible,
+	bool _onlyVisibleAsUnqualifiedNames
+) const
 {
 	solAssert(!_name.empty(), "Attempt to resolve empty name.");
 	vector<Declaration const*> result;
+
 	if (m_declarations.count(_name))
-		result = m_declarations.at(_name);
+	{
+		if (_onlyVisibleAsUnqualifiedNames)
+			result += m_declarations.at(_name) | ranges::views::filter(&Declaration::isVisibleAsUnqualifiedName) | ranges::to_vector;
+		else
+			result += m_declarations.at(_name);
+	}
+
 	if (_alsoInvisible && m_invisibleDeclarations.count(_name))
-		result += m_invisibleDeclarations.at(_name);
+	{
+		if (_onlyVisibleAsUnqualifiedNames)
+			result += m_invisibleDeclarations.at(_name) | ranges::views::filter(&Declaration::isVisibleAsUnqualifiedName) | ranges::to_vector;
+		else
+			result += m_invisibleDeclarations.at(_name);
+	}
+
 	if (result.empty() && _recursive && m_enclosingContainer)
-		result = m_enclosingContainer->resolveName(_name, true, _alsoInvisible);
+		result = m_enclosingContainer->resolveName(_name, true, _alsoInvisible, false);
+
 	return result;
 }
 
diff --git a/libsolidity/analysis/DeclarationContainer.h b/libsolidity/analysis/DeclarationContainer.h
@@ -39,11 +39,10 @@ class DeclarationContainer
 public:
 	using Homonyms = std::vector<std::pair<langutil::SourceLocation const*, std::vector<Declaration const*>>>;
 
-	explicit DeclarationContainer(
-		ASTNode const* _enclosingNode = nullptr,
-		DeclarationContainer* _enclosingContainer = nullptr
-	):
-		m_enclosingNode(_enclosingNode), m_enclosingContainer(_enclosingContainer)
+	DeclarationContainer() = default;
+	explicit DeclarationContainer(ASTNode const* _enclosingNode, DeclarationContainer* _enclosingContainer):
+		m_enclosingNode(_enclosingNode),
+		m_enclosingContainer(_enclosingContainer)
 	{
 		if (_enclosingContainer)
 			_enclosingContainer->m_innerContainers.emplace_back(this);
@@ -57,7 +56,21 @@ class DeclarationContainer
 	bool registerDeclaration(Declaration const& _declaration, ASTString const* _name, langutil::SourceLocation const* _location, bool _invisible, bool _update);
 	bool registerDeclaration(Declaration const& _declaration, bool _invisible, bool _update);
 
-	std::vector<Declaration const*> resolveName(ASTString const& _name, bool _recursive = false, bool _alsoInvisible = false) const;
+	/// Finds all declarations that in the current scope can be referred to using specified name.
+	/// @param _name the name to look for.
+	/// @param _recursive if true and there are no matching declarations in the current container,
+	///        recursively searches the enclosing containers as well.
+	/// @param _alsoInvisible if true, include invisible declaration in the results.
+	/// @param _onlyVisibleAsUnqualifiedNames if true, only include declarations which can be
+	///        referenced outside of the current scope without being qualified with the name of the
+	///        scope. I.e. when they shadow names in the enclosing scope(s). This option is **not**
+	///        passed to the enclosing scope when used together with @a _recursive.
+	std::vector<Declaration const*> resolveName(
+		ASTString const& _name,
+		bool _recursive = false,
+		bool _alsoInvisible = false,
+		bool _onlyVisibleAsUnqualifiedNames = false
+	) const;
 	ASTNode const* enclosingNode() const { return m_enclosingNode; }
 	DeclarationContainer const* enclosingContainer() const { return m_enclosingContainer; }
 	std::map<ASTString, std::vector<Declaration const*>> const& declarations() const { return m_declarations; }
@@ -80,8 +93,8 @@ class DeclarationContainer
 	void populateHomonyms(std::back_insert_iterator<Homonyms> _it) const;
 
 private:
-	ASTNode const* m_enclosingNode;
-	DeclarationContainer const* m_enclosingContainer;
+	ASTNode const* m_enclosingNode = nullptr;
+	DeclarationContainer const* m_enclosingContainer = nullptr;
 	std::vector<DeclarationContainer const*> m_innerContainers;
 	std::map<ASTString, std::vector<Declaration const*>> m_declarations;
 	std::map<ASTString, std::vector<Declaration const*>> m_invisibleDeclarations;
diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp
@@ -182,7 +182,8 @@ vector<Declaration const*> NameAndTypeResolver::nameFromCurrentScope(ASTString c
 Declaration const* NameAndTypeResolver::pathFromCurrentScope(vector<ASTString> const& _path) const
 {
 	solAssert(!_path.empty(), "");
-	vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true);
+	vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true, false, true);
+
 	for (size_t i = 1; i < _path.size() && candidates.size() == 1; i++)
 	{
 		if (!m_scopes.count(candidates.front()))
@@ -627,7 +628,10 @@ void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _subScope)
 		solAssert(dynamic_cast<SourceUnit const*>(&_subScope), "Unexpected scope type.");
 	else
 	{
-		bool newlyAdded = m_scopes.emplace(&_subScope, make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())).second;
+		bool newlyAdded = m_scopes.emplace(
+			&_subScope,
+			make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())
+		).second;
 		solAssert(newlyAdded, "Unable to add new scope.");
 	}
 	m_currentScope = &_subScope;
diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp
@@ -545,6 +545,18 @@ bool Declaration::isEventOrErrorParameter() const
 	return dynamic_cast<EventDefinition const*>(scope()) || dynamic_cast<ErrorDefinition const*>(scope());
 }
 
+bool Declaration::isVisibleAsUnqualifiedName() const
+{
+	if (!scope())
+		return true;
+	if (isStructMember() || isEnumValue() || isEventOrErrorParameter())
+		return false;
+	if (auto functionDefinition = dynamic_cast<FunctionDefinition const*>(scope()))
+		if (!functionDefinition->isImplemented())
+			return false; // parameter of a function without body
+	return true;
+}
+
 DeclarationAnnotation& Declaration::annotation() const
 {
 	return initAnnotation<DeclarationAnnotation>();
diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h
@@ -260,6 +260,12 @@ class Declaration: public ASTNode, public Scopable
 	/// @returns true if this is a declaration of a parameter of an event.
 	bool isEventOrErrorParameter() const;
 
+	/// @returns false if the declaration can never be referenced without being qualified with a scope.
+	/// Usually the name alone can be used to refer to the corresponding entity.
+	/// But, for example, struct member names or enum member names always require a prefix.
+	/// Another example is event parameter names, which do not participate in any proper scope.
+	bool isVisibleAsUnqualifiedName() const;
+
 	/// @returns the type of expressions referencing this declaration.
 	/// This can only be called once types of variable declarations have already been resolved.
 	virtual Type const* type() const = 0;
diff --git a/test/libsolidity/semanticTests/errors/named_parameters_shadowing_types.sol b/test/libsolidity/semanticTests/errors/named_parameters_shadowing_types.sol
@@ -0,0 +1,24 @@
+contract C {
+    enum EnumType {A, B, C}
+
+    struct StructType {
+        uint x;
+    }
+
+    error E1(StructType StructType);
+    error E2(EnumType StructType, StructType EnumType);
+
+    function f() public {
+        revert E1({StructType: StructType(42)});
+    }
+
+    function g() public {
+        revert E2({EnumType: StructType(42), StructType: EnumType.B});
+    }
+}
+// ====
+// compileToEwasm: also
+// compileViaYul: also
+// ----
+// f() -> FAILURE, hex"33a54193", hex"000000000000000000000000000000000000000000000000000000000000002a"
+// g() -> FAILURE, hex"374b9387", hex"0000000000000000000000000000000000000000000000000000000000000001", hex"000000000000000000000000000000000000000000000000000000000000002a"
diff --git a/test/libsolidity/syntaxTests/enums/enum_member_shadowing.sol b/test/libsolidity/syntaxTests/enums/enum_member_shadowing.sol
@@ -0,0 +1,12 @@
+contract C {
+    struct S {
+        uint x;
+    }
+
+    enum E {E, S, C, a, f}
+
+    uint a;
+
+    function f() public pure {}
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_builtin_type_used_as_param_name.sol b/test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_builtin_type_used_as_param_name.sol
@@ -0,0 +1,5 @@
+contract C {
+    error E(int bytes, bytes x);
+}
+// ----
+// ParserError 2314: (29-34): Expected ',' but got 'bytes'
diff --git a/test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_param_name.sol b/test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_param_name.sol
@@ -0,0 +1,12 @@
+contract C {
+    enum EnumType {A, B, C}
+
+    struct StructType {
+        uint x;
+    }
+
+    error E1(StructType StructType);
+    error E2(EnumType EnumType);
+    error E3(EnumType StructType, StructType EnumType);
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/events/event_param_type_shadowed_by_param_name.sol b/test/libsolidity/syntaxTests/events/event_param_type_shadowed_by_param_name.sol
@@ -0,0 +1,13 @@
+contract C {
+    enum EnumType {A, B, C}
+
+    struct StructType {
+        uint x;
+    }
+
+    event E1(StructType StructType);
+    event E2(EnumType EnumType);
+    event E3(EnumType StructType, StructType EnumType);
+    event E4(StructType indexed StructType) anonymous;
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_name_vs_contract.sol b/test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_name_vs_contract.sol
@@ -0,0 +1,20 @@
+interface I {
+    function f(uint I) external;         // OK
+}
+
+library L {
+    function f(uint L) public pure {}    // warning
+}
+
+abstract contract A {
+    function f(uint A) public pure {}    // warning
+    function g(uint A) public virtual;   // OK
+}
+
+contract C {
+    function f(uint C) public pure {}    // warning
+}
+// ----
+// Warning 2519: (91-97): This declaration shadows an existing declaration.
+// Warning 2519: (168-174): This declaration shadows an existing declaration.
+// Warning 2519: (283-289): This declaration shadows an existing declaration.
diff --git a/test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_vs_its_function.sol b/test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_vs_its_function.sol
@@ -0,0 +1,5 @@
+contract C {
+    function f(uint f) pure public {}
+}
+// ----
+// Warning 2519: (28-34): This declaration shadows an existing declaration.
diff --git a/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_free_function.sol b/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_free_function.sol
diff --git a/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_functions.sol b/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_functions.sol
diff --git a/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_state_variable.sol b/test/libsolidity/syntaxTests/scoping/name_shadowing_local_variable_vs_state_variable.sol
diff --git a/test/libsolidity/syntaxTests/structs/member_type_eq_name.sol b/test/libsolidity/syntaxTests/structs/member_type_eq_name.sol
@@ -3,4 +3,4 @@ contract C {
   function f(function(S memory) external) public {}
 }
 // ----
-// TypeError 5172: (25-26): Name has to refer to a struct, enum or contract.
+// DeclarationError 7920: (25-26): Identifier not found or not unique.
diff --git a/test/libsolidity/syntaxTests/structs/member_type_eq_name_2.sol b/test/libsolidity/syntaxTests/structs/member_type_eq_name_2.sol
@@ -0,0 +1,6 @@
+contract C {
+    enum E {a, b, c}
+    struct S {E X; uint E;}
+    struct T {E T; uint E;}
+    struct U {E E;}
+}

Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,7 @@ Compiler Features:`
`10`	`10`
`11`	`11`
`12`	`12`	`Bugfixes:`
	`13`	`+ * Name Resolver: Prevent member/parameter names from shadowing type names within struct/enum/error/event declarations.`
`13`	`14`
`14`	`15`
`15`	`16`	`### 0.8.4 (2021-04-21)`
Original file line number	Diff line number	Diff line change
`@@ -182,7 +182,8 @@ vector<Declaration const*> NameAndTypeResolver::nameFromCurrentScope(ASTString c`
`182`	`182`	`Declaration const* NameAndTypeResolver::pathFromCurrentScope(vector<ASTString> const& _path) const`
`183`	`183`	`{`
`184`	`184`	`solAssert(!_path.empty(), "");`
`185`		`- vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true);`
	`185`	`+ vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true, false, true);`
	`186`	`+`
`186`	`187`	`for (size_t i = 1; i < _path.size() && candidates.size() == 1; i++)`
`187`	`188`	`{`
`188`	`189`	`if (!m_scopes.count(candidates.front()))`
`@@ -627,7 +628,10 @@ void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _subScope)`
`627`	`628`	`solAssert(dynamic_cast<SourceUnit const*>(&_subScope), "Unexpected scope type.");`
`628`	`629`	`else`
`629`	`630`	`{`
`630`		`- bool newlyAdded = m_scopes.emplace(&_subScope, make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())).second;`
	`631`	`+ bool newlyAdded = m_scopes.emplace(`
	`632`	`+ &_subScope,`
	`633`	`+ make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())`
	`634`	`+ ).second;`
`631`	`635`	`solAssert(newlyAdded, "Unable to add new scope.");`
`632`	`636`	`}`
`633`	`637`	`m_currentScope = &_subScope;`