F401 - Recommend adding unused import bindings to `all` #11314

plredmond · 2024-05-06T19:16:09Z

Followup on #11168 and resolve #10391

User facing changes

F401 now recommends a fix to add unused import bindings to to __all__ if a single __all__ list or tuple is found in __init__.py.
- If there are no __all__ found in the file, fall back to recommending redundant-aliases.
- If there are multiple __all__ or only one but of the wrong type (non list or tuple) then diagnostics are generated without fixes.
fix_title is updated to reflect what the fix/recommendation is.

Subtlety: For a renamed import such as import foo as bees, we can generate a fix to add bees to __all__ but cannot generate a fix to produce a redundant import (because that would break uses of the binding bees).

Implementation changes

Add name field to ImportBinding to contain the name of the binding we want to add to __all__ (important for the import foo as bees case). It previously only contained the AnyImport which can give us information about the import but not the binding.
- Add binding field to UnusedImport to contain the same. (Naming note: the field name field already existed on UnusedImport and contains the qualified name of the imported symbol/module)
Change fix_by_reexporting to branch on the size of dunder_all: Vec<&Expr>
- For length 0 call the edit-producing function make_redundant_alias.
- For length 1 call edit-producing function add_to_dunder_all.
- Otherwise, produce no fix.
Implement the edit-producing function add_to_dunder_all and add unit tests.
Implement several fixture tests: empty __all__ = [], nonempty __all__ = ["foo"], mis-typed __all__ = None, plus-eq __all__ += ["foo"]
UnusedImportContext::Init variant now has two fields: whether the fix is in __init__.py and how many __all__ were found.

Other changes

Remove a spurious pattern match and instead use field lookups b/c the addition of a field would have required changing the unrelated pattern.
Tweak input type of make_redundant_alias

plredmond · 2024-05-06T19:19:45Z

Oops, didn't add clippy to my pre-push hook. Added now.

github-actions · 2024-05-06T19:29:12Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ruff_linter/src/fix/edits.rs

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs

crates/ruff_linter/src/fix/edits.rs

plredmond · 2024-05-07T21:53:13Z

I swear I have fixed my pre-push hook now.

crates/ruff_linter/src/fix/edits.rs

...s/ruff_linter__rules__pyflakes__tests__preview__F401_F401_27__all_mistyped____init__.py.snap

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_29__all_pluseq/__init__.py

crates/ruff_linter/src/fix/edits.rs

...lakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_29__all_pluseq/__init__.py

plredmond · 2024-05-09T16:25:23Z

Almost there, maybe? @zanieb @AlexWaygood Two unresolved threads left.

plredmond · 2024-05-09T17:37:16Z

I believe that i've resolved the outstanding requests. @AlexWaygood it might make sense to do a pass over the fixture tests now since there has been some churn, to see if you agree with how each case is resolved?

AlexWaygood

Much improved, thanks for your patience! A few more comments below.

...linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_5.py.snap

...lakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap

...s/ruff_linter__rules__pyflakes__tests__preview__F401_F401_25__all_nonempty____init__.py.snap

crates/ruff_linter/src/fix/edits.rs

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs

crates/ruff_linter/src/fix/edits.rs

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs

plredmond · 2024-05-13T18:58:02Z

This is ready for review again.

zanieb

Awesome, this is a huge improvement to this rule 🎉

crates/ruff_linter/src/fix/edits.rs

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_26__all_empty/__init__.py

charliermarsh · 2024-05-14T14:47:56Z

(A few small comments but for clarity no need to wait on my approval to merge.)

codspeed-hq · 2024-05-14T17:35:19Z

CodSpeed Performance Report

Merging #11314 will degrade performances by 6.83%

_{Comparing ruff.all (dde9756) with main (96f6288)}

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`ruff.__all__`	Change
❌	`linter/default-rules[pydantic/types.py]`	7.5 ms	8 ms	-6.83%

charliermarsh · 2024-05-14T17:37:49Z

I suspect the CodSpeed regression is a false positive.

plredmond · 2024-05-14T17:38:42Z

Maybe it'll re-run after this last pull and resolve (if codespeed updates its comments).

AlexWaygood · 2024-05-14T17:39:16Z

Rebasing or merging in main often gets CodSpeed to change its mind as well

plredmond · 2024-05-14T17:40:27Z

Ah, that's a good point. If we're comparing to main the performance could very well be different on an outdated branch.

This reverts commit 31768b6.

… smaller

…ove unnecessary import paths

plredmond · 2024-05-14T17:43:55Z

Rebasing.

plredmond · 2024-05-14T17:56:24Z

I've updated the PR description to reflect the diff after all review comments.

Not sure what to do next. CodeSpeed might be a false-positive, but that should have been mitigated by rebasing.

charliermarsh · 2024-05-14T18:00:22Z

crates/ruff_linter/src/fix/edits.rs

+                &lexer::lex(raw, Mode::Expression).collect::<Vec<_>>(),
+                &locator,
+            );
+            // SUT


What does SUT mean?

system under test -- the test is long so i'm marking the bit that's unit tested

This acronym was also unfamiliar to me FWIW

charliermarsh · 2024-05-14T18:01:01Z

I think it's ok to merge personally.

charliermarsh · 2024-05-14T18:01:35Z

I'm looking through the changes again just to see if anything sticks out.

charliermarsh · 2024-05-14T18:02:59Z

The only thing that stands out is that we're allocating an additional string for UnusedImport and we increased the size of the struct.

charliermarsh · 2024-05-14T18:06:15Z

We could consider using binding everywhere for the diagnostic instead of the qualified name, see if that helps? But I think it's fine to merge as-is.

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

…e iterator" This reverts commit 548180b.

…ding-name field (named as .name); update fixture test results

plredmond · 2024-05-14T18:35:19Z

Trying out Charlie's suggestion of using only one string in the violation struct. This caused /a lot/ of fixture changes to have cosmetic changes to help text and rule titles. If it doesn't correct the regression I'll revert.

plredmond · 2024-05-14T20:52:49Z

Ok, that didn't work so I'm going to revert and then merge once tests pass.

… the binding-name field (named as .name); update fixture test results" This reverts commit 0696df5.

plredmond · 2024-05-14T21:47:03Z

Ok! I've tried (and reverted) both of the suggested performance improvements. Do we want to just merge this? I have no more commits to push.

charliermarsh · 2024-05-14T22:56:31Z

Yeah, I'm fine with going ahead and merging.

#11436) ## Summary * Update documentation for F401 following recent PRs * #11168 * #11314 * Deprecate `ignore_init_module_imports` * Add a deprecation pragma to the option and a "warn user once" message when the option is used. * Restore the old behavior for stable (non-preview) mode: * When `ignore_init_module_imports` is set to `true` (default) there are no `__init_.py` fixes (but we get nice fix titles!). * When `ignore_init_module_imports` is set to `false` there are unsafe `__init__.py` fixes to remove unused imports. * When preview mode is enabled, it overrides `ignore_init_module_imports`. * Fixed a bug in fix titles where `import foo as bar` would recommend reexporting `bar as bar`. It now says to reexport `foo as foo`. (In this case we don't issue a fix, fwiw; it was just a fix title bug.) ## Test plan Added new fixture tests that reuse the existing fixtures for `__init__.py` files. Each of the three situations listed above has fixture tests. The F401 "stable" tests cover: > * When `ignore_init_module_imports` is set to `true` (default) there are no `__init_.py` fixes (but we get nice fix titles!). The F401 "deprecated option" tests cover: > * When `ignore_init_module_imports` is set to `false` there are unsafe `__init__.py` fixes to remove unused imports. These complement existing "preview" tests that show the new behavior which recommends fixes in `__init__.py` according to whether the import is 1st party and other circumstances (for more on that behavior see: #11314).

plredmond marked this pull request as ready for review May 6, 2024 20:07

MichaReiser reviewed May 7, 2024

View reviewed changes

crates/ruff_linter/src/fix/edits.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed May 7, 2024

View reviewed changes

crates/ruff_linter/src/fix/edits.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed May 7, 2024

View reviewed changes

crates/ruff_linter/src/fix/edits.rs Outdated Show resolved Hide resolved

plredmond requested a review from AlexWaygood May 8, 2024 16:35

AlexWaygood reviewed May 8, 2024

View reviewed changes

...lakes/snapshots/ruff_linter__rules__pyflakes__tests__preview__F401_F401_24____init__.py.snap Outdated Show resolved Hide resolved

AlexWaygood reviewed May 9, 2024

View reviewed changes

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_29__all_pluseq/__init__.py Outdated Show resolved Hide resolved

AlexWaygood reviewed May 10, 2024

View reviewed changes

zanieb mentioned this pull request May 12, 2024

Support for ruff to automatically resolve __all__ statements. #11373

Closed

plredmond requested a review from zanieb May 13, 2024 18:57

zanieb approved these changes May 14, 2024

View reviewed changes

charliermarsh reviewed May 14, 2024

View reviewed changes

crates/ruff_linter/src/fix/edits.rs Outdated Show resolved Hide resolved

charliermarsh reviewed May 14, 2024

View reviewed changes

crates/ruff_linter/src/fix/edits.rs Outdated Show resolved Hide resolved

charliermarsh reviewed May 14, 2024

View reviewed changes

crates/ruff_linter/resources/test/fixtures/pyflakes/F401_26__all_empty/__init__.py Outdated Show resolved Hide resolved

plredmond added 3 commits May 14, 2024 10:43

Revert "patch to remove the unfinished dunder_all mutation"

fe7016e

This reverts commit 31768b6.

charlie patch w/proto dunder_all finder

41584e1

find_dunder_all_expr implemented

61cca6c

plredmond added 6 commits May 14, 2024 10:43

remove a now unnecessary branch, which also makes the overall PR diff…

117a1f0

… smaller

call .end() directly instead of going through .range or .range(); rem…

1b167e6

…ove unnecessary import paths

readd erroneously removed newlines to fixture snaps

26d4714

ruff format the fixture __init__.py files

5dbdcfe

cargo insta review -- accept whitespace changes (see git diff -w)

64fa9af

clippy fix -- replace closure with static reference to method

527d395

plredmond force-pushed the ruff.__all__ branch from 31dd04d to 527d395 Compare May 14, 2024 17:43

charliermarsh reviewed May 14, 2024

View reviewed changes

AlexWaygood reviewed May 14, 2024

View reviewed changes

crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs Show resolved Hide resolved

plredmond and others added 3 commits May 14, 2024 11:20

collect only 2 __all__ definitions and then stop pulling the iterator

548180b

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Revert "collect only 2 __all__ definitions and then stop pulling th…

01e974f

…e iterator" This reverts commit 548180b.

remove the qualified-name field from UnusedImport in favor of the bin…

0696df5

…ding-name field (named as .name); update fixture test results

Revert "remove the qualified-name field from UnusedImport in favor of…

dde9756

… the binding-name field (named as .name); update fixture test results" This reverts commit 0696df5.

plredmond merged commit da882b6 into main May 15, 2024
18 of 19 checks passed

plredmond deleted the ruff.__all__ branch May 15, 2024 00:02

plredmond added a commit that referenced this pull request May 15, 2024

docs for updated F401 behavior #11168,#11314

410af7a

plredmond added a commit that referenced this pull request May 15, 2024

docs for updated F401 behavior #11168,#11314

e78fee6

plredmond mentioned this pull request May 15, 2024

F401 - update documentation and deprecate ignore_init_module_imports #11436

Merged

BrewTestBot mentioned this pull request May 23, 2024

ruff 0.4.5 Homebrew/homebrew-core#172580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

F401 - Recommend adding unused import bindings to `all` #11314

F401 - Recommend adding unused import bindings to `all` #11314

plredmond commented May 6, 2024 •

edited

Loading

plredmond commented May 6, 2024

github-actions bot commented May 6, 2024 •

edited

Loading

plredmond commented May 7, 2024

plredmond commented May 9, 2024

plredmond commented May 9, 2024

AlexWaygood left a comment

plredmond commented May 13, 2024

zanieb left a comment

charliermarsh commented May 14, 2024

codspeed-hq bot commented May 14, 2024 •

edited

Loading

charliermarsh commented May 14, 2024

plredmond commented May 14, 2024

AlexWaygood commented May 14, 2024 •

edited

Loading

plredmond commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

charliermarsh May 14, 2024

plredmond May 14, 2024 •

edited

Loading

AlexWaygood May 14, 2024

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

charliermarsh commented May 14, 2024

F401 - Recommend adding unused import bindings to __all__ #11314

F401 - Recommend adding unused import bindings to __all__ #11314

Conversation

plredmond commented May 6, 2024 • edited Loading

User facing changes

Implementation changes

Other changes

plredmond commented May 6, 2024

github-actions bot commented May 6, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

plredmond commented May 7, 2024

plredmond commented May 9, 2024

plredmond commented May 9, 2024

AlexWaygood left a comment

Choose a reason for hiding this comment

plredmond commented May 13, 2024

zanieb left a comment

Choose a reason for hiding this comment

charliermarsh commented May 14, 2024

codspeed-hq bot commented May 14, 2024 • edited Loading

CodSpeed Performance Report

Merging #11314 will degrade performances by 6.83%

Summary

Benchmarks breakdown

charliermarsh commented May 14, 2024

plredmond commented May 14, 2024

AlexWaygood commented May 14, 2024 • edited Loading

plredmond commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

charliermarsh May 14, 2024

Choose a reason for hiding this comment

plredmond May 14, 2024 • edited Loading

Choose a reason for hiding this comment

AlexWaygood May 14, 2024

Choose a reason for hiding this comment

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

charliermarsh commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

plredmond commented May 14, 2024

charliermarsh commented May 14, 2024

F401 - Recommend adding unused import bindings to `all` #11314

F401 - Recommend adding unused import bindings to `all` #11314

plredmond commented May 6, 2024 •

edited

Loading

github-actions bot commented May 6, 2024 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented May 14, 2024 •

edited

Loading

AlexWaygood commented May 14, 2024 •

edited

Loading

plredmond May 14, 2024 •

edited

Loading