Adjust regex to handle square brackets #774
Conversation
zoeyTM
force-pushed
the
fix/overload-brackets
branch
from
c04f678
to
eadaa8b
Compare
| */ | ||
| const functionNameRegex = /^[a-zA-Z$_][a-zA-Z0-9$_,()]*$/; | ||
| const functionNameRegex = /^[a-zA-Z0-9$_]*(\([a-zA-Z0-9$_,\[\]]*\))?$/; |
There was a problem hiding this comment.
Nice catch.
Are there any backwards compatibility concerns here?
There was a problem hiding this comment.
Should the * be a +?
Would the following incorrectly pass (uint256)?
There was a problem hiding this comment.
Should the
*be a+?
Ah that's a great catch, I just pushed an update to fix that (with a new test case to verify)
Are there any backwards compatibility concerns here?
I don't believe so. The biggest potential impacts I was aware of while writing this were:
- The rightmost
*needs to stay an*and not be changed to a+. This change would breakmyFunc()which currently works - Due to how the regex was previously written,
0-9was allowed to be in the function name, which looks unintentional, but is actually correct as that is valid solidity. I added0-9to the first character set to continue this behavior.
I believe these are the only two potential breaking changes of things that should work before and after the change. This technically has a breaking change in that new invalid patterns will fail, but they would have failed later in the code anyways previously, so I don't believe this is actually a breaking change. They just properly throw where intended now.
There was a problem hiding this comment.
Ah I follow now. The move is all about reworking the parameter list.
We support [] in the parameter list and this is allows additional valid identifiers, so no breaking change there - agreed.
I sharpened up the first part in firstpart(secondparts[]), so that the first character can't be a number: https://docs.soliditylang.org/en/v0.8.26/grammar.html#a4.SolidityLexer.Identifier.
It could be argued that we could sharpen up the parameter list as well, but I am happy with the current level of precision. What do you think?
The Solidity grammar suggests that the first character of an identifier must be a letter. It is arguable therefore, that we should do that for the args list, but I think this increment in precision is enough. See https://docs.soliditylang.org/en/v0.8.26/grammar.html#a4.SolidityLexer.Identifier
fixes #762
Adjusted the regex to be more strict in general as there were some invalid cases that were being allowed with the previous regex (even though they would be caught later in the validation process).
Added tests for relevant passing and failing cases
--
I also verified manually that square brackets are allowed in filenames in both unix and windows systems, so that is a non-issue.