From 4c35274b6e695d110cf40d50b49d89ec84863a48 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 2 Oct 2025 15:34:31 -0500 Subject: [PATCH] Don't allow formatters in format on save (#39400) Closes #ISSUE Release Notes: - settings: Removed support for having format steps in both the `format_on_save` and `formatter` settings for languages. `format_on_save` is now restricted to the values of `"on"` and `"off"`, and all format steps should be set under the `formatter` key. If you were using `format_on_save` but not `formatter` this will be migrated for you, otherwise it will require a manual migration. --------- Co-authored-by: Smit --- crates/migrator/src/migrations.rs | 6 + .../src/migrations/m_2025_10_02/settings.rs | 50 +++++ crates/migrator/src/migrator.rs | 177 +++++++++++++++++- crates/onboarding/src/editing_page.rs | 2 +- crates/project/src/lsp_store.rs | 1 - .../settings/src/settings_content/language.rs | 93 +-------- 6 files changed, 229 insertions(+), 100 deletions(-) create mode 100644 crates/migrator/src/migrations/m_2025_10_02/settings.rs diff --git a/crates/migrator/src/migrations.rs b/crates/migrator/src/migrations.rs index a4db2df46437c8e78d3e610f78c5374898b842a2..c0b54b0afc3c74a23026100b706fd68d3476aff3 100644 --- a/crates/migrator/src/migrations.rs +++ b/crates/migrator/src/migrations.rs @@ -105,3 +105,9 @@ pub(crate) mod m_2025_10_01 { pub(crate) use settings::SETTINGS_PATTERNS; } + +pub(crate) mod m_2025_10_02 { + mod settings; + + pub(crate) use settings::remove_formatters_on_save; +} diff --git a/crates/migrator/src/migrations/m_2025_10_02/settings.rs b/crates/migrator/src/migrations/m_2025_10_02/settings.rs new file mode 100644 index 0000000000000000000000000000000000000000..2434ae4d0e100ce58e4cfdd2eee1039188c1d7bc --- /dev/null +++ b/crates/migrator/src/migrations/m_2025_10_02/settings.rs @@ -0,0 +1,50 @@ +use anyhow::Result; +use serde_json::Value; + +pub fn remove_formatters_on_save(value: &mut Value) -> Result<()> { + remove_formatters_on_save_inner(value, &[])?; + let languages = value + .as_object_mut() + .and_then(|obj| obj.get_mut("languages")) + .and_then(|languages| languages.as_object_mut()); + if let Some(languages) = languages { + for (language_name, language) in languages.iter_mut() { + let path = vec!["languages", language_name]; + remove_formatters_on_save_inner(language, &path)?; + } + } + Ok(()) +} + +fn remove_formatters_on_save_inner(value: &mut Value, path: &[&str]) -> Result<()> { + let Some(obj) = value.as_object_mut() else { + return Ok(()); + }; + let Some(format_on_save) = obj.get("format_on_save").cloned() else { + return Ok(()); + }; + let is_format_on_save_set_to_formatter = format_on_save + .as_str() + .map_or(true, |s| s != "on" && s != "off"); + if !is_format_on_save_set_to_formatter { + return Ok(()); + } + + fn fmt_path(path: &[&str], key: &str) -> String { + let mut path = path.to_vec(); + path.push(key); + path.join(".") + } + + anyhow::ensure!( + obj.get("formatter").is_none(), + r#"Setting formatters in both "format_on_save" and "formatter" is deprecated. Please migrate the formatters from {} into {}"#, + fmt_path(path, "format_on_save"), + fmt_path(path, "formatter") + ); + + obj.insert("format_on_save".to_string(), serde_json::json!("on")); + obj.insert("formatter".to_string(), format_on_save); + + Ok(()) +} diff --git a/crates/migrator/src/migrator.rs b/crates/migrator/src/migrator.rs index e6085f9c58a3867540c159530d1803c5ef5a5174..4e649a0791fb0acef9d1b56138972964fd8ee3ac 100644 --- a/crates/migrator/src/migrator.rs +++ b/crates/migrator/src/migrator.rs @@ -76,7 +76,7 @@ fn run_migrations(text: &str, migrations: &[MigrationType]) -> Result Result> { enum MigrationType<'a> { TreeSitter(MigrationPatterns, &'a Query), - #[allow(unused)] - Json(fn(&mut serde_json::Value)), + Json(fn(&mut serde_json::Value) -> Result<()>), } pub fn migrate_settings(text: &str) -> Result> { @@ -200,6 +199,7 @@ pub fn migrate_settings(text: &str) -> Result> { migrations::m_2025_10_01::SETTINGS_PATTERNS, &SETTINGS_QUERY_2025_10_01, ), + MigrationType::Json(migrations::m_2025_10_02::remove_formatters_on_save), ]; run_migrations(text, migrations) } @@ -360,7 +360,7 @@ mod tests { output: Option<&str>, ) { let migrated = run_migrations(input, migrations).unwrap(); - pretty_assertions::assert_eq!(migrated.as_deref(), output); + assert_migrated_correctly(migrated, output); } #[test] @@ -1486,7 +1486,7 @@ mod tests { "default-3": true, "default-4": true, } - } + }, "languages": { "Rust": { "formatter": [ @@ -1549,7 +1549,7 @@ mod tests { { "code_action": "default-2" }, { "code_action": "default-3" }, { "code_action": "default-4" } - ] + ], "languages": { "Rust": { "formatter": [ @@ -1580,7 +1580,11 @@ mod tests { #[test] fn test_flatten_code_action_formatters_array_with_format_on_save_and_multiple_languages() { - assert_migrate_settings( + assert_migrate_settings_with_migrations( + &[MigrationType::TreeSitter( + migrations::m_2025_10_01::SETTINGS_PATTERNS, + &SETTINGS_QUERY_2025_10_01, + )], &r#"{ "formatter": { "code_actions": { @@ -1728,4 +1732,163 @@ mod tests { ), ); } + + #[test] + fn test_format_on_save_formatter_migration_basic() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "format_on_save": "prettier" + }"# + .unindent(), + Some( + &r#"{ + "formatter": "prettier", + "format_on_save": "on" + }"# + .unindent(), + ), + ); + } + + #[test] + fn test_format_on_save_formatter_migration_array() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "format_on_save": ["prettier", {"language_server": "eslint"}] + }"# + .unindent(), + Some( + &r#"{ + "formatter": [ + "prettier", + { + "language_server": "eslint" + } + ], + "format_on_save": "on" + }"# + .unindent(), + ), + ); + } + + #[test] + fn test_format_on_save_on_off_unchanged() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "format_on_save": "on" + }"# + .unindent(), + None, + ); + + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "format_on_save": "off" + }"# + .unindent(), + None, + ); + } + + #[test] + fn test_format_on_save_formatter_migration_in_languages() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "languages": { + "Rust": { + "format_on_save": "rust-analyzer" + }, + "Python": { + "format_on_save": ["ruff", "black"] + } + } + }"# + .unindent(), + Some( + &r#"{ + "languages": { + "Rust": { + "formatter": "rust-analyzer", + "format_on_save": "on" + }, + "Python": { + "formatter": [ + "ruff", + "black" + ], + "format_on_save": "on" + } + } + }"# + .unindent(), + ), + ); + } + + #[test] + fn test_format_on_save_formatter_migration_mixed_global_and_languages() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "format_on_save": "prettier", + "languages": { + "Rust": { + "format_on_save": "rust-analyzer" + }, + "Python": { + "format_on_save": "on" + } + } + }"# + .unindent(), + Some( + &r#"{ + "formatter": "prettier", + "format_on_save": "on", + "languages": { + "Rust": { + "formatter": "rust-analyzer", + "format_on_save": "on" + }, + "Python": { + "format_on_save": "on" + } + } + }"# + .unindent(), + ), + ); + } + + #[test] + fn test_format_on_save_no_migration_when_no_format_on_save() { + assert_migrate_settings_with_migrations( + &[MigrationType::Json( + migrations::m_2025_10_02::remove_formatters_on_save, + )], + &r#"{ + "formatter": ["prettier"] + }"# + .unindent(), + None, + ); + } } diff --git a/crates/onboarding/src/editing_page.rs b/crates/onboarding/src/editing_page.rs index 71b697ebb00ba44449621ee51bd4ac04b9e5c10d..60ae00564a32b06f5ad42446de939e81da927778 100644 --- a/crates/onboarding/src/editing_page.rs +++ b/crates/onboarding/src/editing_page.rs @@ -167,7 +167,7 @@ fn write_font_ligatures(enabled: bool, cx: &mut App) { fn read_format_on_save(cx: &App) -> bool { match AllLanguageSettings::get_global(cx).defaults.format_on_save { - FormatOnSave::On | FormatOnSave::List(_) => true, + FormatOnSave::On => true, FormatOnSave::Off => false, } } diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index d5d6f808abecadf127c06fb2e1c0e75ca9c8d2f7..c91547c23049adc4faba7349b8df8638a9fdbc59 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1366,7 +1366,6 @@ impl LocalLspStore { let formatters = match (trigger, &settings.format_on_save) { (FormatTrigger::Save, FormatOnSave::Off) => &[], - (FormatTrigger::Save, FormatOnSave::List(formatters)) => formatters.as_ref(), (FormatTrigger::Manual, _) | (FormatTrigger::Save, FormatOnSave::On) => { match &settings.formatter { SelectedFormatter::Auto => { diff --git a/crates/settings/src/settings_content/language.rs b/crates/settings/src/settings_content/language.rs index 6a3e87f205f95db633eeb2f99d2fff1462251e06..a91127eb46aa855c4ce62411842d18c8abba61a4 100644 --- a/crates/settings/src/settings_content/language.rs +++ b/crates/settings/src/settings_content/language.rs @@ -569,102 +569,13 @@ pub struct PrettierSettingsContent { } /// Controls the behavior of formatting files when they are saved. -#[derive(Debug, Clone, PartialEq, Eq, MergeFrom)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, MergeFrom)] +#[serde(rename_all = "lowercase")] pub enum FormatOnSave { /// Files should be formatted on save. On, /// Files should not be formatted on save. Off, - List(FormatterList), -} - -impl JsonSchema for FormatOnSave { - fn schema_name() -> Cow<'static, str> { - "OnSaveFormatter".into() - } - - fn json_schema(generator: &mut schemars::SchemaGenerator) -> schemars::Schema { - let formatter_schema = Formatter::json_schema(generator); - - json_schema!({ - "oneOf": [ - { - "type": "array", - "items": formatter_schema - }, - { - "type": "string", - "enum": ["on", "off", "language_server"] - }, - formatter_schema - ] - }) - } -} - -impl Serialize for FormatOnSave { - fn serialize(&self, serializer: S) -> std::result::Result - where - S: serde::Serializer, - { - match self { - Self::On => serializer.serialize_str("on"), - Self::Off => serializer.serialize_str("off"), - Self::List(list) => list.serialize(serializer), - } - } -} - -impl<'de> Deserialize<'de> for FormatOnSave { - fn deserialize(deserializer: D) -> std::result::Result - where - D: Deserializer<'de>, - { - struct FormatDeserializer; - - impl<'d> Visitor<'d> for FormatDeserializer { - type Value = FormatOnSave; - - fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { - formatter.write_str("a valid on-save formatter kind") - } - fn visit_str(self, v: &str) -> std::result::Result - where - E: serde::de::Error, - { - if v == "on" { - Ok(Self::Value::On) - } else if v == "off" { - Ok(Self::Value::Off) - } else if v == "language_server" { - Ok(Self::Value::List(FormatterList::Single( - Formatter::LanguageServer { name: None }, - ))) - } else { - let ret: Result = - Deserialize::deserialize(v.into_deserializer()); - ret.map(Self::Value::List) - } - } - fn visit_map(self, map: A) -> Result - where - A: MapAccess<'d>, - { - let ret: Result = - Deserialize::deserialize(de::value::MapAccessDeserializer::new(map)); - ret.map(Self::Value::List) - } - fn visit_seq(self, map: A) -> Result - where - A: SeqAccess<'d>, - { - let ret: Result = - Deserialize::deserialize(de::value::SeqAccessDeserializer::new(map)); - ret.map(Self::Value::List) - } - } - deserializer.deserialize_any(FormatDeserializer) - } } /// Controls which formatter should be used when formatting code.