Review feedback

crates/ruff/Cargo.toml

@@ -14,6 +14,7 @@ default-run = "ruff"
 
 [dependencies]
 ruff_cache = { workspace = true }
+ruff_db = { workspace = true }
 ruff_diagnostics = { workspace = true }
 ruff_graph = { workspace = true, features = ["serde", "clap"] }
 ruff_linter = { workspace = true, features = ["clap"] }

crates/ruff/src/commands/graph.rs

@@ -3,6 +3,7 @@ use crate::resolve::resolve;
 use crate::{resolve_default_files, ExitStatus};
 use anyhow::Result;
 use log::{debug, warn};
+use ruff_db::system::{SystemPath, SystemPathBuf};
 use ruff_graph::{Direction, ImportMap, ModuleDb, ModuleImports};
 use ruff_linter::warn_user_once;
 use ruff_python_ast::{PySourceType, SourceType};
@@ -54,12 +55,12 @@ pub(crate) fn graph(args: GraphArgs, config_arguments: &ConfigArguments) -> Resu
     let inner_result = Arc::clone(&result);
 
     rayon::scope(move |scope| {
-        for resolved_file in &paths {
+        for resolved_file in paths {
             let Ok(resolved_file) = resolved_file else {
                 continue;
             };
 
-            let path = resolved_file.path().to_path_buf();
+            let path = resolved_file.into_path();
             let package = path
                 .parent()
                 .and_then(|parent| package_roots.get(parent))
@@ -97,16 +98,27 @@ pub(crate) fn graph(args: GraphArgs, config_arguments: &ConfigArguments) -> Resu
                 continue;
             }
 
+            // Convert to system paths.
+            let Ok(package) = package.map(SystemPathBuf::from_path_buf).transpose() else {
+                warn!("Failed to convert package to system path");
+                continue;
+            };
+            let Ok(src_root) = SystemPathBuf::from_path_buf(src_root) else {
+                warn!("Failed to convert source root to system path");
+                continue;
+            };
+            let Ok(path) = SystemPathBuf::from_path_buf(path) else {
+                warn!("Failed to convert path to system path");
+                continue;
+            };
+
             let result = inner_result.clone();
             scope.spawn(move |_| {
                 // Identify any imports via static analysis.
                 let mut imports =
                     ruff_graph::generate(&path, package.as_deref(), string_imports, &db)
                         .unwrap_or_else(|err| {
-                            warn!(
-                                "Failed to generate import map for {path}: {err}",
-                                path = path.display(),
-                            );
+                            warn!("Failed to generate import map for {path}: {err}");
                             ModuleImports::default()
                         });
 
@@ -125,7 +137,17 @@ pub(crate) fn graph(args: GraphArgs, config_arguments: &ConfigArguments) -> Resu
                                         continue;
                                     }
                                 };
-                                imports.insert(entry.into_path());
+                                let path = match SystemPathBuf::from_path_buf(entry.into_path()) {
+                                    Ok(path) => path,
+                                    Err(err) => {
+                                        warn!(
+                                            "Failed to convert path to system path: {}",
+                                            err.display()
+                                        );
+                                        continue;
+                                    }
+                                };
+                                imports.insert(path);
                             }
                         }
                         Err(err) => {

crates/ruff_db/Cargo.toml

@@ -26,6 +26,7 @@ filetime = { workspace = true }
 ignore = { workspace = true, optional = true }
 matchit = { workspace = true }
 salsa = { workspace = true }
+serde = { workspace = true, optional = true }
 path-slash = { workspace = true }
 thiserror = { workspace = true }
 tracing = { workspace = true }
@@ -47,5 +48,6 @@ tempfile = { workspace = true }
 [features]
 cache = ["ruff_cache"]
 os = ["ignore"]
+serde = ["dep:serde", "camino/serde1"]
 # Exposes testing utilities.
 testing = ["tracing-subscriber", "tracing-tree"]

crates/ruff_db/src/system/os.rs

@@ -21,7 +21,7 @@ pub struct OsSystem {
     inner: Arc<OsSystemInner>,
 }
 
-#[derive(Default, Debug, Clone)]
+#[derive(Default, Debug)]
 struct OsSystemInner {
     cwd: SystemPathBuf,
 }

crates/ruff_db/src/system/path.rs

@@ -593,6 +593,27 @@ impl ruff_cache::CacheKey for SystemPathBuf {
     }
 }
 
+#[cfg(feature = "serde")]
+impl serde::Serialize for SystemPath {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        self.0.serialize(serializer)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl serde::Serialize for SystemPathBuf {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        self.0.serialize(serializer)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for SystemPathBuf {
+    fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+        Utf8PathBuf::deserialize(deserializer).map(SystemPathBuf)
+    }
+}
+
 /// A slice of a virtual path on [`System`](super::System) (akin to [`str`]).
 #[repr(transparent)]
 pub struct SystemVirtualPath(str);

crates/ruff_graph/Cargo.toml

@@ -12,7 +12,7 @@ license.workspace = true
 [dependencies]
 red_knot_python_semantic = { workspace = true }
 ruff_cache = { workspace = true }
-ruff_db = { workspace = true, features = ["os"] }
+ruff_db = { workspace = true, features = ["os", "serde"] }
 ruff_linter = { workspace = true }
 ruff_macros = { workspace = true }
 ruff_python_ast = { workspace = true }

crates/ruff_graph/src/collector.rs

@@ -1,20 +1,18 @@
+use red_knot_python_semantic::ModuleName;
 use ruff_python_ast::helpers::collect_import_from_member;
-use ruff_python_ast::name::QualifiedName;
-use ruff_python_ast::visitor::source_order::walk_module;
-use ruff_python_ast::visitor::source_order::{walk_expr, walk_stmt, SourceOrderVisitor};
-use ruff_python_ast::{self as ast, Expr, Mod, Stmt};
-use ruff_python_stdlib::identifiers::is_identifier;
+use ruff_python_ast::visitor::source_order::{walk_body, walk_expr, walk_stmt, SourceOrderVisitor};
+use ruff_python_ast::{self as ast, Expr, ModModule, Stmt};
 
 /// Collect all imports for a given Python file.
 #[derive(Default, Debug)]
-pub(crate) struct Collector<'ast> {
+pub(crate) struct Collector {
     /// Whether to detect imports from string literals.
     string_imports: bool,
     /// The collected imports from the Python AST.
-    imports: Vec<CollectedImport<'ast>>,
+    imports: Vec<CollectedImport>,
 }
 
-impl<'ast> Collector<'ast> {
+impl Collector {
     pub(crate) fn new(string_imports: bool) -> Self {
         Self {
             string_imports,
@@ -23,13 +21,13 @@ impl<'ast> Collector<'ast> {
     }
 
     #[must_use]
-    pub(crate) fn collect(mut self, module: &'ast Mod) -> Vec<CollectedImport<'ast>> {
-        walk_module(&mut self, module);
+    pub(crate) fn collect(mut self, module: &ModModule) -> Vec<CollectedImport> {
+        walk_body(&mut self, &module.body);
         self.imports
     }
 }
 
-impl<'ast> SourceOrderVisitor<'ast> for Collector<'ast> {
+impl<'ast> SourceOrderVisitor<'ast> for Collector {
     fn visit_stmt(&mut self, stmt: &'ast Stmt) {
         match stmt {
             Stmt::ImportFrom(ast::StmtImportFrom {
@@ -41,15 +39,21 @@ impl<'ast> SourceOrderVisitor<'ast> for Collector<'ast> {
                 let module = module.as_deref();
                 let level = *level;
                 for alias in names {
-                    let qualified_name = collect_import_from_member(level, module, &alias.name);
-                    self.imports
-                        .push(CollectedImport::ImportFrom(qualified_name));
+                    if let Some(module_name) = ModuleName::from_components(
+                        collect_import_from_member(level, module, &alias.name)
+                            .segments()
+                            .iter()
+                            .cloned(),
+                    ) {
+                        self.imports.push(CollectedImport::ImportFrom(module_name));
+                    }
                 }
             }
             Stmt::Import(ast::StmtImport { names, range: _ }) => {
                 for alias in names {
-                    let qualified_name = QualifiedName::user_defined(&alias.name);
-                    self.imports.push(CollectedImport::Import(qualified_name));
+                    if let Some(module_name) = ModuleName::new(alias.name.as_str()) {
+                        self.imports.push(CollectedImport::Import(module_name));
+                    }
                 }
             }
             _ => {
@@ -64,9 +68,8 @@ impl<'ast> SourceOrderVisitor<'ast> for Collector<'ast> {
                 // Determine whether the string literal "looks like" an import statement: contains
                 // a dot, and consists solely of valid Python identifiers.
                 let value = value.to_str();
-                if value.split('.').all(is_identifier) {
-                    let qualified_name = QualifiedName::from_dotted_name(value);
-                    self.imports.push(CollectedImport::Import(qualified_name));
+                if let Some(module_name) = ModuleName::new(value) {
+                    self.imports.push(CollectedImport::Import(module_name));
                 }
             }
             walk_expr(self, expr);
@@ -75,9 +78,9 @@ impl<'ast> SourceOrderVisitor<'ast> for Collector<'ast> {
 }
 
 #[derive(Debug)]
-pub(crate) enum CollectedImport<'ast> {
+pub(crate) enum CollectedImport {
     /// The import was part of an `import` statement.
-    Import(QualifiedName<'ast>),
+    Import(ModuleName),
     /// The import was part of an `import from` statement.
-    ImportFrom(QualifiedName<'ast>),
+    ImportFrom(ModuleName),
 }

crates/ruff_graph/src/lib.rs

@@ -3,11 +3,13 @@ pub use crate::db::ModuleDb;
 use crate::resolver::Resolver;
 pub use crate::settings::{Direction, GraphSettings};
 use anyhow::Result;
+use red_knot_python_semantic::SemanticModel;
+use ruff_db::files::system_path_to_file;
+use ruff_db::parsed::parsed_module;
+use ruff_db::system::{SystemPath, SystemPathBuf};
 use ruff_python_ast::helpers::to_module_path;
-use ruff_python_parser::{parse, Mode};
 use serde::{Deserialize, Serialize};
 use std::collections::{BTreeMap, BTreeSet};
-use std::path::{Path, PathBuf};
 
 mod collector;
 mod db;
@@ -16,10 +18,10 @@ mod settings;
 
 #[derive(Debug, Default)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-pub struct ModuleImports(BTreeSet<PathBuf>);
+pub struct ModuleImports(BTreeSet<SystemPathBuf>);
 
 impl ModuleImports {
-    pub fn insert(&mut self, path: PathBuf) {
+    pub fn insert(&mut self, path: SystemPathBuf) {
         self.0.insert(path);
     }
 
@@ -33,7 +35,7 @@ impl ModuleImports {
 
     /// Convert the file paths to be relative to a given path.
     #[must_use]
-    pub fn relative_to(&self, path: &Path) -> Self {
+    pub fn relative_to(&self, path: &SystemPath) -> Self {
         let mut imports = Self::default();
         for import in &self.0 {
             if let Ok(path) = import.strip_prefix(path) {
@@ -46,15 +48,15 @@ impl ModuleImports {
 
 #[derive(Debug, Default)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-pub struct ImportMap(BTreeMap<PathBuf, ModuleImports>);
+pub struct ImportMap(BTreeMap<SystemPathBuf, ModuleImports>);
 
 impl ImportMap {
-    pub fn insert(&mut self, path: PathBuf, imports: ModuleImports) {
+    pub fn insert(&mut self, path: SystemPathBuf, imports: ModuleImports) {
         self.0.insert(path, imports);
     }
 
     #[must_use]
-    pub fn reverse(imports: impl IntoIterator<Item = (PathBuf, ModuleImports)>) -> Self {
+    pub fn reverse(imports: impl IntoIterator<Item = (SystemPathBuf, ModuleImports)>) -> Self {
         let mut reverse = ImportMap::default();
         for (path, imports) in imports {
             for import in imports.0 {
@@ -65,8 +67,8 @@ impl ImportMap {
     }
 }
 
-impl FromIterator<(PathBuf, ModuleImports)> for ImportMap {
-    fn from_iter<I: IntoIterator<Item = (PathBuf, ModuleImports)>>(iter: I) -> Self {
+impl FromIterator<(SystemPathBuf, ModuleImports)> for ImportMap {
+    fn from_iter<I: IntoIterator<Item = (SystemPathBuf, ModuleImports)>>(iter: I) -> Self {
         let mut map = ImportMap::default();
         for (path, imports) in iter {
             map.0.entry(path).or_default().0.extend(imports.0);
@@ -77,29 +79,31 @@ impl FromIterator<(PathBuf, ModuleImports)> for ImportMap {
 
 /// Generate the module imports for a given Python file.
 pub fn generate(
-    path: &Path,
-    package: Option<&Path>,
+    path: &SystemPath,
+    package: Option<&SystemPath>,
     string_imports: bool,
     db: &ModuleDb,
 ) -> Result<ModuleImports> {
     // Read and parse the source code.
-    let source = std::fs::read_to_string(path)?;
-    let parsed = parse(&source, Mode::Module)?;
-    let module_path = package.and_then(|package| to_module_path(package, path));
+    let file = system_path_to_file(db, path)?;
+    let parsed = parsed_module(db, file);
+    let module_path =
+        package.and_then(|package| to_module_path(package.as_std_path(), path.as_std_path()));
+    let model = SemanticModel::new(db, file);
 
     // Collect the imports.
     let imports = Collector::new(string_imports).collect(parsed.syntax());
 
     // Resolve the imports.
     let mut resolved_imports = ModuleImports::default();
     for import in imports {
-        let Some(resolved) = Resolver::new(module_path.as_deref(), db).resolve(&import) else {
+        let Some(resolved) = Resolver::new(&model, module_path.as_deref()).resolve(import) else {
             continue;
         };
         let Some(path) = resolved.as_system_path() else {
             continue;
         };
-        resolved_imports.insert(path.as_std_path().to_path_buf());
+        resolved_imports.insert(path.to_path_buf());
     }
 
     Ok(resolved_imports)