Merge pull request #1637 from ethereum/warn-shadowing-globals

Warn if shadowing built-ins

Changelog.md

@@ -8,6 +8,7 @@ Features:
  * Type Checker: Include types in explicit conversion error message.
  * Type Checker: Raise proper error for arrays too large for ABI encoding.
  * Type checker: Warn if using ``this`` in a constructor.
+ * Type checker: Warn when existing symbols, including builtins, are overwritten.
 
 Bugfixes:
  * Type Checker: Fix invalid "specify storage keyword" warning for reference members of structs.

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -104,29 +104,18 @@ bool NameAndTypeResolver::performImports(SourceUnit& _sourceUnit, map<string, So
 					}
 					else
 						for (Declaration const* declaration: declarations)
-						{
-							ASTString const* name = alias.second ? alias.second.get() : &declaration->name();
-							if (!target.registerDeclaration(*declaration, name))
-							{
-								m_errorReporter.declarationError(
-									imp->location(),
-									"Identifier \"" + *name + "\" already declared."
-								);
+							if (!DeclarationRegistrationHelper::registerDeclaration(
+								target, *declaration, alias.second.get(), &imp->location(), true, m_errorReporter
+							))
 								error = true;
-							}
-						}
 				}
 			else if (imp->name().empty())
 				for (auto const& nameAndDeclaration: scope->second->declarations())
 					for (auto const& declaration: nameAndDeclaration.second)
-						if (!target.registerDeclaration(*declaration, &nameAndDeclaration.first))
-						{
-							m_errorReporter.declarationError(
-								imp->location(),
-								"Identifier \"" + nameAndDeclaration.first + "\" already declared."
-							);
-							error = true;
-						}
+						if (!DeclarationRegistrationHelper::registerDeclaration(
+							target, *declaration, &nameAndDeclaration.first, &imp->location(), true, m_errorReporter
+						))
+							error =  true;
 		}
 	return !error;
 }
@@ -450,6 +439,75 @@ DeclarationRegistrationHelper::DeclarationRegistrationHelper(
 	solAssert(m_currentScope == _currentScope, "Scopes not correctly closed.");
 }
 
+bool DeclarationRegistrationHelper::registerDeclaration(
+	DeclarationContainer& _container,
+	Declaration const& _declaration,
+	string const* _name,
+	SourceLocation const* _errorLocation,
+	bool _warnOnShadow,
+	ErrorReporter& _errorReporter
+)
+{
+	if (!_errorLocation)
+		_errorLocation = &_declaration.location();
+
+	Declaration const* shadowedDeclaration = nullptr;
+	if (_warnOnShadow && !_declaration.name().empty())
+		for (auto const* decl: _container.resolveName(_declaration.name(), true))
+			if (decl != &_declaration)
+			{
+				shadowedDeclaration = decl;
+				break;
+			}
+
+	if (!_container.registerDeclaration(_declaration, _name, !_declaration.isVisibleInContract()))
+	{
+		SourceLocation firstDeclarationLocation;
+		SourceLocation secondDeclarationLocation;
+		Declaration const* conflictingDeclaration = _container.conflictingDeclaration(_declaration, _name);
+		solAssert(conflictingDeclaration, "");
+		bool const comparable =
+			_errorLocation->sourceName &&
+			conflictingDeclaration->location().sourceName &&
+			*_errorLocation->sourceName == *conflictingDeclaration->location().sourceName;
+		if (comparable && _errorLocation->start < conflictingDeclaration->location().start)
+		{
+			firstDeclarationLocation = *_errorLocation;
+			secondDeclarationLocation = conflictingDeclaration->location();
+		}
+		else
+		{
+			firstDeclarationLocation = conflictingDeclaration->location();
+			secondDeclarationLocation = *_errorLocation;
+		}
+
+		_errorReporter.declarationError(
+			secondDeclarationLocation,
+			SecondarySourceLocation().append("The previous declaration is here:", firstDeclarationLocation),
+			"Identifier already declared."
+		);
+		return false;
+	}
+	else if (shadowedDeclaration)
+	{
+		if (dynamic_cast<MagicVariableDeclaration const*>(shadowedDeclaration))
+			_errorReporter.warning(
+				_declaration.location(),
+				"This declaration shadows a builtin symbol."
+			);
+		else
+		{
+			auto shadowedLocation = shadowedDeclaration->location();
+			_errorReporter.warning(
+				_declaration.location(),
+				"This declaration shadows an existing declaration.",
+				SecondarySourceLocation().append("The shadowed declaration is here:", shadowedLocation)
+			);
+		}
+	}
+	return true;
+}
+
 bool DeclarationRegistrationHelper::visit(SourceUnit& _sourceUnit)
 {
 	if (!m_scopes[&_sourceUnit])
@@ -590,30 +648,21 @@ void DeclarationRegistrationHelper::closeCurrentScope()
 void DeclarationRegistrationHelper::registerDeclaration(Declaration& _declaration, bool _opensScope)
 {
 	solAssert(m_currentScope && m_scopes.count(m_currentScope), "No current scope.");
-	if (!m_scopes[m_currentScope]->registerDeclaration(_declaration, nullptr, !_declaration.isVisibleInContract()))
-	{
-		SourceLocation firstDeclarationLocation;
-		SourceLocation secondDeclarationLocation;
-		Declaration const* conflictingDeclaration = m_scopes[m_currentScope]->conflictingDeclaration(_declaration);
-		solAssert(conflictingDeclaration, "");
 
-		if (_declaration.location().start < conflictingDeclaration->location().start)
-		{
-			firstDeclarationLocation = _declaration.location();
-			secondDeclarationLocation = conflictingDeclaration->location();
-		}
-		else
-		{
-			firstDeclarationLocation = conflictingDeclaration->location();
-			secondDeclarationLocation = _declaration.location();
-		}
+	bool warnAboutShadowing = true;
+	// Do not warn about shadowing for structs and enums because their members are
+	// not accessible without prefixes.
+	if (
+		dynamic_cast<StructDefinition const*>(m_currentScope) ||
+		dynamic_cast<EnumDefinition const*>(m_currentScope)
+	)
+		warnAboutShadowing = false;
+	// Do not warn about the constructor shadowing the contract.
+	if (auto fun = dynamic_cast<FunctionDefinition const*>(&_declaration))
+		if (fun->isConstructor())
+			warnAboutShadowing = false;
 
-		m_errorReporter.declarationError(
-			secondDeclarationLocation,
-			SecondarySourceLocation().append("The previous declaration is here:", firstDeclarationLocation),
-			"Identifier already declared."
-		);
-	}
+	registerDeclaration(*m_scopes[m_currentScope], _declaration, nullptr, nullptr, warnAboutShadowing, m_errorReporter);
 
 	_declaration.setScope(m_currentScope);
 	if (_opensScope)

libsolidity/analysis/NameAndTypeResolver.h

@@ -136,6 +136,15 @@ public:
 		ASTNode const* _currentScope = nullptr
 	);
 
+	static bool registerDeclaration(
+		DeclarationContainer& _container,
+		Declaration const& _declaration,
+		std::string const* _name,
+		SourceLocation const* _errorLocation,
+		bool _warnOnShadow,
+		ErrorReporter& _errorReporter
+	);
+
 private:
 	bool visit(SourceUnit& _sourceUnit) override;
 	void endVisit(SourceUnit& _sourceUnit) override;

libsolidity/interface/ErrorReporter.cpp

@@ -42,11 +42,23 @@ void ErrorReporter::warning(string const& _description)
 	error(Error::Type::Warning, SourceLocation(), _description);
 }
 
-void ErrorReporter::warning(SourceLocation const& _location, string const& _description)
+void ErrorReporter::warning(
+	SourceLocation const& _location,
+	string const& _description
+)
 {
 	error(Error::Type::Warning, _location, _description);
 }
 
+void ErrorReporter::warning(
+	SourceLocation const& _location,
+	string const& _description,
+	SecondarySourceLocation const& _secondaryLocation
+)
+{
+	error(Error::Type::Warning, _location, _secondaryLocation, _description);
+}
+
 void ErrorReporter::error(Error::Type _type, SourceLocation const& _location, string const& _description)
 {
 	auto err = make_shared<Error>(_type);

libsolidity/interface/ErrorReporter.h

@@ -41,29 +41,29 @@ public:
 
 	ErrorReporter& operator=(ErrorReporter const& _errorReporter);
 
-	void warning(std::string const& _description = std::string());
+	void warning(std::string const& _description);
+
+	void warning(SourceLocation const& _location, std::string const& _description);
 
 	void warning(
-		SourceLocation const& _location = SourceLocation(),
-		std::string const& _description = std::string()
+		SourceLocation const& _location,
+		std::string const& _description,
+		SecondarySourceLocation const& _secondaryLocation
 	);
 
 	void error(
 		Error::Type _type,
-		SourceLocation const& _location = SourceLocation(),
-		std::string const& _description = std::string()
+		SourceLocation const& _location,
+		std::string const& _description
 	);
 
 	void declarationError(
 		SourceLocation const& _location,
-		SecondarySourceLocation const& _secondaryLocation = SecondarySourceLocation(),
-		std::string const& _description = std::string()
+		SecondarySourceLocation const& _secondaryLocation,
+		std::string const& _description
 	);
 
-	void declarationError(
-		SourceLocation const& _location,
-		std::string const& _description = std::string()
-	);
+	void declarationError(SourceLocation const& _location, std::string const& _description);
 
 	void fatalDeclarationError(SourceLocation const& _location, std::string const& _description);
 

test/libsolidity/Imports.cpp

@@ -20,11 +20,15 @@
  * Tests for high level features like import.
  */
 
-#include <string>
-#include <boost/test/unit_test.hpp>
+#include <test/libsolidity/ErrorCheck.h>
+
 #include <libsolidity/interface/Exceptions.h>
 #include <libsolidity/interface/CompilerStack.h>
 
+#include <boost/test/unit_test.hpp>
+
+#include <string>
+
 using namespace std;
 
 namespace dev
@@ -202,6 +206,67 @@ BOOST_AUTO_TEST_CASE(context_dependent_remappings_order_independent)
 	BOOST_CHECK(d.compile());
 }
 
+BOOST_AUTO_TEST_CASE(shadowing_via_import)
+{
+	CompilerStack c;
+	c.addSource("a", "library A {} pragma solidity >=0.0;");
+	c.addSource("b", "library A {} pragma solidity >=0.0;");
+	c.addSource("c", "import {A} from \"./a\"; import {A} from \"./b\";");
+	BOOST_CHECK(!c.compile());
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_imports)
+{
+	CompilerStack c;
+	c.addSource("B.sol", "contract X {} pragma solidity >=0.0;");
+	c.addSource("b", R"(
+		pragma solidity >=0.0;
+		import * as msg from "B.sol";
+		contract C {
+		}
+	)");
+	BOOST_CHECK(c.compile());
+	auto numErrors = c.errors().size();
+	// Sometimes we get the prerelease warning, sometimes not.
+	BOOST_CHECK(2 <= numErrors && numErrors <= 3);
+	for (auto const& e: c.errors())
+	{
+		string const* msg = e->comment();
+		BOOST_REQUIRE(msg);
+		BOOST_CHECK(
+			msg->find("pre-release") != string::npos ||
+			msg->find("shadows a builtin symbol") != string::npos
+		);
+	}
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_multiple_imports)
+{
+	CompilerStack c;
+	c.addSource("B.sol", "contract msg {} contract block{} pragma solidity >=0.0;");
+	c.addSource("b", R"(
+		pragma solidity >=0.0;
+		import {msg, block} from "B.sol";
+		contract C {
+		}
+	)");
+	BOOST_CHECK(c.compile());
+	auto numErrors = c.errors().size();
+	// Sometimes we get the prerelease warning, sometimes not.
+	BOOST_CHECK(4 <= numErrors && numErrors <= 5);
+	for (auto const& e: c.errors())
+	{
+		string const* msg = e->comment();
+		BOOST_REQUIRE(msg);
+		BOOST_CHECK(
+			msg->find("pre-release") != string::npos ||
+			msg->find("shadows a builtin symbol") != string::npos
+		);
+	}
+}
+
+
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -5498,22 +5498,6 @@ BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_library)
 	CHECK_SUCCESS_NO_WARNINGS(text);
 }
 
-BOOST_AUTO_TEST_CASE(does_not_warn_non_magic_msg_value)
-{
-	char const* text = R"(
-		contract C {
-			struct msg {
-				uint256 value;
-			}
-
-			function f() {
-				msg.value;
-			}
-		}
-	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
-}
-
 BOOST_AUTO_TEST_CASE(does_not_warn_msg_value_in_modifier_following_non_payable_public_function)
 {
 	char const* text = R"(
@@ -6127,6 +6111,85 @@ BOOST_AUTO_TEST_CASE(no_unused_inline_asm)
 	CHECK_SUCCESS_NO_WARNINGS(text);
 }
 
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_functions)
+{
+	char const* text = R"(
+		contract C {
+			function keccak256() {}
+		}
+	)";
+	CHECK_WARNING(text, "shadows a builtin symbol");
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_variables)
+{
+	char const* text = R"(
+		contract C {
+			function f() {
+				uint msg;
+				msg;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "shadows a builtin symbol");
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_parameters)
+{
+	char const* text = R"(
+		contract C {
+			function f(uint require) {
+				require = 2;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "shadows a builtin symbol");
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_return_parameters)
+{
+	char const* text = R"(
+		contract C {
+			function f() returns (uint require) {
+				require = 2;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "shadows a builtin symbol");
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_events)
+{
+	char const* text = R"(
+		contract C {
+			event keccak256();
+		}
+	)";
+	CHECK_WARNING(text, "shadows a builtin symbol");
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_ignores_struct)
+{
+	char const* text = R"(
+		contract C {
+			struct a {
+				uint msg;
+			}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
+BOOST_AUTO_TEST_CASE(shadowing_builtins_ignores_constructor)
+{
+	char const* text = R"(
+		contract C {
+			function C() {}
+		}
+	)";
+	CHECK_SUCCESS_NO_WARNINGS(text);
+}
+
 BOOST_AUTO_TEST_CASE(callable_crash)
 {
 	char const* text = R"(
@@ -6245,14 +6308,6 @@ BOOST_AUTO_TEST_CASE(create2_as_variable)
 	CHECK_WARNING_ALLOW_MULTI(text, "Variable is shadowed in inline assembly by an instruction of the same name");
 }
 
-BOOST_AUTO_TEST_CASE(shadowing_warning_can_be_removed)
-{
-	char const* text = R"(
-		contract C {function f() {assembly {}}}
-	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
-}
-
 BOOST_AUTO_TEST_CASE(warn_unspecified_storage)
 {
 	char const* text = R"(