ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.19k stars 5.75k forks source link

Disallow ``this`` and ``super`` and ``_`` as identifiers. #9604

Closed ekpyron closed 3 years ago

ekpyron commented 4 years ago

I just realized that uint this = 4; uint super = 7; uint _ = 42; is still allowed and only a warning. I think the next breaking release should disallow this. The last one is probably the most controversial one, but _ is the placeholder keyword for modifiers, and I'd think it'd be nice to not have any keywords used as non-keyword in general...

axic commented 3 years ago

While we agreed to this on a recent meeting, what do we actually mean by "disallow"? A specific error saying these are reserved identifiers?

We already have shadowing warning for this and super, but not for _ (unless within a modifier, I guess):

Warning: This declaration shadows a builtin symbol.
 --> super.sol:3:5:
  |
3 |     uint this = 4; uint super = 7; uint _ = 42;
  |     ^^^^^^^^^

Warning: This declaration shadows a builtin symbol.
 --> super.sol:3:20:
  |
3 |     uint this = 4; uint super = 7; uint _ = 42;
  |                    ^^^^^^^^^^

Actually there is no warning inside modifiers:

  modifier m() {
    uint _ = 42;

    _;
  }

The shadowing warning for super and this are also present within free functions, so I'd definitely call the lack of shadowing warning for _ a bug, which is happening because _; is turned into a specific AST node (PlaceholderStatement) as opposed to an identifer.

chriseth commented 3 years ago

I would say disallow means it is an error to define an identifier that is called 'super', 'this' or '_'.

axic commented 3 years ago

But on that note we should disallow any other special to be overridden, such as msg, block, type, etc.

I think what is more important here is to disallow _ as an identifier given it has varying behaviour depending whether it is in a function or a modifier. Started adding tests for that when I asked the questions above.

axic commented 3 years ago

@hrkrshnn pushed the tests I added for underscores, I think we can merge that independently.

hrkrshnn commented 3 years ago

@chriseth @axic I was wondering what's a good way to proceed in here? One way would be to check for names of FunctionDefinition, VariableDeclaration, Modifiers etc. in SyntaxChecker. What do you think?

axic commented 3 years ago

I'd do it in the SyntaxChecker, but some of these things are implemented in the NameAndTypeResolver, iirc.

chriseth commented 3 years ago

Implemented in https://github.com/ethereum/solidity/pull/10255