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

[ New Rule ] No unused imports #417

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

juanpcapurro
Copy link
Contributor

@juanpcapurro juanpcapurro commented Feb 28, 2023

I've modified both openzeppelin-contracts and exactly to manually add this rule and check for false positives, couldn't find any in the current implementation. However, I'm concerned I might be forgetting to register some usage regarding inline assembly or some other less-used feature, so feedback is of course appreciated on that.

as always, see git history for details.

@dbale-altoros dbale-altoros changed the title No unused imports [ New Rule ] No unused imports Mar 6, 2023
@dbale-altoros
Copy link
Collaborator

@juanpcapurro This is a cool rule! Thanks for that.
This is marked as draft... not sure if it's not finished yet, or you put it that way because it depends on the previous PR

@juanpcapurro
Copy link
Contributor Author

juanpcapurro commented Mar 6, 2023

@juanpcapurro This is a cool rule! Thanks for that. This is marked as draft... not sure if it's not finished yet, or you put it that way because it depends on the previous PR

Hi Diego, this PR is not in a mergable state sadly, since it still has many false positives yet. so far I've found the implementation errors out when an imported name is used as:

  • type for
    • variable inside a function
    • struct definition
  • value for
    • revert statement
    • accessing one if its members

and also has the following errors:

  • reports original name when aliased name is not used

so a full review isn't in order yet. However, I'd of course greatly appreciate feedback on how I'm approaching the fix, if I'm organizing the code right, those kinds of things.

Small chores, such as not repeating myself for the different errors that have the same message, I'll probably fix later when I have the entire feature figured out though.

edit: the way I'm looking for false positives is by running the linter against a 'real world' codebase and checking manually. I'd love to hear on better ways to do it (either because it's faster or because the assertions can more easily be part of our codebase) if you have any ideas

@dbale-altoros
Copy link
Collaborator

Ok, no worries... You made such great contributions here... I cannot say other thing than thank you.
I don't know how much time I will be given to approach next tasks on solhint... as soon as I have some spare time I'll try to give you some feedback (if any)

recognize usages in:
- using Library for Type
- variable definitions
- member access
- type cast (through FunctionCall AST node)
- revert value (through FunctionCall AST node)
- New expressions
- array type definitions

recognizing & reporting the  aliased import where applicable
@juanpcapurro juanpcapurro marked this pull request as ready for review March 9, 2023 23:22
@juanpcapurro
Copy link
Contributor Author

@fvictorio @dbale-altoros ✨ should be ready for review now ✨

Copy link
Collaborator

@dbale-altoros dbale-altoros left a comment

Choose a reason for hiding this comment

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

I tested it and seems pretty tight

@dbale-altoros dbale-altoros merged commit bad43a2 into protofire:master Jun 28, 2023
@dbale-altoros dbale-altoros self-assigned this Jun 28, 2023
@0xCourtney
Copy link

Great to see this rule being added, any idea on when the next release will be?

@juanpcapurro
Copy link
Contributor Author

Great to see this rule being added, any idea on when the next release will be?

This rule is already released in solhint-community 3.5.1, where a few devs and myself are continuing development

@dbale-altoros
Copy link
Collaborator

@0xCourtney
We are going to fix as many issues as we can to provide an updated version
I have no fix date for next release but it will be soon

@dbale-altoros
Copy link
Collaborator

@juanpcapurro Como estas ?
Te consulto algo.
Veo que esta regla asi como esta codeada tiene varios falsos positivos que fueron arreglados en la version de community
Te molestaria si tomo los fixes de ese repo para esta regla asi la dejamos bien aca tambien ?

Gracias y disculpa la molestia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants