Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add detectors to include override exclude args #2440

Conversation

nsiregar
Copy link
Contributor

@nsiregar nsiregar commented Apr 23, 2024

Add detectors_to_include args to override exclude_* args

Closes #2429

Summary by CodeRabbit

  • New Features

    • Enhanced detector selection flexibility in the main application based on user preferences.
    • Introduced logging in the EVM summary printer for improved debugging.
    • Added new configuration options for failure levels in end-to-end tests.
  • Enhancements

    • Updated the Example detector with comprehensive wiki information.
    • Improved handling of unused state variables with special conditions for abstract contracts.
  • Bug Fixes

    • Corrected typos in various modules and comments for clarity.
    • Added detection of specific unused variables in test snapshots.
  • Documentation

    • Updated comments and documentation across various files to correct typos and improve clarity.

Copy link

coderabbitai bot commented Apr 23, 2024

Walkthrough

Walkthrough

The 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

Files Change Summary
plugin_example/setup.py Package name changed, dependency version updated.
plugin_example/.../example.py Updated WIKI attributes in Example detector class.
slither/__main__.py Added __include_detectors function for dynamic detector management.
slither/core/.../function.py Typo corrected in comments.
slither/detectors/.../naming_convention.py, .../divide_before_multiply.py, .../convert.py, .../analysis.py Typo corrections in comments.
slither/detectors/.../unused_state_variables.py New condition for unused variable detection in abstract contracts.
slither/printers/.../evm.py Added logging capabilities.
slither/utils/command_line.py Modified FailOnLevel class and updated configuration handling.
tests/e2e/.../slither.config.json, .../unused_state_sol__0.txt Configuration and test output adjustments.
tests/e2e/.../unused_state.sol Added new library, abstract contracts, and inheritance structure in test data.
tests/unit/.../test_ssa_generation.py Documentation typo corrected.

Assessment against linked issues

Objective Addressed Explanation
Add detectors_to_include functionality

Recent Review Details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits Files that changed from the base of the PR and between ded705d and e2daa8a.
Files selected for processing (2)
  • slither/main.py (3 hunks)
  • slither/utils/command_line.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • slither/main.py
Additional Context Used
Ruff (4)
slither/utils/command_line.py (4)

126-126: Test for membership should be not in


157-157: Ambiguous variable name: l


171-171: Ambiguous variable name: l


240-240: Test for membership should be not in


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@nsiregar nsiregar changed the base branch from master to dev April 23, 2024 00:29
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Actionable comments outside the diff hunks (3)
slither/slithir/convert.py (3)

Line range hint 354-354: Use not in instead of in for 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: Use not in instead of in for 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: Use not in instead of in for 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:

slither/__main__.py Outdated Show resolved Hide resolved
slither/__main__.py Outdated Show resolved Hide resolved
@nsiregar nsiregar force-pushed the add-detectors-to-include-override-exclude-args branch from 6d92113 to 002b06b Compare April 23, 2024 00:47
@DarkaMaul
Copy link
Contributor

Of note, this will clash with the switches introduced by #2428 ( --include , --remove).

@nsiregar nsiregar force-pushed the add-detectors-to-include-override-exclude-args branch from 002b06b to 03487e8 Compare April 23, 2024 16:21
@nsiregar
Copy link
Contributor Author

switch updated to --include-detectors instead --include only

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Out of diff range and nitpick comments (4)
slither/utils/command_line.py (4)

Line range hint 126-126: Use not in for 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 variable l to a more descriptive name.

- for l in lines:
+ for line in lines:

Line range hint 171-171: Consider renaming the variable l to a more descriptive name for consistency.

- for l in lines:
+ for line in lines:

Line range hint 240-240: Use not in for membership testing to improve clarity.

- if detector in detectors:
+ if detector not in detectors:

Comment on lines 264 to 260
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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Comment on lines 434 to 423
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"],
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 nsiregar force-pushed the add-detectors-to-include-override-exclude-args branch from 1b5210f to 68a289b Compare April 26, 2024 17:43
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of detectors_to_include.

It's good practice to document new configuration options to help future developers understand their purpose quickly.


Line range hint 126-126: Use not in for 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 variable l to line for better readability.

- for l in lines:
+ for line in lines:

Using descriptive variable names improves code readability and maintainability.


Line range hint 240-240: Use not in for 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 nsiregar force-pushed the add-detectors-to-include-override-exclude-args branch from 68a289b to 57dbd17 Compare April 26, 2024 18:07
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Out of diff range and nitpick comments (5)
slither/utils/command_line.py (5)

51-51: Add documentation for the new detectors_to_include argument.

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: Use not in for 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 variable l to a more descriptive name.

- for l in lines:
+ for line in lines:

Using line instead of l improves readability and reduces ambiguity in the code.


Line range hint 171-171: Consider renaming the variable l to a more descriptive name.

- next_level = get_level(l)
+ next_level = get_level(line)

Using line instead of l improves readability and reduces ambiguity in the code.


Line range hint 240-240: Use not in for 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 nsiregar force-pushed the add-detectors-to-include-override-exclude-args branch from 57dbd17 to e2daa8a Compare April 26, 2024 18:17
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of detectors_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 use not in for 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 key in defaults_flag_in_config.


Line range hint 157-157: Consider renaming the variable l to line for better readability.

- for l in lines:
+ for line in lines:

Renaming l to line enhances readability and reduces ambiguity, making the code easier to understand and maintain.


Line range hint 171-171: Consider renaming the variable l to line for better readability.

- for l in lines:
+ for line in lines:

Renaming l to line enhances readability and reduces ambiguity, making the code easier to understand and maintain.


Line range hint 240-240: Change membership test to use not in for 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_wiki in detector.WIKI.

@0xalpharush 0xalpharush merged commit 7d3b73c into crytic:dev Jun 4, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add detectors_to_include which overrides exclude_*
3 participants