From 9e2cb5f40bb5b2bc34dcf0e7280e165182b49565 Mon Sep 17 00:00:00 2001 From: Piotr Magiera Date: Mon, 25 Nov 2024 15:48:28 +0100 Subject: [PATCH] LS: Send project model updates through channel commit-id:d41ad059 --- .../src/lang/db/swapper.rs | 27 +---- crates/cairo-lang-language-server/src/lib.rs | 40 ++++--- .../src/project/mod.rs | 113 ++++++++++++------ .../src/project/scarb.rs | 19 +-- .../src/server/routing/traits.rs | 18 +-- .../src/toolchain/scarb.rs | 5 + 6 files changed, 117 insertions(+), 105 deletions(-) diff --git a/crates/cairo-lang-language-server/src/lang/db/swapper.rs b/crates/cairo-lang-language-server/src/lang/db/swapper.rs index 7aeebd8786e..06d96e65a9a 100644 --- a/crates/cairo-lang-language-server/src/lang/db/swapper.rs +++ b/crates/cairo-lang-language-server/src/lang/db/swapper.rs @@ -12,12 +12,10 @@ use cairo_lang_utils::{Intern, LookupIntern}; use lsp_types::Url; use tracing::{error, warn}; -use crate::config::Config; use crate::lang::db::AnalysisDatabase; use crate::lang::lsp::LsProtoGroup; use crate::lang::proc_macros::db::ProcMacroGroup; use crate::project::ProjectController; -use crate::server::client::Notifier; use crate::toolchain::scarb::ScarbToolchain; use crate::{Tricks, env_config}; @@ -57,9 +55,7 @@ impl AnalysisDatabaseSwapper { &mut self, db: &mut AnalysisDatabase, open_files: &HashSet, - config: &Config, tricks: &Tricks, - notifier: &Notifier, project_controller: &mut ProjectController, ) { let Ok(elapsed) = self.last_replace.elapsed() else { @@ -76,7 +72,7 @@ impl AnalysisDatabaseSwapper { return; } - self.swap(db, open_files, config, tricks, notifier, project_controller) + self.swap(db, open_files, tricks, project_controller) } /// Swaps the database. @@ -85,9 +81,7 @@ impl AnalysisDatabaseSwapper { &mut self, db: &mut AnalysisDatabase, open_files: &HashSet, - config: &Config, tricks: &Tricks, - notifier: &Notifier, project_controller: &mut ProjectController, ) { let Ok(new_db) = catch_unwind(AssertUnwindSafe(|| { @@ -96,13 +90,7 @@ impl AnalysisDatabaseSwapper { let mut new_db = AnalysisDatabase::new(tricks); self.migrate_proc_macro_state(&mut new_db, db); self.migrate_file_overrides(&mut new_db, db, open_files); - self.detect_crates_for_open_files( - &mut new_db, - open_files, - config, - notifier, - project_controller, - ); + self.detect_crates_for_open_files(open_files, project_controller); new_db })) else { error!("caught panic when preparing new db for swap"); @@ -156,10 +144,7 @@ impl AnalysisDatabaseSwapper { /// Ensures all open files have their crates detected to regenerate crates state. fn detect_crates_for_open_files( &self, - new_db: &mut AnalysisDatabase, open_files: &HashSet, - config: &Config, - notifier: &Notifier, project_controller: &mut ProjectController, ) { for uri in open_files { @@ -168,13 +153,7 @@ impl AnalysisDatabaseSwapper { continue; }; - project_controller.detect_crate_for( - new_db, - &self.scarb_toolchain, - config, - &file_path, - notifier, - ); + project_controller.update_project_for(&self.scarb_toolchain, &file_path); } } } diff --git a/crates/cairo-lang-language-server/src/lib.rs b/crates/cairo-lang-language-server/src/lib.rs index 9c3c1855f0c..d6bfc94600b 100644 --- a/crates/cairo-lang-language-server/src/lib.rs +++ b/crates/cairo-lang-language-server/src/lib.rs @@ -48,7 +48,7 @@ use anyhow::Result; use cairo_lang_filesystem::db::FilesGroup; use cairo_lang_filesystem::ids::FileLongId; use cairo_lang_semantic::plugin::PluginSuite; -use crossbeam::select; +use crossbeam::channel::{Receiver, select_biased}; use lsp_server::Message; use lsp_types::RegistrationParams; use salsa::{Database, Durability}; @@ -60,6 +60,7 @@ use crate::lsp::capabilities::server::{ collect_dynamic_registrations, collect_server_capabilities, }; use crate::lsp::result::LSPResult; +use crate::project::{ProjectController, ProjectUpdate}; use crate::server::client::{Notifier, Requester, Responder}; use crate::server::connection::{Connection, ConnectionInitializer}; use crate::server::panic::is_cancelled; @@ -271,6 +272,7 @@ impl Backend { let Self { mut state, connection } = self; let proc_macro_channels = state.proc_macro_controller.init_channels(); + let project_updates_receiver = state.project_controller.init_channel(); let mut scheduler = Scheduler::new(&mut state, connection.make_sender()); Self::dispatch_setup_tasks(&mut scheduler); @@ -285,7 +287,12 @@ impl Backend { // we basically never hit such a case in CairoLS in happy paths. scheduler.on_sync_task(Self::refresh_diagnostics); - let result = Self::event_loop(&connection, proc_macro_channels, scheduler); + let result = Self::event_loop( + &connection, + proc_macro_channels, + project_updates_receiver, + scheduler, + ); // Trigger cancellation in any background tasks that might still be running. state.db.salsa_runtime_mut().synthetic_write(Durability::LOW); @@ -340,12 +347,21 @@ impl Backend { fn event_loop( connection: &Connection, proc_macro_channels: ProcMacroChannelsReceivers, + project_updates_receiver: Receiver, mut scheduler: Scheduler<'_>, ) -> Result<()> { let incoming = connection.incoming(); loop { - select! { + select_biased! { + // Project updates may significantly change the state, therefore + // they should be handled first in case of multiple operations being ready at once. + // To ensure it, keep project updates channel in the first arm of `select_biased!`. + recv(project_updates_receiver) -> project_update => { + let Ok(project_update) = project_update else { break }; + + scheduler.local(move |state, notifier, _, _| ProjectController::handle_update(state, notifier, project_update)); + } recv(incoming) -> msg => { let Ok(msg) = msg else { break }; @@ -388,13 +404,11 @@ impl Backend { } /// Calls [`lang::db::AnalysisDatabaseSwapper::maybe_swap`] to do its work. - fn maybe_swap_database(state: &mut State, notifier: Notifier) { + fn maybe_swap_database(state: &mut State, _notifier: Notifier) { state.db_swapper.maybe_swap( &mut state.db, &state.open_files, - &state.config, &state.tricks, - ¬ifier, &mut state.project_controller, ); } @@ -405,24 +419,14 @@ impl Backend { } /// Reload crate detection for all open files. - fn reload( - state: &mut State, - notifier: &Notifier, - requester: &mut Requester<'_>, - ) -> LSPResult<()> { + fn reload(state: &mut State, requester: &mut Requester<'_>) -> LSPResult<()> { state.project_controller.clear_loaded_workspaces(); state.config.reload(requester, &state.client_capabilities)?; for uri in state.open_files.iter() { let Some(file_id) = state.db.file_for_url(uri) else { continue }; if let FileLongId::OnDisk(file_path) = state.db.lookup_intern_file(file_id) { - state.project_controller.detect_crate_for( - &mut state.db, - &state.scarb_toolchain, - &state.config, - &file_path, - notifier, - ); + state.project_controller.update_project_for(&state.scarb_toolchain, &file_path); } } diff --git a/crates/cairo-lang-language-server/src/project/mod.rs b/crates/cairo-lang-language-server/src/project/mod.rs index 83b5f6a398f..d6c16233c66 100644 --- a/crates/cairo-lang-language-server/src/project/mod.rs +++ b/crates/cairo-lang-language-server/src/project/mod.rs @@ -1,3 +1,4 @@ +use std::cell::OnceCell; use std::collections::HashSet; use std::path::{Path, PathBuf}; @@ -5,46 +6,44 @@ use anyhow::Context; use cairo_lang_compiler::db::validate_corelib; use cairo_lang_compiler::project::{setup_project, update_crate_roots_from_project_config}; use cairo_lang_project::ProjectConfig; -use tracing::{error, trace, warn}; +use crossbeam::channel::{Receiver, Sender}; +use tracing::{debug, error, trace, warn}; pub use self::crate_data::Crate; pub use self::project_manifest_path::*; -use crate::config::Config; -use crate::lang::db::AnalysisDatabase; -use crate::lsp::ext::{CorelibVersionMismatch, ScarbMetadataFailed}; -use crate::project::scarb::{get_workspace_members_manifests, update_crate_roots}; +use crate::lsp::ext::CorelibVersionMismatch; +use crate::project::scarb::{extract_crates, get_workspace_members_manifests}; use crate::project::unmanaged_core_crate::try_to_init_unmanaged_core; use crate::server::client::Notifier; -use crate::state::Owned; +use crate::state::{Owned, State}; use crate::toolchain::scarb::ScarbToolchain; mod crate_data; mod project_manifest_path; -// TODO(mkaput): These two are `pub` temporarily. -pub(crate) mod scarb; -pub(crate) mod unmanaged_core_crate; +mod scarb; +mod unmanaged_core_crate; pub struct ProjectController { loaded_scarb_manifests: Owned>, + sender: OnceCell>, } impl ProjectController { pub fn new() -> Self { - ProjectController { loaded_scarb_manifests: Default::default() } + ProjectController { loaded_scarb_manifests: Default::default(), sender: Default::default() } } - /// Tries to detect the crate root the config that contains a cairo file, and add it to the - /// system. + pub fn init_channel(&mut self) -> Receiver { + let (sender, receiver) = crossbeam::channel::unbounded(); + + self.sender.set(sender).expect("failed to set sender once cell"); + receiver + } + + /// Tries to fetch changes to the project model that are necessary for the file analysis. #[tracing::instrument(skip_all)] - pub fn detect_crate_for( - &mut self, - db: &mut AnalysisDatabase, - scarb_toolchain: &ScarbToolchain, - config: &Config, - file_path: &Path, - notifier: &Notifier, - ) { - match ProjectManifestPath::discover(file_path) { + pub fn update_project_for(&mut self, scarb_toolchain: &ScarbToolchain, file_path: &Path) { + let project_update = match ProjectManifestPath::discover(file_path) { Some(ProjectManifestPath::Scarb(manifest_path)) => { if self.loaded_scarb_manifests.contains(&manifest_path) { trace!("scarb project is already loaded: {}", manifest_path.display()); @@ -57,22 +56,18 @@ impl ProjectController { format!("failed to refresh scarb workspace: {}", manifest_path.display()) }) .inspect_err(|err| { - warn!("{err:?}"); - notifier.notify::(()); + error!("{err:?}"); }) .ok(); - if let Some(metadata) = metadata { + let maybe_crates = if let Some(metadata) = metadata { self.loaded_scarb_manifests.extend(get_workspace_members_manifests(&metadata)); - update_crate_roots(&metadata, db); + Some(extract_crates(&metadata)) } else { - // Try to set up a corelib at least. - try_to_init_unmanaged_core(db, config, scarb_toolchain); - } + None + }; - if let Err(result) = validate_corelib(db) { - notifier.notify::(result.to_string()); - } + ProjectUpdate::Scarb(maybe_crates) } Some(ProjectManifestPath::CairoProject(config_path)) => { @@ -80,25 +75,67 @@ impl ProjectController { // DB will also be absolute. assert!(config_path.is_absolute()); - try_to_init_unmanaged_core(db, config, scarb_toolchain); + let maybe_project_config = ProjectConfig::from_file(&config_path) + // TODO: send failure notification + .inspect_err(|err| error!("{err:?}")) + .ok(); + ProjectUpdate::CairoProjectToml(maybe_project_config) + } + + None => ProjectUpdate::NoConfig(file_path.to_path_buf()), + }; - if let Ok(config) = ProjectConfig::from_file(&config_path) { - update_crate_roots_from_project_config(db, &config); - }; + self.sender + .get() + .expect("failed to get sender once cell") + .send(project_update) + .expect("the receiver was expected to exist in the main event loop"); + } + + pub fn handle_update(state: &mut State, notifier: Notifier, project_update: ProjectUpdate) { + let db = &mut state.db; + match project_update { + ProjectUpdate::Scarb(crates) => { + if let Some(crates) = crates { + debug!("updating crate roots from scarb metadata: {crates:#?}"); + + for cr in crates { + cr.apply(db); + } + } else { + // Try to set up a corelib at least. + try_to_init_unmanaged_core(db, &state.config, &state.scarb_toolchain); + } } + ProjectUpdate::CairoProjectToml(maybe_project_config) => { + try_to_init_unmanaged_core(db, &state.config, &state.scarb_toolchain); - None => { - try_to_init_unmanaged_core(db, config, scarb_toolchain); + if let Some(project_config) = maybe_project_config { + update_crate_roots_from_project_config(db, &project_config); + } + } + ProjectUpdate::NoConfig(file_path) => { + try_to_init_unmanaged_core(db, &state.config, &state.scarb_toolchain); - if let Err(err) = setup_project(&mut *db, file_path) { + if let Err(err) = setup_project(&mut *db, &file_path) { let file_path_s = file_path.to_string_lossy(); error!("error loading file {file_path_s} as a single crate: {err}"); } } } + + if let Err(result) = validate_corelib(db) { + notifier.notify::(result.to_string()); + } } pub fn clear_loaded_workspaces(&mut self) { self.loaded_scarb_manifests.clear() } } + +pub enum ProjectUpdate { + Scarb(Option>), + CairoProjectToml(Option), + NoConfig(PathBuf), +} diff --git a/crates/cairo-lang-language-server/src/project/scarb.rs b/crates/cairo-lang-language-server/src/project/scarb.rs index 146b22811b8..0f7087b9e5d 100644 --- a/crates/cairo-lang-language-server/src/project/scarb.rs +++ b/crates/cairo-lang-language-server/src/project/scarb.rs @@ -28,22 +28,19 @@ pub fn get_workspace_members_manifests(metadata: &Metadata) -> Vec { .collect() } -/// Updates crate roots in the database with the information from Scarb metadata. +/// Extract information about crates that should be loaded to db from Scarb metadata. /// -/// This function attempts to be graceful. Any erroneous cases will be reported as warnings in logs, -/// and the database will be left intact for problematic crates. +/// This function attempts to be graceful. Any erroneous cases will be reported as warnings in logs. /// -/// In all real-world scenarios, this function should always initialize the `core` crate. +/// In all real-world scenarios, this function should always extract info about the `core` crate. /// Technically, it is possible for `scarb metadata` to omit `core` if working on a `no-core` /// package, but in reality enabling `no-core` makes sense only for the `core` package itself. To /// leave a trace of unreal cases, this function will log a warning if `core` is missing. -/// -/// Package ID is used as a discriminator for all crates except `core`. -// FIXME(mkaput): Currently this logic is feeding all compilation units of the single package at +// FIXME(mkaput): Currently this logic is collecting all compilation units of the single package at // once. Often packages declare several targets (lib, starknet-contract, test), which currently // causes overriding of the crate within single call of this function. This is an UX problem, for // which we do not know the solution yet. -pub fn update_crate_roots(metadata: &Metadata, db: &mut AnalysisDatabase) { +pub fn extract_crates(metadata: &Metadata) -> Vec { // A crate can appear as a component in multiple compilation units. // We use a map here to make sure we include dependencies and cfg sets from all CUs. // We can keep components with assigned group id separately as they are not affected by this; @@ -243,15 +240,11 @@ pub fn update_crate_roots(metadata: &Metadata, db: &mut AnalysisDatabase) { }); } - debug!("updating crate roots from scarb metadata: {crates:#?}"); - if !crates.iter().any(Crate::is_core) { warn!("core crate is missing in scarb metadata, did not initialize it"); } - for cr in crates { - cr.apply(db); - } + crates } /// Perform sanity checks on crate _source path_, and chop it into directory path and file stem. diff --git a/crates/cairo-lang-language-server/src/server/routing/traits.rs b/crates/cairo-lang-language-server/src/server/routing/traits.rs index 64e1e2918ad..6f589a43716 100644 --- a/crates/cairo-lang-language-server/src/server/routing/traits.rs +++ b/crates/cairo-lang-language-server/src/server/routing/traits.rs @@ -92,7 +92,7 @@ impl SyncRequestHandler for ExecuteCommand { )] fn run( state: &mut State, - notifier: Notifier, + _notifier: Notifier, requester: &mut Requester<'_>, params: ExecuteCommandParams, ) -> LSPResult> { @@ -101,7 +101,7 @@ impl SyncRequestHandler for ExecuteCommand { if let Ok(cmd) = command { match cmd { ServerCommands::Reload => { - Backend::reload(state, ¬ifier, requester)?; + Backend::reload(state, requester)?; } } } @@ -177,7 +177,7 @@ impl SyncNotificationHandler for DidChangeWatchedFiles { #[tracing::instrument(name = "workspace/didChangeWatchedFiles", skip_all)] fn run( state: &mut State, - notifier: Notifier, + _notifier: Notifier, requester: &mut Requester<'_>, params: DidChangeWatchedFilesParams, ) -> LSPResult<()> { @@ -197,7 +197,7 @@ impl SyncNotificationHandler for DidChangeWatchedFiles { // metadata call, so it is easy to fall in a loop here. if ["Scarb.toml", "cairo_project.toml"].map(Some).contains(&changed_file_name.to_str()) { - Backend::reload(state, ¬ifier, requester)?; + Backend::reload(state, requester)?; } } @@ -233,7 +233,7 @@ impl SyncNotificationHandler for DidOpenTextDocument { )] fn run( state: &mut State, - notifier: Notifier, + _notifier: Notifier, _requester: &mut Requester<'_>, params: DidOpenTextDocumentParams, ) -> LSPResult<()> { @@ -244,13 +244,7 @@ impl SyncNotificationHandler for DidOpenTextDocument { if uri.scheme() == "file" { let Ok(path) = uri.to_file_path() else { return Ok(()) }; - state.project_controller.detect_crate_for( - &mut state.db, - &state.scarb_toolchain, - &state.config, - &path, - ¬ifier, - ); + state.project_controller.update_project_for(&state.scarb_toolchain, &path); } if let Some(file_id) = state.db.file_for_url(&uri) { diff --git a/crates/cairo-lang-language-server/src/toolchain/scarb.rs b/crates/cairo-lang-language-server/src/toolchain/scarb.rs index 3f77da3291e..d52fead5d5c 100644 --- a/crates/cairo-lang-language-server/src/toolchain/scarb.rs +++ b/crates/cairo-lang-language-server/src/toolchain/scarb.rs @@ -8,6 +8,7 @@ use scarb_metadata::{Metadata, MetadataCommand}; use tracing::{error, warn}; use crate::env_config; +use crate::lsp::ext::ScarbMetadataFailed; use crate::server::client::Notifier; pub const SCARB_TOML: &str = "Scarb.toml"; @@ -126,6 +127,10 @@ impl ScarbToolchain { if !self.is_silent { self.notifier.notify::(()); + + if result.is_err() { + self.notifier.notify::(()); + } } result