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)
| 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]]
| ) | ||
| 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?
There was a problem hiding this comment.
i here is the index for the element data_dependency, for the first data_dependency element, my idea was to put its must dependency into the result , next onwards I try to see whether must dependency is the same or not. that is why for i>0 i do intersection, but for i=0 i do union
| 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
| @@ -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)
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)
Co-authored-by: alpharush <[email protected]>
This implements #175 .
Linter and reformatter ran locally.
Brief analysis description:
Summary by CodeRabbit
New Features
Tests