[pyflakes] Fix infinite loop with unused local import in __init__.py (F401)#15517
[pyflakes] Fix infinite loop with unused local import in __init__.py (F401)#15517
pyflakes] Fix infinite loop with unused local import in __init__.py (F401)#15517Conversation
this fixes the existing tests that the first fix broke
|
MichaReiser
left a comment
There was a problem hiding this comment.
I'd prefer if we can move the test closer to the rule rather than making it a CLI tests. We only use CLI tests for features that can't be tested otherwise but we prefer unit tests or tool specific integration tests for everything else because they're faster to run, easier to find, and there's less code "between" the test and the implementation if something goes wrong.
I'm requesting review from @AlexWaygood to review the rule behavior change.
|
Nice diagnosis! The change to the rule's semantics look correct to me. It's nice that this is a targeted fix as well. I think I tried to fix this a while back, but made the mistake of trying to untangle some of the more spaghetti-code-like parts of this rule at the same time, and got bogged down in a tricky refactor that was hard to pull off. I agree that I think it should be possible to test this fix using fixtures rather than a CLI integration test. We already have several mock packages set up here that we use for testing F401; can we create another mock package for this scenario? https://github.com/astral-sh/ruff/tree/main/crates/ruff_linter/resources/test/fixtures/pyflakes. The actual tests that use the fixtures are here: https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/rules/pyflakes/mod.rs |
|
Thank you both for the suggestions! For some reason I immediately assumed that multiple files meant I needed an integration test, but these examples were very helpful. I first made the unit test changes on |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Summary
This fixes the infinite loop reported in #12897, where an
unused-importthat is undefined at the scope of__all__is "fixed" by adding it to__all__repeatedly. These changes make it so that only imports in the global scope will be suggested to add to__all__and the unused local import is simply removed.Test Plan
Added a CLI integration test that sets up the same module structure as the original report
Closes #12897