Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path by AlexWaygood · Pull Request #17178 · astral-sh/ruff

AlexWaygood · 2025-04-03T14:28:39Z

Summary

If a package in site-packages had this directory structure:

# bar/__init__.py
from .a import A

# bar/a.py
class A: ...

then we would fail to resolve the from .a import A import if (as is usually the case!) the site-packages search path was located inside a .venv directory that was a subdirectory of the project's first-party search path. The reason for this is a bug in file_to_module in the module resolver. In this loop, we would identify that /project_root/.venv/lib/python3.13/site-packages/foo/__init__.py can be turned into a path relative to the first-party search path (/project_root):

ruff/crates/red_knot_python_semantic/src/module_resolver/resolver.rs

Lines 101 to 110 in 6e2b8f9

    
           let module_name = loop { 
        
               let candidate = search_paths.next()?; 
        
               let relative_path = match path { 
        
                   SystemOrVendoredPathRef::System(path) => candidate.relativize_system_path(path), 
        
                   SystemOrVendoredPathRef::Vendored(path) => candidate.relativize_vendored_path(path), 
        
               }; 
        
               if let Some(relative_path) = relative_path { 
        
                   break relative_path.to_module_name()?; 
        
               } 
        
           };

but we'd then try to turn the relative path (.venv/lib/python3.13/site-packages/foo/__init__.py) into a module path, realise that it wasn't a valid module path... and therefore immediately break out of the loop before trying any other search paths (such as the site-packages search path).

This bug was originally reported on Discord by @MatthewMckee4.

Test Plan

I added a unit test for file_to_module in resolver.rs, and an integration test that shows we can now resolve the import correctly in infer.rs.

github-actions · 2025-04-03T14:30:39Z

`mypy_primer` results

No ecosystem changes detected ✅

AlexWaygood · 2025-04-03T14:33:29Z

Test Plan

I added a unit test for file_to_module in resolver.rs, and an integration test that shows we can now resolve the import correctly in infer.rs.

I also manually checked that we are able to resolve the import from pydantic import BaseModel (when pydantic is installed into a virtual environment in a subdirectory of the project root), and we don't manage to resolve that import on main.

I guess we don't run mypy_primer on any projects that use pydantic right now!

MatthewMckee4 · 2025-04-03T14:36:38Z

I now get "Module pydantic has no member BaseModelred-knot(lint:unresolved-import)"

This error does not show up with model_validator for example

Edit: not on all files though, ill try to reproduce

carljm

Looks good!

carljm · 2025-04-03T14:39:04Z

crates/red_knot_python_semantic/src/db.rs

-        /// Path to a custom typeshed directory
-        custom_typeshed: Option<SystemPathBuf>,
+        /// Paths to the directory to use for `site-packages`
+        site_packages: Vec<SystemPathBuf>,


You removed custom_typeshed because we aren't using it anymore? (I guess mdtest now supports this so those tests have been able to move there.)

Yes, exactly

AlexWaygood · 2025-04-03T14:42:07Z

I now get "Module pydantic has no member BaseModelred-knot(lint:unresolved-import)"

This error does not show up with model_validator for example

Huh. How are you running this branch?

If I run the following commands:

mkdir experiment
cd experiment
uv venv
uv pip install pydantic
echo "from pydantic import BaseModel; from typing_extensions import reveal_type; reveal_type(BaseModel)" > foo.py
cargo run --manifest-path ../ruff/Cargo.toml -p red_knot check foo.py

Where ../ruff is a path to my local Ruff clone, and I have this branch of red-knot checked out, then red-knot now reports this for me:

info: revealed-type
 --> /Users/alexw/dev/experiment4/foo.py:1:76
  |
1 | from pydantic import BaseModel; from typing_extensions import reveal_type; reveal_type(BaseModel)
  |                                                                            ^^^^^^^^^^^^^^^^^^^^^^ Revealed type is `Literal[BaseModel]`
  |

Found 1 diagnostic

MatthewMckee4 · 2025-04-03T14:46:39Z

That works perfectly fine for me, just in one of my codebases this import fails, ill keep trying to reproduce

AlexWaygood · 2025-04-03T14:47:58Z

I think I'll land this for now anyway since it definitely fixes a bug (both test cases added here fail on main), even if there are more bugs to fix as well as this. But please do report it if you find a repro for the remaining issues!

MatthewMckee4 · 2025-04-03T14:51:32Z

Ah sorry im being stupid, my ruff server was linked to my fork of ruff and my ide was showing the old errors, sorry about that

AlexWaygood · 2025-04-03T14:52:34Z

No worries at all -- very easily done :-)

* origin/main: [red-knot] Add `Type::TypeVar` variant (#17102) [red-knot] update to latest Salsa with fixpoint caching fix (#17179) Upgrade to Rust 1.86 and bump MSRV to 1.84 (#17171) [red-knot] Avoid unresolved-reference in unreachable code (#17169) Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path (#17178) [DO NOT LAND] bump Salsa version (#17176) [syntax-errors] Detect duplicate keys in `match` mapping patterns (#17129)

@MatthewMckee4

…site-packages` search path is a subdirectory of the first-party search path (astral-sh#17178) ## Summary If a package in `site-packages` had this directory structure: ```py # bar/__init__.py from .a import A # bar/a.py class A: ... ``` then we would fail to resolve the `from .a import A` import _if_ (as is usually the case!) the `site-packages` search path was located inside a `.venv` directory that was a subdirectory of the project's first-party search path. The reason for this is a bug in `file_to_module` in the module resolver. In this loop, we would identify that `/project_root/.venv/lib/python3.13/site-packages/foo/__init__.py` can be turned into a path relative to the first-party search path (`/project_root`): https://github.com/astral-sh/ruff/blob/6e2b8f9696e87d786cfed6304dc3baa0f029d341/crates/red_knot_python_semantic/src/module_resolver/resolver.rs#L101-L110 but we'd then try to turn the relative path (.venv/lib/python3.13/site-packages/foo/__init__.py`) into a module path, realise that it wasn't a valid module path... and therefore immediately `break` out of the loop before trying any other search paths (such as the `site-packages` search path). This bug was originally reported on Discord by @MatthewMckee4. ## Test Plan I added a unit test for `file_to_module` in `resolver.rs`, and an integration test that shows we can now resolve the import correctly in `infer.rs`.

…ges` test as an mdtest (#17222) ## Summary This PR does the following things: - Fixes the `python` configuration setting for mdtest (added in #17221) so that it expects a path pointing to a venv's `sys.prefix` variable rather than the a path pointing to the venv's `site-packages` subdirectory. This brings the `python` setting in mdtest in sync with our CLI `--python` flag. - Tweaks mdtest so that it automatically creates a valid `pyvenv.cfg` file for you if you don't specify one. This makes it much more ergonomic to write an mdtest with a custom `python` setting: red-knot will reject a `python` setting that points to a directory that doesn't have a `pyvenv.cfg` file in it - Tweaks mdtest so that it doesn't check a custom `pyvenv.cfg` as Python source code if you _do_ add a custom `pyvenv.cfg` file for your mock virtual environment in an mdtest. (You get a lot of diagnostics about Python syntax errors in the `pyvenv.cfg` file, otherwise!) - Rewrites the test added in #17178 as an mdtest, and deletes the original test that was added in that PR ## Test Plan I verified that the new mdtest fails if I revert the changes to `resolver.rs` that were added in #17178

AlexWaygood added 2 commits April 3, 2025 14:54

fix bugs in file_to_module

e5be9c7

add integration test

a6f181d

AlexWaygood added bug Something isn't working ty Multi-file analysis & type inference labels Apr 3, 2025

AlexWaygood requested review from carljm, dcreager and sharkdp as code owners April 3, 2025 14:28

carljm approved these changes Apr 3, 2025

View reviewed changes

AlexWaygood merged commit a1eb834 into main Apr 3, 2025
23 checks passed

AlexWaygood deleted the alex/file-to-module-bug branch April 3, 2025 14:48

AlexWaygood mentioned this pull request Apr 5, 2025

[red-knot] Fix python setting in mdtests, and rewrite a site-packages test as an mdtest #17222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path#17178

Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path#17178
AlexWaygood merged 2 commits intomainfrom
alex/file-to-module-bug

AlexWaygood commented Apr 3, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Test Plan

Uh oh!

MatthewMckee4 commented Apr 3, 2025 •

edited

Loading

Uh oh!

carljm left a comment

Uh oh!

carljm Apr 3, 2025

Uh oh!

AlexWaygood Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

MatthewMckee4 commented Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

Uh oh!

MatthewMckee4 commented Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	let module_name = loop {
	let candidate = search_paths.next()?;
	let relative_path = match path {
	SystemOrVendoredPathRef::System(path) => candidate.relativize_system_path(path),
	SystemOrVendoredPathRef::Vendored(path) => candidate.relativize_vendored_path(path),
	};
	if let Some(relative_path) = relative_path {
	break relative_path.to_module_name()?;
	}
	};

Comments

Conversation

AlexWaygood commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 3, 2025

mypy_primer results

Uh oh!

AlexWaygood commented Apr 3, 2025

Test Plan

Uh oh!

MatthewMckee4 commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

MatthewMckee4 commented Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

Uh oh!

MatthewMckee4 commented Apr 3, 2025

Uh oh!

AlexWaygood commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlexWaygood commented Apr 3, 2025 •

edited

Loading

`mypy_primer` results

MatthewMckee4 commented Apr 3, 2025 •

edited

Loading