Merge pull request #9877 from a3d4/fix-homonymous-declarations-warnings

Fix shadowing/same-name warnings for later declarations

Changelog.md

@@ -9,6 +9,7 @@ Compiler Features:
 
 Bugfixes:
  * Type Checker: Fix internal compiler error caused by storage parameters with nested mappings in libraries.
+ * Name Resolver: Fix shadowing/same-name warnings for later declarations.
 
 
 ### 0.7.3 (2020-10-07)
@@ -65,8 +66,8 @@ Bugfixes:
  * Code generator: Fix internal error on stripping dynamic types from return parameters on EVM versions without ``RETURNDATACOPY``.
  * Type Checker: Add missing check against nested dynamic arrays in ABI encoding functions when ABIEncoderV2 is disabled.
  * Type Checker: Correct the error message for invalid named parameter in a call to refer to the right argument.
- * Type Checker: Correct the warning for homonymous, but not shadowing declarations.
  * Type Checker: Disallow ``virtual`` for modifiers in libraries.
+ * Name Resolver: Correct the warning for homonymous, but not shadowing declarations.
  * Type system: Fix internal error on implicit conversion of contract instance to the type of its ``super``.
  * Type system: Fix internal error on implicit conversion of string literal to a calldata string.
  * Type system: Fix named parameters in overloaded function and event calls being matched incorrectly if the order differs from the declaration.

libsolidity/analysis/DeclarationContainer.cpp

@@ -100,6 +100,7 @@ bool DeclarationContainer::isInvisible(ASTString const& _name) const
 bool DeclarationContainer::registerDeclaration(
 	Declaration const& _declaration,
 	ASTString const* _name,
+	langutil::SourceLocation const* _location,
 	bool _invisible,
 	bool _update
 )
@@ -115,15 +116,34 @@ bool DeclarationContainer::registerDeclaration(
 		m_declarations.erase(*_name);
 		m_invisibleDeclarations.erase(*_name);
 	}
-	else if (conflictingDeclaration(_declaration, _name))
+	else
+	{
+		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.isEventParameter());
+		if (m_enclosingContainer && !special)
+			m_homonymCandidates.emplace_back(*_name, _location ? _location : &_declaration.location());
+	}
+
 	vector<Declaration const*>& decls = _invisible ? m_invisibleDeclarations[*_name] : m_declarations[*_name];
 	if (!util::contains(decls, &_declaration))
 		decls.push_back(&_declaration);
 	return true;
 }
 
+bool DeclarationContainer::registerDeclaration(
+	Declaration const& _declaration,
+	bool _invisible,
+	bool _update
+)
+{
+	return registerDeclaration(_declaration, nullptr, nullptr, _invisible, _update);
+}
+
 vector<Declaration const*> DeclarationContainer::resolveName(ASTString const& _name, bool _recursive, bool _alsoInvisible) const
 {
 	solAssert(!_name.empty(), "Attempt to resolve empty name.");
@@ -164,3 +184,16 @@ vector<ASTString> DeclarationContainer::similarNames(ASTString const& _name) con
 
 	return similar;
 }
+
+void DeclarationContainer::populateHomonyms(back_insert_iterator<Homonyms> _it) const
+{
+	for (DeclarationContainer const* innerContainer: m_innerContainers)
+		innerContainer->populateHomonyms(_it);
+
+	for (auto [name, location]: m_homonymCandidates)
+	{
+		vector<Declaration const*> const& declarations = m_enclosingContainer->resolveName(name, true, true);
+		if (!declarations.empty())
+			_it = make_pair(location, declarations);
+	}
+}

libsolidity/analysis/DeclarationContainer.h

@@ -24,9 +24,9 @@
 #pragma once
 
 #include <libsolidity/ast/ASTForward.h>
+#include <liblangutil/Exceptions.h>
+#include <liblangutil/SourceLocation.h>
 #include <boost/noncopyable.hpp>
-#include <map>
-#include <set>
 
 namespace solidity::frontend
 {
@@ -38,17 +38,26 @@ namespace solidity::frontend
 class DeclarationContainer
 {
 public:
+	using Homonyms = std::vector<std::pair<langutil::SourceLocation const*, std::vector<Declaration const*>>>;
+
 	explicit DeclarationContainer(
 		ASTNode const* _enclosingNode = nullptr,
-		DeclarationContainer const* _enclosingContainer = nullptr
+		DeclarationContainer* _enclosingContainer = nullptr
 	):
-		m_enclosingNode(_enclosingNode), m_enclosingContainer(_enclosingContainer) {}
+		m_enclosingNode(_enclosingNode), m_enclosingContainer(_enclosingContainer)
+	{
+		if (_enclosingContainer)
+			_enclosingContainer->m_innerContainers.emplace_back(this);
+	}
 	/// Registers the declaration in the scope unless its name is already declared or the name is empty.
 	/// @param _name the name to register, if nullptr the intrinsic name of @a _declaration is used.
-	/// @param _invisible if true, registers the declaration, reports name clashes but does not return it in @a resolveName
+	/// @param _location alternative location, used to point at homonymous declarations.
-	/// @param _update if true, replaces a potential declaration that is already present
+	/// @param _invisible if true, registers the declaration, reports name clashes but does not return it in @a resolveName.
+	/// @param _update if true, replaces a potential declaration that is already present.
 	/// @returns false if the name was already declared.
-	bool registerDeclaration(Declaration const& _declaration, ASTString const* _name = nullptr, bool _invisible = false, bool _update = false);
+	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;
 	ASTNode const* enclosingNode() const { return m_enclosingNode; }
 	DeclarationContainer const* enclosingContainer() const { return m_enclosingContainer; }
@@ -67,11 +76,18 @@ public:
 	/// Searches this and all parent containers.
 	std::vector<ASTString> similarNames(ASTString const& _name) const;
 
+	/// Populates a vector of (location, declaration) pairs, where location is a location of an inner-scope declaration,
+	/// and declaration is the corresponding homonymous outer-scope declaration.
+	void populateHomonyms(std::back_insert_iterator<Homonyms> _it) const;
+
 private:
 	ASTNode const* m_enclosingNode;
 	DeclarationContainer const* m_enclosingContainer;
+	std::vector<DeclarationContainer const*> m_innerContainers;
 	std::map<ASTString, std::vector<Declaration const*>> m_declarations;
 	std::map<ASTString, std::vector<Declaration const*>> m_invisibleDeclarations;
+	/// List of declarations (name and location) to check later for homonymity.
+	std::vector<std::pair<std::string, langutil::SourceLocation const*>> m_homonymCandidates;
 };
 
 }

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -28,6 +28,7 @@
 #include <liblangutil/ErrorReporter.h>
 #include <libsolutil/StringUtils.h>
 #include <boost/algorithm/string.hpp>
+#include <unordered_set>
 
 using namespace std;
 using namespace solidity::langutil;
@@ -47,7 +48,7 @@ NameAndTypeResolver::NameAndTypeResolver(
 	m_scopes[nullptr] = make_shared<DeclarationContainer>();
 	for (Declaration const* declaration: _globalContext.declarations())
 	{
-		solAssert(m_scopes[nullptr]->registerDeclaration(*declaration), "Unable to register global declaration.");
+		solAssert(m_scopes[nullptr]->registerDeclaration(*declaration, false, false), "Unable to register global declaration.");
 	}
 }
 
@@ -149,7 +150,7 @@ bool NameAndTypeResolver::updateDeclaration(Declaration const& _declaration)
 {
 	try
 	{
-		m_scopes[nullptr]->registerDeclaration(_declaration, nullptr, false, true);
+		m_scopes[nullptr]->registerDeclaration(_declaration, false, true);
 		solAssert(_declaration.scope() == nullptr, "Updated declaration outside global scope.");
 	}
 	catch (langutil::FatalError const&)
@@ -202,7 +203,7 @@ Declaration const* NameAndTypeResolver::pathFromCurrentScope(vector<ASTString> c
 		return nullptr;
 }
 
-void NameAndTypeResolver::warnVariablesNamedLikeInstructions()
+void NameAndTypeResolver::warnVariablesNamedLikeInstructions() const
 {
 	for (auto const& instruction: evmasm::c_instructions)
 	{
@@ -223,6 +224,52 @@ void NameAndTypeResolver::warnVariablesNamedLikeInstructions()
 	}
 }
 
+void NameAndTypeResolver::warnHomonymDeclarations() const
+{
+	DeclarationContainer::Homonyms homonyms;
+	m_scopes.at(nullptr)->populateHomonyms(back_inserter(homonyms));
+
+	for (auto [innerLocation, outerDeclarations]: homonyms)
+	{
+		solAssert(innerLocation && !outerDeclarations.empty(), "");
+
+		bool magicShadowed = false;
+		SecondarySourceLocation homonymousLocations;
+		SecondarySourceLocation shadowedLocations;
+		for (Declaration const* outerDeclaration: outerDeclarations)
+		{
+			solAssert(outerDeclaration, "");
+			if (dynamic_cast<MagicVariableDeclaration const*>(outerDeclaration))
+				magicShadowed = true;
+			else if (!outerDeclaration->isVisibleInContract())
+				homonymousLocations.append("The other declaration is here:", outerDeclaration->location());
+			else
+				shadowedLocations.append("The shadowed declaration is here:", outerDeclaration->location());
+		}
+
+		if (magicShadowed)
+			m_errorReporter.warning(
+				2319_error,
+				*innerLocation,
+				"This declaration shadows a builtin symbol."
+			);
+		if (!homonymousLocations.infos.empty())
+			m_errorReporter.warning(
+				8760_error,
+				*innerLocation,
+				"This declaration has the same name as another declaration.",
+				homonymousLocations
+			);
+		if (!shadowedLocations.infos.empty())
+			m_errorReporter.warning(
+				2519_error,
+				*innerLocation,
+				"This declaration shadows an existing declaration.",
+				shadowedLocations
+			);
+	}
+}
+
 void NameAndTypeResolver::setScope(ASTNode const* _node)
 {
 	m_currentScope = m_scopes[_node].get();
@@ -281,8 +328,8 @@ bool NameAndTypeResolver::resolveNamesAndTypesInternal(ASTNode& _node, bool _res
 		}
 
 		// make "this" and "super" invisible.
-		m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), nullptr, true, true);
+		m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), true, true);
-		m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), nullptr, true, true);
+		m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), true, true);
 		m_globalContext.resetCurrentContract();
 
 		return success;
@@ -303,7 +350,7 @@ void NameAndTypeResolver::importInheritedScope(ContractDefinition const& _base)
 		for (auto const& declaration: nameAndDeclaration.second)
 			// Import if it was declared in the base, is not the constructor and is visible in derived classes
 			if (declaration->scope() == &_base && declaration->isVisibleInDerivedContracts())
-				if (!m_currentScope->registerDeclaration(*declaration))
+				if (!m_currentScope->registerDeclaration(*declaration, false, false))
 				{
 					SourceLocation firstDeclarationLocation;
 					SourceLocation secondDeclarationLocation;
@@ -458,19 +505,11 @@ bool DeclarationRegistrationHelper::registerDeclaration(
 		_errorLocation = &_declaration.location();
 
 	string name = _name ? *_name : _declaration.name();
-	Declaration const* shadowedDeclaration = nullptr;
-	// 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 warnOnShadow = !_declaration.isStructMember() && !_declaration.isEnumValue() && !_declaration.isEventParameter();
-	if (warnOnShadow && !name.empty() && _container.enclosingContainer())
-		for (auto const* decl: _container.enclosingContainer()->resolveName(name, true, true))
-			shadowedDeclaration = decl;
 
 	// We use "invisible" for both inactive variables in blocks and for members invisible in contracts.
 	// They cannot both be true at the same time.
 	solAssert(!(_inactive && !_declaration.isVisibleInContract()), "");
-	if (!_container.registerDeclaration(_declaration, _name, !_declaration.isVisibleInContract() || _inactive))
+	if (!_container.registerDeclaration(_declaration, _name, _errorLocation, !_declaration.isVisibleInContract() || _inactive, false))
 	{
 		SourceLocation firstDeclarationLocation;
 		SourceLocation secondDeclarationLocation;
@@ -499,34 +538,7 @@ bool DeclarationRegistrationHelper::registerDeclaration(
 		);
 		return false;
 	}
-	else if (shadowedDeclaration)
-	{
-		if (dynamic_cast<MagicVariableDeclaration const*>(shadowedDeclaration))
-			_errorReporter.warning(
-				2319_error,
-				*_errorLocation,
-				"This declaration shadows a builtin symbol."
-			);
-		else
-		{
-			auto shadowedLocation = shadowedDeclaration->location();
 
-			if (!shadowedDeclaration->isVisibleInContract())
-				_errorReporter.warning(
-					8760_error,
-					_declaration.location(),
-					"This declaration has the same name as another declaration.",
-					SecondarySourceLocation().append("The other declaration is here:", shadowedLocation)
-				);
-			else
-				_errorReporter.warning(
-					2519_error,
-					_declaration.location(),
-					"This declaration shadows an existing declaration.",
-					SecondarySourceLocation().append("The shadowed declaration is here:", shadowedLocation)
-			);
-		}
-	}
 	return true;
 }
 
@@ -556,8 +568,8 @@ bool DeclarationRegistrationHelper::visit(ImportDirective& _import)
 bool DeclarationRegistrationHelper::visit(ContractDefinition& _contract)
 {
 	m_globalContext.setCurrentContract(_contract);
-	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), nullptr, false, true);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), false, true);
-	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), nullptr, false, true);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), false, true);
 	m_currentContract = &_contract;
 
 	return ASTVisitor::visit(_contract);
@@ -566,8 +578,8 @@ bool DeclarationRegistrationHelper::visit(ContractDefinition& _contract)
 void DeclarationRegistrationHelper::endVisit(ContractDefinition& _contract)
 {
 	// make "this" and "super" invisible.
-	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), nullptr, true, true);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), true, true);
-	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), nullptr, true, true);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), true, true);
 	m_globalContext.resetCurrentContract();
 	m_currentContract = nullptr;
 	ASTVisitor::endVisit(_contract);
@@ -612,11 +624,14 @@ void DeclarationRegistrationHelper::endVisitNode(ASTNode& _node)
 
 void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _subScope)
 {
-	shared_ptr<DeclarationContainer> container{make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())};
+	if (m_scopes.count(&_subScope))
-	bool newlyAdded = m_scopes.emplace(&_subScope, move(container)).second;
+		// Source units are the only AST nodes for which containers can be created from multiple places due to imports.
-	// Source units are the only AST nodes for which containers can be created from multiple places
+		solAssert(dynamic_cast<SourceUnit const*>(&_subScope), "Unexpected scope type.");
-	// due to imports.
+	else
-	solAssert(newlyAdded || dynamic_cast<SourceUnit const*>(&_subScope), "Unable to add new scope.");
+	{
+		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;
 }
 

libsolidity/analysis/NameAndTypeResolver.h

@@ -93,7 +93,10 @@ public:
 	Declaration const* pathFromCurrentScope(std::vector<ASTString> const& _path) const;
 
 	/// Generate and store warnings about variables that are named like instructions.
-	void warnVariablesNamedLikeInstructions();
+	void warnVariablesNamedLikeInstructions() const;
+
+	/// Generate and store warnings about declarations with the same name.
+	void warnHomonymDeclarations() const;
 
 	/// @returns a list of similar identifiers in the current and enclosing scopes. May return empty string if no suggestions.
 	std::string similarNameSuggestions(ASTString const& _name) const;

libsolidity/interface/CompilerStack.cpp

@@ -343,6 +343,8 @@ bool CompilerStack::analyze()
 			if (source->ast && !resolver.performImports(*source->ast, sourceUnitsByName))
 				return false;
 
+		resolver.warnHomonymDeclarations();
+
 		for (Source const* source: m_sourceOrder)
 			if (source->ast && !resolver.resolveNamesAndTypes(*source->ast))
 				return false;

test/libsolidity/smtCheckerTests/typecast/function_type_to_function_type_internal.sol

@@ -11,6 +11,7 @@ contract C {
 	}
 }
 // ----
+// Warning 2519: (128-159): This declaration shadows an existing declaration.
 // Warning 6328: (207-227): CHC: Assertion violation happens here.
 // Warning 6328: (231-245): CHC: Assertion violation happens here.
 // Warning 5729: (214-218): Assertion checker does not yet implement this type of function call.

test/libsolidity/syntaxTests/functionCalls/named_arguments_duplicate_parameter.sol

@@ -8,4 +8,5 @@ contract test {
 }
 // ----
 // Warning 2519: (31-37): This declaration shadows an existing declaration.
+// Warning 2519: (39-45): This declaration shadows an existing declaration.
 // TypeError 6995: (159-160): Duplicate named argument "a".

test/libsolidity/syntaxTests/functionCalls/named_arguments_empty.sol

@@ -8,4 +8,5 @@ contract test {
 }
 // ----
 // Warning 2519: (31-37): This declaration shadows an existing declaration.
+// Warning 2519: (39-45): This declaration shadows an existing declaration.
 // TypeError 6160: (153-158): Wrong argument count for function call: 0 arguments given but expected 2.

test/libsolidity/syntaxTests/functionCalls/named_arguments_wrong_count.sol

@@ -8,4 +8,5 @@ contract test {
 }
 // ----
 // Warning 2519: (31-37): This declaration shadows an existing declaration.
+// Warning 2519: (39-45): This declaration shadows an existing declaration.
 // TypeError 6160: (153-162): Wrong argument count for function call: 1 arguments given but expected 2.

test/libsolidity/syntaxTests/scoping/double_variable_declaration_disjoint_scope_activation.sol

@@ -5,5 +5,6 @@ contract test {
     }
 }
 // ----
+// Warning 2519: (57-63): This declaration shadows an existing declaration.
 // Warning 2072: (57-63): Unused local variable.
 // Warning 2072: (75-81): Unused local variable.

test/libsolidity/syntaxTests/scoping/double_variable_declaration_same_and_disjoint_scope.sol

@@ -6,5 +6,5 @@ contract test {
     }
 }
 // ----
-// Warning 2519: (73-79): This declaration shadows an existing declaration.
 // DeclarationError 2333: (91-97): Identifier already declared.
+// Warning 2519: (73-79): This declaration shadows an existing declaration.

test/libsolidity/syntaxTests/scoping/name_pseudo_shadowing2.sol

@@ -0,0 +1,7 @@
+contract test {
+    function e() external { }
+    function f() public pure { uint e; e = 0; }
+    function e(int) external { }
+}
+// ----
+// Warning 8760: (77-83): This declaration has the same name as another declaration.

test/libsolidity/syntaxTests/scoping/name_shadowing3.sol

@@ -0,0 +1,10 @@
+function e() {}
+contract test {
+    function f() pure public { uint e; uint g; uint h; e = g = h = 0; }
+    function g() pure public {}
+}
+function h() {}
+// ----
+// Warning 2519: (63-69): This declaration shadows an existing declaration.
+// Warning 2519: (71-77): This declaration shadows an existing declaration.
+// Warning 2519: (79-85): This declaration shadows an existing declaration.