From e0dc74b895727525f261a9abe190872a58e8999e Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Thu, 2 Feb 2017 11:39:12 +0000 Subject: Warn about shadowing variables. --- libsolidity/analysis/NameAndTypeResolver.cpp | 129 ++++++++++++++++++--------- libsolidity/analysis/NameAndTypeResolver.h | 9 ++ 2 files changed, 98 insertions(+), 40 deletions(-) (limited to 'libsolidity/analysis') diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp index aac90311..df83f382 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 84628778..a498c7ba 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; -- cgit