diff --git a/crates/red_knot/src/db.rs b/crates/red_knot/src/db.rs index d4650e2b9c..7ddb426336 100644 --- a/crates/red_knot/src/db.rs +++ b/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; } diff --git a/crates/red_knot/src/files.rs b/crates/red_knot/src/files.rs index 69b72281cf..4dd2ddb106 100644 --- a/crates/red_knot/src/files.rs +++ b/crates/red_knot/src/files.rs @@ -15,9 +15,9 @@ type Map = hashbrown::HashMap; 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)] +// 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(Default)] pub struct Files { inner: Arc>, } @@ -36,6 +36,16 @@ impl Files { pub fn path(&self, id: FileId) -> Arc { 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 { @@ -63,7 +73,7 @@ struct FilesInner { by_path: Map, // TODO should we use a map here to reclaim the space for removed files? // TODO I think we should use our own path abstraction here to avoid having to normalize paths - // and dealing with non-utf paths everywhere. + // and dealing with non-utf paths everywhere. by_id: IndexVec>, } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index d7e02749f2..53007bd088 100644 --- a/crates/red_knot/src/main.rs +++ b/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( - move |changes| { - file_changes_notifier.notify(changes); - }, - program.files().clone(), - )?; + let mut file_watcher = FileWatcher::new(move |changes| { + file_changes_notifier.notify(changes); + })?; 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) { + fn notify(&self, changes: Vec) { self.sender .send(OrchestratorMessage::FileChanges(changes)) .unwrap(); @@ -250,10 +244,7 @@ impl Orchestrator { } } - fn debounce_changes(&self, changes: Vec) { - let mut aggregated_changes = AggregatedChanges::default(); - aggregated_changes.extend(changes); - + fn debounce_changes(&self, mut changes: Vec) { 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), - ApplyChanges(AggregatedChanges), + ApplyChanges(Vec), Exit, } @@ -312,77 +303,7 @@ enum OrchestratorMessage { revision: usize, }, - FileChanges(Vec), -} - -#[derive(Default, Debug)] -struct AggregatedChanges { - changes: FxHashMap, -} - -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(&mut self, changes: I) - where - I: IntoIterator, - 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 + '_ { - self.changes - .iter() - .map(|(id, kind)| FileChange::new(*id, *kind)) - } + FileChanges(Vec), } fn setup_tracing() { diff --git a/crates/red_knot/src/program/mod.rs b/crates/red_knot/src/program/mod.rs index 6da7fd4080..4f7c538ea6 100644 --- a/crates/red_knot/src/program/mod.rs +++ b/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(&mut self, changes: I) where - I: IntoIterator, + I: IntoIterator, { + 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 { 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 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 { - Self { id: file_id, kind } - } - - pub fn file_id(&self) -> FileId { + 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, +} + +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(&mut self, changes: I) + where + I: IntoIterator, + { + 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 + '_ { + self.changes.iter().map(|(id, kind)| FileChange { + id: *id, + kind: *kind, + }) + } +} diff --git a/crates/red_knot/src/watch.rs b/crates/red_knot/src/watch.rs index 80204a219b..5c8ee3fb27 100644 --- a/crates/red_knot/src/watch.rs +++ b/crates/red_knot/src/watch.rs @@ -1,38 +1,38 @@ -use anyhow::Context; use std::path::Path; -use crate::files::Files; -use crate::program::{FileChange, FileChangeKind}; +use anyhow::Context; 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); + fn handle(&self, changes: Vec); } impl EventHandler for F where - F: Fn(Vec) + Send + 'static, + F: Fn(Vec) + Send + 'static, { - fn handle(&self, changes: Vec) { + fn handle(&self, changes: Vec) { let f = self; f(changes); } } impl FileWatcher { - pub fn new(handler: E, files: Files) -> anyhow::Result + pub fn new(handler: E) -> anyhow::Result where E: EventHandler, { - Self::from_handler(Box::new(handler), files) + Self::from_handler(Box::new(handler)) } - fn from_handler(handler: Box, files: Files) -> anyhow::Result { + fn from_handler(handler: Box) -> anyhow::Result { let watcher = recommended_watcher(move |changes: notify::Result| { 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(FileChange::new(id, change_kind)); + changes.push(FileWatcherChange::new(path, change_kind)); } }