Rename father/son to predecessor/successor #2423
Conversation
This is a breaking change in the API but for the moment, we keep the compatibility layer in the cfg/node.py.
DarkaMaul
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughWalkthroughThe changes involve renaming variables and methods related to node relationships in the control flow graph (CFG) across multiple files. The terms Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (34)
slither/detectors/statements/write_after_write.py (1)
Line range hint
64-70: Consider simplifying the nestedifstatements as suggested by Ruff (SIM102).- if isinstance(ir, InternalCall): - if not ir.function: - return - if ir.function.all_high_level_calls() or ir.function.all_library_calls(): - _remove_states(written) + if isinstance(ir, InternalCall) and ir.function and (ir.function.all_high_level_calls() or ir.function.all_library_calls()): + _remove_states(written)slither/detectors/variables/uninitialized_local_variables.py (1)
Line range hint
125-125: The loop control variablefunctionis not used within the loop body, which could be a potential issue or oversight.Consider using the variable or removing it if unnecessary.
Tools
Ruff
70-76: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/operations/missing_zero_address_validation.py (1)
Line range hint
133-143: Consider simplifying the nestedifstatements as suggested by Ruff (SIM102).- if not sv_addrs_written and not addr_calls: - continue - if var.type == ElementaryType("address") and is_tainted(var, function, ignore_generic_taint=True): - if not (self._zero_address_validation_in_modifier(var, function.modifiers_statements) or self._zero_address_validation(var, node, [])): - var_nodes[var].append(node) + if sv_addrs_written or addr_calls: + if var.type == ElementaryType("address") and is_tainted(var, function, ignore_generic_taint=True) and not (self._zero_address_validation_in_modifier(var, function.modifiers_statements) or self._zero_address_validation(var, node, [])): + var_nodes[var].append(node)slither/detectors/variables/predeclaration_usage_local.py (1)
Line range hint
88-88: Convert tonot infor membership testing.- if not node in self.fix_point_information: + if node not in self.fix_point_information:slither/detectors/operations/cache_array_length.py (1)
Line range hint
128-134: Refactor nestedifstatements to a singleifstatement usingand.- if isinstance(op, Length) and op.value == array: + if isinstance(op, Length) and op.value == array:Apply similar changes to other nested
ifstatements as indicated by the static analysis tool.Also applies to: 145-148
Tools
Ruff
145-148: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/reentrancy/reentrancy.py (2)
113-113: Refactor to simplify the merging of dictionaries.Consider using dictionary comprehension directly in the
union_dictfunction to simplify this code segment.
Line range hint
27-27: Optimize dictionary key checks.- if k not in old_info.keys(): + if k not in old_info:Simplify the checks for dictionary keys as suggested by static analysis (Ruff, SIM118).
Also applies to: 34-34
slither/slithir/utils/ssa.py (7)
Line range hint
559-559: Refactor the membership test to usenot in.- not node.variable_declaration.name in local_variables_definition + node.variable_declaration.name not in local_variables_definitionTools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
612-612: Refactor the membership test to usenot in.- not variable.index in reference_variables_instances + variable.index not in reference_variables_instancesTools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
628-628: Refactor the membership test to usenot in.- not variable.index in temporary_variables_instances + variable.index not in temporary_variables_instancesTools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
634-634: Refactor the membership test to usenot in.- not variable.index in tuple_variables_instances + variable.index not in tuple_variables_instancesTools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
668-671: Replace theif-elseblock with a ternary operator for clarity and conciseness.- if isinstance(v, list): - v = _get_traversal(v, *instances) - else: - v = get(v, *instances) + v = _get_traversal(v, *instances) if isinstance(v, list) else get(v, *instances)Tools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
503-504: Avoid using single-letter variable names to improve code readability.- l = [v.refers_to for v in ir.rvalues] - l = [item for sublist in l for item in sublist] + references = [v.refers_to for v in ir.rvalues] + flattened_references = [item for sublist in references for item in sublist]Tools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
Line range hint
295-297: Combine nestedifstatements into a singleifstatement using logicalandto simplify the code.- if condition1: - if condition2: - action() + if condition1 and condition2: + action()Also applies to: 296-297, 360-361, 427-428, 500-502, 501-502, 507-508, 511-512
Tools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not inslither/tools/read_storage/read_storage.py (2)
Line range hint
477-477: Undefined name 'Expression' in type hint.Please ensure that the
Expressionclass is correctly imported or defined within the scope of this file, or adjust the type hint accordingly.
Line range hint
911-912: Simplify nestedifstatements.- if isinstance(target_variable_type_type, UserDefinedType) and isinstance( - target_variable_type_type.type, Structure - ): + if isinstance(target_variable_type_type, UserDefinedType) and isinstance(target_variable_type_type.type, Structure):Combining these conditions into a single line improves readability and adheres to Pythonic best practices.
tests/unit/slithir/test_ssa_generation.py (1)
Line range hint
209-217: Refactor nestedifstatements into a singleifwith combined conditions.According to the static analysis tool Ruff, the nested
ifstatements can be simplified. This change will improve the readability and reduce the complexity of the code:- if (x > 0) { - if (x > 1) { - y = 2; - } else { - y = 3; - } - } else { - y = 4; - } + if (x > 1) { + y = 2; + } else if (x > 0) { + y = 3; + } else { + y = 4; + }slither/core/cfg/node.py (1)
Line range hint
940-942: Useraise ... fromsyntax for clearer exception chaining.- raise SlitherException( + raise SlitherException(...) from errorThis change will help to distinguish the raised exception from errors that occur during exception handling itself, adhering to best practices in Python exception management.
slither/core/declarations/contract.py (3)
Line range hint
63-63: Remove quotes from type annotations for Python 3.7+ compatibility.- from slither.core.cfg.node import "Node" + from slither.core.cfg.node import NodeApply this change to all similar instances where type annotations are quoted. This will ensure compatibility with Python 3.7 and newer, where quoting is not required for forward declarations if from
__future__ import annotationsis used.Also applies to: 64-64, 68-68, 70-70, 71-71, 72-72, 75-75, 76-76, 78-78, 79-79, 80-80, 82-82, 83-83, 97-97, 98-98, 104-104, 108-108, 109-109, 111-111, 112-112, 116-116, 531-531, 778-778, 782-782, 1580-1580, 1581-1581
Line range hint
967-967: Undefined nameEventused in type annotations.It appears that
Eventis intended to be used as a type annotation, but it is not defined or imported in this file. This will cause a runtime error if type checking is performed. Please defineEventor import it appropriately.Also applies to: 977-977
Line range hint
1342-1343: Consider simplifying nestedifstatements.- if condition1: - if condition2: + if condition1 and condition2:This change simplifies the logic and improves the readability of the code by reducing the nesting level.
Also applies to: 1416-1417
slither/core/declarations/function.py (3)
Line range hint
622-622: Convert tonot infor membership testing.- if not node in self._nodes_ordered_dominators: + if node not in self._nodes_ordered_dominators:This change improves readability and follows Pythonic best practices.
Line range hint
1577-1577: Remove extraneous parentheses.- if not (ir.rvalues): + if not ir.rvalues:This change simplifies the expression by removing unnecessary parentheses.
Line range hint
126-126: Remove quotes from type annotations.- from slither.core.cfg.node import Node, NodeType + from slither.core.cfg.node import Node + from slither.core.cfg.node import NodeTypeThis change removes unnecessary quotes from type annotations, improving code readability and adhering to Python 3.7+ type hinting standards.
Also applies to: 127-127, 131-131, 132-132, 133-133, 135-135, 136-136, 137-137, 140-140, 141-141, 142-142, 144-144, 145-145, 146-146, 147-147, 148-148, 149-149, 150-150, 151-151, 152-152, 153-153, 154-154, 155-155, 156-156, 157-157, 158-158, 159-159, 160-160, 166-166, 167-167, 168-168, 170-170, 171-171, 172-172, 173-173, 174-174, 175-175, 176-176, 177-177, 178-178, 179-179, 180-180, 181-181, 182-182, 183-183, 184-184, 185-185, 186-186, 188-188, 190-190, 208-208, 221-221, 1094-1094, 1101-1101, 1721-1721
slither/solc_parsing/declarations/function.py (10)
Line range hint
59-59: Remove unnecessary quotes from type annotations.- contract_parser: Optional["ContractSolc"], + contract_parser: Optional[ContractSolc],
Line range hint
197-198: Combine nestedifstatements usingandfor clarity and simplicity.- if "kind" in attributes: - if attributes["kind"] == "receive": + if "kind" in attributes and attributes["kind"] == "receive":Also applies to: 203-204, 228-229
Line range hint
469-471: Use the simpler form of thegetmethod when a default ofNoneis implied.- init_expression = statement.get("initializationExpression", None) - condition = statement.get("condition", None) - loop_expression = statement.get("loopExpression", None) + init_expression = statement.get("initializationExpression") + condition = statement.get("condition") + loop_expression = statement.get("loopExpression")
Line range hint
496-496: Use the simpler form of thegetmethod when a default ofNoneis implied.- attributes = statement.get("attributes", None) + attributes = statement.get("attributes")
Line range hint
685-685: Use the simpler form of thegetmethod when a default ofNoneis implied.- parameters = parameters_list.get("parameters", None) + parameters = parameters_list.get("parameters")
Line range hint
738-738: Use the simpler form of thegetmethod when a default ofNoneis implied.- externalCall = statement.get("externalCall", None) + externalCall = statement.get("externalCall")
Line range hint
768-768: Use the simpler form of thegetmethod when a default ofNoneis implied.- block = statement.get("block", None) + block = statement.get("block")
Line range hint
779-779: Use the simpler form of thegetmethod when a default ofNoneis implied.- params = statement.get("parameters", None) + params = statement.get("parameters")
Line range hint
1058-1058: Use the simpler form of thegetmethod when a default ofNoneis implied.- expression = statement.get("expression", None) + expression = statement.get("expression")
Line range hint
1372-1372: Avoid reusing loop control variable names that shadow iterable names.- for m in ExportValues(m).result(): + for modifier in ExportValues(m).result():
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (30)
- examples/scripts/taint_mapping.py (1 hunks)
- slither/analyses/write/are_variables_written.py (1 hunks)
- slither/core/cfg/node.py (8 hunks)
- slither/core/declarations/contract.py (2 hunks)
- slither/core/declarations/function.py (3 hunks)
- slither/core/dominators/utils.py (2 hunks)
- slither/detectors/assembly/incorrect_return.py (1 hunks)
- slither/detectors/functions/modifier.py (2 hunks)
- slither/detectors/functions/out_of_order_retryable.py (2 hunks)
- slither/detectors/operations/cache_array_length.py (2 hunks)
- slither/detectors/operations/missing_zero_address_validation.py (1 hunks)
- slither/detectors/reentrancy/reentrancy.py (2 hunks)
- slither/detectors/statements/calls_in_loop.py (1 hunks)
- slither/detectors/statements/costly_operations_in_loop.py (1 hunks)
- slither/detectors/statements/delegatecall_in_loop.py (1 hunks)
- slither/detectors/statements/divide_before_multiply.py (1 hunks)
- slither/detectors/statements/msg_value_in_loop.py (1 hunks)
- slither/detectors/statements/write_after_write.py (1 hunks)
- slither/detectors/variables/predeclaration_usage_local.py (1 hunks)
- slither/detectors/variables/uninitialized_local_variables.py (1 hunks)
- slither/detectors/variables/uninitialized_storage_variables.py (1 hunks)
- slither/slithir/utils/ssa.py (2 hunks)
- slither/solc_parsing/declarations/function.py (12 hunks)
- slither/tools/read_storage/read_storage.py (1 hunks)
- slither/utils/code_complexity.py (3 hunks)
- slither/utils/upgradeability.py (1 hunks)
- slither/vyper_parsing/declarations/function.py (1 hunks)
- tests/unit/slithir/test_implicit_returns.py (5 hunks)
- tests/unit/slithir/test_ssa_generation.py (4 hunks)
- tests/unit/slithir/test_ternary_expressions.py (2 hunks)
Additional context used
Ruff
slither/utils/code_complexity.py
36-36: Ambiguous variable name:
l(E741)
56-56: Remove quotes from type annotation (UP037)
Remove quotes
examples/scripts/taint_mapping.py
27-30: Use ternary operator
read = [ir.variable_left] if isinstance(ir, Index) else ir.readinstead ofif-else-block (SIM108)Replace
if-else-block withread = [ir.variable_left] if isinstance(ir, Index) else ir.read
56-57: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingandslither/analyses/write/are_variables_written.py
57-62: Use a single
ifstatement instead of nestedifstatements (SIM102)
96-96: Use
state.nodes.get(node, None)instead of anifblock (SIM401)Replace with
state.nodes.get(node, None)slither/detectors/variables/uninitialized_storage_variables.py
118-118: Loop control variable
functionnot used within loop body (B007)slither/detectors/statements/write_after_write.py
64-70: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/variables/uninitialized_local_variables.py
70-76: Use a single
ifstatement instead of nestedifstatements (SIM102)
125-125: Loop control variable
functionnot used within loop body (B007)slither/detectors/operations/missing_zero_address_validation.py
133-143: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/variables/predeclaration_usage_local.py
88-88: Test for membership should be
not in(E713)Convert to
not inslither/detectors/statements/divide_before_multiply.py
22-23: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
26-32: Use a single
ifstatement instead of nestedifstatements (SIM102)
27-32: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
31-32: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
38-39: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
42-48: Use a single
ifstatement instead of nestedifstatements (SIM102)
43-48: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
47-48: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
81-82: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
109-113: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/operations/cache_array_length.py
128-134: Use a single
ifstatement instead of nestedifstatements (SIM102)
145-148: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/reentrancy/reentrancy.py
27-27: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
34-34: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
188-191: Use a single
ifstatement instead of nestedifstatements (SIM102)
189-191: Use a single
ifstatement instead of nestedifstatements (SIM102)
190-191: Use a single
ifstatement instead of nestedifstatements (SIM102)
285-286: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingandslither/slithir/utils/ssa.py
88-88: Test for membership should be
not in(E713)Convert to
not in
230-230: Test for membership should be
not in(E713)Convert to
not in
295-297: Use a single
ifstatement instead of nestedifstatements (SIM102)
296-297: Use a single
ifstatement instead of nestedifstatements (SIM102)
360-361: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
402-402: Test for membership should be
not in(E713)Convert to
not in
427-428: Use a single
ifstatement instead of nestedifstatements (SIM102)
500-502: Use a single
ifstatement instead of nestedifstatements (SIM102)
501-502: Use a single
ifstatement instead of nestedifstatements (SIM102)
503-503: Ambiguous variable name:
l(E741)
504-504: Ambiguous variable name:
l(E741)
507-508: Use a single
ifstatement instead of nestedifstatements (SIM102)
511-512: Use a single
ifstatement instead of nestedifstatements (SIM102)
559-559: Test for membership should be
not in(E713)Convert to
not in
612-612: Test for membership should be
not in(E713)Convert to
not in
628-628: Test for membership should be
not in(E713)Convert to
not in
634-634: Test for membership should be
not in(E713)Convert to
not in
668-671: Use ternary operator
v = _get_traversal(v, *instances) if isinstance(v, list) else get(v, *instances)instead ofif-else-block (SIM108)Replace
if-else-block withv = _get_traversal(v, *instances) if isinstance(v, list) else get(v, *instances)slither/tools/read_storage/read_storage.py
477-477: Undefined name
Expression(F821)
911-912: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingandtests/unit/slithir/test_ssa_generation.py
209-217: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/core/cfg/node.py
131-131: Remove quotes from type annotation (UP037)
Remove quotes
132-132: Remove quotes from type annotation (UP037)
Remove quotes
136-136: Remove quotes from type annotation (UP037)
Remove quotes
137-137: Remove quotes from type annotation (UP037)
Remove quotes
140-140: Remove quotes from type annotation (UP037)
Remove quotes
142-142: Remove quotes from type annotation (UP037)
Remove quotes
145-145: Remove quotes from type annotation (UP037)
Remove quotes
146-146: Remove quotes from type annotation (UP037)
Remove quotes
156-156: Remove quotes from type annotation (UP037)
Remove quotes
157-157: Remove quotes from type annotation (UP037)
Remove quotes
159-159: Remove quotes from type annotation (UP037)
Remove quotes
159-159: Remove quotes from type annotation (UP037)
Remove quotes
161-161: Remove quotes from type annotation (UP037)
Remove quotes
162-162: Remove quotes from type annotation (UP037)
Remove quotes
163-163: Remove quotes from type annotation (UP037)
Remove quotes
181-181: Remove quotes from type annotation (UP037)
Remove quotes
197-197: Remove quotes from type annotation (UP037)
Remove quotes
197-197: Remove quotes from type annotation (UP037)
Remove quotes
198-198: Remove quotes from type annotation (UP037)
Remove quotes
199-199: Remove quotes from type annotation (UP037)
Remove quotes
231-233: Use a single
ifstatement instead of nestedifstatements (SIM102)
232-233: Use a single
ifstatement instead of nestedifstatements (SIM102)
560-561: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
940-942: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)slither/core/declarations/contract.py
63-63: Remove quotes from type annotation (UP037)
Remove quotes
64-64: Remove quotes from type annotation (UP037)
Remove quotes
68-68: Remove quotes from type annotation (UP037)
Remove quotes
70-70: Remove quotes from type annotation (UP037)
Remove quotes
71-71: Remove quotes from type annotation (UP037)
Remove quotes
72-72: Remove quotes from type annotation (UP037)
Remove quotes
75-75: Remove quotes from type annotation (UP037)
Remove quotes
76-76: Remove quotes from type annotation (UP037)
Remove quotes
78-78: Remove quotes from type annotation (UP037)
Remove quotes
79-79: Remove quotes from type annotation (UP037)
Remove quotes
80-80: Remove quotes from type annotation (UP037)
Remove quotes
82-82: Remove quotes from type annotation (UP037)
Remove quotes
83-83: Remove quotes from type annotation (UP037)
Remove quotes
97-97: Remove quotes from type annotation (UP037)
Remove quotes
98-98: Remove quotes from type annotation (UP037)
Remove quotes
104-104: Remove quotes from type annotation (UP037)
Remove quotes
108-108: Remove quotes from type annotation (UP037)
Remove quotes
109-109: Remove quotes from type annotation (UP037)
Remove quotes
111-111: Remove quotes from type annotation (UP037)
Remove quotes
112-112: Remove quotes from type annotation (UP037)
Remove quotes
116-116: Remove quotes from type annotation (UP037)
Remove quotes
116-116: Remove quotes from type annotation (UP037)
Remove quotes
116-116: Remove quotes from type annotation (UP037)
Remove quotes
531-531: Remove quotes from type annotation (UP037)
Remove quotes
778-778: Remove quotes from type annotation (UP037)
Remove quotes
782-782: Remove quotes from type annotation (UP037)
Remove quotes
967-967: Undefined name
Event(F821)
977-977: Undefined name
Event(F821)
1342-1343: Use a single
ifstatement instead of nestedifstatements (SIM102)
1416-1417: Use a single
ifstatement instead of nestedifstatements (SIM102)
1580-1580: Remove quotes from type annotation (UP037)
Remove quotes
1581-1581: Remove quotes from type annotation (UP037)
Remove quotes
slither/core/declarations/function.py
126-126: Remove quotes from type annotation (UP037)
Remove quotes
127-127: Remove quotes from type annotation (UP037)
Remove quotes
131-131: Remove quotes from type annotation (UP037)
Remove quotes
132-132: Remove quotes from type annotation (UP037)
Remove quotes
133-133: Remove quotes from type annotation (UP037)
Remove quotes
135-135: Remove quotes from type annotation (UP037)
Remove quotes
136-136: Remove quotes from type annotation (UP037)
Remove quotes
137-137: Remove quotes from type annotation (UP037)
Remove quotes
140-140: Remove quotes from type annotation (UP037)
Remove quotes
141-141: Remove quotes from type annotation (UP037)
Remove quotes
142-142: Remove quotes from type annotation (UP037)
Remove quotes
144-144: Remove quotes from type annotation (UP037)
Remove quotes
145-145: Remove quotes from type annotation (UP037)
Remove quotes
146-146: Remove quotes from type annotation (UP037)
Remove quotes
147-147: Remove quotes from type annotation (UP037)
Remove quotes
148-148: Remove quotes from type annotation (UP037)
Remove quotes
149-149: Remove quotes from type annotation (UP037)
Remove quotes
150-150: Remove quotes from type annotation (UP037)
Remove quotes
151-151: Remove quotes from type annotation (UP037)
Remove quotes
152-152: Remove quotes from type annotation (UP037)
Remove quotes
153-153: Remove quotes from type annotation (UP037)
Remove quotes
154-154: Remove quotes from type annotation (UP037)
Remove quotes
155-155: Remove quotes from type annotation (UP037)
Remove quotes
156-156: Remove quotes from type annotation (UP037)
Remove quotes
157-157: Remove quotes from type annotation (UP037)
Remove quotes
158-158: Remove quotes from type annotation (UP037)
Remove quotes
159-159: Remove quotes from type annotation (UP037)
Remove quotes
160-160: Remove quotes from type annotation (UP037)
Remove quotes
166-166: Remove quotes from type annotation (UP037)
Remove quotes
167-167: Remove quotes from type annotation (UP037)
Remove quotes
168-168: Remove quotes from type annotation (UP037)
Remove quotes
170-170: Remove quotes from type annotation (UP037)
Remove quotes
171-171: Remove quotes from type annotation (UP037)
Remove quotes
172-172: Remove quotes from type annotation (UP037)
Remove quotes
173-173: Remove quotes from type annotation (UP037)
Remove quotes
174-174: Remove quotes from type annotation (UP037)
Remove quotes
175-175: Remove quotes from type annotation (UP037)
Remove quotes
176-176: Remove quotes from type annotation (UP037)
Remove quotes
177-177: Remove quotes from type annotation (UP037)
Remove quotes
178-178: Remove quotes from type annotation (UP037)
Remove quotes
179-179: Remove quotes from type annotation (UP037)
Remove quotes
180-180: Remove quotes from type annotation (UP037)
Remove quotes
181-181: Remove quotes from type annotation (UP037)
Remove quotes
182-182: Remove quotes from type annotation (UP037)
Remove quotes
183-183: Remove quotes from type annotation (UP037)
Remove quotes
184-184: Remove quotes from type annotation (UP037)
Remove quotes
185-185: Remove quotes from type annotation (UP037)
Remove quotes
186-186: Remove quotes from type annotation (UP037)
Remove quotes
188-188: Remove quotes from type annotation (UP037)
Remove quotes
190-190: Remove quotes from type annotation (UP037)
Remove quotes
208-208: Remove quotes from type annotation (UP037)
Remove quotes
221-221: Remove quotes from type annotation (UP037)
Remove quotes
622-622: Test for membership should be
not in(E713)Convert to
not in
1094-1094: Remove quotes from type annotation (UP037)
Remove quotes
1101-1101: Remove quotes from type annotation (UP037)
Remove quotes
1577-1577: Avoid extraneous parentheses (UP034)
Remove extraneous parentheses
1721-1721: Remove quotes from type annotation (UP037)
Remove quotes
slither/solc_parsing/declarations/function.py
59-59: Remove quotes from type annotation (UP037)
Remove quotes
197-198: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
203-204: Use a single
ifstatement instead of nestedifstatements (SIM102)
228-229: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
469-469: Use
statement.get("initializationExpression")instead ofstatement.get("initializationExpression", None)(SIM910)Replace
statement.get("initializationExpression", None)withstatement.get("initializationExpression")
470-470: Use
statement.get("condition")instead ofstatement.get("condition", None)(SIM910)Replace
statement.get("condition", None)withstatement.get("condition")
471-471: Use
statement.get("loopExpression")instead ofstatement.get("loopExpression", None)(SIM910)Replace
statement.get("loopExpression", None)withstatement.get("loopExpression")
496-496: Use
statement.get("attributes")instead ofstatement.get("attributes", None)(SIM910)Replace
statement.get("attributes", None)withstatement.get("attributes")
685-685: Use
parameters_list.get("parameters")instead ofparameters_list.get("parameters", None)(SIM910)Replace
parameters_list.get("parameters", None)withparameters_list.get("parameters")
738-738: Use
statement.get("externalCall")instead ofstatement.get("externalCall", None)(SIM910)Replace
statement.get("externalCall", None)withstatement.get("externalCall")
768-768: Use
statement.get("block")instead ofstatement.get("block", None)(SIM910)Replace
statement.get("block", None)withstatement.get("block")
779-779: Use
statement.get("parameters")instead ofstatement.get("parameters", None)(SIM910)Replace
statement.get("parameters", None)withstatement.get("parameters")
1058-1058: Use
statement.get("expression")instead ofstatement.get("expression", None)(SIM910)Replace
statement.get("expression", None)withstatement.get("expression")
1216-1217: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
1372-1372: Loop control variable
moverrides iterable it iterates (B020)
GitHub Check: Lint Code Base
examples/scripts/taint_mapping.py
[failure] 20-20:
E0601: Using variable 'KEY' before assignment (used-before-assignment)
[failure] 58-58:
E0601: Using variable 'function' before assignment (used-before-assignment)
Additional comments not posted (27)
slither/utils/code_complexity.py (2)
19-19: Change fromsonstosuccessorscorrectly implemented.
41-42: Renaming tosuccessorsandpredecessorsis consistent and correctly implemented.Also applies to: 52-53
slither/detectors/assembly/incorrect_return.py (1)
75-75: Change fromsonstosuccessorscorrectly implemented in the detection logic.examples/scripts/taint_mapping.py (1)
49-50: Change fromsonstosuccessorscorrectly implemented in the recursive visit logic.tests/unit/slithir/test_ternary_expressions.py (1)
27-27: Change fromsonstosuccessorscorrectly implemented in the test logic.slither/detectors/statements/calls_in_loop.py (1)
54-55: The update fromsonstosuccessorsis correctly implemented and aligns with the new terminology.slither/detectors/functions/modifier.py (2)
29-29: The update fromsonstosuccessorsis correctly implemented and aligns with the new terminology.Also applies to: 32-32
83-84: The update fromsonstosuccessorsis correctly implemented and aligns with the new terminology.slither/detectors/statements/delegatecall_in_loop.py (1)
48-49: The update fromsonstosuccessorsis correctly implemented and aligns with the new terminology.slither/detectors/statements/costly_operations_in_loop.py (1)
49-50: The update fromsonstosuccessorsis correctly implemented and aligns with the new terminology.slither/core/dominators/utils.py (2)
10-23: The changes to usepredecessorsinstead offathersare correctly implemented and consistent with the PR's objectives.
96-100: The updates to usepredecessorsand handle specific corner cases in the dominance frontier computation are correctly implemented.slither/detectors/statements/msg_value_in_loop.py (1)
62-63: The update to iterate oversuccessorsinstead ofsonsis correctly implemented and aligns with the PR's objectives.slither/detectors/variables/uninitialized_storage_variables.py (1)
60-92: The updates to handlepredecessorsandsuccessorsare correctly implemented and align with the PR's objectives.slither/detectors/statements/write_after_write.py (1)
97-100: The renaming fromsonstosuccessorsis correctly implemented here, aligning with the PR's objectives.slither/detectors/functions/out_of_order_retryable.py (1)
84-97: The renaming fromfatherstopredecessorsis correctly implemented here, aligning with the PR's objectives.slither/detectors/variables/predeclaration_usage_local.py (1)
106-107: Updated loop iteration to usesuccessorsinstead ofsonsaligns with the new terminology.slither/detectors/statements/divide_before_multiply.py (2)
116-117: Updated loop iteration to usesuccessorsinstead ofsonsaligns with the new terminology.
Line range hint
22-23: Refactor nestedifstatements to a singleifstatement usingand.
[REFACTOR_SUGGESTion]- if isinstance(ir, Binary): - if ir.type == BinaryType.DIVISION: + if isinstance(ir, Binary) and ir.type == BinaryType.DIVISION:Apply similar changes to other nested
ifstatements as indicated by the static analysis tool.Also applies to: 26-32, 27-32, 31-32, 38-39, 42-48, 43-48, 47-48, 81-82, 109-113
tests/unit/slithir/test_implicit_returns.py (1)
30-31: Updated test assertions to reflect the new node relationship terminology (successor_true,successor_false).Also applies to: 35-36, 38-39, 96-97, 102-103, 139-140, 145-146
slither/detectors/operations/cache_array_length.py (1)
144-148: Updated loop iteration to usesuccessorsinstead ofsonsaligns with the new terminology.Tools
Ruff
145-148: Use a single
ifstatement instead of nestedifstatements (SIM102)slither/detectors/reentrancy/reentrancy.py (2)
252-252: Ensure proper handling of node traversal and state updates.The changes in the exploration logic and node context handling appear to be correctly implemented, maintaining the integrity of the reentrancy detection logic.
Also applies to: 255-255, 257-257, 262-262, 264-264, 266-266, 267-267, 269-269, 272-272, 273-273, 274-274, 276-276, 277-277, 278-278, 280-280, 281-281
Line range hint
188-191: Refactor to combine nestedifstatements.
[REFACTOR_SUGGESTion]- if is_subset(new_info.calls, self.calls): - if is_subset(new_info.send_eth, self.send_eth): - if is_subset(new_info.reads, self.reads): - if dict_are_equal(new_info.reads_prior_calls, self.reads_prior_calls): - return True + if is_subset(new_info.calls, self.calls) and is_subset(new_info.send_eth, self.send_eth) and is_subset(new_info.reads, self.reads) and dict_are_equal(new_info.reads_prior_calls, self.reads_prior_calls): + return TrueCombine nested
ifstatements into a single statement to improve readability and reduce complexity.Also applies to: 189-191, 190-191, 285-286
slither/vyper_parsing/declarations/function.py (1)
251-252: Ensure recursive node reachability updates.The recursive update for node reachability in the control flow graph is correctly implemented, ensuring that all successors are visited.
tests/unit/slithir/test_ssa_generation.py (1)
158-158: Ensure the number of predecessors matches the number of arguments in the phi-function.This assertion correctly ensures that each phi-function has as many arguments as the node has predecessors, which is a fundamental property of SSA form in control flow graphs.
slither/core/cfg/node.py (1)
1052-1088: Ensure backward compatibility while deprecating old method names.This change introduces a compatibility layer using
__getattr__to handle deprecated method names by redirecting them to the new method names and logging a deprecation warning. This is a good practice to maintain backward compatibility while transitioning to new terminology.slither/core/declarations/contract.py (1)
1481-1481: Renaming of node relationship methods toadd_successorandadd_predecessoraligns with PR objectives.The changes correctly implement the renaming from
add_sonandadd_fathertoadd_successorandadd_predecessor, respectively. This aligns with the PR's goal to make terminology more intuitive and is consistent across the provided changes.Also applies to: 1482-1482, 1513-1513, 1514-1514
| @@ -227,7 +227,7 @@ def generate_ssa_irs( | |||
| return | |||
|
|
|||
| if node.type in [NodeType.ENDIF, NodeType.ENDLOOP] and any( | |||
| not father in visited for father in node.fathers | |||
| not predecessor in visited for predecessor in node.predecessors | |||
There was a problem hiding this comment.
Refactor the membership test to use not in.
- not predecessor in visited
+ predecessor not in visitedCommittable 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.
| not predecessor in visited for predecessor in node.predecessors | |
| predecessor not in visited for predecessor in node.predecessors |
Tools
Ruff
230-230: Test for membership should be
not in(E713)Convert to
not in
| @@ -434,7 +434,7 @@ def find_hardcoded_slot_in_fallback(self, contract: Contract) -> Optional[StateV | |||
| while len(queue) > 0: | |||
| node = queue.pop(0) | |||
| visited.append(node) | |||
| queue.extend(son for son in node.sons if son not in visited) | |||
| queue.extend(son for son in node.successors if son not in visited) | |||
There was a problem hiding this comment.
Update variable name for clarity and consistency.
- queue.extend(son for son in node.successors if son not in visited)
+ queue.extend(successor for successor in node.successors if successor not in visited)This change aligns with the PR's objective to replace "son" with "successor" throughout the codebase, enhancing clarity and consistency.
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.
| queue.extend(son for son in node.successors if son not in visited) | |
| queue.extend(successor for successor in node.successors if successor not in visited) |
| worklist = list(from_node.successors) | ||
| while worklist: | ||
| first, *worklist = worklist | ||
| if first == to: | ||
| return True | ||
| visited.add(first) | ||
| for successor in first.sons: | ||
| for successor in first.successors: |
There was a problem hiding this comment.
Optimize the graph traversal by checking for the destination node early.
Consider checking if first == to immediately after dequeuing from worklist to potentially save unnecessary iterations:
while worklist:
first, *worklist = worklist
+ if first == to:
+ return True
visited.add(first)
for successor in first.successors:
if successor not in visited:
worklist.append(successor)
return FalseCommittable 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.
| worklist = list(from_node.successors) | |
| while worklist: | |
| first, *worklist = worklist | |
| if first == to: | |
| return True | |
| visited.add(first) | |
| for successor in first.sons: | |
| for successor in first.successors: | |
| worklist = list(from_node.successors) | |
| while worklist: | |
| first, *worklist = worklist | |
| if first == to: | |
| return True | |
| visited.add(first) | |
| for successor in first.successors: | |
| if successor not in visited: | |
| worklist.append(successor) | |
| return False |
| queue_f1.extend(son for son in node_f1.successors if son not in visited) | ||
| queue_f2.extend(son for son in node_f2.successors if son not in visited) |
There was a problem hiding this comment.
Optimize CFG traversal in function comparison.
Consider using a set for visited to improve performance during CFG traversal, as list operations can be costly for large graphs.
| self._successors: List["Node"] = [] | ||
| self._predecessors: List["Node"] = [] |
There was a problem hiding this comment.
Refactor type annotations by removing quotes.
- self._successors: List["Node"] = []
- self._predecessors: List["Node"] = []
+ self._successors: List[Node] = []
+ self._predecessors: List[Node] = []
- self._dominators: Set["Node"] = set()
- self._immediate_dominator: Optional["Node"] = None
+ self._dominators: Set[Node] = set()
+ self._immediate_dominator: Optional[Node] = None
- self._dom_successors: Set["Node"] = set()
- self._dominance_frontier: Set["Node"] = set()
+ self._dom_successors: Set[Node] = set()
+ self._dominance_frontier: Set[Node] = set()
- self._phi_origins_state_variables: Dict[str, Tuple[StateVariable, Set["Node"]]] = {}
- self._phi_origins_local_variables: Dict[str, Tuple[LocalVariable, Set["Node"]]] = {}
+ self._phi_origins_state_variables: Dict[str, Tuple[StateVariable, Set[Node]]] = {}
+ self._phi_origins_local_variables: Dict[str, Tuple[LocalVariable, Set[Node]]] = {}
- self._ssa_vars_written: List["SlithIRVariable"] = []
- self._ssa_vars_read: List["SlithIRVariable"] = []
+ self._ssa_vars_written: List[SlithIRVariable] = []
+ self._ssa_vars_read: List[SlithIRVariable] = []
- self._ssa_state_vars_written: List[StateIRVariable] = []
- self._ssa_state_vars_read: List[StateIRVariable] = []
+ self._ssa_state_vars_written: List[StateIRVariable] = []
+ self._ssa_state_vars_read: List[StateIRVariable] = []Also applies to: 136-137, 140-142, 145-146, 156-157, 159-163, 181-181, 197-199, 198-198
Committable suggestion was skipped due to low confidence.
Tools
Ruff
131-131: Remove quotes from type annotation (UP037)
Remove quotes
132-132: Remove quotes from type annotation (UP037)
Remove quotes
| if not son in explored: | ||
| to_explore.add(son) | ||
| for successor in node.successors: | ||
| if not successor in explored: |
There was a problem hiding this comment.
Refactor the membership test to use not in.
- not successor in explored
+ successor not in visitedCommittable suggestion was skipped due to low confidence.
Tools
Ruff
402-402: Test for membership should be
not in(E713)Convert to
not in
| if node.type == NodeType.IFLOOP: | ||
| if skip_if_loop == 0: |
There was a problem hiding this comment.
Combine nested if statements using and for clarity and simplicity.
- if node.type == NodeType.IFLOOP:
- if skip_if_loop == 0:
+ if node.type == NodeType.IFLOOP and skip_if_loop == 0: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.
| if node.type == NodeType.IFLOOP: | |
| if skip_if_loop == 0: | |
| if node.type == NodeType.IFLOOP and skip_if_loop == 0: |
Tools
Ruff
1216-1217: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
| if not self.successors and self.type != NodeType.THROW: | ||
| if SolidityFunction("revert()") not in self.solidity_calls: | ||
| if SolidityFunction("revert(string)") not in self.solidity_calls: |
There was a problem hiding this comment.
Optimize conditional checks by combining them into a single if statement.
- if not self.successors and self.type != NodeType.THROW:
- if SolidityFunction("revert()") not in self.solidity_calls:
- if SolidityFunction("revert(string)") not in self.solidity_calls:
+ if not self.successors and self.type != NodeType.THROW and SolidityFunction("revert()") not in self.solidity_calls and SolidityFunction("revert(string)") not in self.solidity_calls:
return TrueCommittable 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.
| if not self.successors and self.type != NodeType.THROW: | |
| if SolidityFunction("revert()") not in self.solidity_calls: | |
| if SolidityFunction("revert(string)") not in self.solidity_calls: | |
| if not self.successors and self.type != NodeType.THROW and SolidityFunction("revert()") not in self.solidity_calls and SolidityFunction("revert(string)") not in self.solidity_calls: | |
| return True |
Tools
Ruff
231-233: Use a single
ifstatement instead of nestedifstatements (SIM102)
232-233: Use a single
ifstatement instead of nestedifstatements (SIM102)
| if ( | ||
| node.type == NodeType.VARIABLE | ||
| and len(node.sons) == 1 # Should always be true for a node that has a STARTLOOP son | ||
| and node.sons[0].type == NodeType.STARTLOOP | ||
| and len(node.successors) | ||
| == 1 # Should always be true for a node that has a STARTLOOP successor | ||
| and node.successors[0].type == NodeType.STARTLOOP | ||
| ): | ||
| if node.variable_declaration in fathers_context: | ||
| fathers_context.remove(node.variable_declaration) | ||
| if node.variable_declaration in predecessors_context: |
There was a problem hiding this comment.
Consider simplifying the nested if statements as suggested by Ruff (SIM102).
- if node.variable_declaration in predecessors_context:
- predecessors_context.remove(node.variable_declaration)
+ if node.type == NodeType.VARIABLE and len(node.successors) == 1 and node.successors[0].type == NodeType.STARTLOOP and node.variable_declaration in predecessors_context:
+ predecessors_context.remove(node.variable_declaration)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.
| if ( | |
| node.type == NodeType.VARIABLE | |
| and len(node.sons) == 1 # Should always be true for a node that has a STARTLOOP son | |
| and node.sons[0].type == NodeType.STARTLOOP | |
| and len(node.successors) | |
| == 1 # Should always be true for a node that has a STARTLOOP successor | |
| and node.successors[0].type == NodeType.STARTLOOP | |
| ): | |
| if node.variable_declaration in fathers_context: | |
| fathers_context.remove(node.variable_declaration) | |
| if node.variable_declaration in predecessors_context: | |
| if ( | |
| node.type == NodeType.VARIABLE | |
| and len(node.successors) | |
| == 1 # Should always be true for a node that has a STARTLOOP successor | |
| and node.successors[0].type == NodeType.STARTLOOP | |
| and node.variable_declaration in predecessors_context | |
| ): | |
| predecessors_context.remove(node.variable_declaration) |
Tools
Ruff
70-76: Use a single
ifstatement instead of nestedifstatements (SIM102)
| if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]: | ||
| ret += [v for v in variables_to_write if v not in variables_written] | ||
|
|
||
| # Explore sons if | ||
| # Explore successors if | ||
| # - Before is none: its the first time we explored the node | ||
| # - variables_written is not before: it means that this path has a configuration of set variables | ||
| # that we haven't seen yet | ||
| before = state.nodes[node] if node in state.nodes else None | ||
| if before is None or variables_written not in before: | ||
| state.nodes[node].append(variables_written) | ||
| for son in node.sons: | ||
| ret += _visit(son, state, variables_written, variables_to_write) | ||
| for successor in node.successors: | ||
| ret += _visit(successor, state, variables_written, variables_to_write) |
There was a problem hiding this comment.
The updates to handle successors are correctly implemented. Consider using state.nodes.get(node, None) for more concise and Pythonic access to dictionary elements.
- before = state.nodes[node] if node in state.nodes else None
+ before = state.nodes.get(node, None)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.
| if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]: | |
| ret += [v for v in variables_to_write if v not in variables_written] | |
| # Explore sons if | |
| # Explore successors if | |
| # - Before is none: its the first time we explored the node | |
| # - variables_written is not before: it means that this path has a configuration of set variables | |
| # that we haven't seen yet | |
| before = state.nodes[node] if node in state.nodes else None | |
| if before is None or variables_written not in before: | |
| state.nodes[node].append(variables_written) | |
| for son in node.sons: | |
| ret += _visit(son, state, variables_written, variables_to_write) | |
| for successor in node.successors: | |
| ret += _visit(successor, state, variables_written, variables_to_write) | |
| if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]: | |
| ret += [v for v in variables_to_write if v not in variables_written] | |
| # Explore successors if | |
| # - Before is none: its the first time we explored the node | |
| # - variables_written is not before: it means that this path has a configuration of set variables | |
| # that we haven't seen yet | |
| before = state.nodes.get(node, None) | |
| if before is None or variables_written not in before: | |
| state.nodes[node].append(variables_written) | |
| for successor in node.successors: | |
| ret += _visit(successor, state, variables_written, variables_to_write) |
Tools
Ruff
96-96: Use
state.nodes.get(node, None)instead of anifblock (SIM401)Replace with
state.nodes.get(node, None)
| @@ -1046,6 +1049,43 @@ def __str__(self) -> str: | |||
| txt = str(self._node_type.value) + additional_info | |||
| return txt | |||
|
|
|||
| def __getattr__(self, item: str): | |||
There was a problem hiding this comment.
We should avoid this, and do a breaking change
Doing the getattr trick will lead to more issues in the long term I think
There was a problem hiding this comment.
I've attempted to keep the compatibility layer minimal here to avoid future issues - however, I believe that implementing a complete breaking change will significantly simplify the code.
This is a breaking change in the API but for the moment, we keep the compatibility layer in the cfg/node.py.
Fixes #2398
The main changes are in
node.pywhere we use a__getattr__to proxy calls tosonorfathermethods (with an explicit list), print a deprecation warning and forward to the new implementation.I expect the linter to break, but I tried to keep changes minimal.
Of note, this PR is quite large because it tries to replace usages of
sonorfathervariable in common places in the code.Summary by CodeRabbit
Refactor
sonstosuccessorsandfatherstopredecessors.Bug Fixes
These changes make the codebase more intuitive and maintainable, ensuring clearer understanding and future-proofing against deprecated terms.