From 3ecbebbd3a69c93d31dc6096d57ac1264376b3ef Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 16 Oct 2025 13:11:20 -0500 Subject: [PATCH] Revert deprecate code actions on format (#40409) Closes #40334 This reverts the change made in #39983, and includes a replacement migration that will transform formatter settings values consisting of only `code_action` format steps into the previously deprecated `code_actions_on_format` in an attempt to restore the behavior to what it was before the migration that deprecated `code_actions_on_format`. This PR will result in a modified order in the `code_actions_on_format` setting if it existed, however the decision was made to explicitly ignore this for now, as this PR is primarily targeting users who have already had the deprecation migration run, and no longer have the `code_actions_on_format` key Release Notes: - Fixed an issue with a settings migration that deprecated the `code_actions_on_format` setting. The `code_actions_on_format` setting has been un-deprecated, and affected users will have the bad migration rolled back with an updated migration --------- Co-authored-by: Cole Miller Co-authored-by: Mikayla Maki Co-authored-by: HactarCE <6060305+HactarCE@users.noreply.github.com> --- assets/settings/default.json | 5 +- crates/language/src/language_settings.rs | 3 + crates/migrator/src/migrations.rs | 4 +- .../src/migrations/m_2025_10_10/settings.rs | 70 ---- .../src/migrations/m_2025_10_16/settings.rs | 68 ++++ crates/migrator/src/migrator.rs | 301 ++++-------------- crates/migrator/src/patterns.rs | 1 + crates/migrator/src/patterns/settings.rs | 21 ++ crates/project/src/lsp_store.rs | 31 ++ .../settings/src/settings_content/language.rs | 5 + crates/settings_ui/src/page_data.rs | 21 ++ docs/src/languages/javascript.md | 6 +- 12 files changed, 212 insertions(+), 324 deletions(-) delete mode 100644 crates/migrator/src/migrations/m_2025_10_10/settings.rs create mode 100644 crates/migrator/src/migrations/m_2025_10_16/settings.rs diff --git a/assets/settings/default.json b/assets/settings/default.json index 75d212278228174f1afa6cbe3b272077833b0e8c..1bc968b987cf878d698c1bfd6abd862519c18a62 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -1521,6 +1521,7 @@ // A value of 45 preserves colorful themes while ensuring legibility. "minimum_contrast": 45 }, + "code_actions_on_format": {}, // Settings related to running tasks. "tasks": { "variables": {}, @@ -1690,7 +1691,9 @@ "preferred_line_length": 72 }, "Go": { - "formatter": [{ "code_action": "source.organizeImports" }, "language_server"], + "code_actions_on_format": { + "source.organizeImports": true + }, "debuggers": ["Delve"] }, "GraphQL": { diff --git a/crates/language/src/language_settings.rs b/crates/language/src/language_settings.rs index 0fc28672edadf144916f44e4d642c82e81fc8470..6fbbefd0d9d50db43b319a72d380ec3af72633fa 100644 --- a/crates/language/src/language_settings.rs +++ b/crates/language/src/language_settings.rs @@ -142,6 +142,8 @@ pub struct LanguageSettings { pub auto_indent_on_paste: bool, /// Controls how the editor handles the autoclosed characters. pub always_treat_brackets_as_autoclosed: bool, + /// Which code actions to run on save + pub code_actions_on_format: HashMap, /// Whether to perform linked edits pub linked_edits: bool, /// Task configuration for this language. @@ -566,6 +568,7 @@ impl settings::Settings for AllLanguageSettings { always_treat_brackets_as_autoclosed: settings .always_treat_brackets_as_autoclosed .unwrap(), + code_actions_on_format: settings.code_actions_on_format.unwrap(), linked_edits: settings.linked_edits.unwrap(), tasks: LanguageTaskSettings { variables: tasks.variables.unwrap_or_default(), diff --git a/crates/migrator/src/migrations.rs b/crates/migrator/src/migrations.rs index 1b8ede68b1eb8686325c896723d1fdc762d02b73..1936c6d7ac179c33474186f0c0e704afc3df5e00 100644 --- a/crates/migrator/src/migrations.rs +++ b/crates/migrator/src/migrations.rs @@ -118,8 +118,8 @@ pub(crate) mod m_2025_10_03 { pub(crate) use settings::SETTINGS_PATTERNS; } -pub(crate) mod m_2025_10_10 { +pub(crate) mod m_2025_10_16 { mod settings; - pub(crate) use settings::remove_code_actions_on_format; + pub(crate) use settings::restore_code_actions_on_format; } diff --git a/crates/migrator/src/migrations/m_2025_10_10/settings.rs b/crates/migrator/src/migrations/m_2025_10_10/settings.rs deleted file mode 100644 index 1d07be71a139b60e4b362d26c68b25922f04a233..0000000000000000000000000000000000000000 --- a/crates/migrator/src/migrations/m_2025_10_10/settings.rs +++ /dev/null @@ -1,70 +0,0 @@ -use anyhow::Result; -use serde_json::Value; - -pub fn remove_code_actions_on_format(value: &mut Value) -> Result<()> { - remove_code_actions_on_format_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_code_actions_on_format_inner(language, &path)?; - } - } - Ok(()) -} - -fn remove_code_actions_on_format_inner(value: &mut Value, path: &[&str]) -> Result<()> { - let Some(obj) = value.as_object_mut() else { - return Ok(()); - }; - let Some(code_actions_on_format) = obj.get("code_actions_on_format").cloned() else { - return Ok(()); - }; - - fn fmt_path(path: &[&str], key: &str) -> String { - let mut path = path.to_vec(); - path.push(key); - path.join(".") - } - - anyhow::ensure!( - code_actions_on_format.is_object(), - r#"The `code_actions_on_format` setting is deprecated, but it is in an invalid state and cannot be migrated at {}. Please ensure the code_actions_on_format setting is a Map"#, - fmt_path(path, "code_actions_on_format"), - ); - - let code_actions_map = code_actions_on_format.as_object().unwrap(); - let mut code_actions = vec![]; - for (code_action, code_action_enabled) in code_actions_map { - if code_action_enabled.as_bool().map_or(false, |b| !b) { - continue; - } - code_actions.push(code_action.clone()); - } - - let mut formatter_array = vec![]; - if let Some(formatter) = obj.get("formatter") { - if let Some(array) = formatter.as_array() { - formatter_array = array.clone(); - } else { - formatter_array.insert(0, formatter.clone()); - } - }; - let found_code_actions = !code_actions.is_empty(); - formatter_array.splice( - 0..0, - code_actions - .into_iter() - .map(|code_action| serde_json::json!({"code_action": code_action})), - ); - - obj.remove("code_actions_on_format"); - if found_code_actions { - obj.insert("formatter".to_string(), Value::Array(formatter_array)); - } - - Ok(()) -} diff --git a/crates/migrator/src/migrations/m_2025_10_16/settings.rs b/crates/migrator/src/migrations/m_2025_10_16/settings.rs new file mode 100644 index 0000000000000000000000000000000000000000..c9a8b7600cbf8842453318ed8dc9b47c3a73beaf --- /dev/null +++ b/crates/migrator/src/migrations/m_2025_10_16/settings.rs @@ -0,0 +1,68 @@ +use anyhow::Result; +use serde_json::Value; + +use crate::patterns::migrate_language_setting; + +pub fn restore_code_actions_on_format(value: &mut Value) -> Result<()> { + migrate_language_setting(value, restore_code_actions_on_format_inner) +} + +fn restore_code_actions_on_format_inner(value: &mut Value, path: &[&str]) -> Result<()> { + let Some(obj) = value.as_object_mut() else { + return Ok(()); + }; + let code_actions_on_format = obj + .get("code_actions_on_format") + .cloned() + .unwrap_or_else(|| Value::Object(Default::default())); + + fn fmt_path(path: &[&str], key: &str) -> String { + let mut path = path.to_vec(); + path.push(key); + path.join(".") + } + + let Some(mut code_actions_map) = code_actions_on_format.as_object().cloned() else { + anyhow::bail!( + r#"The `code_actions_on_format` is in an invalid state and cannot be migrated at {}. Please ensure the code_actions_on_format setting is a Map"#, + fmt_path(path, "code_actions_on_format"), + ); + }; + + let Some(formatter) = obj.get("formatter") else { + return Ok(()); + }; + let formatter_array = if let Some(array) = formatter.as_array() { + array.clone() + } else { + vec![formatter.clone()] + }; + let mut code_action_formatters = Vec::new(); + for formatter in formatter_array { + let Some(code_action) = formatter.get("code_action") else { + return Ok(()); + }; + let Some(code_action_name) = code_action.as_str() else { + anyhow::bail!( + r#"The `code_action` is in an invalid state and cannot be migrated at {}. Please ensure the code_action setting is a String"#, + fmt_path(path, "formatter"), + ); + }; + code_action_formatters.push(code_action_name.to_string()); + } + + code_actions_map.extend( + code_action_formatters + .into_iter() + .rev() + .map(|code_action| (code_action, Value::Bool(true))), + ); + + obj.remove("formatter"); + obj.insert( + "code_actions_on_format".into(), + Value::Object(code_actions_map), + ); + + Ok(()) +} diff --git a/crates/migrator/src/migrator.rs b/crates/migrator/src/migrator.rs index aea11f98c460cc6c72120138ca1068be9ea60923..0adc4d14276e8d49d44af5cc29daff13657e3b8b 100644 --- a/crates/migrator/src/migrator.rs +++ b/crates/migrator/src/migrator.rs @@ -213,7 +213,7 @@ pub fn migrate_settings(text: &str) -> Result> { migrations::m_2025_10_03::SETTINGS_PATTERNS, &SETTINGS_QUERY_2025_10_03, ), - MigrationType::Json(migrations::m_2025_10_10::remove_code_actions_on_format), + MigrationType::Json(migrations::m_2025_10_16::restore_code_actions_on_format), ]; run_migrations(text, migrations) } @@ -367,6 +367,7 @@ mod tests { pretty_assertions::assert_eq!(migrated.as_deref(), output); } + #[track_caller] fn assert_migrate_settings(input: &str, output: Option<&str>) { let migrated = migrate_settings(input).unwrap(); assert_migrated_correctly(migrated, output); @@ -1341,7 +1342,11 @@ mod tests { #[test] fn test_flatten_code_action_formatters_basic_array() { - assert_migrate_settings( + assert_migrate_settings_with_migrations( + &[MigrationType::TreeSitter( + migrations::m_2025_10_01::SETTINGS_PATTERNS, + &SETTINGS_QUERY_2025_10_01, + )], &r#"{ "formatter": [ { @@ -1368,7 +1373,11 @@ mod tests { #[test] fn test_flatten_code_action_formatters_basic_object() { - 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": { @@ -1500,7 +1509,11 @@ mod tests { #[test] fn test_flatten_code_action_formatters_array_with_multiple_action_blocks_in_defaults_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": { @@ -1916,297 +1929,91 @@ mod tests { } #[test] - fn test_code_actions_on_format_migration_basic() { + fn test_restore_code_actions_on_format() { assert_migrate_settings_with_migrations( &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, + migrations::m_2025_10_16::restore_code_actions_on_format, )], &r#"{ - "code_actions_on_format": { - "source.organizeImports": true, - "source.fixAll": true - } - }"# - .unindent(), - Some( - &r#"{ - "formatter": [ - { - "code_action": "source.organizeImports" - }, - { - "code_action": "source.fixAll" - } - ] - } - "# - .unindent(), - ), - ); - } - - #[test] - fn test_code_actions_on_format_migration_filters_false_values() { - assert_migrate_settings_with_migrations( - &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, - )], - &r#"{ - "code_actions_on_format": { - "a": true, - "b": false, - "c": true + "formatter": { + "code_action": "foo" } }"# .unindent(), Some( &r#"{ - "formatter": [ - { - "code_action": "a" - }, - { - "code_action": "c" - } - ] + "code_actions_on_format": { + "foo": true + } } "# .unindent(), ), ); - } - #[test] - fn test_code_actions_on_format_migration_with_existing_formatter_object() { assert_migrate_settings_with_migrations( &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, + migrations::m_2025_10_16::restore_code_actions_on_format, )], &r#"{ - "formatter": "prettier", - "code_actions_on_format": { - "source.organizeImports": true - } - }"# - .unindent(), - Some( - &r#"{ - "formatter": [ - { - "code_action": "source.organizeImports" - }, - "prettier" - ] - }"# - .unindent(), - ), - ); - } - - #[test] - fn test_code_actions_on_format_migration_with_existing_formatter_array() { - assert_migrate_settings_with_migrations( - &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, - )], - &r#"{ - "formatter": ["prettier", {"language_server": "eslint"}], - "code_actions_on_format": { - "source.organizeImports": true, - "source.fixAll": true - } + "formatter": [ + { "code_action": "foo" }, + "auto" + ] }"# .unindent(), - Some( - &r#"{ - "formatter": [ - { - "code_action": "source.organizeImports" - }, - { - "code_action": "source.fixAll" - }, - "prettier", - { - "language_server": "eslint" - } - ] - }"# - .unindent(), - ), + None, ); - } - #[test] - fn test_code_actions_on_format_migration_in_languages() { assert_migrate_settings_with_migrations( &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, + migrations::m_2025_10_16::restore_code_actions_on_format, )], &r#"{ - "languages": { - "JavaScript": { - "code_actions_on_format": { - "source.fixAll.eslint": true - } - }, - "Go": { - "code_actions_on_format": { - "source.organizeImports": true - } - } + "formatter": { + "code_action": "foo" + }, + "code_actions_on_format": { + "bar": true, + "baz": false } }"# .unindent(), Some( &r#"{ - "languages": { - "JavaScript": { - "formatter": [ - { - "code_action": "source.fixAll.eslint" - } - ] - }, - "Go": { - "formatter": [ - { - "code_action": "source.organizeImports" - } - ] - } + "code_actions_on_format": { + "foo": true, + "bar": true, + "baz": false } }"# .unindent(), ), ); - } - #[test] - fn test_code_actions_on_format_migration_in_languages_with_existing_formatter() { assert_migrate_settings_with_migrations( &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, + migrations::m_2025_10_16::restore_code_actions_on_format, )], &r#"{ - "languages": { - "JavaScript": { - "formatter": "prettier", - "code_actions_on_format": { - "source.fixAll.eslint": true, - "source.organizeImports": false - } + "formatter": [ + { "code_action": "foo" }, + { "code_action": "qux" }, + ], + "code_actions_on_format": { + "bar": true, + "baz": false } - } }"# .unindent(), Some( &r#"{ - "languages": { - "JavaScript": { - "formatter": [ - { - "code_action": "source.fixAll.eslint" - }, - "prettier" - ] + "code_actions_on_format": { + "foo": true, + "qux": true, + "bar": true, + "baz": false } - } - }"# - .unindent(), - ), - ); - } - - #[test] - fn test_code_actions_on_format_migration_mixed_global_and_languages() { - assert_migrate_settings_with_migrations( - &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, - )], - &r#"{ - "formatter": "prettier", - "code_actions_on_format": { - "source.fixAll": true - }, - "languages": { - "Rust": { - "formatter": "rust-analyzer", - "code_actions_on_format": { - "source.organizeImports": true - } - }, - "Python": { - "code_actions_on_format": { - "source.organizeImports": true, - "source.fixAll": false - } - } - } - }"# - .unindent(), - Some( - &r#"{ - "formatter": [ - { - "code_action": "source.fixAll" - }, - "prettier" - ], - "languages": { - "Rust": { - "formatter": [ - { - "code_action": "source.organizeImports" - }, - "rust-analyzer" - ] - }, - "Python": { - "formatter": [ - { - "code_action": "source.organizeImports" - } - ] - } - } - }"# - .unindent(), - ), - ); - } - - #[test] - fn test_code_actions_on_format_no_migration_when_not_present() { - assert_migrate_settings_with_migrations( - &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, - )], - &r#"{ - "formatter": ["prettier"] - }"# - .unindent(), - None, - ); - } - - #[test] - fn test_code_actions_on_format_migration_all_false_values() { - assert_migrate_settings_with_migrations( - &[MigrationType::Json( - migrations::m_2025_10_10::remove_code_actions_on_format, - )], - &r#"{ - "code_actions_on_format": { - "a": false, - "b": false - }, - "formatter": "prettier" - }"# - .unindent(), - Some( - &r#"{ - "formatter": "prettier" }"# .unindent(), ), diff --git a/crates/migrator/src/patterns.rs b/crates/migrator/src/patterns.rs index 3848baf23ba0d324995f18e3a53921948291153b..4132c93d9367a8dee200200e03dcc46ee073e67f 100644 --- a/crates/migrator/src/patterns.rs +++ b/crates/migrator/src/patterns.rs @@ -10,4 +10,5 @@ pub(crate) use settings::{ SETTINGS_ASSISTANT_PATTERN, SETTINGS_ASSISTANT_TOOLS_PATTERN, SETTINGS_DUPLICATED_AGENT_PATTERN, SETTINGS_EDIT_PREDICTIONS_ASSISTANT_PATTERN, SETTINGS_LANGUAGES_PATTERN, SETTINGS_NESTED_KEY_VALUE_PATTERN, SETTINGS_ROOT_KEY_VALUE_PATTERN, + migrate_language_setting, }; diff --git a/crates/migrator/src/patterns/settings.rs b/crates/migrator/src/patterns/settings.rs index 72fd02b153a5cf6e3158790f1c5d09a9f643ebf9..a068cce23b013a3435188c03ceebe866883c4e6d 100644 --- a/crates/migrator/src/patterns/settings.rs +++ b/crates/migrator/src/patterns/settings.rs @@ -108,3 +108,24 @@ pub const SETTINGS_DUPLICATED_AGENT_PATTERN: &str = r#"(document (#eq? @agent1 "agent") (#eq? @agent2 "agent") )"#; + +/// Migrate language settings, +/// calls `migrate_fn` with the top level object as well as all language settings under the "languages" key +/// Fails early if `migrate_fn` returns an error at any point +pub fn migrate_language_setting( + value: &mut serde_json::Value, + migrate_fn: fn(&mut serde_json::Value, path: &[&str]) -> anyhow::Result<()>, +) -> anyhow::Result<()> { + migrate_fn(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]; + migrate_fn(language, &path)?; + } + } + Ok(()) +} diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index eeb378f2e237193b0feceda22176ed1e1d480328..5777b4fbbc554717cd188c24abb3fe90efb98746 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1336,6 +1336,32 @@ impl LocalLspStore { })?; } + // Formatter for `code_actions_on_format` that runs before + // the rest of the formatters + let mut code_actions_on_format_formatters = None; + let should_run_code_actions_on_format = !matches!( + (trigger, &settings.format_on_save), + (FormatTrigger::Save, &FormatOnSave::Off) + ); + if should_run_code_actions_on_format { + let have_code_actions_to_run_on_format = settings + .code_actions_on_format + .values() + .any(|enabled| *enabled); + if have_code_actions_to_run_on_format { + zlog::trace!(logger => "going to run code actions on format"); + code_actions_on_format_formatters = Some( + settings + .code_actions_on_format + .iter() + .filter_map(|(action, enabled)| enabled.then_some(action)) + .cloned() + .map(Formatter::CodeAction) + .collect::>(), + ); + } + } + let formatters = match (trigger, &settings.format_on_save) { (FormatTrigger::Save, FormatOnSave::Off) => &[], (FormatTrigger::Manual, _) | (FormatTrigger::Save, FormatOnSave::On) => { @@ -1343,6 +1369,11 @@ impl LocalLspStore { } }; + let formatters = code_actions_on_format_formatters + .iter() + .flatten() + .chain(formatters); + for formatter in formatters { let formatter = if formatter == &Formatter::Auto { if settings.prettier.allowed { diff --git a/crates/settings/src/settings_content/language.rs b/crates/settings/src/settings_content/language.rs index 049ab4255c0a276d85121079f6b1d9657f485aa7..33bb1705666b00616a2340699da0facc969a0cf4 100644 --- a/crates/settings/src/settings_content/language.rs +++ b/crates/settings/src/settings_content/language.rs @@ -300,6 +300,11 @@ pub struct LanguageSettingsContent { /// /// Default: true pub use_on_type_format: Option, + /// Which code actions to run on save after the formatter. + /// These are not run if formatting is off. + /// + /// Default: {} (or {"source.organizeImports": true} for Go). + pub code_actions_on_format: Option>, /// Whether to perform linked edits of associated ranges, if the language server supports it. /// For example, when editing opening tag, the contents of the closing tag will be edited as well. /// diff --git a/crates/settings_ui/src/page_data.rs b/crates/settings_ui/src/page_data.rs index f2a8fc2d954ddbf1498666820b84847185f9197f..36f320c12696743eb17639d67fea19681aff6a04 100644 --- a/crates/settings_ui/src/page_data.rs +++ b/crates/settings_ui/src/page_data.rs @@ -5395,6 +5395,27 @@ fn language_settings_data() -> Vec { metadata: None, files: USER | LOCAL, }), + SettingsPageItem::SettingItem(SettingItem { + title: "Code Actions On Format", + description: "Additional Code Actions To Run When Formatting", + field: Box::new( + SettingField { + pick: |settings_content| { + language_settings_field(settings_content, |language| { + &language.code_actions_on_format + }) + }, + pick_mut: |settings_content| { + language_settings_field_mut(settings_content, |language| { + &mut language.code_actions_on_format + }) + }, + } + .unimplemented(), + ), + metadata: None, + files: USER | LOCAL, + }), SettingsPageItem::SectionHeader("Autoclose"), SettingsPageItem::SettingItem(SettingItem { title: "Use Autoclose", diff --git a/docs/src/languages/javascript.md b/docs/src/languages/javascript.md index c71071a9b37c74c2226796083af3ae557751da8e..b78d48f3e32656d7dabc135d48675e8a0e630db6 100644 --- a/docs/src/languages/javascript.md +++ b/docs/src/languages/javascript.md @@ -92,10 +92,8 @@ the formatter: { "languages": { "JavaScript": { - "formatter": { - "code_actions": { - "source.fixAll.eslint": true - } + "code_actions_on_format": { + "source.fixAll.eslint": true } } }