Fix cgroup view sort

Summary:
cgroup view sorting was broken by previous change because when a SingleCgroupModelFieldId is convert into a CgroupModelFieldId, a None path is used, which always queries None as result.
When sorting cgroup view, we use SingleCgroupModelFieldId as sort order to sort nested CgroupModels (sort by query result). If query returns None, then all CgroupModels have the same order and thus no sorting happens.
The main issue is that None path should only be used for listing available field ids (see common_field_ids.rs). Anywhere else should be invalid.
The fix is simply making path non-None everywhere except for using as Sequence. This diff introduces some defensive mechanisms to make it harder to use None path for query.
A private trait DelegatedSequence which has no variant on its own but wraps another Sequence is created. It delegates all methods to its inner Sequence. Conversion from SingleCgroupModelFieldId to CgroupModelFieldId is now explicit (no more From trait). The same is applied to VectorFieldId and BTreeMapFieldId which work similarly.

Reviewed By: brianc118

Differential Revision: D48651323

fbshipit-source-id: 6e31f72d534e60442222f5ae5fd53c9b7ba0bd00

below/dump/src/cgroup.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use model::CgroupModelFieldId;
 use model::SingleCgroupModelFieldId;
 
 use super::*;
@@ -125,12 +126,17 @@ impl Dumper for Cgroup {
             let mut children = Vec::from_iter(&model.children);
             //sort
             if let Some(field_id) = &handle.select {
+                // field_id that queries its own data
+                let field_id = CgroupModelFieldId {
+                    path: Some(vec![]),
+                    subquery_id: field_id.to_owned(),
+                };
                 if handle.opts.sort {
-                    model::sort_queriables(&mut children, &field_id.to_owned().into(), false);
+                    model::sort_queriables(&mut children, &field_id, false);
                 }
 
                 if handle.opts.rsort {
-                    model::sort_queriables(&mut children, &field_id.to_owned().into(), true);
+                    model::sort_queriables(&mut children, &field_id, false);
                 }
 
                 if (handle.opts.sort || handle.opts.rsort) && handle.opts.top != 0 {

below/model/src/cgroup.rs

@@ -75,34 +75,25 @@ pub struct CgroupModelFieldId {
     pub subquery_id: SingleCgroupModelFieldId,
 }
 
-// For sorting CgroupModel with SingleCgroupModelFieldId
-impl From<SingleCgroupModelFieldId> for CgroupModelFieldId {
-    fn from(v: SingleCgroupModelFieldId) -> Self {
+impl FieldId for CgroupModelFieldId {
+    type Queriable = CgroupModel;
+}
+
+impl DelegatedSequence for CgroupModelFieldId {
+    type Delegate = SingleCgroupModelFieldId;
+    fn get_delegate(&self) -> &Self::Delegate {
+        &self.subquery_id
+    }
+    fn from_delegate(delegate: Self::Delegate) -> Self {
         Self {
             path: None,
-            subquery_id: v,
+            subquery_id: delegate,
         }
     }
 }
 
-impl FieldId for CgroupModelFieldId {
-    type Queriable = CgroupModel;
-}
-
 impl Sequence for CgroupModelFieldId {
-    const CARDINALITY: usize = SingleCgroupModelFieldId::CARDINALITY;
-    fn next(&self) -> Option<Self> {
-        self.subquery_id.next().map(Self::from)
-    }
-    fn previous(&self) -> Option<Self> {
-        self.subquery_id.previous().map(Self::from)
-    }
-    fn first() -> Option<Self> {
-        SingleCgroupModelFieldId::first().map(Self::from)
-    }
-    fn last() -> Option<Self> {
-        SingleCgroupModelFieldId::last().map(Self::from)
-    }
+    impl_sequence_for_delegated_sequence!();
 }
 
 impl std::string::ToString for CgroupModelFieldId {

below/model/src/lib.rs

@@ -307,6 +307,35 @@ pub trait Nameable {
     fn name() -> &'static str;
 }
 
+/// Sequence that wraps a delegate sequence and owns no variants.
+trait DelegatedSequence {
+    type Delegate: Sequence;
+    fn get_delegate(&self) -> &Self::Delegate;
+    // Not using From trait as conversion should only be used for Sequence
+    fn from_delegate(delegate: Self::Delegate) -> Self;
+}
+
+/// Implements Sequence for DelegatedSequence. Must be a macro due to orphan
+/// rule. See https://github.com/rust-lang/rfcs/issues/1124
+macro_rules! impl_sequence_for_delegated_sequence {
+    () => {
+        const CARDINALITY: usize = <Self as DelegatedSequence>::Delegate::CARDINALITY;
+        fn next(&self) -> Option<Self> {
+            self.get_delegate().next().map(Self::from_delegate)
+        }
+        fn previous(&self) -> Option<Self> {
+            self.get_delegate().previous().map(Self::from_delegate)
+        }
+        fn first() -> Option<Self> {
+            <Self as DelegatedSequence>::Delegate::first().map(Self::from_delegate)
+        }
+        fn last() -> Option<Self> {
+            <Self as DelegatedSequence>::Delegate::last().map(Self::from_delegate)
+        }
+    };
+}
+pub(crate) use impl_sequence_for_delegated_sequence;
+
 /// Type that makes Vec Queriable if Vec's inner type is Queriable. Uses `idx`
 /// to query into a Vec. Uses `subquery_id` to query into the selected item.
 #[derive(Clone, Debug, PartialEq)]
@@ -316,38 +345,30 @@ pub struct VecFieldId<F: FieldId> {
     pub subquery_id: F,
 }
 
-impl<F: FieldId> From<F> for VecFieldId<F> {
-    fn from(subquery_id: F) -> Self {
-        Self {
-            idx: None,
-            subquery_id,
-        }
-    }
-}
-
 impl<F: FieldId> FieldId for VecFieldId<F>
 where
     <F as FieldId>::Queriable: Sized,
 {
     type Queriable = Vec<F::Queriable>;
 }
 
-impl<F: FieldId + Sequence> Sequence for VecFieldId<F> {
-    const CARDINALITY: usize = <F as Sequence>::CARDINALITY;
-    fn next(&self) -> Option<Self> {
-        self.subquery_id.next().map(Self::from)
-    }
-    fn previous(&self) -> Option<Self> {
-        self.subquery_id.previous().map(Self::from)
+impl<F: FieldId + Sequence> DelegatedSequence for VecFieldId<F> {
+    type Delegate = F;
+    fn get_delegate(&self) -> &Self::Delegate {
+        &self.subquery_id
     }
-    fn first() -> Option<Self> {
-        <F as Sequence>::first().map(Self::from)
-    }
-    fn last() -> Option<Self> {
-        <F as Sequence>::last().map(Self::from)
+    fn from_delegate(delegate: Self::Delegate) -> Self {
+        Self {
+            idx: None,
+            subquery_id: delegate,
+        }
     }
 }
 
+impl<F: FieldId + Sequence> Sequence for VecFieldId<F> {
+    impl_sequence_for_delegated_sequence!();
+}
+
 impl<F: FieldId + ToString> ToString for VecFieldId<F> {
     fn to_string(&self) -> String {
         match self.idx {
@@ -394,38 +415,30 @@ pub struct BTreeMapFieldId<K, F: FieldId> {
     pub subquery_id: F,
 }
 
-impl<K, F: FieldId> From<F> for BTreeMapFieldId<K, F> {
-    fn from(subquery_id: F) -> Self {
-        Self {
-            key: None,
-            subquery_id,
-        }
-    }
-}
-
 impl<K: Ord, F: FieldId> FieldId for BTreeMapFieldId<K, F>
 where
     <F as FieldId>::Queriable: Sized,
 {
     type Queriable = BTreeMap<K, F::Queriable>;
 }
 
-impl<K, F: FieldId + Sequence> Sequence for BTreeMapFieldId<K, F> {
-    const CARDINALITY: usize = <F as Sequence>::CARDINALITY;
-    fn next(&self) -> Option<Self> {
-        self.subquery_id.next().map(Self::from)
-    }
-    fn previous(&self) -> Option<Self> {
-        self.subquery_id.previous().map(Self::from)
-    }
-    fn first() -> Option<Self> {
-        <F as Sequence>::first().map(Self::from)
+impl<K, F: FieldId + Sequence> DelegatedSequence for BTreeMapFieldId<K, F> {
+    type Delegate = F;
+    fn get_delegate(&self) -> &Self::Delegate {
+        &self.subquery_id
     }
-    fn last() -> Option<Self> {
-        <F as Sequence>::last().map(Self::from)
+    fn from_delegate(delegate: Self::Delegate) -> Self {
+        Self {
+            key: None,
+            subquery_id: delegate,
+        }
     }
 }
 
+impl<K, F: FieldId + Sequence> Sequence for BTreeMapFieldId<K, F> {
+    impl_sequence_for_delegated_sequence!();
+}
+
 impl<K: ToString, F: FieldId + ToString> ToString for BTreeMapFieldId<K, F> {
     fn to_string(&self) -> String {
         match &self.key {

below/view/src/cgroup_tabs.rs

@@ -17,6 +17,7 @@ use std::collections::HashSet;
 use cursive::utils::markup::StyledString;
 use model::sort_queriables;
 use model::CgroupModel;
+use model::CgroupModelFieldId;
 use model::Queriable;
 use model::SingleCgroupModel;
 use model::SingleCgroupModelFieldId;
@@ -117,7 +118,12 @@ impl CgroupTab {
 
             let mut children = Vec::from_iter(&cgroup.children);
             if let Some(sort_order) = state.sort_order.as_ref() {
-                sort_queriables(&mut children, &sort_order.to_owned().into(), state.reverse);
+                // field_id that query its own data
+                let field_id = CgroupModelFieldId {
+                    path: Some(vec![]),
+                    subquery_id: sort_order.clone(),
+                };
+                sort_queriables(&mut children, &field_id, state.reverse);
             }
 
             // Stop at next level (one below <root>)