impl Detect editable installs and auto re-analyze on changes. #2207#2646
impl Detect editable installs and auto re-analyze on changes. #2207#2646asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class support for detecting Python editable installs and using that information to (a) classify “source vs third-party” correctly in the LSP and (b) expand/refresh file watching so editable source roots are re-analyzed when editable-install metadata changes.
Changes:
- Introduces
pyrefly_util::editable_installto detect editable installs viadirect_url.json(PEP 610),.pth, and.egg-link, with caching + unit tests. - Switches LSP “third-party vs source” logic to use the shared editable detector.
- Extends watcher patterns and invalidation to clear the editable-path cache and re-watch when editable metadata changes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/state/state.rs | Clears editable-install cache on watcher events when editable metadata changes. |
| pyrefly/lib/state/lsp.rs | Removes local editable detection/cache and uses shared util API for editable source roots. |
| pyrefly/lib/lsp/non_wasm/server.rs | Forces re-watch and clears cache when editable metadata changes. |
| crates/pyrefly_util/src/lib.rs | Exposes new editable_install module. |
| crates/pyrefly_util/src/editable_install.rs | Implements shared editable-install detection + cache + tests. |
| crates/pyrefly_config/src/config.rs | Adds editable roots + editable metadata files to watch patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if editable_metadata_changed { | ||
| clear_editable_source_paths_cache(); | ||
| } |
There was a problem hiding this comment.
did_change_watched_files clears the editable-path cache here, and Transaction::invalidate_events also clears the same cache when editable metadata changes. This causes redundant global mutex locking/clearing on every editable-metadata event. Consider centralizing the cache clear in one place (e.g., reorder to invalidate first so the transaction clears before setup_file_watcher_if_necessary, or make invalidate_events optionally skip clearing when already done).
| result.insert(WatchPattern::root(root.dupe(), "**/*.pth".to_owned())); | ||
| result.insert(WatchPattern::root(root.dupe(), "**/*.egg-link".to_owned())); | ||
| result.insert(WatchPattern::root( | ||
| root, | ||
| "**/*.dist-info/direct_url.json".to_owned(), |
There was a problem hiding this comment.
The new watcher globs for editable metadata are very broad (**/*.pth / **/*.egg-link) under each site-packages root. Python only considers .pth/.egg-link files at the site-packages root, so watching recursively can add unnecessary watcher load/noise in large environments. Consider narrowing these patterns (e.g., *.pth / *.egg-link, and similarly limiting direct_url.json to the expected top-level *.dist-info/direct_url.json locations) to reduce watcher overhead.
| result.insert(WatchPattern::root(root.dupe(), "**/*.pth".to_owned())); | |
| result.insert(WatchPattern::root(root.dupe(), "**/*.egg-link".to_owned())); | |
| result.insert(WatchPattern::root( | |
| root, | |
| "**/*.dist-info/direct_url.json".to_owned(), | |
| result.insert(WatchPattern::root(root.dupe(), "*.pth".to_owned())); | |
| result.insert(WatchPattern::root(root.dupe(), "*.egg-link".to_owned())); | |
| result.insert(WatchPattern::root( | |
| root, | |
| "*.dist-info/direct_url.json".to_owned(), |
| let site_packages: Vec<PathBuf> = config.site_package_path().cloned().collect(); | ||
| let editable_paths = get_editable_source_paths(&site_packages); | ||
|
|
||
| config | ||
| .search_path() | ||
| .chain(config.site_package_path()) | ||
| .cloned() | ||
| .chain(site_packages.iter().cloned()) | ||
| .chain(editable_paths.iter().cloned()) | ||
| .cartesian_product(PYTHON_EXTENSIONS.iter().chain(COMPILED_FILE_SUFFIXES)) | ||
| .for_each(|(s, suffix)| { | ||
| result.insert(WatchPattern::root( | ||
| InternedPath::from_path(s), | ||
| InternedPath::from_path(&s), | ||
| format!("**/*.{suffix}"), | ||
| )); | ||
| }); | ||
|
|
||
| for site_package_path in &site_packages { | ||
| let root = InternedPath::from_path(site_package_path); | ||
| result.insert(WatchPattern::root(root.dupe(), "**/*.pth".to_owned())); | ||
| result.insert(WatchPattern::root(root.dupe(), "**/*.egg-link".to_owned())); | ||
| result.insert(WatchPattern::root( | ||
| root, | ||
| "**/*.dist-info/direct_url.json".to_owned(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
ConfigFile::get_paths_to_watch now depends on editable install detection (editable source roots + editable metadata files), but there are no unit tests covering this function’s output even though this file has extensive config-related tests. Adding a focused test that constructs a temp site-packages with direct_url.json / .pth / .egg-link and asserts the returned WatchPatterns include the editable roots + metadata globs would help prevent regressions.
This comment has been minimized.
This comment has been minimized.
asukaminato0721
force-pushed
the
2207-editable
branch
from
e3351c2 to
e21406b
Compare
asukaminato0721
force-pushed
the
2207-editable
branch
from
e21406b to
d0659c0
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes part of #2207
Added a shared editable-install detector (direct_url.json, .pth, .egg-link) with caching and tests, and wired LSP “source vs third-party” logic to use it.
Extended LSP watch patterns to include editable source roots plus editable metadata files, and rewatch/invalidate when those metadata files change.
Cleared editable-path cache on watch events (LSP + CLI watch) so changes pick up new editable roots.
Test Plan
add test