Actually generate AppliedPatchSet objects (#250)

* Start actually computing AppliedPatchSet values

* Improve patch_apply documentation and flesh out applied patch code

* Add file link notes

* Stub out where tests for snapshot subsystem will go

* Create baseline tests

* Fix build failure by silencing Clippy

src/change_processor.rs

@@ -51,6 +51,9 @@ impl ChangeProcessor {
             imfs.change_receiver()
         };
 
+        // Crossbeam's select macro generates code that Clippy doesn't like, and
+        // Clippy blames us for it.
+        #[allow(clippy::drop_copy)]
         loop {
             select! {
                 recv(imfs_receiver) -> event => {

src/snapshot/mod.rs

@@ -13,6 +13,8 @@
 //!    usually contain data that hasn't actually changed, and how coarsely
 //!    the snapshots happen is defined outside this part of the code.
 //!
+//!    See `src/snapshot_middleware` for implementation.
+//!
 //! 2. Input snapshots are turned into `PatchSet` objects by Rojo's diffing
 //!    algorithm via `compute_patch_set`. This operation doesn't mutate the
 //!    instance tree, so work at this point can be thrown away.
@@ -22,12 +24,16 @@
 //!    changes after the fact, since Rojo needs to assign IDs to created
 //!    instances, which happens during patch application, not generation!
 //!
+//!    See `src/snapshot/patch_compute.rs` for implementation.
+//!
 //! 3. Patch sets are applied to the tree with `apply_patch_set`, which
 //!    mutates the relevant instances. `apply_patch_set` returns a new
 //!    object, `AppliedPatchSet`. Applied patch sets describe the transform
 //!    that was applied, and are suitable for cases where another tree needs
 //!    to be synchronized with Rojo's, like the Rojo Studio plugin.
 //!
+//!    See `src/snapshot/patch_apply.rs` for implementation.
+//!
 //! The aim with this approach is to reduce the number of bugs that arise from
 //! attempting to manually update instances in response to filesystem updates.
 //! Instead of surgically identifying what needs to change, we can do rough
@@ -55,3 +61,6 @@ pub use patch::*;
 pub use patch_apply::apply_patch_set;
 pub use patch_compute::compute_patch_set;
 pub use tree::*;
+
+#[cfg(test)]
+mod tests;

src/snapshot/patch.rs

@@ -63,7 +63,7 @@ pub struct PatchUpdate {
 #[derive(Debug, Clone, Default)]
 pub struct AppliedPatchSet {
     pub removed: Vec<RbxId>,
-    pub added: Vec<AppliedPatchAdd>,
+    pub added: Vec<RbxId>,
     pub updated: Vec<AppliedPatchUpdate>,
 }
 
@@ -77,11 +77,6 @@ impl AppliedPatchSet {
     }
 }
 
-#[derive(Debug, Clone)]
-pub struct AppliedPatchAdd {
-    pub instance_id: RbxId,
-}
-
 #[derive(Debug, Clone)]
 pub struct AppliedPatchUpdate {
     pub id: RbxId,
@@ -92,3 +87,15 @@ pub struct AppliedPatchUpdate {
     pub changed_properties: HashMap<String, Option<RbxValue>>,
     pub changed_metadata: Option<InstanceMetadata>,
 }
+
+impl AppliedPatchUpdate {
+    pub fn new(id: RbxId) -> Self {
+        Self {
+            id,
+            changed_name: None,
+            changed_class_name: None,
+            changed_properties: HashMap::new(),
+            changed_metadata: None,
+        }
+    }
+}

src/snapshot/patch_apply.rs

@@ -5,10 +5,13 @@ use std::collections::HashMap;
 use rbx_dom_weak::{RbxId, RbxInstanceProperties, RbxValue};
 
 use super::{
-    patch::{AppliedPatchSet, PatchSet, PatchUpdate},
+    patch::{AppliedPatchSet, AppliedPatchUpdate, PatchSet, PatchUpdate},
     InstancePropertiesWithMeta, InstanceSnapshot, RojoTree,
 };
 
+/// Consumes the input `PatchSet`, applying all of its prescribed changes to the
+/// tree and returns an `AppliedPatchSet`, which can be used to keep another
+/// tree in sync with Rojo's.
 pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatchSet {
     let mut context = PatchApplyContext::default();
 
@@ -17,9 +20,11 @@ pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatch
     }
 
     for add_patch in patch_set.added_instances {
-        apply_add_child(&mut context, tree, add_patch.parent_id, &add_patch.instance);
+        apply_add_child(&mut context, tree, add_patch.parent_id, add_patch.instance);
     }
 
+    // Updates need to be applied after additions, which reduces the complexity
+    // of updates significantly.
     for update_patch in patch_set.updated_instances {
         apply_update_child(&mut context, tree, update_patch);
     }
@@ -27,10 +32,45 @@ pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatch
     finalize_patch_application(context, tree)
 }
 
+/// All of the ephemeral state needing during application of a patch.
 #[derive(Default)]
 struct PatchApplyContext {
+    /// A map from transient snapshot IDs (generated by snapshot middleware) to
+    /// instance IDs in the actual tree. These are both the same data type so
+    /// that they fit into the same `RbxValue::Ref` type.
+    ///
+    /// At this point in the patch process, IDs in instance properties have been
+    /// partially translated from 'snapshot space' into 'tree space' by the
+    /// patch computation process. An ID not existing in this map means either:
+    ///
+    /// 1. The ID is already in tree space and refers to an instance that
+    ///    existed in the tree before this patch was applied.
+    ///
+    /// 2. The ID if in snapshot space, but points to an instance that was not
+    ///    part of the snapshot that was put through the patch computation
+    ///    function.
+    ///
+    /// #2 should not occur in well-formed projects, but is indistinguishable
+    /// from #1 right now. It could happen if two model files try to reference
+    /// eachother.
     snapshot_id_to_instance_id: HashMap<RbxId, RbxId>,
-    properties_to_apply: HashMap<RbxId, HashMap<String, RbxValue>>,
+
+    /// The properties of instances added by the current `PatchSet`.
+    ///
+    /// Instances added to the tree can refer to eachother via Ref properties,
+    /// but we need to make sure they're correctly transformed from snapshot
+    /// space into tree space (via `snapshot_id_to_instance_id`).
+    ///
+    /// It's not possible to do that transformation for refs that refer to added
+    /// instances until all the instances have actually been inserted into the
+    /// tree. For simplicity, we defer application of _all_ properties on added
+    /// instances instead of just Refs.
+    ///
+    /// This doesn't affect updated instances, since they're always applied
+    /// after we've added all the instances from the patch.
+    added_instance_properties: HashMap<RbxId, HashMap<String, RbxValue>>,
+
+    /// The current applied patch result, describing changes made to the tree.
     applied_patch_set: AppliedPatchSet,
 }
 
@@ -45,22 +85,24 @@ struct PatchApplyContext {
 /// where we build up a map of snapshot IDs to instance IDs as they're created,
 /// then apply properties all at once at the end.
 fn finalize_patch_application(context: PatchApplyContext, tree: &mut RojoTree) -> AppliedPatchSet {
-    for (id, mut properties) in context.properties_to_apply {
+    for (id, properties) in context.added_instance_properties {
+        // This should always succeed since instances marked as added in our
+        // patch should be added without fail.
         let mut instance = tree
             .get_instance_mut(id)
             .expect("Invalid instance ID in deferred property map");
 
-        for property_value in properties.values_mut() {
+        for (key, mut property_value) in properties {
             if let RbxValue::Ref { value: Some(id) } = property_value {
-                if let Some(&instance_id) = context.snapshot_id_to_instance_id.get(id) {
-                    *property_value = RbxValue::Ref {
+                if let Some(&instance_id) = context.snapshot_id_to_instance_id.get(&id) {
+                    property_value = RbxValue::Ref {
                         value: Some(instance_id),
                     };
                 }
             }
-        }
 
-        *instance.properties_mut() = properties;
+            instance.properties_mut().insert(key, property_value);
+        }
     }
 
     context.applied_patch_set
@@ -71,7 +113,7 @@ fn apply_remove_instance(context: &mut PatchApplyContext, tree: &mut RojoTree, r
         Some(_) => context.applied_patch_set.removed.push(removed_id),
         None => {
             log::warn!(
-                "Patch application error: Tried to remove instance {} but it did not exist.",
+                "Patch misapplication: Tried to remove instance {} but it did not exist.",
                 removed_id
             );
         }
@@ -82,50 +124,64 @@ fn apply_add_child(
     context: &mut PatchApplyContext,
     tree: &mut RojoTree,
     parent_id: RbxId,
-    snapshot: &InstanceSnapshot,
+    snapshot: InstanceSnapshot,
 ) {
     let properties = InstancePropertiesWithMeta {
         properties: RbxInstanceProperties {
-            name: snapshot.name.clone().into_owned(),
-            class_name: snapshot.class_name.clone().into_owned(),
+            name: snapshot.name.into_owned(),
+            class_name: snapshot.class_name.into_owned(),
 
             // Property assignment is deferred until after we know about all
-            // instances in this patch.
+            // instances in this patch. See `PatchApplyContext` for details.
             properties: HashMap::new(),
         },
-        metadata: Default::default(), // TODO
+        metadata: snapshot.metadata,
     };
 
     let id = tree.insert_instance(properties, parent_id);
 
+    context.applied_patch_set.added.push(id);
+
     context
-        .properties_to_apply
-        .insert(id, snapshot.properties.clone());
+        .added_instance_properties
+        .insert(id, snapshot.properties);
 
     if let Some(snapshot_id) = snapshot.snapshot_id {
         context.snapshot_id_to_instance_id.insert(snapshot_id, id);
     }
 
-    for child_snapshot in &snapshot.children {
+    for child_snapshot in snapshot.children {
         apply_add_child(context, tree, id, child_snapshot);
     }
 }
 
 fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patch: PatchUpdate) {
+    let mut applied_patch = AppliedPatchUpdate::new(patch.id);
+
     if let Some(metadata) = patch.changed_metadata {
-        tree.update_metadata(patch.id, metadata);
+        tree.update_metadata(patch.id, metadata.clone());
+        applied_patch.changed_metadata = Some(metadata);
     }
 
-    let mut instance = tree
-        .get_instance_mut(patch.id)
-        .expect("Instance referred to by patch does not exist");
+    let mut instance = match tree.get_instance_mut(patch.id) {
+        Some(instance) => instance,
+        None => {
+            log::warn!(
+                "Patch misapplication: Instance {}, referred to by update patch, did not exist.",
+                patch.id
+            );
+            return;
+        }
+    };
 
     if let Some(name) = patch.changed_name {
-        *instance.name_mut() = name;
+        *instance.name_mut() = name.clone();
+        applied_patch.changed_name = Some(name);
     }
 
     if let Some(class_name) = patch.changed_class_name {
-        *instance.class_name_mut() = class_name;
+        *instance.class_name_mut() = class_name.clone();
+        applied_patch.changed_class_name = Some(class_name);
     }
 
     for (key, property_entry) in patch.changed_properties {
@@ -134,6 +190,10 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc
             // instance IDs if they referred to an instance that was created as
             // part of this patch.
             Some(RbxValue::Ref { value: Some(id) }) => {
+                // If our ID is not found in this map, then it either refers to
+                // an existing instance NOT added by this patch, or there was an
+                // error. See `PatchApplyContext::snapshot_id_to_instance_id`
+                // for more info.
                 let new_id = context
                     .snapshot_id_to_instance_id
                     .get(&id)
@@ -141,20 +201,24 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc
                     .unwrap_or(id);
 
                 instance.properties_mut().insert(
-                    key,
+                    key.clone(),
                     RbxValue::Ref {
                         value: Some(new_id),
                     },
                 );
             }
-            Some(value) => {
-                instance.properties_mut().insert(key, value);
+            Some(ref value) => {
+                instance.properties_mut().insert(key.clone(), value.clone());
             }
             None => {
                 instance.properties_mut().remove(&key);
             }
         }
+
+        applied_patch.changed_properties.insert(key, property_entry);
     }
+
+    context.applied_patch_set.updated.push(applied_patch)
 }
 
 #[cfg(test)]

src/snapshot/tests/apply.rs

@@ -0,0 +1,37 @@
+use rbx_dom_weak::RbxInstanceProperties;
+
+use crate::snapshot::{
+    apply_patch_set, InstancePropertiesWithMeta, PatchSet, PatchUpdate, RojoTree,
+};
+
+#[test]
+fn reify_folder() {
+    let mut tree = empty_tree();
+
+    let patch_set = PatchSet {
+        updated_instances: vec![PatchUpdate {
+            id: tree.get_root_id(),
+            changed_name: Some("Hello, world!".to_owned()),
+            changed_class_name: Some("Folder".to_owned()),
+            changed_properties: Default::default(),
+            changed_metadata: None,
+        }],
+        ..Default::default()
+    };
+
+    let _applied_patch_set = apply_patch_set(&mut tree, patch_set);
+
+    // TODO: Make assertions about tree using snapshots
+    // TODO: make assertions about applied patch set using snapshots
+}
+
+fn empty_tree() -> RojoTree {
+    RojoTree::new(InstancePropertiesWithMeta {
+        properties: RbxInstanceProperties {
+            name: "ROOT".to_owned(),
+            class_name: "ROOT".to_owned(),
+            properties: Default::default(),
+        },
+        metadata: Default::default(),
+    })
+}

src/snapshot/tests/compute.rs

@@ -0,0 +1,35 @@
+use std::borrow::Cow;
+
+use rbx_dom_weak::RbxInstanceProperties;
+
+use crate::snapshot::{compute_patch_set, InstancePropertiesWithMeta, InstanceSnapshot, RojoTree};
+
+#[test]
+fn reify_folder() {
+    let tree = empty_tree();
+
+    let folder = InstanceSnapshot {
+        snapshot_id: None,
+        metadata: Default::default(),
+        name: Cow::Borrowed("Some Folder"),
+        class_name: Cow::Borrowed("Folder"),
+        properties: Default::default(),
+        children: Vec::new(),
+    };
+
+    let _patch_set = compute_patch_set(&folder, &tree, tree.get_root_id());
+
+    // TODO: Make assertions about patch set using snapshots. This needs patches
+    // to be serializable and also to have ID redactions more readily available.
+}
+
+fn empty_tree() -> RojoTree {
+    RojoTree::new(InstancePropertiesWithMeta {
+        properties: RbxInstanceProperties {
+            name: "ROOT".to_owned(),
+            class_name: "ROOT".to_owned(),
+            properties: Default::default(),
+        },
+        metadata: Default::default(),
+    })
+}

src/snapshot/tests/mod.rs

@@ -0,0 +1,2 @@
+mod apply;
+mod compute;