From 26ce43ca3f87e9a795f1f5889dba5f3a55ed8024 Mon Sep 17 00:00:00 2001 From: Marek Kaput Date: Tue, 27 Aug 2024 08:03:03 +0200 Subject: [PATCH] LS: Look for unmanaged `core` in Scarb (#6248) --- Cargo.lock | 1 + crates/cairo-lang-language-server/Cargo.toml | 1 + crates/cairo-lang-language-server/src/lib.rs | 42 +++++---- .../src/project/unmanaged_core_crate.rs | 91 ++++++++++++++++--- .../src/toolchain/scarb.rs | 68 ++++++++++++-- 5 files changed, 166 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52b1594bc2d..acc5a55613f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -669,6 +669,7 @@ dependencies = [ "serde", "serde_json", "smol_str", + "tempfile", "test-log", "tokio", "tower-lsp", diff --git a/crates/cairo-lang-language-server/Cargo.toml b/crates/cairo-lang-language-server/Cargo.toml index 06346c3aab3..8014fece00f 100644 --- a/crates/cairo-lang-language-server/Cargo.toml +++ b/crates/cairo-lang-language-server/Cargo.toml @@ -33,6 +33,7 @@ scarb-metadata = "1.12" serde = { workspace = true, default-features = true } serde_json.workspace = true smol_str.workspace = true +tempfile = "3" tokio.workspace = true tower-lsp = "0.20.0" tracing = "0.1" diff --git a/crates/cairo-lang-language-server/src/lib.rs b/crates/cairo-lang-language-server/src/lib.rs index c083aa40101..68f29aece21 100644 --- a/crates/cairo-lang-language-server/src/lib.rs +++ b/crates/cairo-lang-language-server/src/lib.rs @@ -571,21 +571,23 @@ impl Backend { async fn detect_crate_for(&self, db: &mut AnalysisDatabase, file_path: PathBuf) { match ProjectManifestPath::discover(&file_path) { Some(ProjectManifestPath::Scarb(manifest_path)) => { - let scarb = self.scarb_toolchain.clone(); - let Ok(metadata) = spawn_blocking(move || { - scarb - .metadata(&manifest_path) - .with_context(|| { - format!( - "failed to refresh scarb workspace: {}", - manifest_path.display() - ) - }) - .inspect_err(|e| { - // TODO(mkaput): Send a notification to the language client. - warn!("{e:?}"); - }) - .ok() + let Ok(metadata) = spawn_blocking({ + let scarb = self.scarb_toolchain.clone(); + move || { + scarb + .metadata(&manifest_path) + .with_context(|| { + format!( + "failed to refresh scarb workspace: {}", + manifest_path.display() + ) + }) + .inspect_err(|e| { + // TODO(mkaput): Send a notification to the language client. + warn!("{e:?}"); + }) + .ok() + } }) .await else { @@ -597,7 +599,11 @@ impl Backend { update_crate_roots(&metadata, db); } else { // Try to set up a corelib at least. - try_to_init_unmanaged_core(&*self.config.read().await, db); + try_to_init_unmanaged_core( + db, + &*self.config.read().await, + &self.scarb_toolchain, + ); } if let Err(result) = validate_corelib(db) { @@ -612,7 +618,7 @@ impl Backend { // DB will also be absolute. assert!(config_path.is_absolute()); - try_to_init_unmanaged_core(&*self.config.read().await, db); + try_to_init_unmanaged_core(db, &*self.config.read().await, &self.scarb_toolchain); if let Ok(config) = ProjectConfig::from_file(&config_path) { update_crate_roots_from_project_config(db, &config); @@ -620,7 +626,7 @@ impl Backend { } None => { - try_to_init_unmanaged_core(&*self.config.read().await, db); + try_to_init_unmanaged_core(db, &*self.config.read().await, &self.scarb_toolchain); if let Err(err) = setup_project(&mut *db, file_path.as_path()) { let file_path_s = file_path.to_string_lossy(); diff --git a/crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs b/crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs index b6be0324f97..2df1cad7c33 100644 --- a/crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs +++ b/crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs @@ -1,17 +1,25 @@ -use std::path; use std::path::{Path, PathBuf}; +use std::sync::OnceLock; +use std::{fs, path}; use anyhow::Context; -use cairo_lang_filesystem::db::init_dev_corelib; +use cairo_lang_filesystem::db::{init_dev_corelib, CORELIB_CRATE_NAME}; +use indoc::indoc; +use tempfile::tempdir; use tracing::{error, warn}; use crate::config::Config; use crate::lang::db::AnalysisDatabase; +use crate::toolchain::scarb::{ScarbToolchain, SCARB_TOML}; /// Try to find a Cairo `core` crate (see [`find_unmanaged_core`]) and initialize it in the /// provided database. -pub fn try_to_init_unmanaged_core(config: &Config, db: &mut AnalysisDatabase) { - if let Some(path) = find_unmanaged_core(config) { +pub fn try_to_init_unmanaged_core( + db: &mut AnalysisDatabase, + config: &Config, + scarb: &ScarbToolchain, +) { + if let Some(path) = find_unmanaged_core(config, scarb) { init_dev_corelib(db, path); } else { warn!("failed to find unmanaged core crate") @@ -22,13 +30,16 @@ pub fn try_to_init_unmanaged_core(config: &Config, db: &mut AnalysisDatabase) { /// do not manage the `core` crate (i.e., anything non-Scarb). /// /// The path is guaranteed to be absolute, so it can be safely used as a `FileId` in LS Salsa DB. -pub fn find_unmanaged_core(config: &Config) -> Option { - // TODO(mkaput): First, try to find Scarb-managed `core` package if we have Scarb toolchain. - // The most reliable way to do this is to create an empty Scarb package, and run - // `scarb metadata` on it. The `core` package will be a component of this empty package. - // For minimal packages, `scarb metadata` should be pretty fast. +pub fn find_unmanaged_core(config: &Config, scarb: &ScarbToolchain) -> Option { find_core_at_config_path(config) - .or_else(cairo_lang_filesystem::detect::detect_corelib) + .or_else(|| find_scarb_managed_core(scarb)) + .or_else(|| { + if cfg!(feature = "testing") { + cairo_lang_filesystem::detect::detect_corelib() + } else { + None + } + }) .and_then(ensure_absolute) } @@ -39,8 +50,8 @@ fn find_core_at_config_path(config: &Config) -> Option { /// Attempts to find the `core` crate source root at a given path. /// -/// In the [starkware-libs/cairo] repository, the `core` crate sits in `./corelib/src`, this is the -/// first place looked for. +/// In the [starkware-libs/cairo] repository, the `core` crate sits in `./corelib/src`. +/// This is the first place looked for. /// The `core` crate is a regular Scarb package, so it sounds obvious that the user may provide a /// path to the directory containing the manifest file, hence next this function looks for `./src`. /// Finally, the input path is considered as a candidate and is just checked for existence. @@ -67,6 +78,62 @@ fn find_core_at_path(root_path: &Path) -> Option { None } +// TODO(#6246): Add tests for this logic. Pay attention to usage of silent mode in ScarbToolchain. +/// Try to find a Scarb-managed `core` package if we have Scarb toolchain. +/// +/// The easiest way to do this is to create an empty Scarb package and run `scarb metadata` on it. +/// The `core` package will be a component of this empty package. +/// For minimal packages, `scarb metadata` should be pretty fast. +/// +/// Because CairoLS is tightly bound to Scarb (due to hard compiler version dependency), +/// we can safely make this lookup once and keep it cached for the entire LS lifetime. +#[tracing::instrument(level = "trace", skip_all)] +fn find_scarb_managed_core(scarb: &ScarbToolchain) -> Option { + let lookup = || { + let workspace = tempdir() + .context("failed to create temporary directory") + .inspect_err(|e| warn!("{e:?}")) + .ok()?; + + let scarb_toml = workspace.path().join(SCARB_TOML); + fs::write( + &scarb_toml, + indoc! {r#" + [package] + name = "cairols_unmanaged_core_lookup" + version = "1.0.0" + "#}, + ) + .context("failed to write Scarb.toml") + .inspect_err(|e| warn!("{e:?}")) + .ok()?; + + let metadata = scarb.silent().metadata(&scarb_toml).inspect_err(|e| warn!("{e:?}")).ok()?; + + // Ensure the workspace directory is deleted after running Scarb. + // We are ignoring the error, leaving doing proper cleanup to the OS. + let _ = workspace + .close() + .context("failed to wipe temporary directory") + .inspect_err(|e| warn!("{e:?}")); + + // Scarb is expected to generate only one compilation unit (for our stub package) + // that will consist of this package and the `core` crate. + // Therefore, we allow ourselves to liberally just look for any first usage of a package + // named `core` in all compilation units components we got. + metadata.compilation_units.into_iter().find_map(|compilation_unit| { + compilation_unit + .components + .iter() + .find(|component| component.name == CORELIB_CRATE_NAME) + .map(|component| component.source_root().to_path_buf().into_std_path_buf()) + }) + }; + + static CACHE: OnceLock> = OnceLock::new(); + CACHE.get_or_init(lookup).clone() +} + /// Makes a path absolute, or logs an error. fn ensure_absolute(path: PathBuf) -> Option { path::absolute(&path) diff --git a/crates/cairo-lang-language-server/src/toolchain/scarb.rs b/crates/cairo-lang-language-server/src/toolchain/scarb.rs index d15e2635c41..445d3974b2d 100644 --- a/crates/cairo-lang-language-server/src/toolchain/scarb.rs +++ b/crates/cairo-lang-language-server/src/toolchain/scarb.rs @@ -4,7 +4,7 @@ use std::sync::{Arc, OnceLock}; use anyhow::{bail, Context, Result}; use scarb_metadata::{Metadata, MetadataCommand}; use tower_lsp::lsp_types::notification::Notification; -use tracing::warn; +use tracing::{error, warn}; use crate::env_config; use crate::server::notifier::Notifier; @@ -22,28 +22,44 @@ pub const SCARB_TOML: &str = "Scarb.toml"; pub struct ScarbToolchain { /// Cached path to the `scarb` executable. scarb_path_cell: Arc>>, + /// The notifier object used to send notifications to the language client. notifier: Notifier, + + /// States whether this instance is in _silent mode_. + /// + /// See [`ScarbToolchain::silent`] for more info. + is_silent: bool, } impl ScarbToolchain { /// Constructs a new [`ScarbToolchain`]. pub fn new(notifier: &Notifier) -> Self { - ScarbToolchain { scarb_path_cell: Default::default(), notifier: notifier.clone() } + ScarbToolchain { + scarb_path_cell: Default::default(), + notifier: notifier.clone(), + is_silent: false, + } } /// Finds the path to the `scarb` executable to use. /// /// This method may send notifications to the language client if there are any actionable issues - /// with the found `scarb` installation, or if it could not be found. + /// with the found `scarb` installation or if it could not be found. fn discover(&self) -> Option<&Path> { self.scarb_path_cell .get_or_init(|| { let path = env_config::scarb_path(); // TODO(mkaput): Perhaps we should display this notification again after reloading? if path.is_none() { - warn!("attempt to use scarb without SCARB env being set"); - self.notifier.send_notification::(()); + if self.is_silent { + // If we are in silent mode, then missing Scarb is probably dealt with + // at the caller site. + warn!("attempt to use scarb without SCARB env being set"); + } else { + error!("attempt to use scarb without SCARB env being set"); + self.notifier.send_notification::(()); + } } path }) @@ -51,6 +67,40 @@ impl ScarbToolchain { .map(PathBuf::as_path) } + /// Creates a clone instance of this object that will be in _silent mode_. + /// + /// Silent mode means that any operations invoked through this instance should avoid performing + /// any user-visible actions. + pub fn silent(&self) -> Self { + if self.is_silent { + // Going silent from silent is noop, so skip any shenanigans we do here. + self.clone() + } else { + Self { + // Disassociate this instance from the shared path cell if it has not been + // initialized yet. + // + // This maintains a good UX for the following scenario (timeline): + // 1. CairoLS is started without a path to Scarb provided. + // 2. Some internal operation is silently attempting to query Scarb, which will + // initialize the cell but only log a warning. + // 3. User-invoked operation makes an attempt to query Scarb. + // + // At this point we want to show missing Scarb notification, + // but without this trick we would never do + // as the path cell would be already initialized. + scarb_path_cell: match self.scarb_path_cell.get() { + Some(_) => self.scarb_path_cell.clone(), + None => Default::default(), + }, + + notifier: self.notifier.clone(), + + is_silent: true, + } + } + } + /// Calls `scarb metadata` for the given `Scarb.toml` and parse its output. /// /// This is a blocking operation that may be long-running. It should only be called from within @@ -66,7 +116,9 @@ impl ScarbToolchain { bail!("could not find scarb executable"); }; - self.notifier.send_notification::(()); + if !self.is_silent { + self.notifier.send_notification::(()); + } let result = MetadataCommand::new() .scarb_path(scarb_path) @@ -75,7 +127,9 @@ impl ScarbToolchain { .exec() .context("failed to execute: scarb metadata"); - self.notifier.send_notification::(()); + if !self.is_silent { + self.notifier.send_notification::(()); + } result }