From 35ca887e0219fce6795bb1261b96390b877ee2be Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Tue, 23 Apr 2024 11:19:17 -0700 Subject: [PATCH] `ruff server`: Ruff configuration from client settings overrides project configuration (#11062) ## Summary This is a follow-up to https://github.com/astral-sh/ruff/pull/10984 that implements configuration resolution for editor configuration. By 'editor configuration', I'm referring to the client settings that correspond to Ruff configuration/options, like `preview`, `select`, and so on. These will be combined with 'project configuration' (configuration taken from project files such as `pyproject.toml`) to generate the final linter and formatter settings used by `RuffSettings`. Editor configuration takes priority over project configuration. In a follow-up pull request, I'll implement a new client setting that allows project configuration to override editor configuration, as per [this issue](https://github.com/astral-sh/ruff-vscode/issues/425). ## Review guide The first commit, e38966d8843becc7234fa7d46009c16af4ba41e9, is just doing re-arrangement so that we can pass the right things to `RuffSettings::resolve`. The actual resolution logic is in the second commit, 0eec9ee75c10e5ec423bd9f5ce1764f4d7a5ad86. It might help to look at these comments individually since the diff is rather messy. ## Test Plan For the settings to show up in VS Code, you'll need to checkout this branch: https://github.com/astral-sh/ruff-vscode/pull/456. To test that the resolution for a specific setting works as expected, run through the following scenarios, setting it in project and editor configuration as needed: | Set in project configuration? | Set in editor configuration? | Expected Outcome | |-------------------------------|--------------------------------------------------|------------------------------------------------------------------------------------------| | No | No | The editor should behave as if the setting was set to its default value. | | Yes | No | The editor should behave as if the setting was set to the value in project configuration. | | No | Yes | The editor should behave as if the setting was set to the value in editor configuration. | | Yes | Yes (but distinctive from project configuration) | The editor should behave as if the setting was set to the value in editor configuration. | An exception to this is `extendSelect`, which does not have an analog in TOML configuration. Instead, you should verify that `extendSelect` amends the `select` setting. If `select` is set in both editor and project configuration, `extendSelect` will only append to the `select` value in editor configuration, so make sure to un-set it there if you're testing `extendSelect` with `select` in project configuration. --- .../ruff_server/src/server/api/diagnostics.rs | 2 +- .../src/server/api/requests/code_action.rs | 4 +- .../api/requests/code_action_resolve.rs | 4 +- .../server/api/requests/execute_command.rs | 4 +- .../src/server/api/requests/format.rs | 2 +- .../src/server/api/requests/format_range.rs | 2 +- crates/ruff_server/src/session.rs | 5 +- crates/ruff_server/src/session/settings.rs | 38 ++++------ crates/ruff_server/src/session/workspace.rs | 53 ++++++++----- .../src/session/workspace/ruff_settings.rs | 74 ++++++++++++++++--- 10 files changed, 127 insertions(+), 61 deletions(-) diff --git a/crates/ruff_server/src/server/api/diagnostics.rs b/crates/ruff_server/src/server/api/diagnostics.rs index b40806f475..6d2893028d 100644 --- a/crates/ruff_server/src/server/api/diagnostics.rs +++ b/crates/ruff_server/src/server/api/diagnostics.rs @@ -6,7 +6,7 @@ pub(super) fn generate_diagnostics(snapshot: &DocumentSnapshot) -> Vec crate::Result { document, snapshot.resolved_client_capabilities(), snapshot.url(), - &snapshot.settings().linter, + snapshot.settings().linter(), snapshot.encoding(), document.version(), )?), @@ -140,7 +140,7 @@ fn organize_imports(snapshot: &DocumentSnapshot) -> crate::Result { let edits = super::code_action_resolve::fix_all_edit( snapshot.document(), - &snapshot.settings().linter, + snapshot.settings().linter(), snapshot.encoding(), ) .with_failure_code(ErrorCode::InternalError)?; @@ -83,7 +83,7 @@ impl super::SyncRequestHandler for ExecuteCommand { Command::OrganizeImports => { let edits = super::code_action_resolve::organize_imports_edit( snapshot.document(), - &snapshot.settings().linter, + snapshot.settings().linter(), snapshot.encoding(), ) .with_failure_code(ErrorCode::InternalError)?; diff --git a/crates/ruff_server/src/server/api/requests/format.rs b/crates/ruff_server/src/server/api/requests/format.rs index c8a9260324..dbfbc1af4e 100644 --- a/crates/ruff_server/src/server/api/requests/format.rs +++ b/crates/ruff_server/src/server/api/requests/format.rs @@ -26,7 +26,7 @@ impl super::BackgroundDocumentRequestHandler for Format { pub(super) fn format_document(snapshot: &DocumentSnapshot) -> Result { let doc = snapshot.document(); let source = doc.contents(); - let formatted = crate::format::format(doc, &snapshot.settings().formatter) + let formatted = crate::format::format(doc, snapshot.settings().formatter()) .with_failure_code(lsp_server::ErrorCode::InternalError)?; // fast path - if the code is the same, return early if formatted == source { diff --git a/crates/ruff_server/src/server/api/requests/format_range.rs b/crates/ruff_server/src/server/api/requests/format_range.rs index 904be4914e..e01b4b042a 100644 --- a/crates/ruff_server/src/server/api/requests/format_range.rs +++ b/crates/ruff_server/src/server/api/requests/format_range.rs @@ -22,7 +22,7 @@ impl super::BackgroundDocumentRequestHandler for FormatRange { let index = document.index(); let range = params.range.to_text_range(text, index, snapshot.encoding()); let formatted_range = - crate::format::format_range(document, &snapshot.settings().formatter, range) + crate::format::format_range(document, snapshot.settings().formatter(), range) .with_failure_code(lsp_server::ErrorCode::InternalError)?; Ok(Some(vec![types::TextEdit { range: formatted_range diff --git a/crates/ruff_server/src/session.rs b/crates/ruff_server/src/session.rs index d1d7b80f2e..51a8909c03 100644 --- a/crates/ruff_server/src/session.rs +++ b/crates/ruff_server/src/session.rs @@ -46,11 +46,11 @@ impl Session { ) -> crate::Result { Ok(Self { position_encoding, + workspaces: workspace::Workspaces::new(workspaces, &global_settings)?, global_settings, resolved_client_capabilities: Arc::new(ResolvedClientCapabilities::new( client_capabilities, )), - workspaces: workspace::Workspaces::new(workspaces)?, }) } @@ -87,7 +87,8 @@ impl Session { } pub(crate) fn open_workspace_folder(&mut self, url: &Url) -> crate::Result<()> { - self.workspaces.open_workspace_folder(url)?; + self.workspaces + .open_workspace_folder(url, &self.global_settings)?; Ok(()) } diff --git a/crates/ruff_server/src/session/settings.rs b/crates/ruff_server/src/session/settings.rs index 84e841ad72..d762feebc6 100644 --- a/crates/ruff_server/src/session/settings.rs +++ b/crates/ruff_server/src/session/settings.rs @@ -1,4 +1,4 @@ -use std::{ffi::OsString, ops::Deref, path::PathBuf, str::FromStr}; +use std::{ops::Deref, str::FromStr}; use lsp_types::Url; use ruff_linter::{line_width::LineLength, RuleSelector}; @@ -11,7 +11,7 @@ pub(crate) type WorkspaceSettingsMap = FxHashMap; /// Resolved client settings for a specific document. These settings are meant to be /// used directly by the server, and are *not* a 1:1 representation with how the client /// sends them. -#[derive(Debug)] +#[derive(Clone, Debug)] #[cfg_attr(test, derive(PartialEq, Eq))] #[allow(clippy::struct_excessive_bools)] pub(crate) struct ResolvedClientSettings { @@ -22,25 +22,22 @@ pub(crate) struct ResolvedClientSettings { #[allow(dead_code)] disable_rule_comment_enable: bool, fix_violation_enable: bool, - // TODO(jane): Remove once editor settings resolution is implemented - #[allow(dead_code)] editor_settings: ResolvedEditorSettings, } /// Contains the resolved values of 'editor settings' - Ruff configuration for the linter/formatter that was passed in via /// LSP client settings. These fields are optional because we don't want to override file-based linter/formatting settings /// if these were un-set. -#[derive(Debug, Default)] +#[derive(Clone, Debug, Default)] #[cfg_attr(test, derive(PartialEq, Eq))] -#[allow(dead_code)] // TODO(jane): Remove once editor settings resolution is implemented pub(crate) struct ResolvedEditorSettings { - lint_preview: Option, - format_preview: Option, - select: Option>, - extend_select: Option>, - ignore: Option>, - exclude: Option>, - line_length: Option, + pub(super) lint_preview: Option, + pub(super) format_preview: Option, + pub(super) select: Option>, + pub(super) extend_select: Option>, + pub(super) ignore: Option>, + pub(super) exclude: Option>, + pub(super) line_length: Option, } /// This is a direct representation of the settings schema sent by the client. @@ -251,14 +248,7 @@ impl ResolvedClientSettings { .collect() }), exclude: Self::resolve_optional(all_settings, |settings| { - Some( - settings - .exclude - .as_ref()? - .iter() - .map(|path| PathBuf::from(OsString::from(path))) - .collect(), - ) + Some(settings.exclude.as_ref()?.clone()) }), line_length: Self::resolve_optional(all_settings, |settings| settings.line_length), }, @@ -306,6 +296,10 @@ impl ResolvedClientSettings { pub(crate) fn fix_violation(&self) -> bool { self.fix_violation_enable } + + pub(crate) fn editor_settings(&self) -> &ResolvedEditorSettings { + &self.editor_settings + } } impl Default for InitializationOptions { @@ -653,7 +647,7 @@ mod tests { select: None, extend_select: None, ignore: Some(vec![RuleSelector::from_str("RUF001").unwrap()]), - exclude: Some(vec![PathBuf::from_str("third_party").unwrap()]), + exclude: Some(vec!["third_party".into()]), line_length: Some(LineLength::try_from(80).unwrap()) } } diff --git a/crates/ruff_server/src/session/workspace.rs b/crates/ruff_server/src/session/workspace.rs index a508a360c8..cb8332c730 100644 --- a/crates/ruff_server/src/session/workspace.rs +++ b/crates/ruff_server/src/session/workspace.rs @@ -12,7 +12,10 @@ use crate::{edit::DocumentVersion, Document}; use self::ruff_settings::RuffSettingsIndex; -use super::{settings, ClientSettings}; +use super::{ + settings::{self, ResolvedClientSettings, ResolvedEditorSettings}, + ClientSettings, +}; mod ruff_settings; @@ -23,7 +26,7 @@ pub(crate) struct Workspaces(BTreeMap); pub(crate) struct Workspace { open_documents: OpenDocuments, - settings: ClientSettings, + settings: ResolvedClientSettings, } pub(crate) struct OpenDocuments { @@ -46,18 +49,28 @@ pub(crate) struct DocumentRef { } impl Workspaces { - pub(super) fn new(workspaces: Vec<(Url, ClientSettings)>) -> crate::Result { + pub(super) fn new( + workspaces: Vec<(Url, ClientSettings)>, + global_settings: &ClientSettings, + ) -> crate::Result { Ok(Self( workspaces .into_iter() - .map(|(url, settings)| Workspace::new(&url, settings)) + .map(|(url, workspace_settings)| { + Workspace::new(&url, &workspace_settings, global_settings) + }) .collect::>()?, )) } - pub(super) fn open_workspace_folder(&mut self, folder_url: &Url) -> crate::Result<()> { + pub(super) fn open_workspace_folder( + &mut self, + folder_url: &Url, + global_settings: &ClientSettings, + ) -> crate::Result<()> { // TODO(jane): find a way to allow for workspace settings to be updated dynamically - let (path, workspace) = Workspace::new(folder_url, ClientSettings::default())?; + let (path, workspace) = + Workspace::new(folder_url, &ClientSettings::default(), global_settings)?; self.0.insert(path, workspace); Ok(()) } @@ -117,12 +130,7 @@ impl Workspaces { ); settings::ResolvedClientSettings::global(global_settings) }, - |workspace| { - settings::ResolvedClientSettings::with_workspace( - &workspace.settings, - global_settings, - ) - }, + |workspace| workspace.settings.clone(), ) } @@ -152,13 +160,19 @@ impl Workspaces { } impl Workspace { - pub(crate) fn new(root: &Url, settings: ClientSettings) -> crate::Result<(PathBuf, Self)> { + pub(crate) fn new( + root: &Url, + workspace_settings: &ClientSettings, + global_settings: &ClientSettings, + ) -> crate::Result<(PathBuf, Self)> { let path = root .to_file_path() .map_err(|()| anyhow!("workspace URL was not a file path!"))?; + let settings = ResolvedClientSettings::with_workspace(workspace_settings, global_settings); + let workspace = Self { - open_documents: OpenDocuments::new(&path), + open_documents: OpenDocuments::new(&path, settings.editor_settings()), settings, }; @@ -166,15 +180,16 @@ impl Workspace { } fn reload_settings(&mut self, root: &Path) { - self.open_documents.reload_settings(root); + self.open_documents + .reload_settings(root, self.settings.editor_settings()); } } impl OpenDocuments { - fn new(path: &Path) -> Self { + fn new(path: &Path, editor_settings: &ResolvedEditorSettings) -> Self { Self { documents: FxHashMap::default(), - settings_index: RuffSettingsIndex::new(path), + settings_index: RuffSettingsIndex::new(path, editor_settings), } } @@ -209,8 +224,8 @@ impl OpenDocuments { Ok(()) } - fn reload_settings(&mut self, root: &Path) { - self.settings_index = RuffSettingsIndex::new(root); + fn reload_settings(&mut self, root: &Path, editor_settings: &ResolvedEditorSettings) { + self.settings_index = RuffSettingsIndex::new(root, editor_settings); } } diff --git a/crates/ruff_server/src/session/workspace/ruff_settings.rs b/crates/ruff_server/src/session/workspace/ruff_settings.rs index d1e13d6370..d1db32b5b1 100644 --- a/crates/ruff_server/src/session/workspace/ruff_settings.rs +++ b/crates/ruff_server/src/session/workspace/ruff_settings.rs @@ -1,5 +1,9 @@ -use ruff_linter::display_settings; +use ruff_linter::{ + display_settings, fs::normalize_path_to, settings::types::FilePattern, + settings::types::PreviewMode, +}; use ruff_workspace::{ + configuration::{Configuration, FormatConfiguration, LintConfiguration, RuleSelection}, pyproject::settings_toml, resolver::{ConfigurationTransformer, Relativity}, }; @@ -10,12 +14,14 @@ use std::{ }; use walkdir::{DirEntry, WalkDir}; +use crate::session::settings::ResolvedEditorSettings; + #[derive(Default)] pub(crate) struct RuffSettings { // settings to pass into the ruff linter - pub(crate) linter: ruff_linter::settings::LinterSettings, + linter: ruff_linter::settings::LinterSettings, // settings to pass into the ruff formatter - pub(crate) formatter: ruff_workspace::FormatterSettings, + formatter: ruff_workspace::FormatterSettings, } pub(super) struct RuffSettingsIndex { @@ -36,8 +42,18 @@ impl std::fmt::Display for RuffSettings { } } +impl RuffSettings { + pub(crate) fn linter(&self) -> &ruff_linter::settings::LinterSettings { + &self.linter + } + + pub(crate) fn formatter(&self) -> &ruff_workspace::FormatterSettings { + &self.formatter + } +} + impl RuffSettingsIndex { - pub(super) fn new(root: &Path) -> Self { + pub(super) fn new(root: &Path, editor_settings: &ResolvedEditorSettings) -> Self { let mut index = BTreeMap::default(); for directory in WalkDir::new(root) @@ -50,7 +66,7 @@ impl RuffSettingsIndex { let Ok(settings) = ruff_workspace::resolver::resolve_root_settings( &pyproject, Relativity::Parent, - &LSPConfigTransformer, + &EditorConfigurationTransformer(editor_settings, root), ) else { continue; }; @@ -86,13 +102,53 @@ impl RuffSettingsIndex { } } -struct LSPConfigTransformer; +struct EditorConfigurationTransformer<'a>(&'a ResolvedEditorSettings, &'a Path); -impl ConfigurationTransformer for LSPConfigTransformer { +impl<'a> ConfigurationTransformer for EditorConfigurationTransformer<'a> { fn transform( &self, - config: ruff_workspace::configuration::Configuration, + project_configuration: ruff_workspace::configuration::Configuration, ) -> ruff_workspace::configuration::Configuration { - config + let ResolvedEditorSettings { + format_preview, + lint_preview, + select, + extend_select, + ignore, + exclude, + line_length, + } = self.0.clone(); + + let project_root = self.1; + + let editor_configuration = Configuration { + lint: LintConfiguration { + preview: lint_preview.map(PreviewMode::from), + rule_selections: vec![RuleSelection { + select, + extend_select: extend_select.unwrap_or_default(), + ignore: ignore.unwrap_or_default(), + ..Default::default() + }], + ..Default::default() + }, + format: FormatConfiguration { + preview: format_preview.map(PreviewMode::from), + ..Default::default() + }, + exclude: exclude.map(|exclude| { + exclude + .into_iter() + .map(|pattern| { + let absolute = normalize_path_to(&pattern, project_root); + FilePattern::User(pattern, absolute) + }) + .collect() + }), + line_length, + ..Default::default() + }; + + editor_configuration.combine(project_configuration) } }