Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions crates/ruff_benchmark/benches/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ fn benchmark_incremental(criterion: &mut Criterion) {
fn setup() -> Case {
let case = setup_tomllib_case();

let result: Vec<_> = case.db.check().unwrap();
let result: Vec<_> = case.db.check();

assert_diagnostics(&case.db, &result, EXPECTED_TOMLLIB_DIAGNOSTICS);

Expand Down Expand Up @@ -159,7 +159,7 @@ fn benchmark_incremental(criterion: &mut Criterion) {
None,
);

let result = db.check().unwrap();
let result = db.check();

assert_eq!(result.len(), EXPECTED_TOMLLIB_DIAGNOSTICS.len());
}
Expand All @@ -179,7 +179,7 @@ fn benchmark_cold(criterion: &mut Criterion) {
setup_tomllib_case,
|case| {
let Case { db, .. } = case;
let result: Vec<_> = db.check().unwrap();
let result: Vec<_> = db.check();

assert_diagnostics(db, &result, EXPECTED_TOMLLIB_DIAGNOSTICS);
},
Expand Down Expand Up @@ -293,7 +293,7 @@ fn benchmark_many_string_assignments(criterion: &mut Criterion) {
},
|case| {
let Case { db, .. } = case;
let result = db.check().unwrap();
let result = db.check();
assert_eq!(result.len(), 0);
},
BatchSize::SmallInput,
Expand Down Expand Up @@ -339,7 +339,7 @@ fn benchmark_many_tuple_assignments(criterion: &mut Criterion) {
},
|case| {
let Case { db, .. } = case;
let result = db.check().unwrap();
let result = db.check();
assert_eq!(result.len(), 0);
},
BatchSize::SmallInput,
Expand Down
12 changes: 11 additions & 1 deletion crates/ruff_db/src/panic.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::any::Any;
use std::backtrace::BacktraceStatus;
use std::cell::Cell;
use std::panic::Location;
Expand All @@ -24,17 +25,25 @@ impl Payload {
None
}
}

pub fn downcast_ref<R: Any>(&self) -> Option<&R> {
self.0.downcast_ref::<R>()
}
}

impl std::fmt::Display for PanicError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(f, "panicked at")?;
write!(f, "panicked at")?;
if let Some(location) = &self.location {
write!(f, " {location}")?;
}
if let Some(payload) = self.payload.as_str() {
write!(f, ":\n{payload}")?;
}
if let Some(query_trace) = self.salsa_backtrace.as_ref() {
let _ = writeln!(f, "{query_trace}");
}

if let Some(backtrace) = &self.backtrace {
match backtrace.status() {
BacktraceStatus::Disabled => {
Expand All @@ -49,6 +58,7 @@ impl std::fmt::Display for PanicError {
_ => {}
}
}

Ok(())
}
}
Expand Down
6 changes: 4 additions & 2 deletions crates/ty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,14 @@ impl MainLoop {
MainLoopMessage::CheckWorkspace => {
let db = db.clone();
let sender = self.sender.clone();
let mut reporter = R::default();

// Spawn a new task that checks the project. This needs to be done in a separate thread
// to prevent blocking the main loop here.
rayon::spawn(move || {
match db.check_with_reporter(&mut reporter) {
match salsa::Cancelled::catch(|| {
let mut reporter = R::default();
db.check_with_reporter(&mut reporter)
}) {
Ok(result) => {
// Send the result back to the main loop for printing.
sender
Expand Down
14 changes: 4 additions & 10 deletions crates/ty/tests/file_watching.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,7 +1135,7 @@ print(sys.last_exc, os.getegid())
Ok(())
})?;

let diagnostics = case.db.check().context("Failed to check project.")?;
let diagnostics = case.db.check();

assert_eq!(diagnostics.len(), 2);
assert_eq!(
Expand All @@ -1160,7 +1160,7 @@ print(sys.last_exc, os.getegid())
})
.expect("Search path settings to be valid");

let diagnostics = case.db.check().context("Failed to check project.")?;
let diagnostics = case.db.check();
assert!(diagnostics.is_empty());

Ok(())
Expand Down Expand Up @@ -1763,10 +1763,7 @@ fn changes_to_user_configuration() -> anyhow::Result<()> {
let foo = case
.system_file(case.project_path("foo.py"))
.expect("foo.py to exist");
let diagnostics = case
.db()
.check_file(foo)
.context("Failed to check project.")?;
let diagnostics = case.db().check_file(foo);

assert!(
diagnostics.is_empty(),
Expand All @@ -1786,10 +1783,7 @@ fn changes_to_user_configuration() -> anyhow::Result<()> {

case.apply_changes(changes);

let diagnostics = case
.db()
.check_file(foo)
.context("Failed to check project.")?;
let diagnostics = case.db().check_file(foo);

assert!(
diagnostics.len() == 1,
Expand Down
24 changes: 7 additions & 17 deletions crates/ty_project/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use ruff_db::files::{File, Files};
use ruff_db::system::System;
use ruff_db::vendored::VendoredFileSystem;
use ruff_db::{Db as SourceDb, Upcast};
use salsa::Event;
use salsa::plumbing::ZalsaDatabase;
use salsa::{Cancelled, Event};
use ty_ide::Db as IdeDb;
use ty_python_semantic::lint::{LintRegistry, RuleSelection};
use ty_python_semantic::{Db as SemanticDb, Program};
Expand Down Expand Up @@ -76,24 +76,21 @@ impl ProjectDatabase {
}

/// Checks all open files in the project and its dependencies.
pub fn check(&self) -> Result<Vec<Diagnostic>, Cancelled> {
pub fn check(&self) -> Vec<Diagnostic> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to remove all Results from here for now because the server already uses many salsa APIs that aren't wrapped in a salsa::Cancelled::catch. Instead, I decided that the server request handlers catch salsa cancellations (only necessary for background tasks because:

  • Only local tasks can mutate the DB
  • All local tasks run on the main loop thread.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not hugely familiar with this part of the code, but this rationale sounds reasonable

let mut reporter = DummyReporter;
let reporter = AssertUnwindSafe(&mut reporter as &mut dyn Reporter);
self.with_db(|db| db.project().check(db, reporter))
self.project().check(self, reporter)
}

/// Checks all open files in the project and its dependencies, using the given reporter.
pub fn check_with_reporter(
&self,
reporter: &mut dyn Reporter,
) -> Result<Vec<Diagnostic>, Cancelled> {
pub fn check_with_reporter(&self, reporter: &mut dyn Reporter) -> Vec<Diagnostic> {
let reporter = AssertUnwindSafe(reporter);
self.with_db(|db| db.project().check(db, reporter))
self.project().check(self, reporter)
}

#[tracing::instrument(level = "debug", skip(self))]
pub fn check_file(&self, file: File) -> Result<Vec<Diagnostic>, Cancelled> {
self.with_db(|db| self.project().check_file(db, file))
pub fn check_file(&self, file: File) -> Vec<Diagnostic> {
self.project().check_file(self, file)
}

/// Returns a mutable reference to the system.
Expand All @@ -107,13 +104,6 @@ impl ProjectDatabase {
Arc::get_mut(&mut self.system)
.expect("ref count should be 1 because `zalsa_mut` drops all other DB references.")
}

pub(crate) fn with_db<F, T>(&self, f: F) -> Result<T, Cancelled>
where
F: FnOnce(&ProjectDatabase) -> T + std::panic::UnwindSafe,
{
Cancelled::catch(|| f(self))
}
}

impl Upcast<dyn SemanticDb> for ProjectDatabase {
Expand Down
11 changes: 3 additions & 8 deletions crates/ty_server/src/server.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
//! Scheduling, I/O, and API endpoints.

use std::num::NonZeroUsize;
// The new PanicInfoHook name requires MSRV >= 1.82
#[expect(deprecated)]
use std::panic::PanicInfo;

use lsp_server::Message;
use lsp_types::{
ClientCapabilities, DiagnosticOptions, DiagnosticServerCapabilities,
Expand All @@ -13,6 +8,8 @@ use lsp_types::{
TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions,
TypeDefinitionProviderCapability, Url,
};
use std::num::NonZeroUsize;
use std::panic::PanicHookInfo;

use self::connection::{Connection, ConnectionInitializer};
use self::schedule::event_loop_thread;
Expand Down Expand Up @@ -125,9 +122,7 @@ impl Server {
}

pub(crate) fn run(self) -> crate::Result<()> {
// The new PanicInfoHook name requires MSRV >= 1.82
#[expect(deprecated)]
type PanicHook = Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>;
type PanicHook = Box<dyn Fn(&PanicHookInfo<'_>) + 'static + Sync + Send>;
Comment on lines -128 to +125
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this change for ruff_server as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan to backport this entire stack to ruff

struct RestorePanicHook {
hook: Option<PanicHook>,
}
Expand Down
Loading
Loading