Fix shadowing struct types by struct member names #10908

a3d4 · 2021-02-08T04:56:20Z

libsolidity/analysis/ReferencesResolver.cpp

libsolidity/analysis/NameAndTypeResolver.h

libsolidity/analysis/NameAndTypeResolver.cpp

libsolidity/analysis/NameAndTypeResolver.h

bshastry · 2021-02-08T09:44:54Z

test/libsolidity/syntaxTests/structs/member_type_eq_name.sol

 }
 // ----
-// TypeError 5172: (25-26): Name has to refer to a struct, enum or contract.
+// DeclarationError 7920: (25-26): Identifier not found or not unique.


Isn't type error the correct error to output here?

Could be, but then it would be the only type error in ReferencesResolver.

bshastry · 2021-02-08T09:47:04Z

test/libsolidity/syntaxTests/structs/member_type_eq_name_2.sol

+    struct S {E X; uint E;}
+    struct T {E T; uint E;}
+    struct U {E E;}
+}


Should this test pass?

According to my understanding, yes, because we allow struct members to have same names as types.

libsolidity/analysis/NameAndTypeResolver.cpp

chriseth · 2021-02-18T13:48:32Z

This sounds like it is fixing a special case and a more general solution is possible. The issue at hand is that we do register a name that could shadow, but there is no code from which it could be accessed by that name. The same is true for a different but similar situation:

abstract contract C {
  function f(uint C) public pure;
}

as opposed to

contract C {
  function f(uint C) public pure {};
}

The second should create an error, the first does not have to. The same is true for event parameter names.

Are these two situations similar or not?

a3d4 · 2021-02-18T14:30:10Z

@chriseth

The second should create an error, the first does not have to.

Currently both samples produce the same warning:

This declaration shadows an existing declaration.

Warning: This declaration shadows an existing declaration.
 --> d:/T/9231.sol:2:14:
  |
2 |   function f(uint C) public pure;
  |              ^^^^^^
Note: The shadowed declaration is here:
 --> d:/T/9231.sol:1:1:
  |
1 | abstract contract C {
  | ^ (Relevant source part starts here and spans across multiple lines).

Warning: This declaration shadows an existing declaration.
 --> d:/T/9231.sol:2:14:
  |
2 |   function f(uint C) public pure {}
  |              ^^^^^^
Note: The shadowed declaration is here:
 --> d:/T/9231.sol:1:1:
  |
1 | contract C {
  | ^ (Relevant source part starts here and spans across multiple lines).

The same is true for event parameter names.

Do you mean something like this:

contract C {
  event e(uint C);
}

It compiles without the shadowing warning or error (which is fine I guess).

I'm having a closer look to see if we can have some unification.

a3d4 · 2021-02-19T04:00:05Z

@chriseth
We can just handle parameters of abstract functions the same way as event parameters, see 4312277.

Or we can extend Declaration to indicate that certain names should not participate in the name lookup, see 31cc995.

chriseth · 2021-02-25T15:18:37Z

The solution looks good!

leonardoalt · 2021-03-04T16:06:58Z

The implemented or discussed solution?

a3d4 · 2021-03-05T02:31:20Z

@chriseth

All names are self-sufficient, but for some, there is no code that can refer to them, isn't it?

I would say that name is self-sufficient if it does not require any qualifier to identify to the corresponding entity :).
But I agree, isVisibleAsUnqualifiedName sounds more precise.

leonardoalt · 2021-03-26T13:26:25Z

Is this ready for review again?

a3d4 · 2021-03-31T20:14:35Z

@leonardoalt

Is this ready for review again?

Commits are not squashed and there is no changelog entry, but otherwise I believe it can be reviewed.

cameel

The functionality looks finished to me, the PR just needs some final polish.

There are a some unused leftovers from previous iterations, commits are unsquashed and minor style tweaks are possible in a few places. It needs to be updated for custom errors and, as stated above, the changelog entry is missing. Also, it could use a few more test cases.

libsolidity/analysis/DeclarationContainer.cpp

libsolidity/analysis/DeclarationContainer.h

libsolidity/ast/AST.h

libsolidity/ast/AST.cpp

libsolidity/ast/AST.h

cameel · 2021-04-27T18:50:28Z

Changelog, tweaks and extra tests added, commits squashed, PR rebased. This is now ready for the final review.

bshastry · 2021-04-28T12:41:03Z

Changelog.md



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


Do we issue an error in case of shadowing? I'm trying to understand how we prevent shadowing, because users could still do it, right?

There's no error. That's the point of the PR. There are some examples in the original issue (#9231) that should make this clearer.

For example before this PR you would get an error here:

enum E {a, b, c} struct T {E T; uint E;}

Error: Name has to refer to a struct, enum or contract. --> test.sol:2:11: | 2 | struct T {E T; uint E;} | ^

That's because the name checker thinks that uint E is shadowing the type E inside the body of the struct.

The PR changes that by making name checker aware that names defined in some kinds of scopes (struct declarations, enum declarations, function declarations without a body, event/error declarations) cannot shadow anything inside that declaration.

@cameel

Prevent member/parameter names from shadowing type names within struct/enum/error/event declarations.

I think, strictly speaking, we do not prevent shadowing, we rather prevent recognition of non-existing shadowing as shadowing. Maybe

Allow member/parameter names to be the same as type names within struct/enum/error/event declarations (avoid false name shadowing errors)

BTW, thanks a lot for taking care of the PR! (I still do hope to come back to the project for real eventually, even if right now it does not sound realistic.)

Well, yeah, I get your point but I have a really hard time expressing this in a changelog entry without making it confusing to users or taking a whole paragraph :) This is the best I could come up with. If anyone has a better suggestion, I can replace it.

BTW, thanks a lot for taking care of the PR! (I still do hope to come back to the project for real eventually, even if right now it does not sound realistic.)

Nice to heard that. I hope we'll get you back at some point :)

bshastry · 2021-04-28T12:41:40Z

test/libsolidity/syntaxTests/enums/enum_member_shadowing.sol

+
+    function f() public pure {}
+}
+// ----


Shouldn't this issue a compiler error? Because enum member shadows type name?

No, because shadowing is legal. It's only when you shadow type names used later within the same scope that it's an error:

{ Type1 Type2 = 1; Type2 x = 2; }

You cannot do that in an enum. I added this test mostly for completeness.

test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_param_name.sol

bshastry · 2021-04-28T12:42:47Z

test/libsolidity/syntaxTests/scoping/name_shadowing_function_parameter_name_vs_contract.sol

+
+abstract contract A {
+    function f(uint A) public pure {}    // warning
+    function g(uint A) public virtual;   // OK


Why is this "OK"? Shadowing still exists right?

See #10908 (comment).

Having a parameter named like a type inside a function declaration that has no body cannot break anything so we do not consider that shadowing. It's clear what it's referring to. You don't want to warn about it and annoy the user. When there is a body, on the other hand, warning makes sense.

libsolidity/analysis/DeclarationContainer.cpp

test/libsolidity/syntaxTests/errors/error_param_type_shadowed_by_param_name.sol

libsolidity/analysis/DeclarationContainer.h

cameel · 2021-04-30T22:27:03Z

Just to summarize the current status: there were some review comments above but all have been addressed. The PR is finished and just waiting for an approve.

hrkrshnn

Looks good. One minor comment, approving anyway.

hrkrshnn · 2021-05-03T11:43:37Z

libsolidity/analysis/DeclarationContainer.h

+	/// @param _onlyVisibleAsUnqualifiedNames if true, only include declarations which can be
+	///        referenced outside of the current scope without being qualified with the name of the
+	///        scope. I.e. when they shadow names in the enclosing scope(s). This option is **not**
+	///        passed to the enclosing scope when used together with @a _recursive.


Do you think we should add an assert for _recursive and _onlyVisibleAsUnqualifiedNames both being true?

I actually did that at first but the assertion fails :) We do call it with both being true and this is the expected behavior unfortunately.

Is the comment here correct, then?

Trying to answer your question pulled me down a rabbit hole today and I found a new problem with the PR. So it needs more work unfortunately :(

Is the comment here correct, then?

Yes, it's correct in that respect because resolveName() does set _onlyVisibleAsUnqualifiedNames in the recursive call to false:

m_enclosingContainer->resolveName(_name, true, _alsoInvisible, false);

The only thing that the assert would change would be not having to document this case. But it turns out that this case does actually happen in practice.

But your comment made me think whether this is really the right behavior. And I think it's not so I changed it to propagate the value of the parameter. It's not a problem in practice though because I think it's not possible to trigger this case it with any currently allowed syntax.

I tried to find a test case that would behave differently depending on whether we propagate _onlyVisibleAsUnqualifiedNames or not and I could not think of any. At first I thought this might be affected:

contract C { enum E {a, b, c} struct S {function (E X) external f; uint E;} }

Turns out it isn't. That's because apparently, unlike a function parameter list, the parameter list of a function type is not a scope. At least the E in E X is never checked for collision with uint E. Hypothetically though, if it was a scope, I think that resolveName() should not consider uint E from the outer scope a candidate - so _onlyVisibleAsUnqualifiedNames should be true when recursively visiting the outer scopes as well.

BTW, the comment wasn't really correct in another respect so I have rewritten it a bit.

cameel · 2021-05-11T15:52:08Z

I found a new case that is not covered by this PR. It's function parameters shadowing types of other parameters:

contract C {
    enum EnumType1 {A, B, C}
    enum EnumType2 {A, B, C}

    function z(EnumType1 EnumType2) external returns (EnumType2 EnumType1) {}
}

This results in Error: Name has to refer to a struct, enum or contract.
This is the same problem this PR solves for structs but this time for function parameters. And unfortunately it does not nicely fit the Declaration::isVisibleAsUnqualifiedName() approach. That's because the function tells us if a declaration can never be referenced with name alone and this is not the case for functions that have a body.

To solve it I'd have to add a special case in resolveName() and that's what isVisibleAsUnqualifiedName() was supposed to let us avoid. So I'm not sure how to proceed here.

cameel · 2021-05-11T15:53:56Z

.../libsolidity/syntaxTests/scoping/name_shadowing_function_return_parameter_vs_struct_enum.sol

+contract C {
+    enum EnumType {A, B, C}
+
+    struct StructType {
+        uint x;
+    }
+
+    function f() external returns (StructType memory StructType) {}
+    function g() external returns (EnumType EnumType) {}
+    function h() external returns (EnumType StructType, StructType memory EnumType) {}
+
+    function z(uint EnumType) external returns (EnumType e) {}
+}
+// ----
+// Warning 2519: (124-152): This declaration shadows an existing declaration.
+// Warning 2519: (192-209): This declaration shadows an existing declaration.
+// Warning 2519: (249-268): This declaration shadows an existing declaration.
+// Warning 2519: (270-296): This declaration shadows an existing declaration.
+// Warning 2519: (317-330): This declaration shadows an existing declaration.
+// TypeError 5172: (124-134): Name has to refer to a struct, enum or contract.


This TypeError is what I'm referring to in #10908 (comment).

cameel · 2021-05-12T18:52:23Z

We discussed it today and the prevailing opinion was that for functions with a body we do want the shadowing to be disallowed. So the behavior that I caught in #10908 (comment) is correct.

As such, this PR is ready for another review. I added new test cases and did some minor reformatting but the only significant change after the last approve from @hrkrshnn was the one mentioned in #10908 (comment).

cameel · 2021-06-03T12:54:37Z

Here's a snippet that summarizes the current state of the PR. Errors/warnings in each case before and after the PR:

enum EnumType {A, B, C}

contract C {
    // struct, event, error
    //
    // BEFORE: Error: Name has to refer to a struct, enum or contract.
    //
    // AFTER: OK
    struct S {uint EnumType; EnumType x;}
    error Ev(uint EnumType, EnumType x);
    event Er(uint EnumType, EnumType x);

    // function with a body
    //
    // BEFORE: Warning: This declaration shadows an existing declaration.
    // BEFORE: Error: Name has to refer to a struct, enum or contract.
    //
    // AFTER: Warning: This declaration shadows an existing declaration.
    // AFTER: Error: Name has to refer to a struct, enum or contract.
    function f(uint EnumType, EnumType x) external {}

    // function without a body
    //
    // BEFORE: Warning: This declaration shadows an existing declaration.
    // BEFORE: Error: Name has to refer to a struct, enum or contract.
    //
    // AFTER: OK
    function g(uint EnumType, EnumType x) external virtual;

    // function type with named parameters
    //
    // BEFORE: Warning: Naming function type parameters is deprecated.
    // BEFORE: Warning: This declaration shadows an existing declaration.
    //
    // AFTER:  Warning: Naming function type parameters is deprecated.
    // AFTER:  Warning: This declaration shadows an existing declaration.
    function (uint EnumType, EnumType x) external h;
}

// contract
//
// BEFORE: Warning: This declaration shadows an existing declaration.
// BEFORE: Error: Name has to refer to a struct, enum or contract.
//
// AFTER:  Warning: This declaration shadows an existing declaration.
// AFTER:  Error: Name has to refer to a struct, enum or contract.
contract A {uint EnumType; EnumType x;}

chriseth · 2021-06-03T14:29:32Z

About the specific example of

contract C {
    enum EnumType1 {A, B, C}
    enum EnumType2 {A, B, C}

    function z(EnumType1 EnumType2) external returns (EnumType2 EnumType1) {}
}

This should be an error as long as we have {} instead of ;, because functions with empty body can have modifiers, while function without implementation cannot have modifiers. This is important because we said that shadowing is not a problem as long as there is no code the shadowing can cover. Modifier invocations are code that are inside the scope of the function, i.e. the parameters are visible in the modifier invocation.

cameel · 2021-06-03T16:31:48Z

This should be an error as long as we have {} instead of ;,

OK. That's exactly what happens:

contract C {
    enum EnumType1 {A, B, C}
    enum EnumType2 {A, B, C}

    // BEFORE: Warning: This declaration shadows an existing declaration.
    // BEFORE: Error: Name has to refer to a struct, enum or contract.
    //
    // AFTER:  Warning: This declaration shadows an existing declaration.
    // AFTER:  Error: Name has to refer to a struct, enum or contract. 
    function z1(EnumType1 EnumType2) external returns (EnumType2 EnumType1) {}

    // BEFORE: Warning: This declaration shadows an existing declaration.
    // BEFORE: Error: Name has to refer to a struct, enum or contract.
    //
    // AFTER:  OK 
    function z2(EnumType1 EnumType2) external virtual returns (EnumType2 EnumType1);
}

Modifier invocations are code that are inside the scope of the function, i.e. the parameters are visible in the modifier invocation.

I just checked and looks like it's possible to shadow the type name inside modifier invocation. You do at least get a warning if the shadowing happens though. This case is unaffected by the PR (works the same before and after):

enum EnumType {A, B, C}
struct StructType {uint x;}

contract C {
    modifier ms(StructType memory s) { _; }
    modifier mi(uint i) { _; }

    // Warning: This declaration shadows an existing declaration.
    function f(uint EnumType) mi(EnumType) external {}

    // OK
    function g(uint x) ms(StructType(x)) external {}

    // Warning: This declaration shadows an existing declaration.
    // Error: Type is not callable
    function h1(uint StructType) ms(StructType(StructType)) external {}

    // Warning: This declaration shadows an existing declaration.
    function h2(uint StructType) external {
        // Error: Type is not callable
        StructType(StructType);
    }
}

The last situation (functions h1() and h2()) is in line with what I would expect, i.e. you cannot use the struct type because it is shadowed, but the error message is wrong. The type is callable, it's just not the type that is being used.

chriseth · 2021-06-04T08:55:06Z

I assume "Type is not callable" refers to

ms(StructType(StructType))
   ^^^^^^^^^^

and I don't see why the error is wrong. StructType resolves to the name of the variable, and since the variable is not of function type, you cannot call it.

cameel · 2021-06-04T09:48:36Z

I said it's wrong because StructType is not a type if it resolves to the name of a variable. It should just say that it's not callable. Or was it meant to say that the type of StructType is not callable?

Still, it's mostly a cosmetic issue and not affected by the PR. Are all the other errors/warnings I listed working as they should?

chriseth · 2021-06-04T10:08:53Z

It could say "type of the expression is not callable" (if StructType were a variable of function type, it would work). I think everything is fine now.

…ation

a3d4 requested review from cameel and chriseth February 8, 2021 06:20

hrkrshnn reviewed Feb 8, 2021

View reviewed changes

libsolidity/analysis/ReferencesResolver.cpp Show resolved Hide resolved

bshastry reviewed Feb 8, 2021

View reviewed changes

a3d4 force-pushed the fix-9231-struct-member-names-shadow-type-names branch from a49107d to d30aa71 Compare February 16, 2021 23:16

hrkrshnn reviewed Feb 17, 2021

View reviewed changes

libsolidity/analysis/NameAndTypeResolver.cpp Outdated Show resolved Hide resolved

a3d4 requested a review from hrkrshnn February 18, 2021 12:50

hrkrshnn reviewed Feb 18, 2021

View reviewed changes

libsolidity/analysis/NameAndTypeResolver.cpp Outdated Show resolved Hide resolved

a3d4 force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 873d46f to 4312277 Compare February 18, 2021 23:15

a3d4 force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 53b0276 to 2905ea5 Compare March 5, 2021 01:09

cameel reviewed Apr 24, 2021

View reviewed changes

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 2905ea5 to e531fc8 Compare April 27, 2021 18:26

cameel requested review from bshastry and hrkrshnn April 27, 2021 18:37

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from e531fc8 to 1ff57a4 Compare April 27, 2021 18:49

bshastry reviewed Apr 28, 2021

View reviewed changes

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 1ff57a4 to 9082d2a Compare April 28, 2021 14:29

cameel requested a review from bshastry April 28, 2021 14:32

cameel self-assigned this Apr 28, 2021

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 9082d2a to 059a1c9 Compare April 28, 2021 14:35

hrkrshnn reviewed Apr 28, 2021

View reviewed changes

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 059a1c9 to 6d8b819 Compare April 29, 2021 21:26

cameel requested a review from hrkrshnn April 29, 2021 21:27

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch from 6d8b819 to d7cee56 Compare April 29, 2021 22:10

hrkrshnn previously approved these changes May 3, 2021

View reviewed changes

cameel dismissed hrkrshnn’s stale review via e34ce54 May 10, 2021 14:17

cameel force-pushed the fix-9231-struct-member-names-shadow-type-names branch 2 times, most recently from e34ce54 to d3166c7 Compare May 11, 2021 15:43

cameel reviewed May 11, 2021

View reviewed changes

a3d4 and others added 3 commits June 9, 2021 12:35

Do not warn about shadowing parameters in functions without implement…

70b8b1c

…ation

Fix shadowing struct types by struct member names

f59145f

Extra tests for shadowing within function parameter lists

362fc66

chriseth force-pushed the fix-9231-struct-member-names-shadow-type-names branch from d3166c7 to 362fc66 Compare June 9, 2021 10:37

chriseth approved these changes Jun 9, 2021

View reviewed changes

chriseth enabled auto-merge June 9, 2021 10:38

chriseth merged commit 0fff4e6 into argotorg:develop Jun 9, 2021



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

Fix shadowing struct types by struct member names #10908

Fix shadowing struct types by struct member names #10908

Uh oh!

Conversation

a3d4 commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chriseth commented Feb 18, 2021

Uh oh!

a3d4 commented Feb 18, 2021

Uh oh!

a3d4 commented Feb 19, 2021

Uh oh!

chriseth commented Feb 25, 2021

Uh oh!

leonardoalt commented Mar 4, 2021

Uh oh!

a3d4 commented Mar 5, 2021

Uh oh!

leonardoalt commented Mar 26, 2021

Uh oh!

a3d4 commented Mar 31, 2021

Uh oh!

cameel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cameel commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cameel commented Apr 30, 2021

Uh oh!

hrkrshnn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel left a comment •

edited

Loading

cameel commented Jun 3, 2021 •

edited

Loading

cameel commented Jun 3, 2021 •

edited

Loading