Must depend on Analysis #2502
Must depend on Analysis #2502
Conversation
|
|
WalkthroughWalkthroughThe recent changes introduce mechanisms to determine and compute "must dependencies" for variables within the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Slither
participant DataDependencyModule
participant MustDependOnFunction
participant SolidityContract
Tester->>Slither: Initialize Slither object
Slither->>DataDependencyModule: Request must dependencies
DataDependencyModule->>MustDependOnFunction: Call get_must_depends_on(variable)
MustDependOnFunction->>SolidityContract: Analyze smart contract for dependencies
SolidityContract->>MustDependOnFunction: Return analyzed dependencies
MustDependOnFunction->>DataDependencyModule: Return must dependencies
DataDependencyModule->>Slither: Provide must dependencies
Slither->>Tester: Return results of must dependency analysis
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: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- slither/analyses/data_dependency/data_dependency.py (1 hunks)
Additional context used
Ruff
slither/analyses/data_dependency/data_dependency.py
328-328: Test for membership should be
not inConvert to
not in(E713)
GitHub Check: Lint Code Base
slither/analyses/data_dependency/data_dependency.py
[warning] 303-303:
R0912: Too many branches (16/12) (too-many-branches)
| def get_must_depends_on(x): | ||
| must_dependencies = compute_must_dependencies(x) | ||
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | ||
| return None | ||
| return list(must_dependencies)[0] | ||
|
|
There was a problem hiding this comment.
Add type annotations and docstring.
The function lacks type annotations and a docstring. Adding these will improve readability and maintainability.
def get_must_depends_on(x: Variable) -> Union[Variable, None]:
"""
Compute the must dependencies for a given variable.
Args:
x (Variable): The target variable.
Returns:
Union[Variable, None]: The must dependency if it exists, otherwise None.
"""
must_dependencies = compute_must_dependencies(x)
if len(must_dependencies) > 1 or len(must_dependencies) == 0:
return None
return list(must_dependencies)[0]| def compute_must_dependencies(x): | ||
| if isinstance(x, (SolidityVariableComposed, Constant)) or ( | ||
| x.function.visibility in ["public", "external"] and x in x.function.parameters | ||
| ): | ||
| return set([x]) | ||
|
|
||
| function_dependencies = {} | ||
| function_dependencies["context"] = {} | ||
| lvalues = [] | ||
|
|
||
| for node in x.function.nodes: | ||
| for ir in node.irs_ssa: | ||
| if isinstance(ir, OperationWithLValue) and ir.lvalue: | ||
| if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage: | ||
| continue | ||
| if isinstance(ir.lvalue, ReferenceVariable): | ||
| lvalue = ir.lvalue.points_to | ||
| if lvalue: | ||
| lvalues.append((lvalue, x.function, ir)) | ||
| lvalues.append((ir.lvalue, x.function, ir)) | ||
|
|
||
| for lvalue_details in lvalues: | ||
| lvalue = lvalue_details[0] | ||
| ir = lvalue_details[2] | ||
|
|
||
| if not lvalue in function_dependencies["context"]: | ||
| function_dependencies["context"][lvalue] = set() | ||
| read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]] | ||
|
|
||
| if isinstance(ir, Index): | ||
| read = [ir.variable_left] | ||
| elif isinstance(ir, InternalCall) and ir.function: | ||
| read = ir.function.return_values_ssa | ||
| else: | ||
| read = ir.read | ||
| for v in read: | ||
| # if not isinstance(v, Constant): | ||
| function_dependencies["context"][lvalue].add(v) | ||
| function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"]) | ||
|
|
||
| must_dependencies = set() | ||
| data_dependencies = ( | ||
| list(function_dependencies["context"][x]) | ||
| if function_dependencies["context"] is not None | ||
| else [] | ||
| ) | ||
| for i, data_dependency in enumerate(data_dependencies): | ||
| result = compute_must_dependencies(data_dependency) | ||
| if i > 0: | ||
| must_dependencies = must_dependencies.intersection(result) | ||
| else: | ||
| must_dependencies = must_dependencies.union(result) | ||
| return must_dependencies |
There was a problem hiding this comment.
Refactor to reduce complexity.
The function has too many branches, making it complex and harder to maintain. Consider breaking it down into smaller helper functions.
def compute_must_dependencies(x):
if is_initial_dependency(x):
return set([x])
function_dependencies = initialize_function_dependencies(x.function)
lvalues = collect_lvalues(x.function)
process_lvalues(lvalues, function_dependencies)
return calculate_must_dependencies(x, function_dependencies)
def is_initial_dependency(x):
return isinstance(x, (SolidityVariableComposed, Constant)) or (
x.function.visibility in ["public", "external"] and x in x.function.parameters
)
def initialize_function_dependencies(function):
return {"context": {}}
def collect_lvalues(function):
lvalues = []
for node in function.nodes:
for ir in node.irs_ssa:
if isinstance(ir, OperationWithLValue) and ir.lvalue:
if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage:
continue
if isinstance(ir.lvalue, ReferenceVariable):
lvalue = ir.lvalue.points_to
if lvalue:
lvalues.append((lvalue, function, ir))
lvalues.append((ir.lvalue, function, ir))
return lvalues
def process_lvalues(lvalues, function_dependencies):
for lvalue_details in lvalues:
lvalue = lvalue_details[0]
ir = lvalue_details[2]
if lvalue not in function_dependencies["context"]:
function_dependencies["context"][lvalue] = set()
read = determine_read_variables(ir)
for v in read:
function_dependencies["context"][lvalue].add(v)
function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"])
def determine_read_variables(ir):
if isinstance(ir, Index):
return [ir.variable_left]
elif isinstance(ir, InternalCall) and ir.function:
return ir.function.return_values_ssa
else:
return ir.read
def calculate_must_dependencies(x, function_dependencies):
must_dependencies = set()
data_dependencies = (
list(function_dependencies["context"][x])
if function_dependencies["context"] is not None
else []
)
for i, data_dependency in enumerate(data_dependencies):
result = compute_must_dependencies(data_dependency)
if i > 0:
must_dependencies = must_dependencies.intersection(result)
else:
must_dependencies = must_dependencies.union(result)
return must_dependenciesTools
Ruff
328-328: Test for membership should be
not inConvert to
not in(E713)
GitHub Check: Lint Code Base
[warning] 303-303:
R0912: Too many branches (16/12) (too-many-branches)
| lvalue = lvalue_details[0] | ||
| ir = lvalue_details[2] | ||
|
|
||
| if not lvalue in function_dependencies["context"]: |
There was a problem hiding this comment.
Fix membership test.
Convert the membership test to not in for better readability.
- if not lvalue in function_dependencies["context"]:
+ if lvalue not in function_dependencies["context"]: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 lvalue in function_dependencies["context"]: | |
| if lvalue not in function_dependencies["context"]: |
Tools
Ruff
328-328: Test for membership should be
not inConvert to
not in(E713)
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/unit/core/test_must_depend_on.py (1)
14-14: Add a final newline.A final newline is missing at the end of the file.
- assert isinstance(result, list) and len(result) <= 1 + assert isinstance(result, list) and len(result) <= 1 +Tools
GitHub Check: Lint Code Base
[warning] 14-14:
C0304: Final newline missing (missing-final-newline)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- slither/analyses/data_dependency/data_dependency.py (1 hunks)
- tests/unit/core/test_data/must_depend_on.sol (1 hunks)
- tests/unit/core/test_must_depend_on.py (1 hunks)
Additional context used
GitHub Check: Lint Code Base
tests/unit/core/test_must_depend_on.py
[warning] 14-14:
C0304: Final newline missing (missing-final-newline)slither/analyses/data_dependency/data_dependency.py
[warning] 308-308:
R0912: Too many branches (16/12) (too-many-branches)
Ruff
slither/analyses/data_dependency/data_dependency.py
333-333: Test for membership should be
not inConvert to
not in(E713)
Additional comments not posted (8)
tests/unit/core/test_must_depend_on.py (3)
1-5: Imports look good.The imported modules and functions are necessary for the test function.
7-7: Constant definition looks good.The constant
TEST_DATA_DIRis correctly defined to locate the test data directory.
9-14: Test function looks good.The function
test_must_depend_on_returnscorrectly sets up aSlitherobject, retrieves function parameters, and asserts the results ofget_must_depends_on.Tools
GitHub Check: Lint Code Base
[warning] 14-14:
C0304: Final newline missing (missing-final-newline)tests/unit/core/test_data/must_depend_on.sol (5)
1-1: Pragma statement looks good.The pragma statement correctly specifies the Solidity version
^0.8.19.
3-5: Interface definition looks good.The interface
IERC20correctly defines thetransferFromfunction.
16-19: Functiongood2looks good.The function
good2correctly callsint_transferFromwith a constantfromaddress.
22-24: Functionbad2looks good.The function
bad2correctly demonstrates a missing return value bug by callingint_transferFromwith a non-constantfromaddress.
26-28: Functionint_transferFromlooks good.The function
int_transferFromcorrectly callstransferFromon theerc20interface.
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> SUPPORTED_TYPES | None: | ||
| """ | ||
| Return must dependency of a variable if exist otherwise return None. | ||
|
|
||
| :param variable: target variable whose must dependency needs to be computed | ||
| :return: Variable | None | ||
| """ | ||
| must_dependencies = compute_must_dependencies(variable) | ||
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | ||
| return [] | ||
| return [list(must_dependencies)[0]] | ||
|
|
There was a problem hiding this comment.
Add type annotations and docstring.
The function lacks type annotations and a docstring. Adding these will improve readability and maintainability.
def get_must_depends_on(variable: SUPPORTED_TYPES) -> List[SUPPORTED_TYPES]:
"""
Compute the must dependencies for a given variable.
Args:
variable (SUPPORTED_TYPES): The target variable.
Returns:
List[SUPPORTED_TYPES]: The must dependency if it exists, otherwise an empty list.
"""
must_dependencies = compute_must_dependencies(variable)
if len(must_dependencies) > 1 or len(must_dependencies) == 0:
return []
return [list(must_dependencies)[0]]| def compute_must_dependencies(v:SUPPORTED_TYPES) -> Set[Variable]: | ||
| if isinstance(v, (SolidityVariableComposed, Constant)) or ( | ||
| v.function.visibility in ["public", "external"] and v in v.function.parameters | ||
| ): | ||
| return set([v]) | ||
|
|
||
| function_dependencies = {} | ||
| function_dependencies["context"] = {} | ||
| lvalues = [] | ||
|
|
||
| for node in v.function.nodes: | ||
| for ir in node.irs_ssa: | ||
| if isinstance(ir, OperationWithLValue) and ir.lvalue: | ||
| if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage: | ||
| continue | ||
| if isinstance(ir.lvalue, ReferenceVariable): | ||
| lvalue = ir.lvalue.points_to | ||
| if lvalue: | ||
| lvalues.append((lvalue, v.function, ir)) | ||
| lvalues.append((ir.lvalue, v.function, ir)) | ||
|
|
||
| for lvalue_details in lvalues: | ||
| lvalue = lvalue_details[0] | ||
| ir = lvalue_details[2] | ||
|
|
||
| if not lvalue in function_dependencies["context"]: | ||
| function_dependencies["context"][lvalue] = set() | ||
| read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]] | ||
|
|
||
| if isinstance(ir, Index): | ||
| read = [ir.variable_left] | ||
| elif isinstance(ir, InternalCall) and ir.function: | ||
| read = ir.function.return_values_ssa | ||
| else: | ||
| read = ir.read | ||
| for variable in read: | ||
| # if not isinstance(variable, Constant): | ||
| function_dependencies["context"][lvalue].add(variable) | ||
| function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"]) | ||
|
|
||
| must_dependencies = set() | ||
| data_dependencies = ( | ||
| list(function_dependencies["context"][v]) | ||
| if function_dependencies["context"] is not None | ||
| else [] | ||
| ) | ||
| for i, data_dependency in enumerate(data_dependencies): | ||
| result = compute_must_dependencies(data_dependency) | ||
| if i > 0: | ||
| must_dependencies = must_dependencies.intersection(result) | ||
| else: | ||
| must_dependencies = must_dependencies.union(result) | ||
| return must_dependencies |
There was a problem hiding this comment.
Refactor to reduce complexity and fix membership test.
The function has too many branches, making it complex and harder to maintain. Convert the membership test to not in.
def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]:
if isinstance(v, (SolidityVariableComposed, Constant)) or (
v.function.visibility in ["public", "external"] and v in v.function.parameters
):
return set([v])
function_dependencies = {}
function_dependencies["context"] = {}
lvalues = []
for node in v.function.nodes:
for ir in node.irs_ssa:
if isinstance(ir, OperationWithLValue) and ir.lvalue:
if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage:
continue
if isinstance(ir.lvalue, ReferenceVariable):
lvalue = ir.lvalue.points_to
if lvalue:
lvalues.append((lvalue, v.function, ir))
lvalues.append((ir.lvalue, v.function, ir))
for lvalue_details in lvalues:
lvalue = lvalue_details[0]
ir = lvalue_details[2]
- if not lvalue in function_dependencies["context"]:
+ if lvalue not in function_dependencies["context"]:
function_dependencies["context"][lvalue] = set()
read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]]
if isinstance(ir, Index):
read = [ir.variable_left]
elif isinstance(ir, InternalCall) and ir.function:
read = ir.function.return_values_ssa
else:
read = ir.read
for variable in read:
# if not isinstance(variable, Constant):
function_dependencies["context"][lvalue].add(variable)
function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"])
must_dependencies = set()
data_dependencies = (
list(function_dependencies["context"][v])
if function_dependencies["context"] is not None
else []
)
for i, data_dependency in enumerate(data_dependencies):
result = compute_must_dependencies(data_dependency)
if i > 0:
must_dependencies = must_dependencies.intersection(result)
else:
must_dependencies = must_dependencies.union(result)
return must_dependenciesCommittable suggestion was skipped due to low confidence.
Tools
Ruff
333-333: Test for membership should be
not inConvert to
not in(E713)
GitHub Check: Lint Code Base
[warning] 308-308:
R0912: Too many branches (16/12) (too-many-branches)
| ) | ||
| for i, data_dependency in enumerate(data_dependencies): | ||
| result = compute_must_dependencies(data_dependency) | ||
| if i > 0: |
There was a problem hiding this comment.
I don't understand this condition as it seems it will always take the intersection. Could you clarify why this is done?
| data_dependencies = ( | ||
| list(function_dependencies["context"][v]) | ||
| if function_dependencies["context"] is not None | ||
| else [] | ||
| ) |
There was a problem hiding this comment.
| data_dependencies = ( | |
| list(function_dependencies["context"][v]) | |
| if function_dependencies["context"] is not None | |
| else [] | |
| ) | |
| data_dependencies = list(function_dependencies["context"].get(v, set())) |
This could probably be a defaultdict with an empty set as the default to avoid this
| :return: Variable | None | ||
| """ | ||
| must_dependencies = compute_must_dependencies(variable) | ||
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: |
There was a problem hiding this comment.
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | |
| if len(must_dependencies) != 1: |
| @@ -293,6 +293,72 @@ def get_all_dependencies_ssa( | |||
| return context.context[KEY_SSA] | |||
|
|
|||
|
|
|||
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> SUPPORTED_TYPES | None: | |||
There was a problem hiding this comment.
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> SUPPORTED_TYPES | None: | |
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> SUPPORTED_TYPES | None: |
The return type doesn't agree with what is actually returned (a list). Since we always return 1 item, maybe it should be Union[SUPPORTED_TYPES, None] (using Union since | limits which python versions can be used
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
tests/unit/core/test_must_depend_on.py (1)
14-14: UseTypeAliasfor type aliasing.For better readability and maintainability, use
TypeAliaswhen defining type aliases.- SUPPORTED_TYPES = Union[Variable, SolidityVariable, Constant] + SUPPORTED_TYPES: TypeAlias = Union[Variable, SolidityVariable, Constant]
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- slither/analyses/data_dependency/data_dependency.py (1 hunks)
- tests/unit/core/test_must_depend_on.py (1 hunks)
Additional context used
GitHub Check: Lint Code Base
tests/unit/core/test_must_depend_on.py
[warning] 23-23:
C0304: Final newline missing (missing-final-newline)
[warning] 8-8:
C0411: standard import "from typing import Union" should be placed before "from slither import Slither" (wrong-import-order)slither/analyses/data_dependency/data_dependency.py
[warning] 309-309:
R0912: Too many branches (16/12) (too-many-branches)
Ruff
slither/analyses/data_dependency/data_dependency.py
334-334: Test for membership should be
not inConvert to
not in(E713)
| from pathlib import Path | ||
| from slither import Slither | ||
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | ||
| from slither.core.variables.variable import Variable | ||
| from slither.core.declarations import ( | ||
| SolidityVariable, | ||
| ) | ||
| from typing import Union | ||
| from slither.slithir.variables import ( | ||
| Constant, | ||
| ) |
There was a problem hiding this comment.
Reorder imports according to PEP8.
Standard imports should be placed before third-party imports.
+ from typing import Union
from pathlib import Path
from slither import Slither
from slither.analyses.data_dependency.data_dependency import get_must_depends_on
from slither.core.variables.variable import Variable
from slither.core.declarations import (
SolidityVariable,
)
- from typing import Union
from slither.slithir.variables import (
Constant,
)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.
| from pathlib import Path | |
| from slither import Slither | |
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | |
| from slither.core.variables.variable import Variable | |
| from slither.core.declarations import ( | |
| SolidityVariable, | |
| ) | |
| from typing import Union | |
| from slither.slithir.variables import ( | |
| Constant, | |
| ) | |
| from typing import Union | |
| from pathlib import Path | |
| from slither import Slither | |
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | |
| from slither.core.variables.variable import Variable | |
| from slither.core.declarations import ( | |
| SolidityVariable, | |
| ) | |
| from slither.slithir.variables import ( | |
| Constant, | |
| ) |
Tools
GitHub Check: Lint Code Base
[warning] 8-8:
C0411: standard import "from typing import Union" should be placed before "from slither import Slither" (wrong-import-order)
| def test_must_depend_on_returns(solc_binary_path): | ||
| solc_path = solc_binary_path("0.8.19") | ||
| file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix() | ||
| slither_obj = Slither(file, solc=solc_path) | ||
| result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[0]) | ||
| assert isinstance(result, list) | ||
| assert len(result) == 0 or (len(result) == 1 and result[0] in SUPPORTED_TYPES) |
There was a problem hiding this comment.
Add more assertions to cover edge cases.
The test function should include more assertions to cover edge cases, such as when the result contains multiple dependencies.
def test_must_depend_on_returns(solc_binary_path):
solc_path = solc_binary_path("0.8.19")
file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix()
slither_obj = Slither(file, solc=solc_path)
result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[0])
assert isinstance(result, list)
assert len(result) == 0 or (len(result) == 1 and result[0] in SUPPORTED_TYPES)
+ # Add more assertions for edge cases
+ assert all(isinstance(dep, SUPPORTED_TYPES) for dep in result)
+ assert len(result) <= 1Committable 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.
| def test_must_depend_on_returns(solc_binary_path): | |
| solc_path = solc_binary_path("0.8.19") | |
| file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix() | |
| slither_obj = Slither(file, solc=solc_path) | |
| result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[0]) | |
| assert isinstance(result, list) | |
| assert len(result) == 0 or (len(result) == 1 and result[0] in SUPPORTED_TYPES) | |
| def test_must_depend_on_returns(solc_binary_path): | |
| solc_path = solc_binary_path("0.8.19") | |
| file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix() | |
| slither_obj = Slither(file, solc=solc_path) | |
| result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[0]) | |
| assert isinstance(result, list) | |
| assert len(result) == 0 or (len(result) == 1 and result[0] in SUPPORTED_TYPES) | |
| # Add more assertions for edge cases | |
| assert all(isinstance(dep, SUPPORTED_TYPES) for dep in result) | |
| assert len(result) <= 1 |
Tools
GitHub Check: Lint Code Base
[warning] 23-23:
C0304: Final newline missing (missing-final-newline)
priyankabose
force-pushed
the
must-depend-on
branch
from
24a022f
to
52416ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
tests/unit/core/test_must_depend_on.py (1)
15-30: Add more assertions to cover edge cases.The test function should include more assertions to cover edge cases, such as when the result contains multiple dependencies.
def test_must_depend_on_returns(solc_binary_path): solc_path = solc_binary_path("0.8.19") file = Path(TEST_DATA_DIR, "must_depend_on.sol").as_posix() slither_obj = Slither(file, solc=solc_path) result = get_must_depends_on(slither_obj.contracts[1].functions[2].parameters[0]) assert isinstance(result, list) assert len(result) == 0 or (len(result) == 1 and result[0] in SUPPORTED_TYPES) + # Add more assertions for edge cases + assert all(isinstance(dep, SUPPORTED_TYPES) for dep in result) + assert len(result) <= 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- slither/analyses/data_dependency/data_dependency.py (1 hunks)
- tests/unit/core/test_data/must_depend_on.sol (1 hunks)
- tests/unit/core/test_must_depend_on.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/unit/core/test_data/must_depend_on.sol
Additional context used
GitHub Check: Lint Code Base
tests/unit/core/test_must_depend_on.py
[warning] 6-6:
C0411: standard import "from typing import Union" should be placed before "from slither import Slither" (wrong-import-order)slither/analyses/data_dependency/data_dependency.py
[warning] 309-309:
R0912: Too many branches (16/12) (too-many-branches)
Ruff
slither/analyses/data_dependency/data_dependency.py
334-334: Test for membership should be
not inConvert to
not in(E713)
| from pathlib import Path | ||
| from slither import Slither | ||
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | ||
| from slither.core.variables.variable import Variable | ||
| from slither.core.declarations import SolidityVariable, SolidityVariableComposed | ||
| from typing import Union | ||
| from slither.slithir.variables import ( | ||
| Constant, | ||
| ) |
There was a problem hiding this comment.
Reorder imports according to PEP8.
Standard imports should be placed before third-party imports.
+ from typing import Union
from pathlib import Path
from slither import Slither
from slither.analyses.data_dependency.data_dependency import get_must_depends_on
from slither.core.variables.variable import Variable
from slither.core.declarations import SolidityVariable, SolidityVariableComposed
- from typing import Union
from slither.slithir.variables import ConstantCommittable 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.
| from pathlib import Path | |
| from slither import Slither | |
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | |
| from slither.core.variables.variable import Variable | |
| from slither.core.declarations import SolidityVariable, SolidityVariableComposed | |
| from typing import Union | |
| from slither.slithir.variables import ( | |
| Constant, | |
| ) | |
| from typing import Union | |
| from pathlib import Path | |
| from slither import Slither | |
| from slither.analyses.data_dependency.data_dependency import get_must_depends_on | |
| from slither.core.variables.variable import Variable | |
| from slither.core.declarations import SolidityVariable, SolidityVariableComposed | |
| from slither.slithir.variables import ( | |
| Constant, | |
| ) |
Tools
GitHub Check: Lint Code Base
[warning] 6-6:
C0411: standard import "from typing import Union" should be placed before "from slither import Slither" (wrong-import-order)
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> List: | ||
| """ | ||
| Return must dependency of a variable if exist otherwise return None. | ||
|
|
||
| :param variable: target variable whose must dependency needs to be computed | ||
| :return: Variable | None | ||
| """ | ||
| must_dependencies = compute_must_dependencies(variable) | ||
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | ||
| return [] | ||
| return [list(must_dependencies)[0]] | ||
|
|
There was a problem hiding this comment.
Add type annotations and improve docstring.
The function lacks detailed type annotations and a clear docstring. Adding these will improve readability and maintainability.
def get_must_depends_on(variable: SUPPORTED_TYPES) -> List[SUPPORTED_TYPES]:
"""
Return must dependency of a variable if it exists, otherwise return an empty list.
:param variable: target variable whose must dependency needs to be computed
:return: List of must dependencies if any, otherwise an empty list
"""
must_dependencies = compute_must_dependencies(variable)
if len(must_dependencies) > 1 or len(must_dependencies) == 0:
return []
return [list(must_dependencies)[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.
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> List: | |
| """ | |
| Return must dependency of a variable if exist otherwise return None. | |
| :param variable: target variable whose must dependency needs to be computed | |
| :return: Variable | None | |
| """ | |
| must_dependencies = compute_must_dependencies(variable) | |
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | |
| return [] | |
| return [list(must_dependencies)[0]] | |
| def get_must_depends_on(variable: SUPPORTED_TYPES) -> List[SUPPORTED_TYPES]: | |
| """ | |
| Return must dependency of a variable if it exists, otherwise return an empty list. | |
| :param variable: target variable whose must dependency needs to be computed | |
| :return: List of must dependencies if any, otherwise an empty list | |
| """ | |
| must_dependencies = compute_must_dependencies(variable) | |
| if len(must_dependencies) > 1 or len(must_dependencies) == 0: | |
| return [] | |
| return [list(must_dependencies)[0]] |
| def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]: | ||
| if isinstance(v, (SolidityVariableComposed, Constant)) or ( | ||
| v.function.visibility in ["public", "external"] and v in v.function.parameters | ||
| ): | ||
| return set([v]) | ||
|
|
||
| function_dependencies = {} | ||
| function_dependencies["context"] = {} | ||
| lvalues = [] | ||
|
|
||
| for node in v.function.nodes: | ||
| for ir in node.irs_ssa: | ||
| if isinstance(ir, OperationWithLValue) and ir.lvalue: | ||
| if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage: | ||
| continue | ||
| if isinstance(ir.lvalue, ReferenceVariable): | ||
| lvalue = ir.lvalue.points_to | ||
| if lvalue: | ||
| lvalues.append((lvalue, v.function, ir)) | ||
| lvalues.append((ir.lvalue, v.function, ir)) | ||
|
|
||
| for lvalue_details in lvalues: | ||
| lvalue = lvalue_details[0] | ||
| ir = lvalue_details[2] | ||
|
|
||
| if not lvalue in function_dependencies["context"]: | ||
| function_dependencies["context"][lvalue] = set() | ||
| read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]] | ||
|
|
||
| if isinstance(ir, Index): | ||
| read = [ir.variable_left] | ||
| elif isinstance(ir, InternalCall) and ir.function: | ||
| read = ir.function.return_values_ssa | ||
| else: | ||
| read = ir.read | ||
| for variable in read: | ||
| # if not isinstance(variable, Constant): | ||
| function_dependencies["context"][lvalue].add(variable) | ||
| function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"]) | ||
|
|
||
| must_dependencies = set() | ||
| data_dependencies = ( | ||
| list(function_dependencies["context"][v]) | ||
| if function_dependencies["context"] is not None | ||
| else [] | ||
| ) | ||
| for i, data_dependency in enumerate(data_dependencies): | ||
| result = compute_must_dependencies(data_dependency) | ||
| if i > 0: | ||
| must_dependencies = must_dependencies.intersection(result) | ||
| else: | ||
| must_dependencies = must_dependencies.union(result) | ||
| return must_dependencies |
There was a problem hiding this comment.
Refactor to reduce complexity and fix membership test.
The function has too many branches, making it complex and harder to maintain. Convert the membership test to not in and refactor to reduce complexity.
def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]:
if isinstance(v, (SolidityVariableComposed, Constant)) or (
v.function.visibility in ["public", "external"] and v in v.function.parameters
):
return set([v])
function_dependencies = {"context": {}}
lvalues = []
for node in v.function.nodes:
for ir in node.irs_ssa:
if isinstance(ir, OperationWithLValue) and ir.lvalue:
if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage:
continue
if isinstance(ir.lvalue, ReferenceVariable):
lvalue = ir.lvalue.points_to
if lvalue:
lvalues.append((lvalue, v.function, ir))
lvalues.append((ir.lvalue, v.function, ir))
for lvalue_details in lvalues:
lvalue = lvalue_details[0]
ir = lvalue_details[2]
if lvalue not in function_dependencies["context"]:
function_dependencies["context"][lvalue] = set()
read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]]
if isinstance(ir, Index):
read = [ir.variable_left]
elif isinstance(ir, InternalCall) and ir.function:
read = ir.function.return_values_ssa
else:
read = ir.read
for variable in read:
function_dependencies["context"][lvalue].add(variable)
function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"])
must_dependencies = set()
data_dependencies = (
list(function_dependencies["context"][v])
if function_dependencies["context"] is not None
else []
)
for i, data_dependency in enumerate(data_dependencies):
result = compute_must_dependencies(data_dependency)
if i > 0:
must_dependencies = must_dependencies.intersection(result)
else:
must_dependencies = must_dependencies.union(result)
return must_dependenciesCommittable 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.
| def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]: | |
| if isinstance(v, (SolidityVariableComposed, Constant)) or ( | |
| v.function.visibility in ["public", "external"] and v in v.function.parameters | |
| ): | |
| return set([v]) | |
| function_dependencies = {} | |
| function_dependencies["context"] = {} | |
| lvalues = [] | |
| for node in v.function.nodes: | |
| for ir in node.irs_ssa: | |
| if isinstance(ir, OperationWithLValue) and ir.lvalue: | |
| if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage: | |
| continue | |
| if isinstance(ir.lvalue, ReferenceVariable): | |
| lvalue = ir.lvalue.points_to | |
| if lvalue: | |
| lvalues.append((lvalue, v.function, ir)) | |
| lvalues.append((ir.lvalue, v.function, ir)) | |
| for lvalue_details in lvalues: | |
| lvalue = lvalue_details[0] | |
| ir = lvalue_details[2] | |
| if not lvalue in function_dependencies["context"]: | |
| function_dependencies["context"][lvalue] = set() | |
| read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]] | |
| if isinstance(ir, Index): | |
| read = [ir.variable_left] | |
| elif isinstance(ir, InternalCall) and ir.function: | |
| read = ir.function.return_values_ssa | |
| else: | |
| read = ir.read | |
| for variable in read: | |
| # if not isinstance(variable, Constant): | |
| function_dependencies["context"][lvalue].add(variable) | |
| function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"]) | |
| must_dependencies = set() | |
| data_dependencies = ( | |
| list(function_dependencies["context"][v]) | |
| if function_dependencies["context"] is not None | |
| else [] | |
| ) | |
| for i, data_dependency in enumerate(data_dependencies): | |
| result = compute_must_dependencies(data_dependency) | |
| if i > 0: | |
| must_dependencies = must_dependencies.intersection(result) | |
| else: | |
| must_dependencies = must_dependencies.union(result) | |
| return must_dependencies | |
| def compute_must_dependencies(v: SUPPORTED_TYPES) -> Set[Variable]: | |
| if isinstance(v, (SolidityVariableComposed, Constant)) or ( | |
| v.function.visibility in ["public", "external"] and v in v.function.parameters | |
| ): | |
| return set([v]) | |
| function_dependencies = {"context": {}} | |
| lvalues = [] | |
| for node in v.function.nodes: | |
| for ir in node.irs_ssa: | |
| if isinstance(ir, OperationWithLValue) and ir.lvalue: | |
| if isinstance(ir.lvalue, LocalIRVariable) and ir.lvalue.is_storage: | |
| continue | |
| if isinstance(ir.lvalue, ReferenceVariable): | |
| lvalue = ir.lvalue.points_to | |
| if lvalue: | |
| lvalues.append((lvalue, v.function, ir)) | |
| lvalues.append((ir.lvalue, v.function, ir)) | |
| for lvalue_details in lvalues: | |
| lvalue = lvalue_details[0] | |
| ir = lvalue_details[2] | |
| if lvalue not in function_dependencies["context"]: | |
| function_dependencies["context"][lvalue] = set() | |
| read: Union[List[Union[LVALUE, SolidityVariableComposed]], List[SlithIRVariable]] | |
| if isinstance(ir, Index): | |
| read = [ir.variable_left] | |
| elif isinstance(ir, InternalCall) and ir.function: | |
| read = ir.function.return_values_ssa | |
| else: | |
| read = ir.read | |
| for variable in read: | |
| function_dependencies["context"][lvalue].add(variable) | |
| function_dependencies["context"] = convert_to_non_ssa(function_dependencies["context"]) | |
| must_dependencies = set() | |
| data_dependencies = ( | |
| list(function_dependencies["context"][v]) | |
| if function_dependencies["context"] is not None | |
| else [] | |
| ) | |
| for i, data_dependency in enumerate(data_dependencies): | |
| result = compute_must_dependencies(data_dependency) | |
| if i > 0: | |
| must_dependencies = must_dependencies.intersection(result) | |
| else: | |
| must_dependencies = must_dependencies.union(result) | |
| return must_dependencies |
Tools
Ruff
334-334: Test for membership should be
not inConvert to
not in(E713)
GitHub Check: Lint Code Base
[warning] 309-309:
R0912: Too many branches (16/12) (too-many-branches)
This implements #175 .
Linter and reformatter ran locally.
Brief analysis description:
Summary by CodeRabbit
New Features
Tests