Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[red-knot] Add support for editable installs to the module resolver #12307

Merged
merged 19 commits into from
Jul 16, 2024
Merged

[red-knot] Add support for editable installs to the module resolver #12307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
287fcec
[red-knot] Add support for editable installs to the module resolver
AlexWaygood Jul 12, 2024
645532b
Deduplicate search path roots
AlexWaygood Jul 12, 2024
a456ef9
Make all module resolution search paths absolute
AlexWaygood Jul 13, 2024
9d68c07
Improve docs
AlexWaygood Jul 13, 2024
99d87b4
Be more lenient about trailing and leading whitespace
AlexWaygood Jul 13, 2024
f7219f2
Make it a Salsa query and add caching tests
AlexWaygood Jul 13, 2024
f664391
Lazily discover editable installations
AlexWaygood Jul 14, 2024
42551d1
Misc review comments
AlexWaygood Jul 15, 2024
1709e91
Simplify
AlexWaygood Jul 15, 2024
64f03c6
Use `report_untracked_read()`
AlexWaygood Jul 15, 2024
b1b2ffb
Avoid an unnecessary allocation
AlexWaygood Jul 15, 2024
1adf37d
Don't add a new method for creating arbitrary directories in tests
AlexWaygood Jul 15, 2024
388fdca
Change test so that it asserts the cache is invalidated after modific…
AlexWaygood Jul 15, 2024
1c267b5
Update crates/red_knot_module_resolver/src/resolver.rs
AlexWaygood Jul 15, 2024
6d8acb9
Cleanup tests
AlexWaygood Jul 15, 2024
e29d3d4
Fix comment
AlexWaygood Jul 15, 2024
23d2d8b
Fix bugs, address review, add TODOs
AlexWaygood Jul 16, 2024
52f457b
Merge branch 'editables' of https://github.com/astral-sh/ruff into ed…
AlexWaygood Jul 16, 2024
0d64399
Merge branch 'main' into editables
AlexWaygood Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/red_knot_module_resolver/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ ruff_python_stdlib = { workspace = true }

compact_str = { workspace = true }
camino = { workspace = true }
ordermap = { workspace = true }
once_cell = { workspace = true }
rustc-hash = { workspace = true }
salsa = { workspace = true }
Expand Down
75 changes: 74 additions & 1 deletion crates/red_knot_module_resolver/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ enum ModuleResolutionPathBufInner {
FirstParty(SystemPathBuf),
StandardLibrary(FilePath),
SitePackages(SystemPathBuf),
EditableInstall(SystemPathBuf),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered just using the SitePackages variant for editable installs, since these search paths do come about as a consequence of .pth files in the site-packages directory. However, the search paths for editable installs aren't necessarily children of the site-packages search path (and indeed most often won't be), and I think it would be too easy to assume that all instances of the SitePackages variant have the site-packages directory as their root.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a problem with introducing a new enum variant for pth-added paths, though I'm also not totally convinced by it: I don't know when we'd ever handle them differently, and it requires one more chunk of duplicated code wherever we handle this enum. I don't think the confusion you mention should occur: in general search paths should never be children of other search paths (this is a really bad setup that leads to the same module being importable under two different names), so it doesn't seem to me to be a likely confusion to assume this would be the case.

I'm not sure about the name EditableInstall. pth files are a feature of the Python import system that are used by the editable-install feature of some package managers, but the feature predates that use, and pth files have been (and I'm sure still are) used in many other ways as well (e.g. for the zipped-egg installation format of setuptools). So I think it's potentially a little misleading to imply that every path discovered in a .pth file is an editable install.

I'd suggest PthEntry or something similar instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the name EditableInstall. pth files are a feature of the Python import system that are used by the editable-install feature of some package managers, but the feature predates that use, and pth files have been (and I'm sure still are) used in many other ways as well (e.g. for the zipped-egg installation format of setuptools). So I think it's potentially a little misleading to imply that every path discovered in a .pth file is an editable install.

Hrm, true, but the validation we do elsewhere should ensure that we only add paths that point to directories when we iterate through .pth files to try to find paths that could potentially be editable installs. While it's true that a directoy path in a .pth file could point to something which wasn't intended to be an editable install, the Python runtime will still consider Python modules at that path to be importable, correct (since they'll be added to sys.path by site.py)? So IIUC a directory path in a .pth file is in effect an editable install, even if it... wasn't meant to be.

All of which is to say, from the perspective of the module resolver, this variant should only really be used if we think it's more-likely-than-not that a path represents a directory that has been editably installed, or a path relative to a path representing a directory that has been editably installed. We should have done sufficient validation at this point that it's more than just "an arbitrary line in a .pth file".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with either name but it might be at the time that we add some documentation to each variant because I don't think the linked order still applies (it doesn't mention editable installs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some more docs makes sense but I won't do it just now, since I may still refactor this pretty soon according to the idea Carl suggested on one of my earlier PRs

Copy link
Contributor

@carljm carljm Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly any path found in a .pth file will be added to sys.path, and particularly if that path is a directory, it will result in modules there being importable. I'm not sure if there is any other validation you're referring to that we do.

The logical jump you're making that I don't quite follow is "if we add a path to sys.path that makes things importable, that's an 'editable install'." I don't think that follows. It's just importable Python source code. "Editable install" is a very specific user-facing feature that implies a certain use case. For example, there's no reason the path added by the pth file line couldn't point to some Python code on a read-only filesystem. We should still support that just fine, but it's not very editable! In that case the pth file entry is being used for some other use case that I wouldn't describe as an "editable install."

}

impl ModuleResolutionPathBufInner {
Expand Down Expand Up @@ -134,6 +135,19 @@ impl ModuleResolutionPathBufInner {
);
path.push(component);
}
Self::EditableInstall(ref mut path) => {
if let Some(extension) = extension {
assert!(
matches!(extension, "pyi" | "py"),
"Extension must be `py` or `pyi`; got `{extension}`"
);
}
assert!(
path.extension().is_none(),
"Cannot push part {component} to {path}, which already has an extension"
);
path.push(component);
}
}
}
}
Expand Down Expand Up @@ -197,6 +211,18 @@ impl ModuleResolutionPathBuf {
.then_some(Self(ModuleResolutionPathBufInner::SitePackages(path)))
}

#[must_use]
pub(crate) fn editable_installation_root(
db: &dyn Db,
path: impl Into<SystemPathBuf>,
) -> Option<Self> {
let path = path.into();
// TODO: Add Salsa invalidation to this system call:
db.system()
.is_directory(&path)
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
.then_some(Self(ModuleResolutionPathBufInner::EditableInstall(path)))
}

#[must_use]
pub(crate) fn is_regular_package(&self, search_path: &Self, resolver: &ResolverState) -> bool {
ModuleResolutionPathRef::from(self).is_regular_package(search_path, resolver)
Expand Down Expand Up @@ -229,6 +255,16 @@ impl ModuleResolutionPathBuf {
pub(crate) fn to_file(&self, search_path: &Self, resolver: &ResolverState) -> Option<File> {
ModuleResolutionPathRef::from(self).to_file(search_path, resolver)
}

pub(crate) fn as_system_path(&self) -> Option<&SystemPath> {
match &self.0 {
ModuleResolutionPathBufInner::Extra(path) => Some(path),
ModuleResolutionPathBufInner::FirstParty(path) => Some(path),
ModuleResolutionPathBufInner::StandardLibrary(_) => None,
ModuleResolutionPathBufInner::SitePackages(path) => Some(path),
ModuleResolutionPathBufInner::EditableInstall(path) => Some(path),
}
}
}

impl fmt::Debug for ModuleResolutionPathBuf {
Expand All @@ -250,6 +286,10 @@ impl fmt::Debug for ModuleResolutionPathBuf {
.debug_tuple("ModuleResolutionPathBuf::StandardLibrary")
.field(path)
.finish(),
ModuleResolutionPathBufInner::EditableInstall(path) => f
.debug_tuple("ModuleResolutionPathBuf::EditableInstall")
.field(path)
.finish(),
}
}
}
Expand All @@ -272,6 +312,7 @@ enum ModuleResolutionPathRefInner<'a> {
FirstParty(&'a SystemPath),
StandardLibrary(FilePathRef<'a>),
SitePackages(&'a SystemPath),
EditableInstall(&'a SystemPath),
}

impl<'a> ModuleResolutionPathRefInner<'a> {
Expand Down Expand Up @@ -306,6 +347,7 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
(Self::Extra(path), Self::Extra(_)) => resolver.system().is_directory(path),
(Self::FirstParty(path), Self::FirstParty(_)) => resolver.system().is_directory(path),
(Self::SitePackages(path), Self::SitePackages(_)) => resolver.system().is_directory(path),
(Self::EditableInstall(path), Self::EditableInstall(_)) => resolver.system().is_directory(path),
(Self::StandardLibrary(path), Self::StandardLibrary(stdlib_root)) => {
match Self::query_stdlib_version(path, search_path, &stdlib_root, resolver) {
TypeshedVersionsQueryResult::DoesNotExist => false,
Expand Down Expand Up @@ -333,6 +375,7 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
(Self::Extra(path), Self::Extra(_)) => is_non_stdlib_pkg(resolver, path),
(Self::FirstParty(path), Self::FirstParty(_)) => is_non_stdlib_pkg(resolver, path),
(Self::SitePackages(path), Self::SitePackages(_)) => is_non_stdlib_pkg(resolver, path),
(Self::EditableInstall(path), Self::EditableInstall(_)) => is_non_stdlib_pkg(resolver, path),
// Unlike the other variants:
// (1) Account for VERSIONS
// (2) Only test for `__init__.pyi`, not `__init__.py`
Expand All @@ -358,6 +401,7 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
(Self::SitePackages(path), Self::SitePackages(_)) => {
system_path_to_file(resolver.db.upcast(), path)
}
(Self::EditableInstall(path), Self::EditableInstall(_)) => system_path_to_file(resolver.db.upcast(), path),
(Self::StandardLibrary(path), Self::StandardLibrary(stdlib_root)) => {
match Self::query_stdlib_version(&path, search_path, &stdlib_root, resolver) {
TypeshedVersionsQueryResult::DoesNotExist => None,
Expand All @@ -374,7 +418,10 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
#[must_use]
fn to_module_name(self) -> Option<ModuleName> {
match self {
Self::Extra(path) | Self::FirstParty(path) | Self::SitePackages(path) => {
Self::Extra(path)
| Self::FirstParty(path)
| Self::SitePackages(path)
| Self::EditableInstall(path) => {
let parent = path.parent()?;
let parent_components = parent.components().map(|component| component.as_str());
let skip_final_part =
Expand Down Expand Up @@ -421,6 +468,9 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
Self::SitePackages(path) => {
ModuleResolutionPathBufInner::SitePackages(path.with_extension("pyi"))
}
Self::EditableInstall(path) => {
ModuleResolutionPathBufInner::EditableInstall(path.with_extension("pyi"))
}
}
}

Expand All @@ -437,6 +487,9 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
Self::SitePackages(path) => Some(ModuleResolutionPathBufInner::SitePackages(
path.with_extension("py"),
)),
Self::EditableInstall(path) => Some(ModuleResolutionPathBufInner::EditableInstall(
path.with_extension("py"),
)),
}
}

Expand Down Expand Up @@ -474,6 +527,13 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
.then_some(Self::SitePackages(path))
})
}
(Self::EditableInstall(root), FilePathRef::System(absolute_path)) => {
absolute_path.strip_prefix(root).ok().and_then(|path| {
path.extension()
.map_or(true, |ext| matches!(ext, "pyi" | "py"))
.then_some(Self::EditableInstall(path))
})
}
(Self::Extra(_), FilePathRef::Vendored(_)) => None,
(Self::FirstParty(_), FilePathRef::Vendored(_)) => None,
(Self::StandardLibrary(root), FilePathRef::Vendored(absolute_path)) => match root {
Expand All @@ -487,6 +547,7 @@ impl<'a> ModuleResolutionPathRefInner<'a> {
}
},
(Self::SitePackages(_), FilePathRef::Vendored(_)) => None,
(Self::EditableInstall(_), FilePathRef::Vendored(_)) => None,
}
}
}
Expand Down Expand Up @@ -562,6 +623,10 @@ impl fmt::Debug for ModuleResolutionPathRef<'_> {
.debug_tuple("ModuleResolutionPathRef::StandardLibrary")
.field(path)
.finish(),
ModuleResolutionPathRefInner::EditableInstall(path) => f
.debug_tuple("ModuleResolutionPathRef::EditableInstall")
.field(path)
.finish(),
}
}
}
Expand All @@ -582,6 +647,9 @@ impl<'a> From<&'a ModuleResolutionPathBuf> for ModuleResolutionPathRef<'a> {
ModuleResolutionPathBufInner::SitePackages(path) => {
ModuleResolutionPathRefInner::SitePackages(path)
}
ModuleResolutionPathBufInner::EditableInstall(path) => {
ModuleResolutionPathRefInner::EditableInstall(path)
}
};
ModuleResolutionPathRef(inner)
}
Expand All @@ -593,6 +661,7 @@ impl PartialEq<SystemPath> for ModuleResolutionPathRef<'_> {
ModuleResolutionPathRefInner::Extra(path) => path == other,
ModuleResolutionPathRefInner::FirstParty(path) => path == other,
ModuleResolutionPathRefInner::SitePackages(path) => path == other,
ModuleResolutionPathRefInner::EditableInstall(path) => path == other,
ModuleResolutionPathRefInner::StandardLibrary(FilePathRef::System(path)) => {
path == other
}
Expand Down Expand Up @@ -625,6 +694,7 @@ impl PartialEq<VendoredPath> for ModuleResolutionPathRef<'_> {
ModuleResolutionPathRefInner::Extra(_) => false,
ModuleResolutionPathRefInner::FirstParty(_) => false,
ModuleResolutionPathRefInner::SitePackages(_) => false,
ModuleResolutionPathRefInner::EditableInstall(_) => false,
ModuleResolutionPathRefInner::StandardLibrary(FilePathRef::System(_)) => false,
ModuleResolutionPathRefInner::StandardLibrary(FilePathRef::Vendored(path)) => {
path == other
Expand Down Expand Up @@ -707,6 +777,9 @@ mod tests {
ModuleResolutionPathRefInner::SitePackages(path) => {
ModuleResolutionPathBufInner::SitePackages(path.to_path_buf())
}
ModuleResolutionPathRefInner::EditableInstall(path) => {
ModuleResolutionPathBufInner::EditableInstall(path.to_path_buf())
}
};
ModuleResolutionPathBuf(inner)
}
Expand Down
Loading
Loading