From e62dd2a0e584154886ff86cc1e9e3e060558b977 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 18 Sep 2025 22:28:17 -0600 Subject: [PATCH] Tighten up MergeFrom trait (#38473) Release Notes: - N/A --- crates/language/src/language_settings.rs | 2 +- crates/settings/src/merge_from.rs | 96 ++++++++++--------- crates/settings/src/settings_content.rs | 24 +++-- .../settings/src/settings_content/language.rs | 26 ++--- crates/settings/src/settings_store.rs | 16 ++-- crates/settings_macros/src/settings_macros.rs | 76 +++------------ 6 files changed, 97 insertions(+), 143 deletions(-) diff --git a/crates/language/src/language_settings.rs b/crates/language/src/language_settings.rs index 64744ee99d24a56abb357e0c034e11afa4dae9d0..0e05123033bf92d537eef5eab258db4eac7e7a56 100644 --- a/crates/language/src/language_settings.rs +++ b/crates/language/src/language_settings.rs @@ -582,7 +582,7 @@ impl settings::Settings for AllLanguageSettings { let mut languages = HashMap::default(); for (language_name, settings) in &all_languages.languages.0 { let mut language_settings = all_languages.defaults.clone(); - settings::merge_from::MergeFrom::merge_from(&mut language_settings, Some(settings)); + settings::merge_from::MergeFrom::merge_from(&mut language_settings, settings); languages.insert( LanguageName(language_name.clone()), load_from_content(language_settings), diff --git a/crates/settings/src/merge_from.rs b/crates/settings/src/merge_from.rs index 11c0785bcb466e26de956475fb5bd4f9821c2790..c12994786ff5fadb6686c6ab1b93d9700195eb2a 100644 --- a/crates/settings/src/merge_from.rs +++ b/crates/settings/src/merge_from.rs @@ -1,29 +1,37 @@ -use std::rc::Rc; - /// Trait for recursively merging settings structures. /// -/// This trait allows settings objects to be merged from optional sources, -/// where `None` values are ignored and `Some` values override existing values. +/// When Zed starts it loads settinsg from `default.json` to initialize +/// everything. These may be further refined by loading the user's settings, +/// and any settings profiles; and then further refined by loading any +/// local project settings. +/// +/// The default behaviour of merging is: +/// * For objects with named keys (HashMap, structs, etc.). The values are merged deeply +/// (so if the default settings has languages.JSON.prettier.allowed = true, and the user's settings has +/// languages.JSON.tab_size = 4; the merged settings file will have both settings). +/// * For options, a None value is ignored, but Some values are merged recursively. +/// * For other types (including Vec), a merge overwrites the current value. /// -/// HashMaps, structs and similar types are merged by combining their contents key-wise, -/// but all other types (including Vecs) are last-write-wins. -/// (Though see also ExtendingVec and SaturatingBool) +/// If you want to break the rules you can (e.g. ExtendingVec, or SaturatingBool). #[allow(unused)] pub trait MergeFrom { + /// Merge from a source of the same type. + fn merge_from(&mut self, other: &Self); + /// Merge from an optional source of the same type. - /// If `other` is `None`, no changes are made. - /// If `other` is `Some(value)`, fields from `value` are merged into `self`. - fn merge_from(&mut self, other: Option<&Self>); + fn merge_from_option(&mut self, other: Option<&Self>) { + if let Some(other) = other { + self.merge_from(other); + } + } } macro_rules! merge_from_overwrites { ($($type:ty),+) => { $( impl MergeFrom for $type { - fn merge_from(&mut self, other: Option<&Self>) { - if let Some(value) = other { - *self = value.clone(); - } + fn merge_from(&mut self, other: &Self) { + *self = other.clone(); } } )+ @@ -51,25 +59,41 @@ merge_from_overwrites!( gpui::FontFeatures ); -impl MergeFrom for Vec { - fn merge_from(&mut self, other: Option<&Self>) { - if let Some(other) = other { - *self = other.clone() +impl MergeFrom for Option { + fn merge_from(&mut self, other: &Self) { + let Some(other) = other else { + return; + }; + if let Some(this) = self { + this.merge_from(other); + } else { + self.replace(other.clone()); } } } +impl MergeFrom for Vec { + fn merge_from(&mut self, other: &Self) { + *self = other.clone() + } +} + +impl MergeFrom for Box { + fn merge_from(&mut self, other: &Self) { + self.as_mut().merge_from(other.as_ref()) + } +} + // Implementations for collections that extend/merge their contents impl MergeFrom for collections::HashMap where K: Clone + std::hash::Hash + Eq, V: Clone + MergeFrom, { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { for (k, v) in other { if let Some(existing) = self.get_mut(k) { - existing.merge_from(Some(v)); + existing.merge_from(v); } else { self.insert(k.clone(), v.clone()); } @@ -82,11 +106,10 @@ where K: Clone + std::hash::Hash + Eq + Ord, V: Clone + MergeFrom, { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { for (k, v) in other { if let Some(existing) = self.get_mut(k) { - existing.merge_from(Some(v)); + existing.merge_from(v); } else { self.insert(k.clone(), v.clone()); } @@ -100,11 +123,10 @@ where // Q: ?Sized + std::hash::Hash + collections::Equivalent + Eq, V: Clone + MergeFrom, { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { for (k, v) in other { if let Some(existing) = self.get_mut(k) { - existing.merge_from(Some(v)); + existing.merge_from(v); } else { self.insert(k.clone(), v.clone()); } @@ -116,8 +138,7 @@ impl MergeFrom for collections::BTreeSet where T: Clone + Ord, { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { for item in other { self.insert(item.clone()); } @@ -128,8 +149,7 @@ impl MergeFrom for collections::HashSet where T: Clone + std::hash::Hash + Eq, { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { for item in other { self.insert(item.clone()); } @@ -137,13 +157,12 @@ where } impl MergeFrom for serde_json::Value { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; + fn merge_from(&mut self, other: &Self) { match (self, other) { (serde_json::Value::Object(this), serde_json::Value::Object(other)) => { for (k, v) in other { if let Some(existing) = this.get_mut(k) { - existing.merge_from(other.get(k)); + existing.merge_from(v); } else { this.insert(k.clone(), v.clone()); } @@ -153,12 +172,3 @@ impl MergeFrom for serde_json::Value { } } } - -impl MergeFrom for Rc { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; - let mut this: T = self.as_ref().clone(); - this.merge_from(Some(other.as_ref())); - *self = Rc::new(this) - } -} diff --git a/crates/settings/src/settings_content.rs b/crates/settings/src/settings_content.rs index 2ef42d8ebd730343f749d3e2e48055a2d02819ad..43402cae0e6c723b4cc2e94f28c1ba7d0c61c928 100644 --- a/crates/settings/src/settings_content.rs +++ b/crates/settings/src/settings_content.rs @@ -827,6 +827,14 @@ pub struct ReplSettingsContent { } #[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +/// An ExtendingVec in the settings can only accumulate new values. +/// +/// This is useful for things like private files where you only want +/// to allow new values to be added. +/// +/// Consider using a HashMap instead of this type +/// (like auto_install_extensions) so that user settings files can both add +/// and remove values from the set. pub struct ExtendingVec(pub Vec); impl Into> for ExtendingVec { @@ -841,13 +849,15 @@ impl From> for ExtendingVec { } impl merge_from::MergeFrom for ExtendingVec { - fn merge_from(&mut self, other: Option<&Self>) { - if let Some(other) = other { - self.0.extend_from_slice(other.0.as_slice()); - } + fn merge_from(&mut self, other: &Self) { + self.0.extend_from_slice(other.0.as_slice()); } } +/// A SaturatingBool in the settings can only ever be set to true, +/// later attempts to set it to false will be ignored. +/// +/// Used by `disable_ai`. #[derive(Debug, Default, Copy, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] pub struct SaturatingBool(pub bool); @@ -858,9 +868,7 @@ impl From for SaturatingBool { } impl merge_from::MergeFrom for SaturatingBool { - fn merge_from(&mut self, other: Option<&Self>) { - if let Some(other) = other { - self.0 |= other.0 - } + fn merge_from(&mut self, other: &Self) { + self.0 |= other.0 } } diff --git a/crates/settings/src/settings_content/language.rs b/crates/settings/src/settings_content/language.rs index ef435d638359825128729d0c024cde8e5c5613c8..6052afee671edba49e05b56ddef147a01866e364 100644 --- a/crates/settings/src/settings_content/language.rs +++ b/crates/settings/src/settings_content/language.rs @@ -34,37 +34,27 @@ pub struct AllLanguageSettingsContent { pub file_types: HashMap, ExtendingVec>, } -fn merge_option(this: &mut Option, other: Option<&T>) { - let Some(other) = other else { return }; - if let Some(this) = this { - this.merge_from(Some(other)); - } else { - this.replace(other.clone()); - } -} - impl merge_from::MergeFrom for AllLanguageSettingsContent { - fn merge_from(&mut self, other: Option<&Self>) { - let Some(other) = other else { return }; - self.file_types.merge_from(Some(&other.file_types)); - merge_option(&mut self.features, other.features.as_ref()); - merge_option(&mut self.edit_predictions, other.edit_predictions.as_ref()); + fn merge_from(&mut self, other: &Self) { + self.file_types.merge_from(&other.file_types); + self.features.merge_from(&other.features); + self.edit_predictions.merge_from(&other.edit_predictions); // A user's global settings override the default global settings and // all default language-specific settings. // - self.defaults.merge_from(Some(&other.defaults)); + self.defaults.merge_from(&other.defaults); for language_settings in self.languages.0.values_mut() { - language_settings.merge_from(Some(&other.defaults)); + language_settings.merge_from(&other.defaults); } // A user's language-specific settings override default language-specific settings. for (language_name, user_language_settings) in &other.languages.0 { if let Some(existing) = self.languages.0.get_mut(language_name) { - existing.merge_from(Some(&user_language_settings)); + existing.merge_from(&user_language_settings); } else { let mut new_settings = self.defaults.clone(); - new_settings.merge_from(Some(&user_language_settings)); + new_settings.merge_from(&user_language_settings); self.languages.0.insert(language_name.clone(), new_settings); } diff --git a/crates/settings/src/settings_store.rs b/crates/settings/src/settings_store.rs index fe2a5cfdfc6493cf3ef374a66c389022748e088b..dc703e50f1de43aee8059e144dc4cb0815b3472d 100644 --- a/crates/settings/src/settings_store.rs +++ b/crates/settings/src/settings_store.rs @@ -870,15 +870,15 @@ impl SettingsStore { if changed_local_path.is_none() { let mut merged = self.default_settings.as_ref().clone(); - merged.merge_from(self.extension_settings.as_deref()); - merged.merge_from(self.global_settings.as_deref()); + merged.merge_from_option(self.extension_settings.as_deref()); + merged.merge_from_option(self.global_settings.as_deref()); if let Some(user_settings) = self.user_settings.as_ref() { - merged.merge_from(Some(&user_settings.content)); - merged.merge_from(user_settings.for_release_channel()); - merged.merge_from(user_settings.for_os()); - merged.merge_from(user_settings.for_profile(cx)); + merged.merge_from(&user_settings.content); + merged.merge_from_option(user_settings.for_release_channel()); + merged.merge_from_option(user_settings.for_os()); + merged.merge_from_option(user_settings.for_profile(cx)); } - merged.merge_from(self.server_settings.as_deref()); + merged.merge_from_option(self.server_settings.as_deref()); self.merged_settings = Rc::new(merged); for setting_value in self.setting_values.values_mut() { @@ -906,7 +906,7 @@ impl SettingsStore { } else { self.merged_settings.as_ref().clone() }; - merged_local_settings.merge_from(Some(local_settings)); + merged_local_settings.merge_from(local_settings); project_settings_stack.push(merged_local_settings); diff --git a/crates/settings_macros/src/settings_macros.rs b/crates/settings_macros/src/settings_macros.rs index 33c136b1f2b3e4bec3528d4dff632e05119bc516..1a7c391847e5093754f241ffccb079cc5ddd1a6b 100644 --- a/crates/settings_macros/src/settings_macros.rs +++ b/crates/settings_macros/src/settings_macros.rs @@ -1,13 +1,11 @@ use proc_macro::TokenStream; use quote::quote; -use syn::{Data, DeriveInput, Fields, Type, parse_macro_input}; +use syn::{Data, DeriveInput, Fields, parse_macro_input}; /// Derives the `MergeFrom` trait for a struct. /// /// This macro automatically implements `MergeFrom` by calling `merge_from` -/// on all fields in the struct. For `Option` fields, it merges by taking -/// the `other` value when `self` is `None`. For other types, it recursively -/// calls `merge_from` on the field. +/// on all fields in the struct. /// /// # Example /// @@ -30,61 +28,25 @@ pub fn derive_merge_from(input: TokenStream) -> TokenStream { Fields::Named(fields) => { let field_merges = fields.named.iter().map(|field| { let field_name = &field.ident; - let field_type = &field.ty; - - if is_option_type(field_type) { - // For Option fields, merge by taking the other value if self is None - quote! { - if let Some(other_value) = other.#field_name.as_ref() { - if self.#field_name.is_none() { - self.#field_name = Some(other_value.clone()); - } else if let Some(self_value) = self.#field_name.as_mut() { - self_value.merge_from(Some(other_value)); - } - } - } - } else { - // For non-Option fields, recursively call merge_from - quote! { - self.#field_name.merge_from(Some(&other.#field_name)); - } + quote! { + self.#field_name.merge_from(&other.#field_name); } }); quote! { - if let Some(other) = other { - #(#field_merges)* - } + #(#field_merges)* } } Fields::Unnamed(fields) => { - let field_merges = fields.unnamed.iter().enumerate().map(|(i, field)| { + let field_merges = fields.unnamed.iter().enumerate().map(|(i, _)| { let field_index = syn::Index::from(i); - let field_type = &field.ty; - - if is_option_type(field_type) { - // For Option fields, merge by taking the other value if self is None - quote! { - if let Some(other_value) = other.#field_index.as_ref() { - if self.#field_index.is_none() { - self.#field_index = Some(other_value.clone()); - } else if let Some(self_value) = self.#field_index.as_mut() { - self_value.merge_from(Some(other_value)); - } - } - } - } else { - // For non-Option fields, recursively call merge_from - quote! { - self.#field_index.merge_from(Some(&other.#field_index)); - } + quote! { + self.#field_index.merge_from(&other.#field_index); } }); quote! { - if let Some(other) = other { - #(#field_merges)* - } + #(#field_merges)* } } Fields::Unit => { @@ -95,9 +57,7 @@ pub fn derive_merge_from(input: TokenStream) -> TokenStream { }, Data::Enum(_) => { quote! { - if let Some(other) = other { - *self = other.clone(); - } + *self = other.clone(); } } Data::Union(_) => { @@ -107,7 +67,7 @@ pub fn derive_merge_from(input: TokenStream) -> TokenStream { let expanded = quote! { impl #impl_generics crate::merge_from::MergeFrom for #name #ty_generics #where_clause { - fn merge_from(&mut self, other: ::core::option::Option<&Self>) { + fn merge_from(&mut self, other: &Self) { use crate::merge_from::MergeFrom as _; #merge_body } @@ -116,17 +76,3 @@ pub fn derive_merge_from(input: TokenStream) -> TokenStream { TokenStream::from(expanded) } - -/// Check if a type is `Option` -fn is_option_type(ty: &Type) -> bool { - match ty { - Type::Path(type_path) => { - if let Some(segment) = type_path.path.segments.last() { - segment.ident == "Option" - } else { - false - } - } - _ => false, - } -}