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

Filtering #2428

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Filtering #2428

wants to merge 3 commits into from

Conversation

DarkaMaul
Copy link
Contributor

Adds three new options to the Slither CLI to filter results.

This PR adds granularity in the filtering system provided by slither on which files, contracts or function to run.

Help

➜ slither --help

...

Filtering:
  The following options allow to control which files will be analyzed by Slither. While the initial steps (parsing, generating the IR) are done on the full project, the
  target for the detectors and/or printers can be restricted using these options. The filtering allow to to select directories, files, contract and functions. Each part
  is compiled as a regex (so A*\.sol) will match every file that starts with A and ends with .sol. 
Examples :
 - `sub1/A.sol:A.constructor` will analyze only the
  function named constructor in the contract A found in file A.sol a directory sub1. 
- `sub1/.*` will analyze all files found in the directory sub1/ 
- `.*:A` will analyze all the contract named A

  --include INCLUDE     Include directory/files/contract/functions and only run the analysis on the specified elements.
  --remove REMOVE       Exclude directory/files/contract/functions and only run the analysis on the specified elements.
  --filter FILTER       Include/Exclude directory/files/contract/functions and only run the analysis on the specified elements. Prefix by +_(or nothing) to include, and by - to exclude.

Implementation details

  1. Migrate filter-paths, include-paths option to the new system and add a deprecation warning

The previous options are kept for backward compatibility but they are removed from the help output
They are migrated to the new filtering system.

  1. Filtering

The filtering is done at the CompilationUnit level, the contracts, and functions method now only return the restricted results instead of the complete results.
This way the impact on current detectors and/or printers is kept minimal.

  1. Filtering syntax

We use a comma separted list of values with this format :

<+-><path/subdir/file>:<contract>.<function>

The leading +- can be used for the filter option to list positive and negative rules.

Each element is compiled as a regex so the following examples work:

  • .*:A.*: Matches every contract that contains an A
  • +sub1/,-sub1/sub12 : Matches everything in sub1 that is not in sub2.

Of note, the exact regex is:

    regex = re.compile(
        r"^(?P<type>[+-])?(?P<path>[^:]+?)(?::(?P<contract>[^.])(?:\.(?P<function>.+)?)?)?$"
    )

And the tests were made using here

Adds three new options to the Slither CLI to filter results.
Copy link

coderabbitai bot commented Apr 17, 2024

Important

Auto Review Skipped

Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

@DarkaMaul
Copy link
Contributor Author

Some note :

The filtering system improve the overall performances because the printers / detectors are not run on code before being filtered out. However, some properties could be cached to further improve performance. This will be tacked in another PR with a better focus on performance.

@0xalpharush
Copy link
Contributor

I think for directory/file we may be able to also avoid lowering the AST to SlithIR by filtering out them here:

slither/slither/slither.py

Lines 151 to 170 in fdf54f6

# Solidity specific
assert compilation_unit_slither.is_solidity
sol_parser = SlitherCompilationUnitSolc(compilation_unit_slither)
self._parsers.append(sol_parser)
for path, ast in compilation_unit.asts.items():
sol_parser.parse_top_level_items(ast, path)
self.add_source_code(path)
for contract in sol_parser._underlying_contract_to_parser:
if contract.name.startswith("SlitherInternalTopLevelContract"):
raise SlitherError(
# region multi-line-string
"""Your codebase has a contract named 'SlitherInternalTopLevelContract'.
Please rename it, this name is reserved for Slither's internals"""
# endregion multi-line
)
sol_parser._contracts_by_id[contract.id] = contract
sol_parser._compilation_unit.contracts.append(contract)
_update_file_scopes(sol_parser)

Need to keep in mind the references to other code may necessitate its analysis even if it's filtered

@DarkaMaul
Copy link
Contributor Author

I think for directory/file we may be able to also avoid lowering the AST to SlithIR by filtering out them here:

slither/slither/slither.py

Lines 151 to 170 in fdf54f6

# Solidity specific
assert compilation_unit_slither.is_solidity
sol_parser = SlitherCompilationUnitSolc(compilation_unit_slither)
self._parsers.append(sol_parser)
for path, ast in compilation_unit.asts.items():
sol_parser.parse_top_level_items(ast, path)
self.add_source_code(path)
for contract in sol_parser._underlying_contract_to_parser:
if contract.name.startswith("SlitherInternalTopLevelContract"):
raise SlitherError(
# region multi-line-string
"""Your codebase has a contract named 'SlitherInternalTopLevelContract'.
Please rename it, this name is reserved for Slither's internals"""
# endregion multi-line
)
sol_parser._contracts_by_id[contract.id] = contract
sol_parser._compilation_unit.contracts.append(contract)
_update_file_scopes(sol_parser)

Need to keep in mind the references to other code may necessitate its analysis even if it's filtered

I tried to solve it in 6c8ee23 but I think it would require a bigger refactoring of the parsing step.

Indeed, this fails the step for the test_using_for_global_collision, and I did not find a easy way of fixing it.

@0xalpharush
Copy link
Contributor

Yeah, it can't be filtered before _update_file_scopes is called bc until then we don't know the defintion-reference dependencies

@0xalpharush
Copy link
Contributor

I think the syntax will be difficult for users. Even the existing use of regex has been confusing for many. Ideally we would have something that is familiar i.e. people picked up how to use it from another common CLI and its intuitive/ easy to debug.

<+-><path/subdir/file>:.

I also don't love the alternative give in #759 but it is simpler in more straightforward:

--exclude-filenames / --include-filenames
--exclude-contracts / --include-contracts
--exclude-functions / --include-functions.

@0xalpharush 0xalpharush marked this pull request as draft June 4, 2024 20:06
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.

2 participants