Tighten up MergeFrom trait (#38473)

Conrad Irwin created

Release Notes:

- N/A

Change summary

crates/language/src/language_settings.rs         |  2 
crates/settings/src/merge_from.rs                | 96 +++++++++--------
crates/settings/src/settings_content.rs          | 24 +++-
crates/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(-)

Detailed changes

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),

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<T: Clone> MergeFrom for Vec<T> {
-    fn merge_from(&mut self, other: Option<&Self>) {
-        if let Some(other) = other {
-            *self = other.clone()
+impl<T: Clone + MergeFrom> MergeFrom for Option<T> {
+    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<T: Clone> MergeFrom for Vec<T> {
+    fn merge_from(&mut self, other: &Self) {
+        *self = other.clone()
+    }
+}
+
+impl<T: MergeFrom> MergeFrom for Box<T> {
+    fn merge_from(&mut self, other: &Self) {
+        self.as_mut().merge_from(other.as_ref())
+    }
+}
+
 // Implementations for collections that extend/merge their contents
 impl<K, V> MergeFrom for collections::HashMap<K, V>
 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<K> + 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<T> MergeFrom for collections::BTreeSet<T>
 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<T> MergeFrom for collections::HashSet<T>
 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<T: MergeFrom + Clone> MergeFrom for Rc<T> {
-    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)
-    }
-}

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<String, bool> 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<T>(pub Vec<T>);
 
 impl<T> Into<Vec<T>> for ExtendingVec<T> {
@@ -841,13 +849,15 @@ impl<T> From<Vec<T>> for ExtendingVec<T> {
 }
 
 impl<T: Clone> merge_from::MergeFrom for ExtendingVec<T> {
-    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<bool> 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
     }
 }

crates/settings/src/settings_content/language.rs 🔗

@@ -34,37 +34,27 @@ pub struct AllLanguageSettingsContent {
     pub file_types: HashMap<Arc<str>, ExtendingVec<String>>,
 }
 
-fn merge_option<T: merge_from::MergeFrom + Clone>(this: &mut Option<T>, 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);
             }

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);
 

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<T>` 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<T> 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<T> 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<T>`
-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,
-    }
-}