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

vendored_typeshed_versions should use db.vendored #13434

Merged
merged 1 commit into from
Sep 21, 2024
Merged

vendored_typeshed_versions should use db.vendored #13434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 8 additions & 24 deletions crates/red_knot_python_semantic/src/module_resolver/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
use rustc_hash::{FxBuildHasher, FxHashSet};
use std::borrow::Cow;
use std::iter::FusedIterator;
use std::ops::Deref;

use rustc_hash::{FxBuildHasher, FxHashSet};

use ruff_db::files::{File, FilePath, FileRootKind};
use ruff_db::system::{DirectoryEntry, System, SystemPath, SystemPathBuf};
use ruff_db::vendored::{VendoredFileSystem, VendoredPath};

use super::module::{Module, ModuleKind};
use super::path::{ModulePath, SearchPath, SearchPathValidationError};
use crate::db::Db;
use crate::module_name::ModuleName;
use crate::module_resolver::typeshed::{vendored_typeshed_versions, TypeshedVersions};
use crate::site_packages::VirtualEnvironment;
use crate::{Program, PythonVersion, SearchPathSettings, SitePackages};

use super::module::{Module, ModuleKind};
use super::path::{ModulePath, SearchPath, SearchPathValidationError};

/// Resolves a module name to a module.
pub fn resolve_module(db: &dyn Db, module_name: ModuleName) -> Option<Module> {
let interned_name = ModuleNameIngredient::new(db, module_name);
Expand Down Expand Up @@ -136,7 +137,7 @@ pub(crate) struct SearchPaths {
/// for the first `site-packages` path
site_packages: Vec<SearchPath>,

typeshed_versions: ResolvedTypeshedVersions,
typeshed_versions: TypeshedVersions,
}

impl SearchPaths {
Expand Down Expand Up @@ -202,11 +203,11 @@ impl SearchPaths {

let search_path = SearchPath::custom_stdlib(db, &custom_typeshed)?;

(ResolvedTypeshedVersions::Custom(parsed), search_path)
(parsed, search_path)
} else {
tracing::debug!("Using vendored stdlib");
(
ResolvedTypeshedVersions::Vendored(vendored_typeshed_versions()),
vendored_typeshed_versions(db),
Copy link
Member Author

@MichaReiser MichaReiser Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's fine to defer validating the vendored typeshed versions. They should be valid and whether we panic here or when trying to resolve the first module doesn't make a real difference. It results in a panic.

The only downside is that getting the versions is now a salsa lookup rather than a static field access. Let's see if this hurts performance in a meaningful way

Copy link
Member Author

@MichaReiser MichaReiser Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, there's a 1% performance drop. That doesn't seem worth it.

I'm just gonna make vendored_typeshed_versions always parse the file without caching. This is an overall simplification and shouldn't hurt performance except in the case when we reconstruct the SearchPathSettings because the project configuration has changed.... but that's a very slow path anyway and parsing the VERSIONS file should be rather fast

SearchPath::vendored_stdlib(),
)
};
Expand Down Expand Up @@ -279,23 +280,6 @@ impl SearchPaths {
}
}

#[derive(Debug, PartialEq, Eq)]
enum ResolvedTypeshedVersions {
Vendored(&'static TypeshedVersions),
Custom(TypeshedVersions),
}

Comment on lines -283 to -286
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why I didn't use Cow for this... 🫤

impl Deref for ResolvedTypeshedVersions {
type Target = TypeshedVersions;

fn deref(&self) -> &Self::Target {
match self {
ResolvedTypeshedVersions::Vendored(versions) => versions,
ResolvedTypeshedVersions::Custom(versions) => versions,
}
}
}

/// Collect all dynamic search paths. For each `site-packages` path:
/// - Collect that `site-packages` path
/// - Collect any search paths listed in `.pth` files in that `site-packages` directory
Expand Down
27 changes: 10 additions & 17 deletions crates/red_knot_python_semantic/src/module_resolver/typeshed/versions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,19 @@ use std::num::{NonZeroU16, NonZeroUsize};
use std::ops::{RangeFrom, RangeInclusive};
use std::str::FromStr;

use once_cell::sync::Lazy;
use rustc_hash::FxHashMap;

use super::vendored::vendored_typeshed_stubs;
use crate::db::Db;
use crate::module_name::ModuleName;
use crate::{Program, PythonVersion};

static VENDORED_VERSIONS: Lazy<TypeshedVersions> = Lazy::new(|| {
pub(in crate::module_resolver) fn vendored_typeshed_versions(db: &dyn Db) -> TypeshedVersions {
TypeshedVersions::from_str(
&vendored_typeshed_stubs()
&db.vendored()
.read_to_string("stdlib/VERSIONS")
.unwrap(),
.expect("The vendored typeshed stubs should contain a VERSIONS file"),
)
.unwrap()
});

pub(crate) fn vendored_typeshed_versions() -> &'static TypeshedVersions {
&VENDORED_VERSIONS
.expect("The VERSIONS file in the vendored typeshed stubs should be well-formed")
}

pub(crate) fn typeshed_versions(db: &dyn Db) -> &TypeshedVersions {
Expand Down Expand Up @@ -332,6 +326,8 @@ mod tests {

use insta::assert_snapshot;

use crate::db::tests::TestDb;

use super::*;

const TYPESHED_STDLIB_DIR: &str = "stdlib";
Expand All @@ -353,12 +349,9 @@ mod tests {

#[test]
fn can_parse_vendored_versions_file() {
let versions_data = include_str!(concat!(
env!("CARGO_MANIFEST_DIR"),
"/vendor/typeshed/stdlib/VERSIONS"
));
let db = TestDb::new();

let versions = TypeshedVersions::from_str(versions_data).unwrap();
let versions = vendored_typeshed_versions(&db);
assert!(versions.len() > 100);
assert!(versions.len() < 1000);

Expand Down Expand Up @@ -395,9 +388,9 @@ mod tests {

#[test]
fn typeshed_versions_consistent_with_vendored_stubs() {
const VERSIONS_DATA: &str = include_str!("../../../vendor/typeshed/stdlib/VERSIONS");
let db = TestDb::new();
let vendored_typeshed_versions = vendored_typeshed_versions(&db);
let vendored_typeshed_dir = Path::new("vendor/typeshed").canonicalize().unwrap();
let vendored_typeshed_versions = TypeshedVersions::from_str(VERSIONS_DATA).unwrap();

let mut empty_iterator = true;

Expand Down
14 changes: 6 additions & 8 deletions crates/ruff_graph/src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use anyhow::Result;
use red_knot_python_semantic::{Db, Program, ProgramSettings, PythonVersion, SearchPathSettings};
use red_knot_python_semantic::{
vendored_typeshed_stubs, Db, Program, ProgramSettings, PythonVersion, SearchPathSettings,
};
use ruff_db::files::{File, Files};
use ruff_db::system::{OsSystem, System, SystemPathBuf};
use ruff_db::vendored::VendoredFileSystem;
Expand All @@ -11,7 +13,6 @@ pub struct ModuleDb {
storage: salsa::Storage<Self>,
files: Files,
system: OsSystem,
vendored: VendoredFileSystem,
}

impl ModuleDb {
Expand All @@ -26,12 +27,10 @@ impl ModuleDb {
.next()
.ok_or_else(|| anyhow::anyhow!("No source roots provided"))?;

let mut search_paths = SearchPathSettings::new(src_root.to_path_buf());
let mut search_paths = SearchPathSettings::new(src_root);

// Add the remaining source roots as extra paths.
for src_root in src_roots {
search_paths.extra_paths.push(src_root.to_path_buf());
}
search_paths.extra_paths.extend(src_roots);

search_paths
};
Expand All @@ -54,7 +53,6 @@ impl ModuleDb {
Self {
storage: self.storage.clone(),
system: self.system.clone(),
vendored: self.vendored.clone(),
files: self.files.snapshot(),
}
}
Expand All @@ -72,7 +70,7 @@ impl Upcast<dyn SourceDb> for ModuleDb {
#[salsa::db]
impl SourceDb for ModuleDb {
fn vendored(&self) -> &VendoredFileSystem {
&self.vendored
vendored_typeshed_stubs()
}

fn system(&self) -> &dyn System {
Expand Down
Loading