[red-knot] Remove Clone from Files (#11213)

crates/red_knot/src/db.rs

@@ -57,6 +57,7 @@ pub trait ParallelDatabase: Database + Send {
     /// We should avoid creating snapshots while running a query because we might want to adopt Salsa in the future (if we can figure out persistent caching).
     /// Unfortunately, the infrastructure doesn't provide an automated way of knowing when a query is run, that's
     /// why we have to "enforce" this constraint manually.
+    #[must_use]
     fn snapshot(&self) -> Snapshot<Self>;
 }
 

crates/red_knot/src/files.rs

@@ -17,7 +17,7 @@ pub struct FileId;
 // TODO we'll need a higher level virtual file system abstraction that allows testing if a file exists
 //  or retrieving its content (ideally lazily and in a way that the memory can be retained later)
 //  I suspect that we'll end up with a FileSystem trait and our own Path abstraction.
-#[derive(Clone, Default)]
+#[derive(Default)]
 pub struct Files {
     inner: Arc<RwLock<FilesInner>>,
 }
@@ -36,6 +36,16 @@ impl Files {
     pub fn path(&self, id: FileId) -> Arc<Path> {
         self.inner.read().path(id)
     }
+
+    /// Snapshots files for a new database snapshot.
+    ///
+    /// This method should not be used outside a database snapshot.
+    #[must_use]
+    pub fn snapshot(&self) -> Files {
+        Files {
+            inner: self.inner.clone(),
+        }
+    }
 }
 
 impl Debug for Files {

crates/red_knot/src/main.rs

@@ -1,11 +1,9 @@
 #![allow(clippy::dbg_macro)]
 
-use std::collections::hash_map::Entry;
 use std::path::Path;
 use std::sync::Mutex;
 
 use crossbeam::channel as crossbeam_channel;
-use rustc_hash::FxHashMap;
 use tracing::subscriber::Interest;
 use tracing::{Level, Metadata};
 use tracing_subscriber::filter::LevelFilter;
@@ -14,10 +12,9 @@ use tracing_subscriber::{Layer, Registry};
 use tracing_tree::time::Uptime;
 
 use red_knot::db::{HasJar, ParallelDatabase, QueryError, SemanticDb, SourceDb, SourceJar};
-use red_knot::files::FileId;
 use red_knot::module::{ModuleSearchPath, ModuleSearchPathKind};
 use red_knot::program::check::ExecutionMode;
-use red_knot::program::{FileChange, FileChangeKind, Program};
+use red_knot::program::{FileWatcherChange, Program};
 use red_knot::watch::FileWatcher;
 use red_knot::Workspace;
 
@@ -72,12 +69,9 @@ fn main() -> anyhow::Result<()> {
     let file_changes_notifier = main_loop.file_changes_notifier();
 
     // Watch for file changes and re-trigger the analysis.
-    let mut file_watcher = FileWatcher::new(
+    let mut file_watcher = FileWatcher::new(move |changes| {
-        move |changes| {
         file_changes_notifier.notify(changes);
-        },
+    })?;
-        program.files().clone(),
-    )?;
 
     file_watcher.watch_folder(workspace_folder)?;
 
@@ -157,7 +151,7 @@ impl MainLoop {
                 }
                 MainLoopMessage::ApplyChanges(changes) => {
                     // Automatically cancels any pending queries and waits for them to complete.
-                    program.apply_changes(changes.iter());
+                    program.apply_changes(changes);
                 }
                 MainLoopMessage::CheckCompleted(diagnostics) => {
                     dbg!(diagnostics);
@@ -184,7 +178,7 @@ struct FileChangesNotifier {
 }
 
 impl FileChangesNotifier {
-    fn notify(&self, changes: Vec<FileChange>) {
+    fn notify(&self, changes: Vec<FileWatcherChange>) {
         self.sender
             .send(OrchestratorMessage::FileChanges(changes))
             .unwrap();
@@ -250,10 +244,7 @@ impl Orchestrator {
         }
     }
 
-    fn debounce_changes(&self, changes: Vec<FileChange>) {
+    fn debounce_changes(&self, mut changes: Vec<FileWatcherChange>) {
-        let mut aggregated_changes = AggregatedChanges::default();
-        aggregated_changes.extend(changes);
-
         loop {
             // Consume possibly incoming file change messages before running a new analysis, but don't wait for more than 100ms.
             crossbeam_channel::select! {
@@ -263,7 +254,7 @@ impl Orchestrator {
                             return self.shutdown();
                         }
                         Ok(OrchestratorMessage::FileChanges(file_changes)) => {
-                            aggregated_changes.extend(file_changes);
+                            changes.extend(file_changes);
                         }
 
                         Ok(OrchestratorMessage::CheckProgramCompleted { .. })=> {
@@ -279,7 +270,7 @@ impl Orchestrator {
                 },
                 default(std::time::Duration::from_millis(10)) => {
                     // No more file changes after 10 ms, send the changes and schedule a new analysis
-                    self.sender.send(MainLoopMessage::ApplyChanges(aggregated_changes)).unwrap();
+                    self.sender.send(MainLoopMessage::ApplyChanges(changes)).unwrap();
                     self.sender.send(MainLoopMessage::CheckProgram { revision: self.revision}).unwrap();
                     return;
                 }
@@ -298,7 +289,7 @@ impl Orchestrator {
 enum MainLoopMessage {
     CheckProgram { revision: usize },
     CheckCompleted(Vec<String>),
-    ApplyChanges(AggregatedChanges),
+    ApplyChanges(Vec<FileWatcherChange>),
     Exit,
 }
 
@@ -312,77 +303,7 @@ enum OrchestratorMessage {
         revision: usize,
     },
 
-    FileChanges(Vec<FileChange>),
+    FileChanges(Vec<FileWatcherChange>),
-}
-
-#[derive(Default, Debug)]
-struct AggregatedChanges {
-    changes: FxHashMap<FileId, FileChangeKind>,
-}
-
-impl AggregatedChanges {
-    fn add(&mut self, change: FileChange) {
-        match self.changes.entry(change.file_id()) {
-            Entry::Occupied(mut entry) => {
-                let merged = entry.get_mut();
-
-                match (merged, change.kind()) {
-                    (FileChangeKind::Created, FileChangeKind::Deleted) => {
-                        // Deletion after creations means that ruff never saw the file.
-                        entry.remove();
-                    }
-                    (FileChangeKind::Created, FileChangeKind::Modified) => {
-                        // No-op, for ruff, modifying a file that it doesn't yet know that it exists is still considered a creation.
-                    }
-
-                    (FileChangeKind::Modified, FileChangeKind::Created) => {
-                        // Uhh, that should probably not happen. Continue considering it a modification.
-                    }
-
-                    (FileChangeKind::Modified, FileChangeKind::Deleted) => {
-                        *entry.get_mut() = FileChangeKind::Deleted;
-                    }
-
-                    (FileChangeKind::Deleted, FileChangeKind::Created) => {
-                        *entry.get_mut() = FileChangeKind::Modified;
-                    }
-
-                    (FileChangeKind::Deleted, FileChangeKind::Modified) => {
-                        // That's weird, but let's consider it a modification.
-                        *entry.get_mut() = FileChangeKind::Modified;
-                    }
-
-                    (FileChangeKind::Created, FileChangeKind::Created)
-                    | (FileChangeKind::Modified, FileChangeKind::Modified)
-                    | (FileChangeKind::Deleted, FileChangeKind::Deleted) => {
-                        // No-op transitions. Some of them should be impossible but we handle them anyway.
-                    }
-                }
-            }
-            Entry::Vacant(entry) => {
-                entry.insert(change.kind());
-            }
-        }
-    }
-
-    fn extend<I>(&mut self, changes: I)
-    where
-        I: IntoIterator<Item = FileChange>,
-        I::IntoIter: ExactSizeIterator,
-    {
-        let iter = changes.into_iter();
-        self.changes.reserve(iter.len());
-
-        for change in iter {
-            self.add(change);
-        }
-    }
-
-    fn iter(&self) -> impl Iterator<Item = FileChange> + '_ {
-        self.changes
-            .iter()
-            .map(|(id, kind)| FileChange::new(*id, *kind))
-    }
 }
 
 fn setup_tracing() {

crates/red_knot/src/program/mod.rs

@@ -1,6 +1,9 @@
-use std::path::Path;
+use std::collections::hash_map::Entry;
+use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
+use rustc_hash::FxHashMap;
+
 use crate::db::{
     Database, Db, DbRuntime, HasJar, HasJars, JarsStorage, LintDb, LintJar, ParallelDatabase,
     QueryResult, SemanticDb, SemanticJar, Snapshot, SourceDb, SourceJar,
@@ -37,10 +40,17 @@ impl Program {
 
     pub fn apply_changes<I>(&mut self, changes: I)
     where
-        I: IntoIterator<Item = FileChange>,
+        I: IntoIterator<Item = FileWatcherChange>,
     {
+        let mut aggregated_changes = AggregatedChanges::default();
+
+        aggregated_changes.extend(changes.into_iter().map(|change| FileChange {
+            id: self.files.intern(&change.path),
+            kind: change.kind,
+        }));
+
         let (source, semantic, lint) = self.jars_mut();
-        for change in changes {
+        for change in aggregated_changes.iter() {
             semantic.module_resolver.remove_module(change.id);
             semantic.symbol_tables.remove(&change.id);
             source.sources.remove(&change.id);
@@ -140,7 +150,7 @@ impl ParallelDatabase for Program {
     fn snapshot(&self) -> Snapshot<Self> {
         Snapshot::new(Self {
             jars: self.jars.snapshot(),
-            files: self.files.clone(),
+            files: self.files.snapshot(),
             workspace: self.workspace.clone(),
         })
     }
@@ -188,22 +198,30 @@ impl HasJar<LintJar> for Program {
     }
 }
 
+#[derive(Clone, Debug)]
+pub struct FileWatcherChange {
+    path: PathBuf,
+    kind: FileChangeKind,
+}
+
+impl FileWatcherChange {
+    pub fn new(path: PathBuf, kind: FileChangeKind) -> Self {
+        Self { path, kind }
+    }
+}
+
 #[derive(Copy, Clone, Debug)]
-pub struct FileChange {
+struct FileChange {
     id: FileId,
     kind: FileChangeKind,
 }
 
 impl FileChange {
-    pub fn new(file_id: FileId, kind: FileChangeKind) -> Self {
+    fn file_id(self) -> FileId {
-        Self { id: file_id, kind }
-    }
-
-    pub fn file_id(&self) -> FileId {
         self.id
     }
 
-    pub fn kind(&self) -> FileChangeKind {
+    fn kind(self) -> FileChangeKind {
         self.kind
     }
 }
@@ -214,3 +232,74 @@ pub enum FileChangeKind {
     Modified,
     Deleted,
 }
+
+#[derive(Default, Debug)]
+struct AggregatedChanges {
+    changes: FxHashMap<FileId, FileChangeKind>,
+}
+
+impl AggregatedChanges {
+    fn add(&mut self, change: FileChange) {
+        match self.changes.entry(change.file_id()) {
+            Entry::Occupied(mut entry) => {
+                let merged = entry.get_mut();
+
+                match (merged, change.kind()) {
+                    (FileChangeKind::Created, FileChangeKind::Deleted) => {
+                        // Deletion after creations means that ruff never saw the file.
+                        entry.remove();
+                    }
+                    (FileChangeKind::Created, FileChangeKind::Modified) => {
+                        // No-op, for ruff, modifying a file that it doesn't yet know that it exists is still considered a creation.
+                    }
+
+                    (FileChangeKind::Modified, FileChangeKind::Created) => {
+                        // Uhh, that should probably not happen. Continue considering it a modification.
+                    }
+
+                    (FileChangeKind::Modified, FileChangeKind::Deleted) => {
+                        *entry.get_mut() = FileChangeKind::Deleted;
+                    }
+
+                    (FileChangeKind::Deleted, FileChangeKind::Created) => {
+                        *entry.get_mut() = FileChangeKind::Modified;
+                    }
+
+                    (FileChangeKind::Deleted, FileChangeKind::Modified) => {
+                        // That's weird, but let's consider it a modification.
+                        *entry.get_mut() = FileChangeKind::Modified;
+                    }
+
+                    (FileChangeKind::Created, FileChangeKind::Created)
+                    | (FileChangeKind::Modified, FileChangeKind::Modified)
+                    | (FileChangeKind::Deleted, FileChangeKind::Deleted) => {
+                        // No-op transitions. Some of them should be impossible but we handle them anyway.
+                    }
+                }
+            }
+            Entry::Vacant(entry) => {
+                entry.insert(change.kind());
+            }
+        }
+    }
+
+    fn extend<I>(&mut self, changes: I)
+    where
+        I: IntoIterator<Item = FileChange>,
+    {
+        let iter = changes.into_iter();
+        let (lower, _) = iter.size_hint();
+        self.changes.reserve(lower);
+
+        for change in iter {
+            self.add(change);
+        }
+    }
+
+    fn iter(&self) -> impl Iterator<Item = FileChange> + '_ {
+        self.changes.iter().map(|(id, kind)| FileChange {
+            id: *id,
+            kind: *kind,
+        })
+    }
+}

crates/red_knot/src/watch.rs

@@ -1,38 +1,38 @@
-use anyhow::Context;
 use std::path::Path;
 
-use crate::files::Files;
+use anyhow::Context;
-use crate::program::{FileChange, FileChangeKind};
 use notify::event::{CreateKind, RemoveKind};
 use notify::{recommended_watcher, Event, EventKind, RecommendedWatcher, RecursiveMode, Watcher};
 
+use crate::program::{FileChangeKind, FileWatcherChange};
+
 pub struct FileWatcher {
     watcher: RecommendedWatcher,
 }
 
 pub trait EventHandler: Send + 'static {
-    fn handle(&self, changes: Vec<FileChange>);
+    fn handle(&self, changes: Vec<FileWatcherChange>);
 }
 
 impl<F> EventHandler for F
 where
-    F: Fn(Vec<FileChange>) + Send + 'static,
+    F: Fn(Vec<FileWatcherChange>) + Send + 'static,
 {
-    fn handle(&self, changes: Vec<FileChange>) {
+    fn handle(&self, changes: Vec<FileWatcherChange>) {
         let f = self;
         f(changes);
     }
 }
 
 impl FileWatcher {
-    pub fn new<E>(handler: E, files: Files) -> anyhow::Result<Self>
+    pub fn new<E>(handler: E) -> anyhow::Result<Self>
     where
         E: EventHandler,
     {
-        Self::from_handler(Box::new(handler), files)
+        Self::from_handler(Box::new(handler))
     }
 
-    fn from_handler(handler: Box<dyn EventHandler>, files: Files) -> anyhow::Result<Self> {
+    fn from_handler(handler: Box<dyn EventHandler>) -> anyhow::Result<Self> {
         let watcher = recommended_watcher(move |changes: notify::Result<Event>| {
             match changes {
                 Ok(event) => {
@@ -50,8 +50,7 @@ impl FileWatcher {
 
                     for path in event.paths {
                         if path.is_file() {
-                            let id = files.intern(&path);
+                            changes.push(FileWatcherChange::new(path, change_kind));
-                            changes.push(FileChange::new(id, change_kind));
                         }
                     }