Enhanced Solidity syntax fixer with additional automated fixes and compatibility updates #108
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mpatibility updates
WalkthroughAdds a new method detect_and_fix_syntax_issues to SoliditySyntaxFixer that performs sequential automated Solidity code fixes (abstract contracts, virtual functions, call.value migration, call success handling, pragma upgrades, SPDX insertion, and require normalization) while tracking applied fixes per file and returning updated content. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Fixer as SoliditySyntaxFixer
participant Tracker as fixes_applied (dict)
Caller->>Fixer: detect_and_fix_syntax_issues(content, file_path)
activate Fixer
note over Fixer: Analyze content for legacy patterns
rect rgb(240,248,255)
note right of Fixer: Apply sequential fixes
Fixer->>Fixer: Mark contracts abstract if needed
Fixer->>Fixer: Add virtual to unimplemented functions
Fixer->>Fixer: Migrate .call.value(...) to {value: ...}
Fixer->>Fixer: Add success handling for calls
Fixer->>Fixer: Upgrade pragma solidity if outdated
Fixer->>Fixer: Insert SPDX identifier if missing
Fixer->>Fixer: Normalize/validate require patterns
end
Fixer->>Tracker: Record applied fixes for file_path with timestamp
Fixer-->>Caller: Return possibly transformed content
deactivate Fixer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
Static_agent/Slither_agent/solidity_syntax_fixer.py (4)
46-46: Use explicit exception conversion flag.The string conversion should use an explicit flag for better type safety and clarity.
Apply this diff to use explicit conversion:
- return False, f"Error during compilation: {str(e)}" + return False, f"Error during compilation: {e!s}"
48-51: Remove unused variable.The
original_contentvariable is assigned but never used in the method.Apply this diff to remove the unused variable:
def detect_and_fix_syntax_issues(self, content: str, file_path: str) -> str: """Detect and fix common Solidity syntax issues""" - original_content = content fixes = []
99-107: Remove extraneous f-string prefix.The f-string at line 104 doesn't contain any placeholders.
Apply this diff to remove the unnecessary f-prefix:
- fixes.append(f"Updated deprecated .value() syntax to modern {{value: ...}} format") + fixes.append("Updated deprecated .value() syntax to modern {value: ...} format")
138-142: Ineffective require pattern fix.The fix for old require statements only adds a log entry but doesn't actually modify the content. This creates inconsistency between reported fixes and actual changes.
Apply this diff to either remove the ineffective fix or implement the actual transformation:
- # Fix 7: Fix require statements that use old call patterns - old_require_pattern = r'require\s*\(\s*address\([^)]+\)\.call\.value\([^)]+\)\(\)\s*\)' - if re.search(old_require_pattern, content): - # This is handled by the earlier fixes, but let's ensure consistency - fixes.append("Fixed deprecated require statement with old call pattern") + # Fix 7: Additional cleanup for any remaining old call patterns + old_require_pattern = r'require\s*\(\s*address\(([^)]+)\)\.call\.value\(([^)]+)\)\(\)\s*\)' + def fix_old_require(match): + addr_expr = match.group(1) + value_expr = match.group(2) + fixes.append("Fixed deprecated require statement with old call pattern") + return f'(bool success, ) = address({addr_expr}).call{{value: {value_expr}}}("");\n require(success)' + + content = re.sub(old_require_pattern, fix_old_require, content)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Static_agent/Slither_agent/solidity_syntax_fixer.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
Static_agent/Slither_agent/solidity_syntax_fixer.py
46-46: Use explicit conversion flag
Replace with conversion flag
(RUF010)
50-50: Local variable original_content is assigned to but never used
Remove assignment to unused variable original_content
(F841)
104-104: f-string without any placeholders
Remove extraneous f prefix
(F541)
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (2)
Static_agent/Slither_agent/solidity_syntax_fixer.py (2)
144-150: LGTM! Good fix tracking implementation.The fix tracking with timestamps provides good traceability for applied changes.
119-129: Don't auto-bump pragma to ^0.8.0 without compatibility checksFixer mutates code (adds virtual/abstract and converts .call.value → .call{value:...}) before replacing old pragmas (Static_agent/Slither_agent/solidity_syntax_fixer.py lines ~119-129), which can silently force ^0.8.0 and break contracts.
- Transformation ordering: running fixes before the pragma check causes any file with old pragma + old call patterns to be bumped.
- Version detection is too narrow: only detects '^0.5' or '^0.4' substrings and misses ranges/other specifiers.
- Regex/semantics are brittle and unsafe: (\w+) won't match address(...) or complex expressions; replacement inserts "" (loses original calldata).
- No semver-aware validation or test compilation after the bump.
Require: perform semver-aware parsing, preserve address expressions and calldata in replacements, only change pragma after successful compilation with the target solc (or require manual review).
Comment on lines
+54
to
+82
| pattern = r'contract\s+(\w+)\s*{' | ||
| matches = re.finditer(pattern, content) | ||
| for match in matches: | ||
| contract_name = match.group(1) | ||
| contract_start = match.start() | ||
|
|
||
| # Look for unimplemented functions in this contract | ||
| # Find the contract body | ||
| brace_count = 0 | ||
| contract_end = contract_start | ||
| for i, char in enumerate(content[contract_start:]): | ||
| if char == '{': | ||
| brace_count += 1 | ||
| elif char == '}': | ||
| brace_count -= 1 | ||
| if brace_count == 0: | ||
| contract_end = contract_start + i | ||
| break | ||
|
|
||
| contract_body = content[contract_start:contract_end + 1] | ||
|
|
||
| # Check for unimplemented functions (ending with semicolon) | ||
| func_pattern = r'function\s+\w+\([^)]*\)\s*[^{]*;' | ||
| if re.search(func_pattern, contract_body): | ||
| # This contract has unimplemented functions, make it abstract | ||
| new_contract_def = f'abstract contract {contract_name} {{' | ||
| content = content.replace(match.group(0), new_contract_def) | ||
| fixes.append(f"Made contract '{contract_name}' abstract (has unimplemented functions)") | ||
|
|
There was a problem hiding this comment.
Potential issues with contract parsing and abstract keyword insertion.
The contract parsing logic has several concerns:
- The regex pattern doesn't handle inheritance (e.g.,
contract A is B {) - The brace counting doesn't handle nested structures like structs or mappings properly
- Abstract contracts may not be compatible with pragma versions below 0.6.0
Consider this more robust approach:
- # Fix 1: Abstract contracts with unimplemented functions
- pattern = r'contract\s+(\w+)\s*{'
- matches = re.finditer(pattern, content)
- for match in matches:
- contract_name = match.group(1)
- contract_start = match.start()
-
- # Look for unimplemented functions in this contract
- # Find the contract body
- brace_count = 0
- contract_end = contract_start
- for i, char in enumerate(content[contract_start:]):
- if char == '{':
- brace_count += 1
- elif char == '}':
- brace_count -= 1
- if brace_count == 0:
- contract_end = contract_start + i
- break
-
- contract_body = content[contract_start:contract_end + 1]
-
- # Check for unimplemented functions (ending with semicolon)
- func_pattern = r'function\s+\w+\([^)]*\)\s*[^{]*;'
- if re.search(func_pattern, contract_body):
- # This contract has unimplemented functions, make it abstract
- new_contract_def = f'abstract contract {contract_name} {{'
- content = content.replace(match.group(0), new_contract_def)
- fixes.append(f"Made contract '{contract_name}' abstract (has unimplemented functions)")
+ # Fix 1: Abstract contracts with unimplemented functions
+ # Handle inheritance syntax properly
+ pattern = r'contract\s+(\w+)(\s+is\s+[^{]+)?\s*{'
+ matches = list(re.finditer(pattern, content))
+ for match in reversed(matches): # Process in reverse to maintain positions
+ contract_name = match.group(1)
+ inheritance = match.group(2) or ""
+
+ # Check for unimplemented functions in this contract
+ func_pattern = r'function\s+\w+\([^)]*\)\s*[^{]*;'
+ if re.search(func_pattern, content):
+ # Only apply if pragma supports abstract contracts
+ pragma_match = re.search(r'pragma\s+solidity\s+([^;]+);', content)
+ if pragma_match and not re.search(r'\^0\.[0-5]\.', pragma_match.group(1)):
+ new_contract_def = f'abstract contract {contract_name}{inheritance} {{'
+ content = content.replace(match.group(0), new_contract_def)
+ fixes.append(f"Made contract '{contract_name}' abstract (has unimplemented functions)")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Static_agent/Slither_agent/solidity_syntax_fixer.py around lines 54-82, the
current logic that finds contracts and marks them abstract is fragile: the
contract regex only matches simple "contract Name {" and misses inheritance
clauses, the manual brace counting can be confused by nested braces inside
strings, comments, structs or mappings, and blindly inserting "abstract" can
break compatibility with older pragma versions. Fix by updating the contract
detection regex to capture optional inheritance and any modifiers (e.g.,
r'contract\s+(\w+)\s*(?:is\s+[^{]+)?\s*{'), replace the brittle brace-scan with
a parser-aware approach such as using a simple state machine that skips string
literals and comments and correctly tracks braces (or use an existing Solidity
parser library) to locate the contract body bounds, check the pragma solidity
version at the top and only insert the "abstract" keyword when pragma >= 0.6.0,
and perform the replacement using precise slicing (start/end indices) rather
than global replace to avoid accidental multiple substitutions; also add unit
tests covering inheritance, nested structs/mappings, comments, and older pragma
versions.
Comment on lines
+84
to
+97
| virtual_pattern = r'function\s+(\w+)\s*\([^)]*\)\s*([^{;]*);' | ||
| def add_virtual(match): | ||
| func_name = match.group(1) | ||
| modifiers = match.group(2).strip() | ||
| if 'virtual' not in modifiers: | ||
| if modifiers: | ||
| new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) {modifiers} virtual;" | ||
| else: | ||
| new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) public virtual;" | ||
| fixes.append(f"Added 'virtual' keyword to function '{func_name}'") | ||
| return new_func | ||
| return match.group(0) | ||
|
|
||
| content = re.sub(virtual_pattern, add_virtual, content) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex regex replacement with potential parsing issues.
The function parameter extraction using string splitting is fragile and may break with complex parameter lists or nested parentheses.
Apply this diff for more robust parameter handling:
- # Fix 2: Add virtual keyword to unimplemented functions
- virtual_pattern = r'function\s+(\w+)\s*\([^)]*\)\s*([^{;]*);'
- def add_virtual(match):
- func_name = match.group(1)
- modifiers = match.group(2).strip()
- if 'virtual' not in modifiers:
- if modifiers:
- new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) {modifiers} virtual;"
- else:
- new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) public virtual;"
- fixes.append(f"Added 'virtual' keyword to function '{func_name}'")
- return new_func
- return match.group(0)
-
- content = re.sub(virtual_pattern, add_virtual, content)
+ # Fix 2: Add virtual keyword to unimplemented functions
+ virtual_pattern = r'function\s+(\w+)\s*\(([^)]*)\)\s*([^{;]*);'
+ def add_virtual(match):
+ func_name = match.group(1)
+ params = match.group(2)
+ modifiers = match.group(3).strip()
+ if 'virtual' not in modifiers:
+ if modifiers:
+ new_func = f"function {func_name}({params}) {modifiers} virtual;"
+ else:
+ new_func = f"function {func_name}({params}) public virtual;"
+ fixes.append(f"Added 'virtual' keyword to function '{func_name}'")
+ return new_func
+ return match.group(0)
+
+ content = re.sub(virtual_pattern, add_virtual, content)📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| virtual_pattern = r'function\s+(\w+)\s*\([^)]*\)\s*([^{;]*);' | |
| def add_virtual(match): | |
| func_name = match.group(1) | |
| modifiers = match.group(2).strip() | |
| if 'virtual' not in modifiers: | |
| if modifiers: | |
| new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) {modifiers} virtual;" | |
| else: | |
| new_func = f"function {func_name}({match.group(0).split('(')[1].split(')')[0]}) public virtual;" | |
| fixes.append(f"Added 'virtual' keyword to function '{func_name}'") | |
| return new_func | |
| return match.group(0) | |
| content = re.sub(virtual_pattern, add_virtual, content) | |
| # Fix 2: Add virtual keyword to unimplemented functions | |
| virtual_pattern = r'function\s+(\w+)\s*\(([^)]*)\)\s*([^{;]*);' | |
| def add_virtual(match): | |
| func_name = match.group(1) | |
| params = match.group(2) | |
| modifiers = match.group(3).strip() | |
| if 'virtual' not in modifiers: | |
| if modifiers: | |
| new_func = f"function {func_name}({params}) {modifiers} virtual;" | |
| else: | |
| new_func = f"function {func_name}({params}) public virtual;" | |
| fixes.append(f"Added 'virtual' keyword to function '{func_name}'") | |
| return new_func | |
| return match.group(0) | |
| content = re.sub(virtual_pattern, add_virtual, content) |
🤖 Prompt for AI Agents
In Static_agent/Slither_agent/solidity_syntax_fixer.py around lines 84 to 97,
the current replacement extracts the function parameter list by splitting the
full match string which is fragile for complex or nested parameter lists; update
the regex to capture the parameter list and modifiers explicitly (e.g. pattern
with groups for name, params, and modifiers), use match.group(...) to get the
params and modifiers instead of string splitting, preserve existing visibility
or insert "public" only when visibility is missing, append the same fixes
message, and return the rebuilt function string using the captured groups so
nested/complex parameter lists are handled robustly.
Comment on lines
+109
to
+117
| # Fix 4: Add success handling for call operations | ||
| call_pattern = r'require\s*\(\s*(\w+)\.call\{value:\s*([^}]+)\}\s*\(""\)\s*\)' | ||
| def fix_call_handling(match): | ||
| address = match.group(1) | ||
| value = match.group(2) | ||
| fixes.append(f"Added proper success handling for call operation") | ||
| return f'(bool success, ) = {address}.call{{value: {value}}}("");\n require(success)' | ||
|
|
||
| content = re.sub(call_pattern, fix_call_handling, content) |
There was a problem hiding this comment.
Multiple issues with call pattern handling.
- The regex pattern may not handle all call variations
- The f-string at line 114 has no placeholders
- The replacement introduces formatting inconsistencies
Apply this diff to fix the issues:
- # Fix 4: Add success handling for call operations
- call_pattern = r'require\s*\(\s*(\w+)\.call\{value:\s*([^}]+)\}\s*\(""\)\s*\)'
- def fix_call_handling(match):
- address = match.group(1)
- value = match.group(2)
- fixes.append(f"Added proper success handling for call operation")
- return f'(bool success, ) = {address}.call{{value: {value}}}("");\n require(success)'
-
- content = re.sub(call_pattern, fix_call_handling, content)
+ # Fix 4: Add success handling for call operations
+ call_pattern = r'require\s*\(\s*(\w+)\.call\{value:\s*([^}]+)\}\s*\(""\)\s*\)'
+ def fix_call_handling(match):
+ address = match.group(1)
+ value = match.group(2)
+ fixes.append("Added proper success handling for call operation")
+ return f'(bool success, ) = {address}.call{{value: {value}}}("");\n require(success)'
+
+ content = re.sub(call_pattern, fix_call_handling, content)📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Fix 4: Add success handling for call operations | |
| call_pattern = r'require\s*\(\s*(\w+)\.call\{value:\s*([^}]+)\}\s*\(""\)\s*\)' | |
| def fix_call_handling(match): | |
| address = match.group(1) | |
| value = match.group(2) | |
| fixes.append(f"Added proper success handling for call operation") | |
| return f'(bool success, ) = {address}.call{{value: {value}}}("");\n require(success)' | |
| content = re.sub(call_pattern, fix_call_handling, content) | |
| # Fix 4: Add success handling for call operations | |
| call_pattern = r'require\s*\(\s*(\w+)\.call\{value:\s*([^}]+)\}\s*\(""\)\s*\)' | |
| def fix_call_handling(match): | |
| address = match.group(1) | |
| value = match.group(2) | |
| fixes.append("Added proper success handling for call operation") | |
| return f'(bool success, ) = {address}.call{{value: {value}}}("");\n require(success)' | |
| content = re.sub(call_pattern, fix_call_handling, content) |
🧰 Tools
🪛 Ruff (0.13.1)
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In Static_agent/Slither_agent/solidity_syntax_fixer.py around lines 109 to 117,
the current call-fix logic uses a fragile regex, an unnecessary f-string with no
placeholders, and produces inconsistent indentation; replace it by expanding the
regex to match optional whitespace, optional value clause, and optional calldata
(e.g., .call{value: ...}("...") or .call("...") ), capture leading indentation,
address, value (if present) and calldata, stop using an f-string when not
interpolating, and build the replacement string using the captured groups so it
produces a properly indented assignment like "(bool success, ) =
ADDRESS.call{value: VALUE}(CALLDATA);" or without the value clause when absent,
followed by "require(success);" on the next indented line to preserve formatting
and semantics.
aleskrin
approved these changes
Sep 24, 2025
This was referenced Sep 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit