Skip to content

Commit 1ff57a4

Browse files
a3d4cameel
a3d4
authored and
cameel
committed
Fix shadowing struct types by struct member names
1 parent b5b8833 commit 1ff57a4

16 files changed

+132
-19
lines changed

‎Changelog.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Compiler Features:
1010

1111

1212
Bugfixes:
13+
* Name Resolver: Prevent member/parameter names from shadowing type names within struct/enum/error/event declarations.
1314

1415

1516
### 0.8.4 (2021-04-21)

‎libsolidity/analysis/DeclarationContainer.cpp‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
#include <libsolidity/ast/Types.h>
2828
#include <libsolutil/StringUtils.h>
2929

30+
#include <range/v3/view/filter.hpp>
31+
#include <range/v3/range/conversion.hpp>
32+
3033
using namespace std;
3134
using namespace solidity;
3235
using namespace solidity::frontend;
@@ -121,11 +124,7 @@ bool DeclarationContainer::registerDeclaration(
121124
if (conflictingDeclaration(_declaration, _name))
122125
return false;
123126

124-
// Do not warn about shadowing for structs and enums because their members are
125-
// not accessible without prefixes. Also do not warn about event parameters
126-
// because they do not participate in any proper scope.
127-
bool special = _declaration.scope() && (_declaration.isStructMember() || _declaration.isEnumValue() || _declaration.isEventOrErrorParameter());
128-
if (m_enclosingContainer && !special)
127+
if (m_enclosingContainer && _declaration.isVisibleAsUnqualifiedName())
129128
m_homonymCandidates.emplace_back(*_name, _location ? _location : &_declaration.location());
130129
}
131130

@@ -144,16 +143,35 @@ bool DeclarationContainer::registerDeclaration(
144143
return registerDeclaration(_declaration, nullptr, nullptr, _invisible, _update);
145144
}
146145

147-
vector<Declaration const*> DeclarationContainer::resolveName(ASTString const& _name, bool _recursive, bool _alsoInvisible) const
146+
vector<Declaration const*> DeclarationContainer::resolveName(
147+
ASTString const& _name,
148+
bool _recursive,
149+
bool _alsoInvisible,
150+
bool _onlyVisibleAsUnqualifiedNames
151+
) const
148152
{
149153
solAssert(!_name.empty(), "Attempt to resolve empty name.");
150154
vector<Declaration const*> result;
155+
151156
if (m_declarations.count(_name))
152-
result = m_declarations.at(_name);
157+
{
158+
if (_onlyVisibleAsUnqualifiedNames)
159+
result += ranges::to_vector(m_declarations.at(_name) | ranges::views::filter(&Declaration::isVisibleAsUnqualifiedName));
160+
else
161+
result += m_declarations.at(_name);
162+
}
163+
153164
if (_alsoInvisible && m_invisibleDeclarations.count(_name))
154-
result += m_invisibleDeclarations.at(_name);
165+
{
166+
if (_onlyVisibleAsUnqualifiedNames)
167+
result += ranges::to_vector(m_invisibleDeclarations.at(_name) | ranges::views::filter(&Declaration::isVisibleAsUnqualifiedName));
168+
else
169+
result += m_invisibleDeclarations.at(_name);
170+
}
171+
155172
if (result.empty() && _recursive && m_enclosingContainer)
156173
result = m_enclosingContainer->resolveName(_name, true, _alsoInvisible);
174+
157175
return result;
158176
}
159177

‎libsolidity/analysis/DeclarationContainer.h‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,10 @@ class DeclarationContainer
3939
public:
4040
using Homonyms = std::vector<std::pair<langutil::SourceLocation const*, std::vector<Declaration const*>>>;
4141

42-
explicit DeclarationContainer(
43-
ASTNode const* _enclosingNode = nullptr,
44-
DeclarationContainer* _enclosingContainer = nullptr
45-
):
46-
m_enclosingNode(_enclosingNode), m_enclosingContainer(_enclosingContainer)
42+
DeclarationContainer() = default;
43+
explicit DeclarationContainer(ASTNode const* _enclosingNode, DeclarationContainer* _enclosingContainer):
44+
m_enclosingNode(_enclosingNode),
45+
m_enclosingContainer(_enclosingContainer)
4746
{
4847
if (_enclosingContainer)
4948
_enclosingContainer->m_innerContainers.emplace_back(this);
@@ -57,7 +56,12 @@ class DeclarationContainer
5756
bool registerDeclaration(Declaration const& _declaration, ASTString const* _name, langutil::SourceLocation const* _location, bool _invisible, bool _update);
5857
bool registerDeclaration(Declaration const& _declaration, bool _invisible, bool _update);
5958

60-
std::vector<Declaration const*> resolveName(ASTString const& _name, bool _recursive = false, bool _alsoInvisible = false) const;
59+
std::vector<Declaration const*> resolveName(
60+
ASTString const& _name,
61+
bool _recursive = false,
62+
bool _alsoInvisible = false,
63+
bool _onlyVisibleAsUnqualifiedNames = false
64+
) const;
6165
ASTNode const* enclosingNode() const { return m_enclosingNode; }
6266
DeclarationContainer const* enclosingContainer() const { return m_enclosingContainer; }
6367
std::map<ASTString, std::vector<Declaration const*>> const& declarations() const { return m_declarations; }
@@ -80,8 +84,8 @@ class DeclarationContainer
8084
void populateHomonyms(std::back_insert_iterator<Homonyms> _it) const;
8185

8286
private:
83-
ASTNode const* m_enclosingNode;
84-
DeclarationContainer const* m_enclosingContainer;
87+
ASTNode const* m_enclosingNode = nullptr;
88+
DeclarationContainer const* m_enclosingContainer = nullptr;
8589
std::vector<DeclarationContainer const*> m_innerContainers;
8690
std::map<ASTString, std::vector<Declaration const*>> m_declarations;
8791
std::map<ASTString, std::vector<Declaration const*>> m_invisibleDeclarations;

‎libsolidity/analysis/NameAndTypeResolver.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,8 @@ vector<Declaration const*> NameAndTypeResolver::nameFromCurrentScope(ASTString c
182182
Declaration const* NameAndTypeResolver::pathFromCurrentScope(vector<ASTString> const& _path) const
183183
{
184184
solAssert(!_path.empty(), "");
185-
vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true);
185+
vector<Declaration const*> candidates = m_currentScope->resolveName(_path.front(), true, false, true);
186+
186187
for (size_t i = 1; i < _path.size() && candidates.size() == 1; i++)
187188
{
188189
if (!m_scopes.count(candidates.front()))
@@ -627,7 +628,10 @@ void DeclarationRegistrationHelper::enterNewSubScope(ASTNode& _subScope)
627628
solAssert(dynamic_cast<SourceUnit const*>(&_subScope), "Unexpected scope type.");
628629
else
629630
{
630-
bool newlyAdded = m_scopes.emplace(&_subScope, make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())).second;
631+
bool newlyAdded = m_scopes.emplace(
632+
&_subScope,
633+
make_shared<DeclarationContainer>(m_currentScope, m_scopes[m_currentScope].get())
634+
).second;
631635
solAssert(newlyAdded, "Unable to add new scope.");
632636
}
633637
m_currentScope = &_subScope;

‎libsolidity/ast/AST.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,18 @@ bool Declaration::isEventOrErrorParameter() const
545545
return dynamic_cast<EventDefinition const*>(scope()) || dynamic_cast<ErrorDefinition const*>(scope());
546546
}
547547

548+
bool Declaration::isVisibleAsUnqualifiedName() const
549+
{
550+
if (!scope())
551+
return true;
552+
if (isStructMember() || isEnumValue() || isEventOrErrorParameter())
553+
return false;
554+
if (auto functionDefinition = dynamic_cast<FunctionDefinition const*>(scope()))
555+
if (!functionDefinition->isImplemented())
556+
return false; // parameter of a function without body
557+
return true;
558+
}
559+
548560
DeclarationAnnotation& Declaration::annotation() const
549561
{
550562
return initAnnotation<DeclarationAnnotation>();

‎libsolidity/ast/AST.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,12 @@ class Declaration: public ASTNode, public Scopable
260260
/// @returns true if this is a declaration of a parameter of an event.
261261
bool isEventOrErrorParameter() const;
262262

263+
/// @returns false if the declaration can never be referenced without being qualified with a scope.
264+
/// Usually the name alone can be used to refer to the corresponding entity.
265+
/// But, for example, struct member names or enum member names always require a prefix.
266+
/// Another example is event parameter names, which do not participate in any proper scope.
267+
bool isVisibleAsUnqualifiedName() const;
268+
263269
/// @returns the type of expressions referencing this declaration.
264270
/// This can only be called once types of variable declarations have already been resolved.
265271
virtual Type const* type() const = 0;

‎test/libsolidity/syntaxTests/enums/enum_member_shadowing.sol‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
contract C {
2+
struct S {
3+
uint x;
4+
}
5+
6+
enum E {E, S, C, a, f}
7+
8+
uint a;
9+
10+
function f() public pure {}
11+
}
12+
// ----

‎test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_param_name.sol‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
contract C {
2+
enum EnumType {A, B, C}
3+
4+
struct StructType {
5+
uint x;
6+
}
7+
8+
error E1(StructType StructType);
9+
error E2(EnumType EnumType);
10+
error E3(EnumType StrucType, StructType EnumType);
11+
}
12+
// ----

‎test/libsolidity/syntaxTests/events/event_param_type_shadowed_by_param_name.sol‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
contract C {
2+
enum EnumType {A, B, C}
3+
4+
struct StructType {
5+
uint x;
6+
}
7+
8+
event E1(StructType StructType);
9+
event E2(EnumType EnumType);
10+
event E3(EnumType StrucType, StructType EnumType);
11+
event E4(StructType indexed StructType) anonymous;
12+
}
13+
// ----

‎test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_name_vs_contract.sol‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
interface I {
2+
function f(uint I) external; // OK
3+
}
4+
5+
library L {
6+
function f(uint L) public pure {} // warning
7+
}
8+
9+
abstract contract A {
10+
function f(uint A) public pure {} // warning
11+
function g(uint A) public virtual; // OK
12+
}
13+
14+
contract C {
15+
function f(uint C) public pure {} // warning
16+
}
17+
// ----
18+
// Warning 2519: (91-97): This declaration shadows an existing declaration.
19+
// Warning 2519: (168-174): This declaration shadows an existing declaration.
20+
// Warning 2519: (283-289): This declaration shadows an existing declaration.

0 commit comments

Comments
 (0)