Skip to content

Commit d56d241

Browse files
authored
Notify users for invalid client settings (#16361)
## Summary As mentioned in #16296 (comment) This PR updates the client settings resolver to notify the user if there are any errors in the config using a very basic approach. In addition, each error related to specific settings are logged. This isn't the best approach because it can log the same message multiple times when both workspace and global settings are provided and they both are the same. This is the case for a single workspace VS Code instance. I do have some ideas on how to improve this and will explore them during my free time (low priority): * Avoid resolving the global settings multiple times as they're static * Include the source of the setting (workspace or global?) * Maybe use a struct (`ResolvedClientSettings` + `Vec<ClientSettingsResolverError>`) instead to make unit testing easier ## Test Plan Using: ```jsonc { "ruff.logLevel": "debug", // Invalid settings "ruff.configuration": "$RANDOM", "ruff.lint.select": ["RUF000", "I001"], "ruff.lint.extendSelect": ["B001", "B002"], "ruff.lint.ignore": ["I999", "F401"] } ``` The error logs: ``` 2025-02-27 12:30:04.318736000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found 2025-02-27 12:30:04.319196000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found 2025-02-27 12:30:04.320549000 ERROR Unknown rule selectors found in `lint.select`: ["RUF000"] 2025-02-27 12:30:04.320669000 ERROR Unknown rule selectors found in `lint.extendSelect`: ["B001"] 2025-02-27 12:30:04.320764000 ERROR Unknown rule selectors found in `lint.ignore`: ["I999"] ``` Notification preview: <img width="470" alt="Screenshot 2025-02-27 at 12 29 06 PM" src="https://github.com/user-attachments/assets/61f41d5c-2558-46b3-a1ed-82114fd8ec22" />
1 parent 7dad0c4 commit d56d241

File tree

1 file changed

+71
-27
lines changed

1 file changed

+71
-27
lines changed

crates/ruff_server/src/session/settings.rs

+71-27
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ use serde::Deserialize;
66
use serde_json::{Map, Value};
77
use thiserror::Error;
88

9-
use ruff_linter::{line_width::LineLength, RuleSelector};
9+
use ruff_linter::line_width::LineLength;
10+
use ruff_linter::rule_selector::ParseError;
11+
use ruff_linter::RuleSelector;
1012
use ruff_workspace::options::Options;
1113

1214
/// Maps a workspace URI to its associated client settings. Used during server initialization.
@@ -319,7 +321,9 @@ impl ResolvedClientSettings {
319321
}
320322

321323
fn new_impl(all_settings: &[&ClientSettings]) -> Self {
322-
Self {
324+
let mut contains_invalid_settings = false;
325+
326+
let settings = Self {
323327
fix_all: Self::resolve_or(all_settings, |settings| settings.fix_all, true),
324328
organize_imports: Self::resolve_or(
325329
all_settings,
@@ -369,6 +373,7 @@ impl ResolvedClientSettings {
369373
tracing::error!(
370374
"Failed to load settings from `configuration`: {err}"
371375
);
376+
contains_invalid_settings = true;
372377
None
373378
}
374379
}
@@ -381,34 +386,25 @@ impl ResolvedClientSettings {
381386
settings.format.as_ref()?.preview
382387
}),
383388
select: Self::resolve_optional(all_settings, |settings| {
384-
settings
385-
.lint
386-
.as_ref()?
387-
.select
388-
.as_ref()?
389-
.iter()
390-
.map(|rule| RuleSelector::from_str(rule).ok())
391-
.collect()
389+
Self::resolve_rules(
390+
settings.lint.as_ref()?.select.as_ref()?,
391+
RuleSelectorKey::Select,
392+
&mut contains_invalid_settings,
393+
)
392394
}),
393395
extend_select: Self::resolve_optional(all_settings, |settings| {
394-
settings
395-
.lint
396-
.as_ref()?
397-
.extend_select
398-
.as_ref()?
399-
.iter()
400-
.map(|rule| RuleSelector::from_str(rule).ok())
401-
.collect()
396+
Self::resolve_rules(
397+
settings.lint.as_ref()?.extend_select.as_ref()?,
398+
RuleSelectorKey::ExtendSelect,
399+
&mut contains_invalid_settings,
400+
)
402401
}),
403402
ignore: Self::resolve_optional(all_settings, |settings| {
404-
settings
405-
.lint
406-
.as_ref()?
407-
.ignore
408-
.as_ref()?
409-
.iter()
410-
.map(|rule| RuleSelector::from_str(rule).ok())
411-
.collect()
403+
Self::resolve_rules(
404+
settings.lint.as_ref()?.ignore.as_ref()?,
405+
RuleSelectorKey::Ignore,
406+
&mut contains_invalid_settings,
407+
)
412408
}),
413409
exclude: Self::resolve_optional(all_settings, |settings| settings.exclude.clone()),
414410
line_length: Self::resolve_optional(all_settings, |settings| settings.line_length),
@@ -418,6 +414,37 @@ impl ResolvedClientSettings {
418414
ConfigurationPreference::EditorFirst,
419415
),
420416
},
417+
};
418+
419+
if contains_invalid_settings {
420+
show_err_msg!(
421+
"Ruff received invalid settings from the editor. Refer to the logs for more information."
422+
);
423+
}
424+
425+
settings
426+
}
427+
428+
fn resolve_rules(
429+
rules: &[String],
430+
key: RuleSelectorKey,
431+
contains_invalid_settings: &mut bool,
432+
) -> Option<Vec<RuleSelector>> {
433+
let (mut known, mut unknown) = (vec![], vec![]);
434+
for rule in rules {
435+
match RuleSelector::from_str(rule) {
436+
Ok(selector) => known.push(selector),
437+
Err(ParseError::Unknown(_)) => unknown.push(rule),
438+
}
439+
}
440+
if !unknown.is_empty() {
441+
*contains_invalid_settings = true;
442+
tracing::error!("Unknown rule selectors found in `{key}`: {unknown:?}");
443+
}
444+
if known.is_empty() {
445+
None
446+
} else {
447+
Some(known)
421448
}
422449
}
423450

@@ -427,7 +454,7 @@ impl ResolvedClientSettings {
427454
/// Use [`ResolvedClientSettings::resolve_or`] for settings that should have default values.
428455
fn resolve_optional<T>(
429456
all_settings: &[&ClientSettings],
430-
get: impl Fn(&ClientSettings) -> Option<T>,
457+
get: impl FnMut(&ClientSettings) -> Option<T>,
431458
) -> Option<T> {
432459
all_settings.iter().map(Deref::deref).find_map(get)
433460
}
@@ -446,6 +473,23 @@ impl ResolvedClientSettings {
446473
}
447474
}
448475

476+
#[derive(Copy, Clone)]
477+
enum RuleSelectorKey {
478+
Select,
479+
ExtendSelect,
480+
Ignore,
481+
}
482+
483+
impl std::fmt::Display for RuleSelectorKey {
484+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
485+
match self {
486+
RuleSelectorKey::Select => f.write_str("lint.select"),
487+
RuleSelectorKey::ExtendSelect => f.write_str("lint.extendSelect"),
488+
RuleSelectorKey::Ignore => f.write_str("lint.ignore"),
489+
}
490+
}
491+
}
492+
449493
impl ResolvedClientSettings {
450494
pub(crate) fn fix_all(&self) -> bool {
451495
self.fix_all

0 commit comments

Comments
 (0)