pycore: don't stop source folder detection at the first package - #869
pycore: don't stop source folder detection at the first package#869quazardous wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5GqJKaXZzFzmFEYgo4pPR
|
The original implementation has the advantage that for pure python projects where the root directory is the actual source folder, it would be really fast. The comment in Lines 79 to 90 in 7e04460 This change of always recursing on a package's subdirectories would cause all subfolders in a source folder to have to be scanned all the time to look for potential additional source folders, which potentially can cause some performance issues for projects that contains a huge directory with lots of non-python files, for example, if the project contains That said, this performance issue would have also already existed on projects that uses a non-package I'm not really quite sure what the right solution here should be. On one hand, the fixed behavior is definitely more correct, but springing performance trap to unsuspecting project also doesn't seem ideal either. |
|
Thanks — that's a fair concern. Two facts that may narrow it, then I'll happily follow your lead. The walk is already bounded twice. One caveat on "unusual layouts can set their prefs": On the design itself — I'd rather follow your call than push my patch. But I don't think the current code is cleanly either option: the early
I'm glad to rewrite this for the second option if you prefer it. I'll rebase either way — the conflict is only CHANGELOG/tests, |
`_find_source_folders` returned as soon as a folder had a package among its children, leaving every sibling folder unscanned. On a project whose root holds a package, detection therefore stopped at the root and no other source folder was ever found. That contradicts what the existing tests describe. Both `test_multi_source_folders` and `test_multi_source_folders2` expect two roots side by side -- they only pass because in their layouts no package sits directly at the root, so the early return never fires. The consequence is silent. Imports resolving through the unscanned folders simply do not resolve, so rename skips those occurrences instead of reporting them: a repository-wide rename comes back having quietly renamed a subset. Apply the same rule at every level instead of stopping at the first match, and skip package subfolders when recursing -- descending into a package would make `package.sub` importable as plain `sub`. Measured on a project with two packages at the root and further code under `src/`: 1 source folder detected before, 15 after, and an occurrence that resolved 0 of its 2 references now resolves both. This costs time where it previously skipped work. The same rename went from 3.5s to 43.8s, and touches 19 files rather than 8 -- the added time is spent analysing code that was invisible before, not overhead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5GqJKaXZzFzmFEYgo4pPR
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5GqJKaXZzFzmFEYgo4pPR
7e04460 to
91f6f7a
Compare
The defect
_find_source_foldersreturns as soon as a folder has a package among its children:So on a project whose root holds a package, detection stops at the root and no other source folder is ever found.
Why this looks like a defect rather than a design choice
Two existing tests already describe side-by-side roots:
test_multi_source_folderssrcandtesttest_multi_source_folders2srcBoth pass today only because in their layouts no package sits directly at the root, so the early return never fires. Multi-root detection is clearly intended; the early return silently disables it for any repository that grew a package at its top level.
Why it matters
The consequence is silent, which is the worst part. Imports that would resolve through the unscanned folders simply do not resolve, so
Renameskips those occurrences rather than reporting them — a repository-wide rename comes back having quietly renamed a subset. There is no error and nounsureoccurrence to review.The change
Apply the same rule at every level instead of stopping at the first match, and skip package subfolders when recursing — descending into a package would make
package.subimportable as plainsub.Two tests added. The first fails on
master; the second guards the subpackage boundary and passes either way.Measurements
On a real project whose root holds two packages with further code under
src/:from pkg import modsrc/pkg/mod.pyropetest/: 2122 passed, 7 skipped, 5 xfailed.On the cost, plainly: this spends time where detection previously skipped work, so a project that gains source folders pays for analysing code that was invisible before. It is not overhead, but it is real, and on a project where the old single root was the right answer there is nothing to gain and time to lose.
source_foldersin the prefs still overrides detection for anyone who wants to narrow it back.I measured a second case where the coverage actually changes — an occurrence resolving 0 of 2 references before and 2 of 2 after — but that rename needs #868 to run at all on this codebase, so I am not claiming it for this PR alone.
Relation to #868
Independent, and based on
master. #868 fixes the walker matching tokens inside string literals; this one fixes which folders are searched. Either can land first.🤖 Generated with Claude Code
https://claude.ai/code/session_01V5GqJKaXZzFzmFEYgo4pPR