diff --git a/Changelog.md b/Changelog.md index 24870cd24..86bfdb246 100644 --- a/Changelog.md +++ b/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. diff --git a/libsolidity/analysis/DeclarationContainer.cpp b/libsolidity/analysis/DeclarationContainer.cpp index b514c2c39..a3d9ec8ce 100644 --- a/libsolidity/analysis/DeclarationContainer.cpp +++ b/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,8 +116,18 @@ bool DeclarationContainer::registerDeclaration( m_declarations.erase(*_name); m_invisibleDeclarations.erase(*_name); } - else if (conflictingDeclaration(_declaration, _name)) - return false; + 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& decls = _invisible ? m_invisibleDeclarations[*_name] : m_declarations[*_name]; if (!util::contains(decls, &_declaration)) @@ -124,6 +135,15 @@ bool DeclarationContainer::registerDeclaration( return true; } +bool DeclarationContainer::registerDeclaration( + Declaration const& _declaration, + bool _invisible, + bool _update +) +{ + return registerDeclaration(_declaration, nullptr, nullptr, _invisible, _update); +} + vector DeclarationContainer::resolveName(ASTString const& _name, bool _recursive, bool _alsoInvisible) const { solAssert(!_name.empty(), "Attempt to resolve empty name."); @@ -164,3 +184,13 @@ vector DeclarationContainer::similarNames(ASTString const& _name) con return similar; } + +void DeclarationContainer::populateHomonyms(back_insert_iterator _it) const +{ + for (DeclarationContainer const* innerContainer: m_innerContainers) + innerContainer->populateHomonyms(_it); + + for (auto [name, location]: m_homonymCandidates) + for (auto const* declaration: m_enclosingContainer->resolveName(name, true, true)) + _it = make_pair(location, declaration); +} diff --git a/libsolidity/analysis/DeclarationContainer.h b/libsolidity/analysis/DeclarationContainer.h index 8036a5a58..6350e7d0a 100644 --- a/libsolidity/analysis/DeclarationContainer.h +++ b/libsolidity/analysis/DeclarationContainer.h @@ -24,6 +24,8 @@ #pragma once #include +#include +#include #include #include #include @@ -38,17 +40,26 @@ namespace solidity::frontend class DeclarationContainer { public: + using Homonyms = std::vector>; + 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 _update if true, replaces a potential declaration that is already present + /// @param _location alternative location, used to point at homonymous declarations. + /// @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 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 +78,18 @@ public: /// Searches this and all parent containers. std::vector 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 _it) const; + private: ASTNode const* m_enclosingNode; DeclarationContainer const* m_enclosingContainer; + std::vector m_innerContainers; std::map> m_declarations; std::map> m_invisibleDeclarations; + /// List of declarations (name and location) to check later for homonymity. + std::vector> m_homonymCandidates; }; } diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp index 2f8f0d77f..60506ca87 100644 --- a/libsolidity/analysis/NameAndTypeResolver.cpp +++ b/libsolidity/analysis/NameAndTypeResolver.cpp @@ -28,6 +28,7 @@ #include #include #include +#include using namespace std; using namespace solidity::langutil; @@ -47,7 +48,7 @@ NameAndTypeResolver::NameAndTypeResolver( m_scopes[nullptr] = make_shared(); 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 c return nullptr; } -void NameAndTypeResolver::warnVariablesNamedLikeInstructions() +void NameAndTypeResolver::warnVariablesNamedLikeInstructions() const { for (auto const& instruction: evmasm::c_instructions) { @@ -223,6 +224,49 @@ void NameAndTypeResolver::warnVariablesNamedLikeInstructions() } } +void NameAndTypeResolver::warnHomonymDeclarations() const +{ + DeclarationContainer::Homonyms homonyms; + m_scopes.at(nullptr)->populateHomonyms(back_inserter(homonyms)); + + unordered_set noMoreMagic; + for (auto [innerLocation, outerDeclaration]: homonyms) + { + solAssert(innerLocation && outerDeclaration, ""); + + if (dynamic_cast(outerDeclaration)) + { + // avoids duplecated warnings + // (some magic variables ("revert", "require") appear in different flavors under the same name) + if (noMoreMagic.insert(innerLocation).second) + m_errorReporter.warning( + 2319_error, + *innerLocation, + "This declaration shadows a builtin symbol." + ); + } + else + { + SourceLocation const& outerLocation = outerDeclaration->location(); + + if (!outerDeclaration->isVisibleInContract()) + m_errorReporter.warning( + 8760_error, + *innerLocation, + "This declaration has the same name as another declaration.", + SecondarySourceLocation().append("The other declaration is here:", outerLocation) + ); + else + m_errorReporter.warning( + 2519_error, + *innerLocation, + "This declaration shadows an existing declaration.", + SecondarySourceLocation().append("The shadowed declaration is here:", outerLocation) + ); + } + } +} + void NameAndTypeResolver::setScope(ASTNode const* _node) { m_currentScope = m_scopes[_node].get(); @@ -281,8 +325,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.currentSuper(), nullptr, true, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), true, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), true, true); m_globalContext.resetCurrentContract(); return success; @@ -303,7 +347,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 +502,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 +535,7 @@ bool DeclarationRegistrationHelper::registerDeclaration( ); return false; } - else if (shadowedDeclaration) - { - if (dynamic_cast(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 +565,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.currentSuper(), nullptr, false, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), false, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), false, true); m_currentContract = &_contract; return ASTVisitor::visit(_contract); @@ -566,8 +575,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.currentSuper(), nullptr, true, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), true, true); + m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), true, true); m_globalContext.resetCurrentContract(); m_currentContract = nullptr; ASTVisitor::endVisit(_contract); @@ -612,11 +621,14 @@ void DeclarationRegistrationHelper::endVisitNode(ASTNode& _node) void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _subScope) { - shared_ptr container{make_shared(m_currentScope, m_scopes[m_currentScope].get())}; - 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. - solAssert(newlyAdded || dynamic_cast(&_subScope), "Unable to add new scope."); + if (m_scopes.count(&_subScope)) + // Source units are the only AST nodes for which containers can be created from multiple places due to imports. + solAssert(dynamic_cast(&_subScope), "Unexpected scope type."); + else + { + bool newlyAdded = m_scopes.emplace(&_subScope, make_shared(m_currentScope, m_scopes[m_currentScope].get())).second; + solAssert(newlyAdded, "Unable to add new scope."); + } m_currentScope = &_subScope; } diff --git a/libsolidity/analysis/NameAndTypeResolver.h b/libsolidity/analysis/NameAndTypeResolver.h index 3412ce2b7..da90c690b 100644 --- a/libsolidity/analysis/NameAndTypeResolver.h +++ b/libsolidity/analysis/NameAndTypeResolver.h @@ -93,7 +93,10 @@ public: Declaration const* pathFromCurrentScope(std::vector 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; diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp index 1dae2b00e..0c1b6b49d 100644 --- a/libsolidity/interface/CompilerStack.cpp +++ b/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; diff --git a/test/libsolidity/smtCheckerTests/typecast/function_type_to_function_type_internal.sol b/test/libsolidity/smtCheckerTests/typecast/function_type_to_function_type_internal.sol index dcf0a6f5f..4680d5367 100644 --- a/test/libsolidity/smtCheckerTests/typecast/function_type_to_function_type_internal.sol +++ b/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. diff --git a/test/libsolidity/syntaxTests/functionCalls/named_arguments_duplicate_parameter.sol b/test/libsolidity/syntaxTests/functionCalls/named_arguments_duplicate_parameter.sol index 98587ac60..17ee8dc45 100644 --- a/test/libsolidity/syntaxTests/functionCalls/named_arguments_duplicate_parameter.sol +++ b/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". diff --git a/test/libsolidity/syntaxTests/functionCalls/named_arguments_empty.sol b/test/libsolidity/syntaxTests/functionCalls/named_arguments_empty.sol index f00ae0227..0c4da5c73 100644 --- a/test/libsolidity/syntaxTests/functionCalls/named_arguments_empty.sol +++ b/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. diff --git a/test/libsolidity/syntaxTests/functionCalls/named_arguments_wrong_count.sol b/test/libsolidity/syntaxTests/functionCalls/named_arguments_wrong_count.sol index 5e8c4f40f..5acc2b444 100644 --- a/test/libsolidity/syntaxTests/functionCalls/named_arguments_wrong_count.sol +++ b/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. diff --git a/test/libsolidity/syntaxTests/multiSource/free_function_resolution_override_virtual.sol b/test/libsolidity/syntaxTests/multiSource/free_function_resolution_override_virtual.sol index a3d3ccd9e..b5a38c3d0 100644 --- a/test/libsolidity/syntaxTests/multiSource/free_function_resolution_override_virtual.sol +++ b/test/libsolidity/syntaxTests/multiSource/free_function_resolution_override_virtual.sol @@ -16,4 +16,5 @@ contract D is C { // ---- // Warning 2519: (s1.sol:65-134): This declaration shadows an existing declaration. // Warning 2519: (s2.sol:85-155): This declaration shadows an existing declaration. +// Warning 2519: (s2.sol:85-155): This declaration shadows an existing declaration. // DeclarationError 1686: (s2.sol:17-64): Function with same name and parameter types defined twice. diff --git a/test/libsolidity/syntaxTests/scoping/double_variable_declaration_disjoint_scope_activation.sol b/test/libsolidity/syntaxTests/scoping/double_variable_declaration_disjoint_scope_activation.sol index 2e9970163..e6beef51a 100644 --- a/test/libsolidity/syntaxTests/scoping/double_variable_declaration_disjoint_scope_activation.sol +++ b/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. diff --git a/test/libsolidity/syntaxTests/scoping/double_variable_declaration_same_and_disjoint_scope.sol b/test/libsolidity/syntaxTests/scoping/double_variable_declaration_same_and_disjoint_scope.sol index 47c3d1302..f457b183f 100644 --- a/test/libsolidity/syntaxTests/scoping/double_variable_declaration_same_and_disjoint_scope.sol +++ b/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.