Consider submodule imports to detect unused imports#5011
Consider submodule imports to detect unused imports#5011dhruvmanila wants to merge 3 commits intomainfrom
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
dhruvmanila
force-pushed
the
dhruv/unused-imports
branch
from
16e44fb to
059ba0c
Compare
PR Check ResultsBenchmarkLinuxWindows |
|
So, it seems that there are some conflicting behavior between import foo
import foo.bar
import foo
import bar as fooI would argue to actually mark both as unused import similar to $ ruff check --no-cache --isolated --select=F401,F811 src/F401.py
src/F401.py:2:8: F811 Redefinition of unused `foo` from line 15
src/F401.py:2:8: F401 [*] `foo` imported but unused
Found 2 errors.
[*] 1 potentially fixable with the --fix option. |
|
Hmm, I'm not totally certain on the right behavior here. Given the example from #60 import multiprocessing.pool
import multiprocessing.process
z = multiprocessing.pool.ThreadPool()Wouldn't we end up flagging |
@charliermarsh Do you mean But to answer your question, no it won't flag them. I've added a test case for the mentioned case: https://github.com/astral-sh/ruff/pull/5011/files#diff-cdbee39a5dfd985e12efeff0edaae1b8f79c8fdc2655e64ad12160cf84659930 (Sorry for the delay, I missed this in the notifications) |
| /// A map from binding ID to bound name. | ||
| binding_name: FxHashMap<BindingId, &'a str>, | ||
|
|
There was a problem hiding this comment.
Hash maps are kind of expensive (48 bytes or something in size and require an allocation per scope). Could we instead move the name to Binding, recompute it on demand (implement a helper on binding that uses BindingId to extract the name, I would prefer this approach), or store it as a another IndexVec<BindingId, &'str> on the semantic model (since we need an entry for every name).
|
@charliermarsh Any thoughts on how to proceed here? I'm totally fine with rejecting this solution altogether :) |
dhruvmanila
requested review from
charliermarsh
and removed request for
charliermarsh
|
@dhruvmanila - Sorry for the delay (again), but let's close this out for now. I appreciate the work but submodule imports are complicated and I'm having trouble reasoning about the change, whether it's correct, and how it should be reflected in other rules (no fault of yours -- it just requires some investigation on my part, and I don't feel it's worth investing time in right now). |
|
Hey, I totally understand 👍 |
…ed-import` (`F401`) (#20200) # Summary The PR under review attempts to make progress towards the age-old problem of submodule imports, specifically with regards to their treatment by the rule [`unused-import` (`F401`)](https://docs.astral.sh/ruff/rules/unused-import/). Some related issues: - #60 - #4656 Prior art: - #13965 - #5010 - #5011 - #666 See the PR summary for a detailed description.
3 participants
Summary
Take the below code as an example:
Here, a single binding is created (
foo) for both imports and the binding created forfoo.barwill shadow the one created forfoo.some. Now, if the bindingfoois unused, then it means all of the bindings shadowing this is unused as well.There's a conflict which I've commented down below. This is resulting in 2 test failures.
Test Plan
cargo testfixes: #60
fixes: #4655