Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: protect scanned files against closing by LSP #4735

Merged
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
1 change: 1 addition & 0 deletions crates/biome_cli/src/execute/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ pub(crate) fn run(migrate_payload: MigratePayload) -> Result<(), CliDiagnostic>
content: FileContent::FromClient(biome_config_content.to_string()),
version: 0,
document_file_source: Some(JsonFileSource::json().into()),
persist_node_cache: false,
})?;
let parsed = parse_json_with_cache(
&biome_config_content,
Expand Down
1 change: 1 addition & 0 deletions crates/biome_cli/src/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ pub fn execute_mode(
path: report_file.clone(),
version: 0,
document_file_source: None,
persist_node_cache: false,
})?;
let code = session.app.workspace.format_file(FormatFileParams {
path: report_file.clone(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ impl<'ctx, 'app> WorkspaceFile<'ctx, 'app> {
path: biome_path,
version: 0,
content: FileContent::FromClient(input.clone()),
persist_node_cache: false,
},
)
.with_file_path_and_code(path.display().to_string(), category!("internalError/fs"))?;
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_cli/src/execute/std_in.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub(crate) fn run<'a>(
version: 0,
content: FileContent::FromClient(content.into()),
document_file_source: None,
persist_node_cache: false,
})?;
let printed = workspace.format_file(FormatFileParams {
path: biome_path.clone(),
Expand Down Expand Up @@ -82,6 +83,7 @@ pub(crate) fn run<'a>(
version: 0,
content: FileContent::FromClient(content.into()),
document_file_source: None,
persist_node_cache: false,
})?;
// apply fix file of the linter
let file_features = workspace.file_features(SupportsFeatureParams {
Expand Down
10 changes: 10 additions & 0 deletions crates/biome_fs/src/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ impl BiomePath {
pub fn is_to_inspect(&self) -> bool {
self.kind.contains(FileKind::Inspectable)
}

pub fn to_path_buf(&self) -> PathBuf {
self.path.clone()
}
Comment on lines +197 to +199
Copy link
Member

Choose a reason for hiding this comment

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

Do you think there's a need for this function? &BiomePath already exposes this function, because BiomePath implements Deref, which returns &Path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. Then I’ll remove it again 👍

}

#[cfg(feature = "serde")]
Expand All @@ -214,6 +218,12 @@ impl Deref for BiomePath {
}
}

impl From<BiomePath> for PathBuf {
fn from(path: BiomePath) -> Self {
path.path
}
}

impl PartialOrd for BiomePath {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
Expand Down
1 change: 1 addition & 0 deletions crates/biome_lsp/src/handlers/text_document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub(crate) async fn did_open(
version,
content: FileContent::FromClient(content),
document_file_source: Some(language_hint),
persist_node_cache: true,
})?;

session.insert_document(url.clone(), doc);
Expand Down
4 changes: 3 additions & 1 deletion crates/biome_lsp/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,9 @@ impl LanguageServer for LSPServer {
let result = self
.session
.workspace
.unregister_project_folder(UnregisterProjectFolderParams { path: project_path })
.unregister_project_folder(UnregisterProjectFolderParams {
path: project_path.into(),
})
.map_err(into_lsp_error);

if let Err(err) = result {
Expand Down
1 change: 0 additions & 1 deletion crates/biome_service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ pub mod documentation;
pub mod file_handlers;

pub mod matcher;
mod scanner;
pub mod settings;
pub mod workspace;

Expand Down
53 changes: 29 additions & 24 deletions crates/biome_service/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,16 @@ pub struct WorkspaceSettings {
}

impl WorkspaceSettings {
/// Returns the key of the current project.
pub fn get_current_project_key(&self) -> ProjectKey {
*self.current_project.read().unwrap()
}

/// Sets which project is the current one by its key.
pub fn set_current_project(&self, key: ProjectKey) {
*self.current_project.write().unwrap() = key;
}

/// Retrieves the settings of the current workspace folder
pub fn get_current_settings(&self) -> Option<Settings> {
trace!("Current key {:?}", self.current_project);
Expand All @@ -78,6 +84,15 @@ impl WorkspaceSettings {
.map(|data| data.settings.clone())
}

/// Retrieves the files settings of the current workspace folder
pub fn get_current_files_settings(&self) -> Option<FilesSettings> {
trace!("Current key {:?}", self.current_project);
self.data
.pin()
.get(&self.get_current_project_key())
.map(|data| data.settings.files.clone())
}

/// Sets the settings of the current workspace folder.
pub fn set_current_settings(&self, settings: Settings) {
let data = self.data.pin();
Expand All @@ -95,15 +110,6 @@ impl WorkspaceSettings {
data.insert(project_key, project_data);
}

/// Retrieves the files settings of the current workspace folder
pub fn get_current_files_settings(&self) -> Option<FilesSettings> {
trace!("Current key {:?}", self.current_project);
self.data
.pin()
.get(&self.get_current_project_key())
.map(|data| data.settings.files.clone())
}

pub fn get_current_manifest(&self) -> Option<PackageJson> {
self.data
.pin()
Expand Down Expand Up @@ -145,10 +151,10 @@ impl WorkspaceSettings {
}

/// Remove a project using its folder.
pub fn remove_project(&self, workspace_path: &Path) {
pub fn remove_project(&self, project_path: &Path) {
let data = self.data.pin();
for (key, path_to_settings) in data.iter() {
if path_to_settings.path.as_path() == workspace_path {
for (key, project_data) in data.iter() {
if project_data.path.as_path() == project_path {
data.remove(key);
}
}
Expand Down Expand Up @@ -185,25 +191,24 @@ impl WorkspaceSettings {

/// Checks whether the given `path` belongs to the current project and no
/// other project.
pub fn path_belongs_only_to_current_project(&self, path: &BiomePath) -> bool {
let mut belongs_to_current = false;
pub fn path_belongs_only_to_project_with_path(
&self,
path: &BiomePath,
project_path: &Path,
) -> bool {
let mut belongs_to_project = false;
let mut belongs_to_other = false;
for (key, path_to_settings) in self.data.pin().iter() {
if path.strip_prefix(path_to_settings.path.as_path()).is_ok() {
if *key == self.get_current_project_key() {
belongs_to_current = true;
for project_data in self.data.pin().values() {
if path.strip_prefix(project_data.path.as_path()).is_ok() {
if project_data.path.as_path() == project_path {
belongs_to_project = true;
} else {
belongs_to_other = true;
}
}
}

belongs_to_current && !belongs_to_other
}

/// Sets which project is the current one by its key.
pub fn set_current_project(&self, key: ProjectKey) {
*self.current_project.write().unwrap() = key;
belongs_to_project && !belongs_to_other
}

/// Returns the maximum file size setting.
Expand Down
14 changes: 12 additions & 2 deletions crates/biome_service/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
//! format a file with a language that does not have a formatter

mod client;
mod scanner;
mod server;

pub use self::client::{TransportRequest, WorkspaceClient, WorkspaceTransport};
Expand Down Expand Up @@ -541,6 +542,14 @@ pub struct OpenFileParams {
pub content: FileContent,
pub version: i32,
pub document_file_source: Option<DocumentFileSource>,

/// Set to `true` to persist the node cache used during parsing, in order to
/// speed up subsequent reparsing if the document has been edited.
///
/// This should only be enabled if reparsing is to be expected, such as when
/// the file is opened through the LSP Proxy.
#[serde(default)]
pub persist_node_cache: bool,
}

#[derive(Debug, serde::Serialize, serde::Deserialize)]
Expand Down Expand Up @@ -966,7 +975,7 @@ pub struct RegisterProjectFolderParams {
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
#[serde(rename_all = "camelCase")]
pub struct UnregisterProjectFolderParams {
pub path: BiomePath,
pub path: PathBuf,
}

pub trait Workspace: Send + Sync + RefUnwindSafe {
Expand Down Expand Up @@ -1032,7 +1041,8 @@ pub trait Workspace: Send + Sync + RefUnwindSafe {

/// Unregisters a workspace project folder.
///
/// The settings that belong to that project are deleted.
/// The settings that belong to that project are deleted. Any open files that belong to the
/// project are also closed.
///
/// If a file watcher was registered as a result of a call to `scan_project_folder()`, it will
/// also be unregistered.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use crate::diagnostics::Panic;
use crate::workspace::{DocumentFileSource, FileContent, OpenFileParams};
use crate::{Workspace, WorkspaceError};

use super::server::WorkspaceServer;

pub(crate) struct ScanResult {
/// Diagnostics reported while scanning the project.
pub diagnostics: Vec<Diagnostic>,
Expand All @@ -24,7 +26,10 @@ pub(crate) struct ScanResult {
pub duration: Duration,
}

pub(crate) fn scan(workspace: &dyn Workspace, folder: &Path) -> Result<ScanResult, WorkspaceError> {
pub(crate) fn scan(
workspace: &WorkspaceServer,
folder: &Path,
) -> Result<ScanResult, WorkspaceError> {
init_thread_pool();

let (interner, _path_receiver) = PathInterner::new();
Expand Down Expand Up @@ -133,7 +138,7 @@ impl DiagnosticsCollector {
/// Context object shared between directory traversal tasks.
pub(crate) struct ScanContext<'app> {
/// [Workspace] instance.
pub(crate) workspace: &'app dyn Workspace,
pub(crate) workspace: &'app WorkspaceServer,

/// File paths interner cache used by the filesystem traversal.
interner: PathInterner,
Expand Down Expand Up @@ -188,11 +193,12 @@ impl<'app> TraversalContext for ScanContext<'app> {
/// so panics are caught, and diagnostics are submitted in case of panic too.
fn open_file(ctx: &ScanContext, path: &BiomePath) {
match catch_unwind(move || {
ctx.workspace.open_file(OpenFileParams {
ctx.workspace.open_file_by_scanner(OpenFileParams {
path: path.clone(),
content: FileContent::FromServer,
document_file_source: None,
version: 0,
persist_node_cache: false,
})
}) {
Ok(Ok(())) => {}
Expand Down
Loading
Loading