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

[WIP] Tool slither-format: automatic code improvements #238

Open
wants to merge 93 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@rajeevgopalakrishna
Copy link
Collaborator

commented May 10, 2019

This PR (depends on #237 and #236 and replaces PR #235 which was created against a stale dev branch by mistake) addresses #150 to add a new utility tool slither-format which uses slither detectors to identify code patterns of concern (w.r.t security, readability and optimisation) and automatically fix those code patterns with suggested changes.

The current list of detectors used to detect and fix include: unused-state, solc-version, pragma, naming-convention, external-function, constable-states and constant-function.

Detectors highlight names, context and source-mapping of code constructs which are then used by slither-format to programmatically locate those constructs in the Solidity files and then replace them with changes based on best practices. Lexical analysis for identification of such constructs is confined to the smallest possible region to avoid conflicts with similarly named constructs (with potentially different types or signatures) in other scopes, functions or contracts within the same file (because of shadowing, overloading etc.).

Done: Framework and basic functionality. Unit testing.
WIP: More real-world testing and bug-fixes. More tool options.
Known bugs:

  1. naming-convention formatting doesn't work on NatSpec comments, e.g. @param.
  2. Variables used as indices on LHS (e.g. _to in balances[_to] = 100) don't get reported from Slither in variables that are read and hence are not formatted in naming-convention.
    3. Bugs potentially from incorrectly reported source mappings, which might be related to #218.
All changes specific to slither-format tool, excluding the dependenci…
…es on slither parsing/core and detectors. Single commit because of checking out utils/slither_format from dev-slither-format into this branch.

rajeevgopalakrishna added some commits May 13, 2019

Updates format_unused_state to use filename_absolute and apply only t…
…o variable types. test_unused_state_vars passes.
Updates format_solc_version to use filename_absolute and, directive n…
…ame instead of the earlier expression name. test_solc_version passes. Removes patch_file check for now. Need to change verbose output to JSON format and include patch_file then.
Updates format_pragma to use filename_absolute and, directive name in…
…stead of the earlier expression name. test_pragma passes. Removes patch_file check for now. Need to change verbose output to JSON format and include patch_file then.

@rajeevgopalakrishna rajeevgopalakrishna force-pushed the dev-slither-format-tool-only-new branch 2 times, most recently from 86568a6 to ced9498 May 13, 2019

rajeevgopalakrishna added some commits May 13, 2019

Updates format_naming_convention to use filename_absolute and other J…
…SON output field updates. Removed the use of event.full_name to simply use name instead (so PR #236 not required). naming-convention tests pass (except the index variable test which is expected to fail); run_all_tests passes except that one. Testing requires checking out slither core/parsing changes from dev-slither-changes-for-slither-format-new.
Use source_mapping of variables/events directly instead of get_source…
…_event/var_declaration, which will be removed from slither.

@rajeevgopalakrishna rajeevgopalakrishna requested a review from montyly May 15, 2019

@montyly

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Few recommendations:

@rajeevgopalakrishna rajeevgopalakrishna force-pushed the dev-slither-format-tool-only-new branch from 800f375 to 2e93076 May 17, 2019

@montyly

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

What was done:

  • Refactor to remove unnecessary arguments.
  • The patches are generated directly to the detector json results, which will simplify the pipeline
  • difflib is used to generate the patch, rather than diff
  • The patches are only merged if they correspond to the same issue. For now, merging all the fixes of different finding to one patch is disabled; I am not sure it's something we want to promote, as its error-prone. We could re-enable later

Additionally, there is one change to the naming convention detector:

  • allow private and internal functions to start with _

Once thorough tests are performed, we can merge the PR

montyly added some commits Jun 27, 2019

Add a new type of function: constructor_variable to hold state variab…
…le initialization

Add Function.FunctionType to determine if the function is a constructor/fallback/constructor_variables
API change: function.is_fallback() becomes a property
Minor fixes
Fix travis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.