fix imports using symlinked paths are not updated #1336#3029
fix imports using symlinked paths are not updated #1336#3029asukaminato0721 wants to merge 4 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes LSP + import-resolution behavior when Python modules are imported via symlinked paths, ensuring edits to the real (target) file propagate through symlinked imports and that retargeted symlinks invalidate cached import finding.
Changes:
- Canonicalize symlinked filesystem hits during module finding so imports resolve to the real target path.
- Prefer in-memory (open/unsaved) contents when the resolved path (or its canonical target) is open.
- Invalidate import finding when a watched symlink path is modified, and add regression tests for symlink behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pyrefly/lib/test/lsp/lsp_interaction/did_change.rs | Adds an LSP regression test ensuring didChange updates diagnostics through symlinked imports. |
| pyrefly/lib/state/state.rs | Prefers memory-backed module paths for imports and invalidates find-cache on symlink modification events. |
| pyrefly/lib/module/finder.rs | Canonicalizes found module/package paths and adds a unit test for resolving symlink modules to their real paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| fn canonicalize_found_path(path: PathBuf) -> PathBuf { | ||
| path.canonicalize().unwrap_or(path) |
There was a problem hiding this comment.
canonicalize_found_path() unconditionally canonicalizes every found import path. That can rewrite paths even when the discovered file itself is not a symlink (e.g. when some parent directory is symlinked, such as macOS /var -> /private/var), which changes ModulePath identity unexpectedly and can break path equality/caching assumptions elsewhere. Consider only canonicalizing when the found entry is actually a symlink (via symlink_metadata().file_type().is_symlink()), or otherwise limiting canonicalization to the specific symlink-resolution cases you intend to fix.
| path.canonicalize().unwrap_or(path) | |
| match path.symlink_metadata() { | |
| Ok(metadata) if metadata.file_type().is_symlink() => path.canonicalize().unwrap_or(path), | |
| _ => path, | |
| } |
| let tempdir = tempfile::tempdir().unwrap(); | ||
| let root = tempdir.path(); | ||
| let target = root.join("test_module.py"); | ||
| TestPath::setup_test_directory( | ||
| root, | ||
| vec![TestPath::file_with_contents( | ||
| "test_module.py", | ||
| "def hello(name):\n pass\n", | ||
| )], | ||
| ); | ||
| symlink(&target, root.join("sym.py")).unwrap(); | ||
|
|
||
| assert_eq!( | ||
| find_module( | ||
| ModuleName::from_str("sym"), | ||
| [root.to_path_buf()].iter(), | ||
| &mut vec![], | ||
| None, | ||
| None, | ||
| false, | ||
| &mut None, | ||
| ) | ||
| .unwrap(), | ||
| FindingOrError::new_finding(ModulePath::filesystem(target)) | ||
| ); |
There was a problem hiding this comment.
This symlink-resolution test compares against target built from tempdir.path(). Since the finder now canonicalizes discovered paths, the actual result may be a different (canonical) spelling of the same path (common on macOS temp dirs), causing a platform-specific failure. Canonicalize the expected target (or derive all paths from a canonicalized tempdir root) before asserting equality.
| let root = tempfile::tempdir().unwrap(); | ||
| let module_path = root.path().join("test_module.py"); | ||
| let symlink_path = root.path().join("sym.py"); | ||
| let importer_path = root.path().join("test_import.py"); | ||
| std::fs::write(&module_path, "def hello(name: str) -> None:\n pass\n").unwrap(); | ||
| symlink(&module_path, &symlink_path).unwrap(); | ||
| std::fs::write(&importer_path, "from sym import hello\nhello(\"John\")\n").unwrap(); | ||
|
|
||
| let mut interaction = LspInteraction::new_with_indexing_mode(IndexingMode::LazyBlocking); | ||
| interaction.set_root(root.path().to_path_buf()); | ||
| interaction | ||
| .initialize(InitializeSettings { | ||
| configuration: Some(Some( | ||
| json!([{"pyrefly": {"displayTypeErrors": "force-on"}}]), | ||
| )), | ||
| workspace_folders: Some(vec![( | ||
| "test".to_owned(), | ||
| Url::from_file_path(root.path()).unwrap(), | ||
| )]), | ||
| ..Default::default() | ||
| }) | ||
| .unwrap(); |
There was a problem hiding this comment.
This test constructs the tempdir root and opens files using root.path() as-is. If import resolution canonicalizes symlink paths (and canonicalization rewrites the tempdir path on some platforms, e.g. macOS), the in-memory file key may not match the canonicalized import target and the test can fail intermittently by platform. Using a canonicalized root path consistently for file creation + set_root/workspace folder URIs will make the test robust.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #1336
The change has two parts.
Import discovery now canonicalizes symlinked filesystem hits, so import sym resolves through the real target path in the finder.
for the live LSP bug, imported modules now prefer the open in-memory file if the resolved path itself or its canonical target is open, which makes unsaved edits in the real file propagate through symlinked imports.
also invalidate import finding when a watched symlink path changes so retargeted symlinks do not stay cached.
Test Plan
add test