-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(forge lint): unused imports #10662
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
base: master
Are you sure you want to change the base?
Conversation
hey @0xrusowsky anything else needed for this one or can go to review? |
@grandizzy Dani said that it could be easier to implement this feature in solar directly. I was waiting for him to provide some guidance on how to do it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is better suited as a builtin lint in solar because its easier to implement during lowering inside of the internal data structures. but because the lint infrastructure is present only in foundry, for now we can store the unused imports as structs in gcx, and gated with a -Z flag that's enabled by forge lint. this way we can also implement it and test it directly in solar test files
I have a vibe coded WIP branch here paradigmxyz/solar@main...dani/unused-imports, I'll try to get this over the line
i see, makes total sense! |
on second thought, this can be implemented using only the AST, like in solhint: https://github.com/protofire/solhint/blob/4dd80890cff0d00051f67f8133877f26482588bb/lib/rules/best-practices/no-unused-import.js#L44 I've done that here in 2 passes: https://github.com/paradigmxyz/solar/blob/c03deb7132224167398f6b829d3c1c88946a1c1e/crates/sema/src/ast_lowering/unused.rs#L7 I think for now it's ok if you just copy the implementation in and we worry about upstreaming it later when we also upstream the lint infra |
solar-ast = { git = "https://github.com/paradigmxyz/solar.git", branch = "main" } | ||
solar-parse = { git = "https://github.com/paradigmxyz/solar.git", branch = "main" } | ||
solar-interface = { git = "https://github.com/paradigmxyz/solar.git", branch = "main" } | ||
solar-sema = { git = "https://github.com/paradigmxyz/solar.git", branch = "main" } | ||
solar-data-structures = { git = "https://github.com/paradigmxyz/solar.git", branch = "main" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with pinning to main. Context here: #10662 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
ci fixes and conflict resolution remain
…ndry into chore/lint-unused-imports
Adds a new lint rule for unused imports.
ref:
forge lint
): new lint rule: "unused imports" #10633 (comment)Closes #10633