diff --git a/Changelog.md b/Changelog.md index 361864626..8a475e4dd 100644 --- a/Changelog.md +++ b/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. diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp index aac903117..df83f382b 100644 --- a/libsolidity/analysis/NameAndTypeResolver.cpp +++ b/libsolidity/analysis/NameAndTypeResolver.cpp @@ -104,29 +104,18 @@ bool NameAndTypeResolver::performImports(SourceUnit& _sourceUnit, mapname(); - 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(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(m_currentScope) || + dynamic_cast(m_currentScope) + ) + warnAboutShadowing = false; + // Do not warn about the constructor shadowing the contract. + if (auto fun = dynamic_cast(&_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) diff --git a/libsolidity/analysis/NameAndTypeResolver.h b/libsolidity/analysis/NameAndTypeResolver.h index 846287786..a498c7ba6 100644 --- a/libsolidity/analysis/NameAndTypeResolver.h +++ b/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; diff --git a/libsolidity/interface/ErrorReporter.cpp b/libsolidity/interface/ErrorReporter.cpp index 6e2667a55..f9ef4ceb4 100644 --- a/libsolidity/interface/ErrorReporter.cpp +++ b/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(_type); diff --git a/libsolidity/interface/ErrorReporter.h b/libsolidity/interface/ErrorReporter.h index e5605d240..8b066a3ee 100644 --- a/libsolidity/interface/ErrorReporter.h +++ b/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); diff --git a/test/libsolidity/Imports.cpp b/test/libsolidity/Imports.cpp index 6aa96fb87..00f093b72 100644 --- a/test/libsolidity/Imports.cpp +++ b/test/libsolidity/Imports.cpp @@ -20,11 +20,15 @@ * Tests for high level features like import. */ -#include -#include +#include + #include #include +#include + +#include + 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() } diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 4b29243ac..48fe4d242 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/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"(