Fix regex patterns #2442
Fix regex patterns #2442
Conversation
DarkaMaul
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughWalkthroughThe changes involve updating the regular expression pattern in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant parse_expression
participant RegExpEngine
User->>parse_expression: Call parse_expression with expression
parse_expression->>RegExpEngine: Apply updated RegExp pattern
RegExpEngine-->>parse_expression: Return matched results
parse_expression-->>User: Return parsed expression
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
DarkaMaul
force-pushed
the
fix/regex-expression
branch
from
b677926 to
c55a095
Compare
|
Do you have an example of what this was failing on? |
|
The regex matched a single character in this list : So any of the following patterns would have matched :
The new regex forces the identifier to be complete :
|
DarkaMaul
force-pushed
the
fix/regex-expression
branch
from
c55a095 to
50bb7ff
Compare
|
@DarkaMaul That makes sense, but the existing tests are failing. Do you have a solidity test case that fails and succeeds with this fix? |
|
I fixed the regex, I'm a bit unsure why this non-capturing group makes sense here. |
Still not sure what necessitated the change, so I'm going to leave this as a draft |
|
I don't think it is easy to write an example where the actual code fails in real world scenarios as the The proposed fix only bring closer the user intent with the code : matching expressions like I've set up a print to check if for any of the current tests in the repo, it would have different results, and so far none have been found. I agree that merging this is not mandatory, but I guess that provides a slight code quality improvement. |
|
Can you run the formatter? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- slither/solc_parsing/expressions/expression_parsing.py (1 hunks)
Additional comments not posted (1)
slither/solc_parsing/expressions/expression_parsing.py (1)
504-506: The updated regex pattern correctly uses alternation to match complete identifiers rather than individual characters. This change should ensure that the function matches the intended keywords (struct,enum,function,modifier) accurately.
| found = re.findall( | ||
| r"(?:struct|enum|function|modifier) \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t | ||
| ) |
There was a problem hiding this comment.
The addition of the non-capturing group in the regex pattern is a good practice as it clarifies that the group is only used for matching and not for capturing. This can slightly improve performance and makes the regex cleaner. Consider adding a comment in the code to explain the use of the non-capturing group for future maintainers.
- r"(?:struct|enum|function|modifier) \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t
+ r"(?:struct|enum|function|modifier) \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t # Use non-capturing group to avoid unnecessary capturesCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| found = re.findall( | |
| r"(?:struct|enum|function|modifier) \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t | |
| ) | |
| found = re.findall( | |
| r"(?:struct|enum|function|modifier) \(([\[\] ()a-zA-Z0-9\.,_]*)\)", t # Use non-capturing group to avoid unnecessary captures | |
| ) |
To properly consider the identifiers, we need to use
()instead of[]in regex definition.Summary by CodeRabbit
Bug Fixes
Improvements