mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-11-02 04:48:06 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Update salsa (#12711)

Browse source
This commit is contained in:
Micha Reiser 2024-08-06 15:17:39 +02:00 • committed by GitHub
parent 8e6aa78796
commit 846f57fd15
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 92 additions and 131 deletions
Download patch file Download diff file Expand all files Collapse all files

11
crates/red_knot/src/main.rs
View file

@ -132,7 +132,7 @@ pub fn main() -> anyhow::Result<()> {
// TODO: Use the `program_settings` to compute the key for the database's persistent
// cache and load the cache if it exists.
let db = RootDatabase::new(workspace_metadata, program_settings, system);
let mut db = RootDatabase::new(workspace_metadata, program_settings, system);
let (main_loop, main_loop_cancellation_token) = MainLoop::new(verbosity);
@ -146,7 +146,6 @@ pub fn main() -> anyhow::Result<()> {
}
})?;
let mut db = salsa::Handle::new(db);
if watch {
main_loop.watch(&mut db)?;
} else {
@ -186,7 +185,7 @@ impl MainLoop {
)
}
fn watch(mut self, db: &mut salsa::Handle<RootDatabase>) -> anyhow::Result<()> {
fn watch(mut self, db: &mut RootDatabase) -> anyhow::Result<()> {
let sender = self.sender.clone();
let watcher = watch::directory_watcher(move |event| {
sender.send(MainLoopMessage::ApplyChanges(event)).unwrap();
@ -198,7 +197,7 @@ impl MainLoop {
}
#[allow(clippy::print_stderr)]
fn run(mut self, db: &mut salsa::Handle<RootDatabase>) {
fn run(mut self, db: &mut RootDatabase) {
// Schedule the first check.
self.sender.send(MainLoopMessage::CheckWorkspace).unwrap();
let mut revision = 0usize;
@ -208,7 +207,7 @@ impl MainLoop {
match message {
MainLoopMessage::CheckWorkspace => {
let db = db.clone();
let db = db.snapshot();
let sender = self.sender.clone();
// Spawn a new task that checks the workspace. This needs to be done in a separate thread
@ -243,7 +242,7 @@ impl MainLoop {
MainLoopMessage::ApplyChanges(changes) => {
revision += 1;
// Automatically cancels any pending queries and waits for them to complete.
db.get_mut().apply_changes(changes);
db.apply_changes(changes);
if let Some(watcher) = self.watcher.as_mut() {
watcher.update(db);
}

15
crates/red_knot_module_resolver/src/db.rs
View file

@ -74,10 +74,6 @@ pub(crate) mod tests {
&self.system
}
fn system_mut(&mut self) -> &mut dyn ruff_db::system::System {
&mut self.system
}
fn files(&self) -> &Files {
&self.files
}
@ -98,12 +94,11 @@ pub(crate) mod tests {
#[salsa::db]
impl salsa::Database for TestDb {
fn salsa_event(&self, event: salsa::Event) {
self.attach(|_| {
tracing::trace!("event: {event:?}");
let mut events = self.events.lock().unwrap();
events.push(event);
});
fn salsa_event(&self, event: &dyn Fn() -> salsa::Event) {
let event = event();
tracing::trace!("event: {event:?}");
let mut events = self.events.lock().unwrap();
events.push(event);
}
}
}

15
crates/red_knot_python_semantic/src/db.rs
View file

@ -77,10 +77,6 @@ pub(crate) mod tests {
&self.system
}
fn system_mut(&mut self) -> &mut dyn System {
&mut self.system
}
fn files(&self) -> &Files {
&self.files
}
@ -112,12 +108,11 @@ pub(crate) mod tests {
#[salsa::db]
impl salsa::Database for TestDb {
fn salsa_event(&self, event: salsa::Event) {
self.attach(|_| {
tracing::trace!("event: {event:?}");
let mut events = self.events.lock().unwrap();
events.push(event);
});
fn salsa_event(&self, event: &dyn Fn() -> salsa::Event) {
let event = event();
tracing::trace!("event: {event:?}");
let mut events = self.events.lock().unwrap();
events.push(event);
}
}
}

1
crates/red_knot_server/Cargo.toml
View file

@ -25,7 +25,6 @@ jod-thread = { workspace = true }
lsp-server = { workspace = true }
lsp-types = { workspace = true }
rustc-hash = { workspace = true }
salsa = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
shellexpand = { workspace = true }

5
crates/red_knot_server/src/server/api.rs
View file

@ -7,6 +7,7 @@ mod requests;
mod traits;
use notifications as notification;
use red_knot_workspace::db::RootDatabase;
use requests as request;
use self::traits::{NotificationHandler, RequestHandler};
@ -84,7 +85,9 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>(
let Ok(path) = url_to_system_path(&url) else {
return Box::new(|_, _| {});
};
let db = session.workspace_db_for_path(path.as_std_path()).cloned();
let db = session
.workspace_db_for_path(path.as_std_path())
.map(RootDatabase::snapshot);
let Some(snapshot) = session.take_snapshot(url) else {
return Box::new(|_, _| {});

2
crates/red_knot_server/src/server/api/notifications/did_close.rs
View file

@ -35,7 +35,7 @@ impl SyncNotificationHandler for DidCloseTextDocumentHandler {
.with_failure_code(ErrorCode::InternalError)?;
if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) {
File::sync_path(db.get_mut(), &path);
File::sync_path(db, &path);
}
clear_diagnostics(key.url(), &notifier)?;

2
crates/red_knot_server/src/server/api/notifications/did_close_notebook.rs
View file

@ -33,7 +33,7 @@ impl SyncNotificationHandler for DidCloseNotebookHandler {
.with_failure_code(lsp_server::ErrorCode::InternalError)?;
if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) {
File::sync_path(db.get_mut(), &path);
File::sync_path(db, &path);
}
Ok(())

4
crates/red_knot_server/src/server/api/notifications/did_open.rs
View file

@ -32,8 +32,8 @@ impl SyncNotificationHandler for DidOpenTextDocumentHandler {
if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) {
// TODO(dhruvmanila): Store the `file` in `DocumentController`
let file = system_path_to_file(&**db, &path).unwrap();
file.sync(db.get_mut());
let file = system_path_to_file(db, &path).unwrap();
file.sync(db);
}
// TODO(dhruvmanila): Publish diagnostics if the client doesn't support pull diagnostics

4
crates/red_knot_server/src/server/api/notifications/did_open_notebook.rs
View file

@ -40,8 +40,8 @@ impl SyncNotificationHandler for DidOpenNotebookHandler {
if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) {
// TODO(dhruvmanila): Store the `file` in `DocumentController`
let file = system_path_to_file(&**db, &path).unwrap();
file.sync(db.get_mut());
let file = system_path_to_file(db, &path).unwrap();
file.sync(db);
}
// TODO(dhruvmanila): Publish diagnostics if the client doesn't support pull diagnostics

2
crates/red_knot_server/src/server/api/requests/diagnostic.rs
View file

@ -25,7 +25,7 @@ impl BackgroundDocumentRequestHandler for DocumentDiagnosticRequestHandler {
fn run_with_snapshot(
snapshot: DocumentSnapshot,
db: Option<salsa::Handle<RootDatabase>>,
db: Option<RootDatabase>,
_notifier: Notifier,
_params: DocumentDiagnosticParams,
) -> Result<DocumentDiagnosticReportResult> {

2
crates/red_knot_server/src/server/api/traits.rs
View file

@ -34,7 +34,7 @@ pub(super) trait BackgroundDocumentRequestHandler: RequestHandler {
fn run_with_snapshot(
snapshot: DocumentSnapshot,
db: Option<salsa::Handle<RootDatabase>>,
db: Option<RootDatabase>,
notifier: Notifier,
params: <<Self as RequestHandler>::RequestType as Request>::Params,
) -> super::Result<<<Self as RequestHandler>::RequestType as Request>::Result>;

19
crates/red_knot_server/src/session.rs
View file

@ -13,7 +13,6 @@ use red_knot_workspace::workspace::WorkspaceMetadata;
use ruff_db::files::{system_path_to_file, File};
use ruff_db::program::{ProgramSettings, SearchPathSettings, TargetVersion};
use ruff_db::system::SystemPath;
use ruff_db::Db as _;
use crate::edit::{DocumentKey, NotebookDocument};
use crate::system::{url_to_system_path, LSPSystem};
@ -43,7 +42,7 @@ pub struct Session {
index: Option<Arc<index::Index>>,
/// Maps workspace root paths to their respective databases.
workspaces: BTreeMap<PathBuf, salsa::Handle<RootDatabase>>,
workspaces: BTreeMap<PathBuf, RootDatabase>,
/// The global position encoding, negotiated during LSP initialization.
position_encoding: PositionEncoding,
/// Tracks what LSP features the client supports and doesn't support.
@ -79,10 +78,7 @@ impl Session {
custom_typeshed: None,
},
};
workspaces.insert(
path,
salsa::Handle::new(RootDatabase::new(metadata, program_settings, system)),
);
workspaces.insert(path, RootDatabase::new(metadata, program_settings, system));
}
Ok(Self {
@ -95,10 +91,7 @@ impl Session {
})
}
pub(crate) fn workspace_db_for_path(
&self,
path: impl AsRef<Path>,
) -> Option<&salsa::Handle<RootDatabase>> {
pub(crate) fn workspace_db_for_path(&self, path: impl AsRef<Path>) -> Option<&RootDatabase> {
self.workspaces
.range(..=path.as_ref().to_path_buf())
.next_back()
@ -108,7 +101,7 @@ impl Session {
pub(crate) fn workspace_db_for_path_mut(
&mut self,
path: impl AsRef<Path>,
) -> Option<&mut salsa::Handle<RootDatabase>> {
) -> Option<&mut RootDatabase> {
self.workspaces
.range_mut(..=path.as_ref().to_path_buf())
.next_back()
@ -168,9 +161,6 @@ impl Session {
let index = self.index.take().unwrap();
for db in self.workspaces.values_mut() {
// Calling `get_mut` on `Handle<Database>` cancels all pending queries and waits for them to stop.
let db = db.get_mut();
// Remove the `index` from each database. This drops the count of `Arc<Index>` down to 1
db.system_mut()
.as_any_mut()
@ -216,7 +206,6 @@ impl Drop for MutIndexGuard<'_> {
if let Some(index) = self.index.take() {
let index = Arc::new(index);
for db in self.session.workspaces.values_mut() {
let db = db.get_mut();
db.system_mut()
.as_any_mut()
.downcast_mut::<LSPSystem>()

75
crates/red_knot_workspace/src/db.rs
View file

@ -1,6 +1,5 @@
use std::panic::{AssertUnwindSafe, RefUnwindSafe};
use salsa::Cancelled;
use std::panic::RefUnwindSafe;
use std::sync::Arc;
use red_knot_module_resolver::{vendored_typeshed_stubs, Db as ResolverDb};
use red_knot_python_semantic::Db as SemanticDb;
@ -9,6 +8,8 @@ use ruff_db::program::{Program, ProgramSettings};
use ruff_db::system::System;
use ruff_db::vendored::VendoredFileSystem;
use ruff_db::{Db as SourceDb, Upcast};
use salsa::plumbing::ZalsaDatabase;
use salsa::{Cancelled, Event};
use crate::lint::Diagnostics;
use crate::workspace::{check_file, Workspace, WorkspaceMetadata};
@ -23,7 +24,7 @@ pub struct RootDatabase {
workspace: Option<Workspace>,
storage: salsa::Storage<RootDatabase>,
files: Files,
system: Box<dyn System + Send + Sync + RefUnwindSafe>,
system: Arc<dyn System + Send + Sync + RefUnwindSafe>,
}
impl RootDatabase {
@ -35,7 +36,7 @@ impl RootDatabase {
workspace: None,
storage: salsa::Storage::default(),
files: Files::default(),
system: Box::new(system),
system: Arc::new(system),
};
let workspace = Workspace::from_metadata(&db, workspace);
@ -60,31 +61,32 @@ impl RootDatabase {
self.with_db(|db| check_file(db, file))
}
/// Returns a mutable reference to the system.
///
/// WARNING: Triggers a new revision, canceling other database handles. This can lead to deadlock.
pub fn system_mut(&mut self) -> &mut dyn System {
// TODO: Use a more official method to cancel other queries.
// https://salsa.zulipchat.com/#narrow/stream/333573-salsa-3.2E0/topic/Expose.20an.20API.20to.20cancel.20other.20queries
let _ = self.zalsa_mut();
Arc::get_mut(&mut self.system).unwrap()
}
pub(crate) fn with_db<F, T>(&self, f: F) -> Result<T, Cancelled>
where
F: FnOnce(&RootDatabase) -> T + std::panic::UnwindSafe,
{
// The `AssertUnwindSafe` here looks scary, but is a consequence of Salsa's design.
// Salsa uses panics to implement cancellation and to recover from cycles. However, the Salsa
// storage isn't `UnwindSafe` or `RefUnwindSafe` because its dependencies `DashMap` and `parking_lot::*` aren't
// unwind safe.
//
// Having to use `AssertUnwindSafe` isn't as big as a deal as it might seem because
// the `UnwindSafe` and `RefUnwindSafe` traits are designed to catch logical bugs.
// They don't protect against [UB](https://internals.rust-lang.org/t/pre-rfc-deprecating-unwindsafe/15974).
// On top of that, `Cancelled` only catches specific Salsa-panics and propagates all other panics.
//
// That still leaves us with possible logical bugs in two sources:
// * In Salsa itself: This must be considered a bug in Salsa and needs fixing upstream.
// Reviewing Salsa code specifically around unwind safety seems doable.
// * Our code: This is the main concern. Luckily, it only involves code that uses internal mutability
// and calls into Salsa queries when mutating the internal state. Using `AssertUnwindSafe`
// certainly makes it harder to catch these issues in our user code.
//
// For now, this is the only solution at hand unless Salsa decides to change its design.
// [Zulip support thread](https://salsa.zulipchat.com/#narrow/stream/145099-general/topic/How.20to.20use.20.60Cancelled.3A.3Acatch.60)
let db = &AssertUnwindSafe(self);
Cancelled::catch(|| f(db))
Cancelled::catch(|| f(self))
}
#[must_use]
pub fn snapshot(&self) -> Self {
Self {
workspace: self.workspace,
storage: self.storage.clone(),
files: self.files.snapshot(),
system: Arc::clone(&self.system),
}
}
}
@ -133,24 +135,23 @@ impl SourceDb for RootDatabase {
&*self.system
}
fn system_mut(&mut self) -> &mut dyn System {
&mut *self.system
}
fn files(&self) -> &Files {
&self.files
}
}
#[salsa::db]
impl salsa::Database for RootDatabase {}
impl salsa::Database for RootDatabase {
fn salsa_event(&self, _event: &dyn Fn() -> Event) {}
}
#[salsa::db]
impl Db for RootDatabase {}
#[cfg(test)]
pub(crate) mod tests {
use crate::db::Db;
use salsa::Event;
use red_knot_module_resolver::{vendored_typeshed_stubs, Db as ResolverDb};
use red_knot_python_semantic::Db as SemanticDb;
use ruff_db::files::Files;
@ -158,6 +159,8 @@ pub(crate) mod tests {
use ruff_db::vendored::VendoredFileSystem;
use ruff_db::{Db as SourceDb, Upcast};
use crate::db::Db;
#[salsa::db]
pub(crate) struct TestDb {
storage: salsa::Storage<Self>,
@ -197,10 +200,6 @@ pub(crate) mod tests {
&self.system
}
fn system_mut(&mut self) -> &mut dyn System {
&mut self.system
}
fn files(&self) -> &Files {
&self.files
}
@ -241,5 +240,7 @@ pub(crate) mod tests {
impl Db for TestDb {}
#[salsa::db]
impl salsa::Database for TestDb {}
impl salsa::Database for TestDb {
fn salsa_event(&self, _event: &dyn Fn() -> Event) {}
}
}

5
crates/red_knot_workspace/src/workspace/files.rs
View file

@ -55,9 +55,10 @@ impl PackageFiles {
///
/// The changes are automatically written back to the database once the view is dropped.
pub fn indexed_mut(db: &mut dyn Db, package: Package) -> Option<IndexedFilesMut> {
// Calling `runtime_mut` cancels all pending salsa queries. This ensures that there are no pending
// Calling `zalsa_mut` cancels all pending salsa queries. This ensures that there are no pending
// reads to the file set.
let _ = db.runtime_mut();
// TODO: Use a non-internal API instead https://salsa.zulipchat.com/#narrow/stream/333573-salsa-3.2E0/topic/Expose.20an.20API.20to.20cancel.20other.20queries
let _ = db.as_dyn_database_mut().zalsa_mut();
let files = package.file_set(db);

9
crates/ruff_db/src/files.rs
View file

@ -252,6 +252,13 @@ impl Files {
.to(FileRevision::now());
}
}
#[must_use]
pub fn snapshot(&self) -> Self {
Self {
inner: Arc::clone(&self.inner),
}
}
}
impl std::fmt::Debug for Files {
@ -265,6 +272,8 @@ impl std::fmt::Debug for Files {
}
}
impl std::panic::RefUnwindSafe for Files {}
/// A file that's either stored on the host system's file system or in the vendored file system.
#[salsa::input]
pub struct File {

16
crates/ruff_db/src/lib.rs
View file

@ -23,7 +23,6 @@ pub type FxDashSet<K> = dashmap::DashSet<K, BuildHasherDefault<FxHasher>>;
pub trait Db: salsa::Database {
fn vendored(&self) -> &VendoredFileSystem;
fn system(&self) -> &dyn System;
fn system_mut(&mut self) -> &mut dyn System;
fn files(&self) -> &Files;
}
@ -104,10 +103,6 @@ mod tests {
&self.system
}
fn system_mut(&mut self) -> &mut dyn System {
&mut self.system
}
fn files(&self) -> &Files {
&self.files
}
@ -125,12 +120,11 @@ mod tests {
#[salsa::db]
impl salsa::Database for TestDb {
fn salsa_event(&self, event: salsa::Event) {
salsa::Database::attach(self, |_| {
tracing::trace!("event: {:?}", event);
let mut events = self.events.lock().unwrap();
events.push(event);
});
fn salsa_event(&self, event: &dyn Fn() -> salsa::Event) {
let event = event();
tracing::trace!("event: {:?}", event);
let mut events = self.events.lock().unwrap();
events.push(event);
}
}
}

4
crates/ruff_db/src/testing.rs
View file

@ -76,9 +76,7 @@ where
let event = events.iter().find(|event| {
if let salsa::EventKind::WillExecute { database_key } = event.kind {
db.lookup_ingredient(database_key.ingredient_index())
.debug_name()
== query_name
db.ingredient_debug_name(database_key.ingredient_index()) == query_name
&& database_key.key_index() == input.as_id()
} else {
false