Add detectors to include override exclude args #2440
Conversation
nsiregar
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughWalkthroughThe update enhances Slither by refining plugin functionality, improving detector management, and addressing typos. It introduces new conditions for detecting unused variables in abstract contracts, enhances logging for EVM summary printing, and makes configuration adjustments for better user control. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional Context UsedRuff (4)
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: 2
Actionable comments outside the diff hunks (3)
slither/slithir/convert.py (3)
Line range hint
354-354: Usenot ininstead ofinfor membership testing to correctly express the logic.- if ins.lvalue.name in to_keep and ins != last_elem: + if ins.lvalue.name not in to_keep and ins != last_elem:
Line range hint
1893-1893: Usenot ininstead ofinfor membership testing to correctly express the logic.- if ins.lvalue.name in to_keep: + if ins.lvalue.name not in to_keep:
Line range hint
1903-1903: Usenot ininstead ofinfor membership testing to correctly express the logic.- if not ins.lvalue.name in to_keep and ins != last_elem: + if ins.lvalue.name not in to_keep and ins != last_elem:
nsiregar
force-pushed
the
add-detectors-to-include-override-exclude-args
branch
from
6d92113
to
002b06b
Compare
|
Of note, this will clash with the switches introduced by #2428 ( |
nsiregar
force-pushed
the
add-detectors-to-include-override-exclude-args
branch
from
002b06b
to
03487e8
Compare
|
switch updated to |
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (4)
slither/utils/command_line.py (4)
Line range hint
126-126: Usenot infor membership testing to improve clarity.- if key in defaults_flag_in_config: + if key not in defaults_flag_in_config:
Line range hint
157-157: Consider renaming the variablelto a more descriptive name.- for l in lines: + for line in lines:
Line range hint
171-171: Consider renaming the variablelto a more descriptive name for consistency.- for l in lines: + for line in lines:
Line range hint
240-240: Usenot infor membership testing to improve clarity.- if detector in detectors: + if detector not in detectors:
slither/__main__.py
Outdated
| def __include_detectors( | ||
| detectors_to_run: Set[Type[AbstractDetector]], | ||
| detectors_to_include: str, | ||
| detectors: Dict[str, Type[AbstractDetector]], | ||
| ) -> List[Type[AbstractDetector]]: | ||
| include_detectors = detectors_to_include.split(",") | ||
|
|
||
| for detector in include_detectors: | ||
| if detector in detectors: | ||
| detectors_to_run.add(detectors[detector]) | ||
| else: | ||
| raise ValueError(f"Error: {detector} is not a detector") | ||
| return list(detectors_to_run) | ||
|
|
There was a problem hiding this comment.
Consider using a set to avoid duplicate detectors in __include_detectors.
- detectors_to_run: List[Type[AbstractDetector]],
+ detectors_to_run: Set[Type[AbstractDetector]],
- return detectors_to_run
+ return list(detectors_to_run)This change ensures that no duplicate detectors are added if they are already present in the list.
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.
| def __include_detectors( | |
| detectors_to_run: Set[Type[AbstractDetector]], | |
| detectors_to_include: str, | |
| detectors: Dict[str, Type[AbstractDetector]], | |
| ) -> List[Type[AbstractDetector]]: | |
| include_detectors = detectors_to_include.split(",") | |
| for detector in include_detectors: | |
| if detector in detectors: | |
| detectors_to_run.add(detectors[detector]) | |
| else: | |
| raise ValueError(f"Error: {detector} is not a detector") | |
| return list(detectors_to_run) | |
| def __include_detectors( | |
| detectors_to_run: Set[Type[AbstractDetector]], | |
| detectors_to_include: str, | |
| detectors: Dict[str, Type[AbstractDetector]], | |
| ) -> List[Type[AbstractDetector]]: | |
| include_detectors = detectors_to_include.split(",") | |
| for detector in include_detectors: | |
| if detector in detectors: | |
| detectors_to_run.add(detectors[detector]) | |
| else: | |
| raise ValueError(f"Error: {detector} is not a detector") | |
| return list(detectors_to_run) |
slither/__main__.py
Outdated
| group_detector.add_argument( | ||
| "--include-detectors", | ||
| help="Comma-separated list of detectors that should be excluded", | ||
| action="store", | ||
| dest="detectors_to_include", | ||
| default=defaults_flag_in_config["detectors_to_include"], | ||
| ) |
There was a problem hiding this comment.
Clarify the help message for the --include-detectors argument.
- help="Comma-separated list of detectors that should be excluded",
+ help="Comma-separated list of detectors that should be included",The help message incorrectly states that the detectors should be excluded. Please correct this to reflect that it is for including detectors.
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.
| group_detector.add_argument( | |
| "--include-detectors", | |
| help="Comma-separated list of detectors that should be excluded", | |
| action="store", | |
| dest="detectors_to_include", | |
| default=defaults_flag_in_config["detectors_to_include"], | |
| ) | |
| group_detector.add_argument( | |
| "--include-detectors", | |
| help="Comma-separated list of detectors that should be included", | |
| action="store", | |
| dest="detectors_to_include", | |
| default=defaults_flag_in_config["detectors_to_include"], | |
| ) |
nsiregar
force-pushed
the
add-detectors-to-include-override-exclude-args
branch
from
1b5210f
to
68a289b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (4)
slither/utils/command_line.py (4)
51-51: Add a comment explaining the purpose ofdetectors_to_include.It's good practice to document new configuration options to help future developers understand their purpose quickly.
Line range hint
126-126: Usenot infor membership testing to improve readability.- if key not in defaults_flag_in_config: + if key not in defaults_flag_in_config:This change makes the condition clearer and aligns with Pythonic best practices.
Line range hint
157-157: Rename the variableltolinefor better readability.- for l in lines: + for line in lines:Using descriptive variable names improves code readability and maintainability.
Line range hint
240-240: Usenot infor membership testing to improve readability.- if not filter_wiki in detector.WIKI: + if filter_wiki not in detector.WIKI:This change makes the condition clearer and aligns with Pythonic best practices.
nsiregar
force-pushed
the
add-detectors-to-include-override-exclude-args
branch
from
68a289b
to
57dbd17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
slither/utils/command_line.py (5)
51-51: Add documentation for the newdetectors_to_includeargument.It's important to maintain comprehensive documentation, especially for new features. Consider adding a comment here explaining the purpose and usage of
detectors_to_include.
Line range hint
126-126: Usenot infor membership testing to improve readability.- if key in defaults_flag_in_config: + if key not in defaults_flag_in_config:This change makes the conditional check more intuitive and aligns with common Python practices.
Line range hint
157-157: Consider renaming the variablelto a more descriptive name.- for l in lines: + for line in lines:Using
lineinstead oflimproves readability and reduces ambiguity in the code.
Line range hint
171-171: Consider renaming the variablelto a more descriptive name.- next_level = get_level(l) + next_level = get_level(line)Using
lineinstead oflimproves readability and reduces ambiguity in the code.
Line range hint
240-240: Usenot infor membership testing to improve readability.- if not filter_wiki in detector.WIKI: + if filter_wiki not in detector.WIKI:This change makes the conditional check more intuitive and aligns with common Python practices.
nsiregar
force-pushed
the
add-detectors-to-include-override-exclude-args
branch
from
57dbd17
to
e2daa8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
slither/utils/command_line.py (5)
51-51: Add a brief comment explaining the purpose ofdetectors_to_include.Adding a comment here would improve code readability and maintainability by clarifying the purpose and usage of this new configuration option.
Line range hint
126-126: Change membership test to usenot infor clarity.- if key not in defaults_flag_in_config: + if key not in defaults_flag_in_config:This change clarifies the intent of the condition, making it easier to understand that it checks for the absence of
keyindefaults_flag_in_config.
Line range hint
157-157: Consider renaming the variableltolinefor better readability.- for l in lines: + for line in lines:Renaming
ltolineenhances readability and reduces ambiguity, making the code easier to understand and maintain.
Line range hint
171-171: Consider renaming the variableltolinefor better readability.- for l in lines: + for line in lines:Renaming
ltolineenhances readability and reduces ambiguity, making the code easier to understand and maintain.
Line range hint
240-240: Change membership test to usenot infor clarity.- if not filter_wiki in detector.WIKI: + if filter_wiki not in detector.WIKI:This change clarifies the intent of the condition, making it easier to understand that it checks for the absence of
filter_wikiindetector.WIKI.
Add
detectors_to_includeargs to overrideexclude_*argsCloses #2429
Summary by CodeRabbit
New Features
Enhancements
Exampledetector with comprehensive wiki information.Bug Fixes
Documentation