Skip checking a file if it failed to read (#12755)

Cargo.lock

@@ -2166,6 +2166,7 @@ dependencies = [
  "rustc-hash 2.0.0",
  "salsa",
  "tempfile",
+ "thiserror",
  "tracing",
  "tracing-subscriber",
  "tracing-tree",

crates/red_knot_workspace/src/db.rs

@@ -150,6 +150,7 @@ impl Db for RootDatabase {}
 #[cfg(test)]
 pub(crate) mod tests {
     use salsa::Event;
+    use std::sync::Arc;
 
     use red_knot_python_semantic::{vendored_typeshed_stubs, Db as SemanticDb};
     use ruff_db::files::Files;
@@ -162,6 +163,7 @@ pub(crate) mod tests {
     #[salsa::db]
     pub(crate) struct TestDb {
         storage: salsa::Storage<Self>,
+        events: std::sync::Arc<std::sync::Mutex<Vec<salsa::Event>>>,
         files: Files,
         system: TestSystem,
         vendored: VendoredFileSystem,
@@ -174,10 +176,24 @@ pub(crate) mod tests {
                 system: TestSystem::default(),
                 vendored: vendored_typeshed_stubs().clone(),
                 files: Files::default(),
+                events: Arc::default(),
             }
         }
     }
 
+    impl TestDb {
+        /// Takes the salsa events.
+        ///
+        /// ## Panics
+        /// If there are any pending salsa snapshots.
+        pub(crate) fn take_salsa_events(&mut self) -> Vec<salsa::Event> {
+            let inner = Arc::get_mut(&mut self.events).expect("no pending salsa snapshots");
+
+            let events = inner.get_mut().unwrap();
+            std::mem::take(&mut *events)
+        }
+    }
+
     impl DbWithTestSystem for TestDb {
         fn test_system(&self) -> &TestSystem {
             &self.system
@@ -228,6 +244,9 @@ pub(crate) mod tests {
 
     #[salsa::db]
     impl salsa::Database for TestDb {
-        fn salsa_event(&self, _event: &dyn Fn() -> Event) {}
+        fn salsa_event(&self, event: &dyn Fn() -> Event) {
+            let mut events = self.events.lock().unwrap();
+            events.push(event());
+        }
     }
 }

crates/red_knot_workspace/src/workspace.rs

@@ -1,9 +1,10 @@
-use salsa::{Durability, Setter as _};
 use std::{collections::BTreeMap, sync::Arc};
 
 use rustc_hash::{FxBuildHasher, FxHashSet};
+use salsa::{Durability, Setter as _};
 
 pub use metadata::{PackageMetadata, WorkspaceMetadata};
+use ruff_db::source::{source_text, SourceDiagnostic};
 use ruff_db::{
     files::{system_path_to_file, File},
     system::{walk_directory::WalkState, SystemPath, SystemPathBuf},
@@ -345,12 +346,27 @@ impl Package {
     }
 }
 
+#[salsa::tracked]
 pub(super) fn check_file(db: &dyn Db, file: File) -> Diagnostics {
     let path = file.path(db);
     let _span = tracing::debug_span!("check_file", file=%path).entered();
     tracing::debug!("Checking file {path}");
 
     let mut diagnostics = Vec::new();
+
+    let source_diagnostics = source_text::accumulated::<SourceDiagnostic>(db.upcast(), file);
+    // TODO(micha): Consider using a single accumulator for all diagnostics
+    diagnostics.extend(
+        source_diagnostics
+            .iter()
+            .map(std::string::ToString::to_string),
+    );
+
+    // Abort checking if there are IO errors.
+    if source_text(db.upcast(), file).has_read_error() {
+        return Diagnostics::from(diagnostics);
+    }
+
     diagnostics.extend_from_slice(lint_syntax(db, file));
     diagnostics.extend_from_slice(lint_semantic(db, file));
     Diagnostics::from(diagnostics)
@@ -398,3 +414,48 @@ fn discover_package_files(db: &dyn Db, path: &SystemPath) -> FxHashSet<File> {
 
     files
 }
+
+#[cfg(test)]
+mod tests {
+    use ruff_db::files::system_path_to_file;
+    use ruff_db::source::source_text;
+    use ruff_db::system::{DbWithTestSystem, SystemPath};
+    use ruff_db::testing::assert_function_query_was_not_run;
+
+    use crate::db::tests::TestDb;
+    use crate::lint::{lint_syntax, Diagnostics};
+    use crate::workspace::check_file;
+
+    #[test]
+    fn check_file_skips_linting_when_file_cant_be_read() -> ruff_db::system::Result<()> {
+        let mut db = TestDb::new();
+        let path = SystemPath::new("test.py");
+
+        db.write_file(path, "x = 10")?;
+        let file = system_path_to_file(&db, path).unwrap();
+
+        // Now the file gets deleted before we had a chance to read its source text.
+        db.memory_file_system().remove_file(path)?;
+        file.sync(&mut db);
+
+        assert_eq!(source_text(&db, file).as_str(), "");
+        assert_eq!(
+            check_file(&db, file),
+            Diagnostics::List(vec![
+                "Failed to read file: No such file or directory".to_string()
+            ])
+        );
+
+        let events = db.take_salsa_events();
+        assert_function_query_was_not_run(&db, lint_syntax, file, &events);
+
+        // The user now creates a new file with an empty text. The source text
+        // content returned by `source_text` remains unchanged, but the diagnostics should get updated.
+        db.write_file(path, "").unwrap();
+
+        assert_eq!(source_text(&db, file).as_str(), "");
+        assert_eq!(check_file(&db, file), Diagnostics::Empty);
+
+        Ok(())
+    }
+}

crates/ruff_db/Cargo.toml

@@ -27,6 +27,7 @@ ignore = { workspace = true, optional = true }
 matchit = { workspace = true }
 salsa = { workspace = true }
 path-slash = { workspace = true }
+thiserror = { workspace = true }
 tracing = { workspace = true }
 tracing-subscriber = { workspace = true, optional = true }
 tracing-tree = { workspace = true, optional = true }

crates/ruff_db/src/source.rs

@@ -1,7 +1,9 @@
+use std::fmt::Formatter;
 use std::ops::Deref;
 use std::sync::Arc;
 
 use countme::Count;
+use salsa::Accumulator;
 
 use ruff_notebook::Notebook;
 use ruff_python_ast::PySourceType;
@@ -15,8 +17,42 @@ use crate::Db;
 pub fn source_text(db: &dyn Db, file: File) -> SourceText {
     let path = file.path(db);
     let _span = tracing::trace_span!("source_text", file = %path).entered();
+    let mut has_read_error = false;
 
-    let is_notebook = match path {
+    let kind = if is_notebook(file.path(db)) {
+        file.read_to_notebook(db)
+            .unwrap_or_else(|error| {
+                tracing::debug!("Failed to read notebook {path}: {error}");
+
+                has_read_error = true;
+                SourceDiagnostic(Arc::new(SourceTextError::FailedToReadNotebook(error)))
+                    .accumulate(db);
+                Notebook::empty()
+            })
+            .into()
+    } else {
+        file.read_to_string(db)
+            .unwrap_or_else(|error| {
+                tracing::debug!("Failed to read file {path}: {error}");
+
+                has_read_error = true;
+                SourceDiagnostic(Arc::new(SourceTextError::FailedToReadFile(error))).accumulate(db);
+                String::new()
+            })
+            .into()
+    };
+
+    SourceText {
+        inner: Arc::new(SourceTextInner {
+            kind,
+            has_read_error,
+            count: Count::new(),
+        }),
+    }
+}
+
+fn is_notebook(path: &FilePath) -> bool {
+    match path {
         FilePath::System(system) => system.extension().is_some_and(|extension| {
             PySourceType::try_from_extension(extension) == Some(PySourceType::Ipynb)
         }),
@@ -26,33 +62,6 @@ pub fn source_text(db: &dyn Db, file: File) -> SourceText {
             })
         }
         FilePath::Vendored(_) => false,
-    };
-
-    if is_notebook {
-        // TODO(micha): Proper error handling and emit a diagnostic. Tackle it together with `source_text`.
-        let notebook = file.read_to_notebook(db).unwrap_or_else(|error| {
-            tracing::error!("Failed to load notebook: {error}");
-            Notebook::empty()
-        });
-
-        return SourceText {
-            inner: Arc::new(SourceTextInner {
-                kind: SourceTextKind::Notebook(notebook),
-                count: Count::new(),
-            }),
-        };
-    }
-
-    let content = file.read_to_string(db).unwrap_or_else(|error| {
-        tracing::error!("Failed to load file: {error}");
-        String::default()
-    });
-
-    SourceText {
-        inner: Arc::new(SourceTextInner {
-            kind: SourceTextKind::Text(content),
-            count: Count::new(),
-        }),
     }
 }
 
@@ -87,6 +96,11 @@ impl SourceText {
     pub fn is_notebook(&self) -> bool {
         matches!(&self.inner.kind, SourceTextKind::Notebook(_))
     }
+
+    /// Returns `true` if there was an error when reading the content of the file.
+    pub fn has_read_error(&self) -> bool {
+        self.inner.has_read_error
+    }
 }
 
 impl Deref for SourceText {
@@ -118,6 +132,7 @@ impl std::fmt::Debug for SourceText {
 struct SourceTextInner {
     count: Count<SourceText>,
     kind: SourceTextKind,
+    has_read_error: bool,
 }
 
 #[derive(Eq, PartialEq)]
@@ -126,6 +141,35 @@ enum SourceTextKind {
     Notebook(Notebook),
 }
 
+impl From<String> for SourceTextKind {
+    fn from(value: String) -> Self {
+        SourceTextKind::Text(value)
+    }
+}
+
+impl From<Notebook> for SourceTextKind {
+    fn from(notebook: Notebook) -> Self {
+        SourceTextKind::Notebook(notebook)
+    }
+}
+
+#[salsa::accumulator]
+pub struct SourceDiagnostic(Arc<SourceTextError>);
+
+impl std::fmt::Display for SourceDiagnostic {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        std::fmt::Display::fmt(&self.0, f)
+    }
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum SourceTextError {
+    #[error("Failed to read notebook: {0}`")]
+    FailedToReadNotebook(#[from] ruff_notebook::NotebookError),
+    #[error("Failed to read file: {0}")]
+    FailedToReadFile(#[from] std::io::Error),
+}
+
 /// Computes the [`LineIndex`] for `file`.
 #[salsa::tracked]
 pub fn line_index(db: &dyn Db, file: File) -> LineIndex {