Skip to content
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 Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/ruff/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ enum InvalidConfigFlagReason {
ValidTomlButInvalidRuffSchema(toml::de::Error),
/// It was a valid ruff config file, but the user tried to pass a
/// value for `extend` as part of the config override.
// `extend` is special, because it affects which config files we look at
/// `extend` is special, because it affects which config files we look at
/// in the first place. We currently only parse --config overrides *after*
/// we've combined them with all the arguments from the various config files
/// that we found, so trying to override `extend` as part of a --config
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ serde = { workspace = true }
serde_json = { workspace = true }
shellexpand = { workspace = true }
thiserror = { workspace = true }
toml = { workspace = true }
tracing = { workspace = true }
tracing-subscriber = { workspace = true }

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"settings": {
"configuration": {
"line-length": 100,
"lint": {
"extend-select": ["I001"]
},
"format": {
"quote-style": "single"
}
},
"lint": {
"extendSelect": ["RUF001"]
}
}
}
94 changes: 79 additions & 15 deletions crates/ruff_server/src/session/index/ruff_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use ruff_workspace::{
resolver::{ConfigurationTransformer, Relativity},
};

use crate::session::settings::{ConfigurationPreference, ResolvedEditorSettings};
use crate::session::settings::{
ConfigurationPreference, ResolvedConfiguration, ResolvedEditorSettings,
};

#[derive(Debug)]
pub struct RuffSettings {
Expand Down Expand Up @@ -363,21 +365,39 @@ impl ConfigurationTransformer for EditorConfigurationTransformer<'_> {
..Configuration::default()
};

// Merge in the editor-specified configuration file, if it exists.
let editor_configuration = if let Some(config_file_path) = configuration {
tracing::debug!(
"Combining settings from editor-specified configuration file at: {}",
config_file_path.display()
);
match open_configuration_file(&config_file_path) {
Ok(config_from_file) => editor_configuration.combine(config_from_file),
err => {
tracing::error!(
"{:?}",
err.context("Unable to load editor-specified configuration file")
.unwrap_err()
// Merge in the editor-specified configuration.
let editor_configuration = if let Some(configuration) = configuration {
match configuration {
ResolvedConfiguration::FilePath(path) => {
tracing::debug!(
"Combining settings from editor-specified configuration file at: {}",
path.display()
);
match open_configuration_file(&path) {
Ok(config_from_file) => editor_configuration.combine(config_from_file),
err => {
tracing::error!(
"{:?}",
err.context("Unable to load editor-specified configuration file")
.unwrap_err()
);
editor_configuration
}
}
}
ResolvedConfiguration::Inline(options) => {
tracing::debug!(
"Combining settings from editor-specified inline configuration"
);
editor_configuration
match Configuration::from_options(options, None, project_root) {
Ok(configuration) => editor_configuration.combine(configuration),
Err(err) => {
tracing::error!(
"Unable to load editor-specified inline configuration: {err:?}",
);
editor_configuration
}
}
}
}
} else {
Expand Down Expand Up @@ -411,3 +431,47 @@ impl ConfigurationTransformer for IdentityTransformer {
config
}
}

#[cfg(test)]
mod tests {
use ruff_linter::line_width::LineLength;
use ruff_workspace::options::Options;

use super::*;

/// This test ensures that the inline configuration is correctly applied to the configuration.
#[test]
fn inline_settings() {
let editor_settings = ResolvedEditorSettings {
configuration: Some(ResolvedConfiguration::Inline(Options {
line_length: Some(LineLength::try_from(120).unwrap()),
..Default::default()
})),
..Default::default()
};

let config = EditorConfigurationTransformer(&editor_settings, Path::new("/src/project"))
.transform(Configuration::default());

assert_eq!(config.line_length.unwrap().value(), 120);
}

/// This test ensures that between the inline configuration and specific settings, the specific
/// settings is prioritized.
#[test]
fn inline_and_specific_settings_resolution_order() {
let editor_settings = ResolvedEditorSettings {
configuration: Some(ResolvedConfiguration::Inline(Options {
line_length: Some(LineLength::try_from(120).unwrap()),
..Default::default()
})),
line_length: Some(LineLength::try_from(100).unwrap()),
..Default::default()
};

let config = EditorConfigurationTransformer(&editor_settings, Path::new("/src/project"))
.transform(Configuration::default());

assert_eq!(config.line_length.unwrap().value(), 100);
}
}
129 changes: 121 additions & 8 deletions crates/ruff_server/src/session/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ use std::{ops::Deref, path::PathBuf, str::FromStr};
use lsp_types::Url;
use rustc_hash::FxHashMap;
use serde::Deserialize;
use serde_json::{Map, Value};
use thiserror::Error;

use ruff_linter::{line_width::LineLength, RuleSelector};
use ruff_workspace::options::Options;

/// Maps a workspace URI to its associated client settings. Used during server initialization.
pub(crate) type WorkspaceSettingsMap = FxHashMap<Url, ClientSettings>;
Expand All @@ -29,9 +32,9 @@ pub(crate) struct ResolvedClientSettings {
/// 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(Clone, Debug)]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(test, derive(Default, PartialEq, Eq))]
pub(crate) struct ResolvedEditorSettings {
pub(super) configuration: Option<PathBuf>,
pub(super) configuration: Option<ResolvedConfiguration>,
pub(super) lint_preview: Option<bool>,
pub(super) format_preview: Option<bool>,
pub(super) select: Option<Vec<RuleSelector>>,
Expand All @@ -42,6 +45,48 @@ pub(crate) struct ResolvedEditorSettings {
pub(super) configuration_preference: ConfigurationPreference,
}

/// The resolved configuration from the client settings.
#[derive(Clone, Debug)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub(crate) enum ResolvedConfiguration {
FilePath(PathBuf),
Inline(Options),
}

impl TryFrom<&ClientConfiguration> for ResolvedConfiguration {
type Error = ResolvedConfigurationError;

fn try_from(value: &ClientConfiguration) -> Result<Self, Self::Error> {
match value {
ClientConfiguration::String(path) => Ok(ResolvedConfiguration::FilePath(
PathBuf::from(shellexpand::full(path)?.as_ref()),
)),
ClientConfiguration::Object(map) => {
let options = toml::Table::try_from(map)?.try_into::<Options>()?;
if options.extend.is_some() {
Err(ResolvedConfigurationError::ExtendNotSupported)
} else {
Ok(ResolvedConfiguration::Inline(options))
}
}
}
}
}

/// An error that can occur when trying to resolve the `configuration` value from the client
/// settings.
#[derive(Debug, Error)]
pub(crate) enum ResolvedConfigurationError {
#[error(transparent)]
EnvVarLookupError(#[from] shellexpand::LookupError<std::env::VarError>),
#[error("error serializing configuration to TOML: {0}")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's unclear to me where we serialize the value to TOML. I understand deserialization but I don't see where we perform any serialization.

I worry that this error message is also not very helpful for users because it's not clear what's going wrong. Can we use a more specific error message? Failed to load configuration from ruff.configurations:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The serialization of map to TOML happens here (try_from):

let options = toml::Table::try_from(map)?.try_into::<Options>()?;

I'll update the error message

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to keep this message for now. The "Failed to load ..." message that you've suggested is already going to be appended before this. The reason I want to keep this is that for something like:

  "ruff.configuration": {
    "line-length": null
  },

We'll get:

Failed to load settings from `configuration`: unsupported unit type

But, with the above message:

Failed to load settings from `configuration`: error serializing configuration to TOML: unsupported unit type

We'll atleast know that the issue is with TOML deserialization

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Failed to load settings from configuration: error serializing configuration to TOML: unsupported unit type

I don't think I would understand this message or even know what I have to do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think one solution would be to handle these null cases because that's the only case where I'm able to get this serialization error. I checked the spec as well (https://toml.io/en/v1.0.0#keys) but can't get the InvalidToml error

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could skip the intermediate JSON deserialization but that would yield to even worse experience:

error: Failed to deserialize initialization options: data did not match any variant of untagged enum InitializationOptions. Falling back to default client settings...

InvalidToml(#[from] toml::ser::Error),
#[error(transparent)]
InvalidRuffSchema(#[from] toml::de::Error),
#[error("using `extend` is unsupported for inline configuration")]
Comment thread
dhruvmanila marked this conversation as resolved.
ExtendNotSupported,
}

/// Determines how multiple conflicting configurations should be resolved - in this
/// case, the configuration from the client settings and configuration from local
/// `.toml` files (aka 'workspace' configuration).
Expand All @@ -57,12 +102,23 @@ pub(crate) enum ConfigurationPreference {
EditorOnly,
}

/// A direct representation of of `configuration` schema within the client settings.
#[derive(Debug, Deserialize)]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[serde(untagged)]
enum ClientConfiguration {
/// A path to a configuration file.
String(String),
/// An object containing the configuration options.
Object(Map<String, Value>),
}

/// This is a direct representation of the settings schema sent by the client.
#[derive(Debug, Deserialize, Default)]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[serde(rename_all = "camelCase")]
pub struct ClientSettings {
configuration: Option<String>,
configuration: Option<ClientConfiguration>,
fix_all: Option<bool>,
organize_imports: Option<bool>,
lint: Option<LintOptions>,
Expand Down Expand Up @@ -306,11 +362,17 @@ impl ResolvedClientSettings {
),
editor_settings: ResolvedEditorSettings {
configuration: Self::resolve_optional(all_settings, |settings| {
settings
.configuration
.as_ref()
.and_then(|config_path| shellexpand::full(config_path).ok())
.map(|config_path| PathBuf::from(config_path.as_ref()))
settings.configuration.as_ref().and_then(|configuration| {
match ResolvedConfiguration::try_from(configuration) {
Ok(configuration) => Some(configuration),
Err(err) => {
tracing::error!(
"Failed to load settings from `configuration`: {err}"
);
None
}
}
})
}),
lint_preview: Self::resolve_optional(all_settings, |settings| {
settings.lint.as_ref()?.preview
Expand Down Expand Up @@ -425,6 +487,10 @@ impl Default for InitializationOptions {
#[cfg(test)]
mod tests {
use insta::assert_debug_snapshot;
use ruff_python_formatter::QuoteStyle;
use ruff_workspace::options::{
FormatOptions as RuffFormatOptions, LintCommonOptions, LintOptions,
};
use serde::de::DeserializeOwned;

#[cfg(not(windows))]
Expand All @@ -445,6 +511,9 @@ mod tests {
const EMPTY_MULTIPLE_WORKSPACE_INIT_OPTIONS_FIXTURE: &str =
include_str!("../../resources/test/fixtures/settings/empty_multiple_workspace.json");

const INLINE_CONFIGURATION_FIXTURE: &str =
include_str!("../../resources/test/fixtures/settings/inline_configuration.json");

fn deserialize_fixture<T: DeserializeOwned>(content: &str) -> T {
serde_json::from_str(content).expect("test fixture JSON should deserialize")
}
Expand Down Expand Up @@ -855,4 +924,48 @@ mod tests {
all_settings.set_preview(true);
assert_preview_all_settings(&all_settings, true);
}

#[test]
fn inline_configuration() {
let options: InitializationOptions = deserialize_fixture(INLINE_CONFIGURATION_FIXTURE);

let AllSettings {
global_settings,
workspace_settings: None,
} = AllSettings::from_init_options(options)
else {
panic!("Expected global settings only");
};

assert_eq!(
ResolvedClientSettings::global(&global_settings),
ResolvedClientSettings {
fix_all: true,
organize_imports: true,
lint_enable: true,
disable_rule_comment_enable: true,
fix_violation_enable: true,
show_syntax_errors: true,
editor_settings: ResolvedEditorSettings {
configuration: Some(ResolvedConfiguration::Inline(Options {
line_length: Some(LineLength::try_from(100).unwrap()),
lint: Some(LintOptions {
common: LintCommonOptions {
extend_select: Some(vec![RuleSelector::from_str("I001").unwrap()]),
..Default::default()
},
..Default::default()
}),
format: Some(RuffFormatOptions {
quote_style: Some(QuoteStyle::Single),
..Default::default()
}),
..Default::default()
})),
extend_select: Some(vec![RuleSelector::from_str("RUF001").unwrap()]),
..Default::default()
}
}
);
}
}
Loading