add cargo clippy to CI

.taskcluster.yml

@@ -37,13 +37,38 @@ tasks:
               cd repo &&
               git config advice.detachedHead false &&
               git checkout ${ref} &&
-              cargo test &&
+              cargo test
-              cargo fmt -- --check
         metadata:
           name: taskchampion-tests
           description: Run tests for taskchampion
           owner: dustin@v.igoro.us
           source: ${repo_url}
+    - $if: run
+      then:
+        provisionerId: 'proj-misc'
+        workerType: 'ci'
+        deadline: {$fromNow: '1 hour'}
+        expires: {$fromNow: '1 day'}
+        payload:
+          maxRunTime: 3600
+          image: rust:latest
+          command:
+            - /bin/bash
+            - '-c'
+            - >-
+              rustup component add rustfmt &&
+              git clone ${repo_url} repo &&
+              cd repo &&
+              git config advice.detachedHead false &&
+              git checkout ${ref} &&
+              rustup component add clippy-preview &&
+              cargo clippy &&
+              cargo fmt -- --check
+        metadata:
+          name: taskchampion-clippy
+          description: Run clippy and rustfmt for taskchampion
+          owner: dustin@v.igoro.us
+          source: ${repo_url}
     - $if: run
       then:
         provisionerId: 'proj-misc'

cli/src/cmd/pending.rs

@@ -29,8 +29,8 @@ subcommand_invocation! {
         let mut t = Table::new();
         t.set_format(table::format());
         t.set_titles(row![b->"id", b->"description"]);
-        for i in 1..working_set.len() {
+        for (i, item) in working_set.iter().enumerate() {
-            if let Some(ref task) = working_set[i] {
+            if let Some(ref task) = item {
                 t.add_row(row![i, task.get_description()]);
             }
         }

cli/src/cmd/shared.rs

@@ -12,22 +12,16 @@ pub(super) fn get_task<S: AsRef<str>>(replica: &mut Replica, task_arg: S) -> Fal
     let task_arg = task_arg.as_ref();
 
     // first try treating task as a working-set reference
-    match task_arg.parse::<usize>() {
+    if let Ok(i) = task_arg.parse::<usize>() {
-        Ok(i) => {
+        if let Some(task) = replica.get_working_set_task(i)? {
-            if let Some(task) = replica.get_working_set_task(i)? {
+            return Ok(task);
-                return Ok(task);
-            }
         }
-        Err(_) => {}
     }
 
-    match Uuid::parse_str(task_arg) {
+    if let Ok(uuid) = Uuid::parse_str(task_arg) {
-        Ok(uuid) => {
+        if let Some(task) = replica.get_task(&uuid)? {
-            if let Some(task) = replica.get_task(&uuid)? {
+            return Ok(task);
-                return Ok(task);
-            }
         }
-        Err(_) => {}
     }
 
     Err(format_err!("Cannot interpret {:?} as a task", task_arg))

sync-server/src/lib.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::new_without_default)]
+
 use std::collections::HashMap;
 
 type Blob = Vec<u8>;
@@ -32,10 +34,7 @@ impl User {
         if last_version == since_version as usize {
             return vec![];
         }
-        self.versions[since_version as usize..last_version]
+        self.versions[since_version as usize..last_version].to_vec()
-            .iter()
-            .map(|r| r.clone())
-            .collect::<Vec<Blob>>()
     }
 
     fn add_version(&mut self, version: u64, blob: Blob) -> VersionAdd {
@@ -72,7 +71,7 @@ impl Server {
         self.users
             .get(username)
             .map(|user| user.get_versions(since_version))
-            .unwrap_or_else(|| vec![])
+            .unwrap_or_default()
     }
 
     /// Add a new version.  If the given version number is incorrect, this responds with the

taskchampion/src/lib.rs

@@ -22,6 +22,7 @@ See the [TaskChampion Book](https://github.com/djmitche/taskchampion/blob/main/d
 for more information about the design and usage of the tool.
 
  */
+
 mod errors;
 mod replica;
 pub mod server;

taskchampion/src/replica.rs

@@ -16,9 +16,9 @@ pub struct Replica {
 
 impl Replica {
     pub fn new(storage: Box<dyn TaskStorage>) -> Replica {
-        return Replica {
+        Replica {
             taskdb: TaskDB::new(storage),
-        };
+        }
     }
 
     #[cfg(test)]
@@ -56,7 +56,7 @@ impl Replica {
     pub fn all_tasks(&mut self) -> Fallible<HashMap<Uuid, Task>> {
         let mut res = HashMap::new();
         for (uuid, tm) in self.taskdb.all_tasks()?.drain(..) {
-            res.insert(uuid.clone(), Task::new(uuid.clone(), tm));
+            res.insert(uuid, Task::new(uuid, tm));
         }
         Ok(res)
     }
@@ -71,10 +71,10 @@ impl Replica {
     pub fn working_set(&mut self) -> Fallible<Vec<Option<Task>>> {
         let working_set = self.taskdb.working_set()?;
         let mut res = Vec::with_capacity(working_set.len());
-        for i in 0..working_set.len() {
+        for item in working_set.iter() {
-            res.push(match working_set[i] {
+            res.push(match item {
                 Some(u) => match self.taskdb.get_task(&u)? {
-                    Some(tm) => Some(Task::new(u, tm)),
+                    Some(tm) => Some(Task::new(*u, tm)),
                     None => None,
                 },
                 None => None,
@@ -88,7 +88,7 @@ impl Replica {
         Ok(self
             .taskdb
             .get_task(uuid)?
-            .map(move |tm| Task::new(uuid.clone(), tm)))
+            .map(move |tm| Task::new(*uuid, tm)))
     }
 
     /// Get an existing task by its working set index
@@ -102,7 +102,7 @@ impl Replica {
                     .map(move |tm| Task::new(uuid, tm)));
             }
         }
-        return Ok(None);
+        Ok(None)
     }
 
     /// Get the working set index for the given task uuid
@@ -126,8 +126,7 @@ impl Replica {
         if self.taskdb.get_task(&uuid)?.is_some() {
             return Err(Error::DBError(format!("Task {} already exists", uuid)).into());
         }
-        self.taskdb
+        self.taskdb.apply(Operation::Create { uuid })?;
-            .apply(Operation::Create { uuid: uuid.clone() })?;
         let mut task = Task::new(uuid, TaskMap::new()).into_mut(self);
         task.set_description(description)?;
         task.set_status(status)?;
@@ -172,7 +171,7 @@ mod tests {
         let uuid = Uuid::new_v4();
 
         let t = rep
-            .new_task(uuid.clone(), Status::Pending, "a task".into())
+            .new_task(uuid, Status::Pending, "a task".into())
             .unwrap();
         assert_eq!(t.get_description(), String::from("a task"));
         assert_eq!(t.get_status(), Status::Pending);
@@ -185,7 +184,7 @@ mod tests {
         let uuid = Uuid::new_v4();
 
         let t = rep
-            .new_task(uuid.clone(), Status::Pending, "a task".into())
+            .new_task(uuid, Status::Pending, "a task".into())
             .unwrap();
 
         let mut t = t.into_mut(&mut rep);
@@ -211,10 +210,10 @@ mod tests {
         let mut rep = Replica::new_inmemory();
         let uuid = Uuid::new_v4();
 
-        rep.new_task(uuid.clone(), Status::Pending, "a task".into())
+        rep.new_task(uuid, Status::Pending, "a task".into())
             .unwrap();
 
-        rep.delete_task(uuid.clone()).unwrap();
+        rep.delete_task(uuid).unwrap();
         assert_eq!(rep.get_task(&uuid).unwrap(), None);
     }
 
@@ -223,7 +222,7 @@ mod tests {
         let mut rep = Replica::new_inmemory();
         let uuid = Uuid::new_v4();
 
-        rep.new_task(uuid.clone(), Status::Pending, "another task".into())
+        rep.new_task(uuid, Status::Pending, "another task".into())
             .unwrap();
 
         let t = rep.get_task(&uuid).unwrap().unwrap();
@@ -247,7 +246,7 @@ mod tests {
         let mut rep = Replica::new_inmemory();
         let uuid = Uuid::new_v4();
 
-        rep.new_task(uuid.clone(), Status::Pending, "to-be-pending".into())
+        rep.new_task(uuid, Status::Pending, "to-be-pending".into())
             .unwrap();
 
         let t = rep.get_working_set_task(1).unwrap().unwrap();

taskchampion/src/task.rs

@@ -123,7 +123,7 @@ impl Task {
     pub fn into_mut(self, replica: &mut Replica) -> TaskMut {
         TaskMut {
             task: self,
-            replica: replica,
+            replica,
             updated_modified: false,
         }
     }
@@ -170,7 +170,7 @@ impl<'r> TaskMut<'r> {
     /// new status puts it in that set.
     pub fn set_status(&mut self, status: Status) -> Fallible<()> {
         if status == Status::Pending {
-            let uuid = self.uuid.clone();
+            let uuid = self.uuid;
             self.replica.add_to_working_set(&uuid)?;
         }
         self.set_string("status", Some(String::from(status.to_taskmap())))
@@ -190,7 +190,7 @@ impl<'r> TaskMut<'r> {
         if !self.updated_modified {
             let now = format!("{}", Utc::now().timestamp());
             self.replica
-                .update_task(self.task.uuid.clone(), "modified", Some(now.clone()))?;
+                .update_task(self.task.uuid, "modified", Some(now.clone()))?;
             self.task.taskmap.insert(String::from("modified"), now);
             self.updated_modified = true;
         }
@@ -200,7 +200,7 @@ impl<'r> TaskMut<'r> {
     fn set_string(&mut self, property: &str, value: Option<String>) -> Fallible<()> {
         self.lastmod()?;
         self.replica
-            .update_task(self.task.uuid.clone(), property, value.as_ref())?;
+            .update_task(self.task.uuid, property, value.as_ref())?;
 
         if let Some(v) = value {
             self.task.taskmap.insert(property.to_string(), v);
@@ -213,7 +213,7 @@ impl<'r> TaskMut<'r> {
     fn set_timestamp(&mut self, property: &str, value: Option<DateTime<Utc>>) -> Fallible<()> {
         self.lastmod()?;
         self.replica.update_task(
-            self.task.uuid.clone(),
+            self.task.uuid,
             property,
             value.map(|v| format!("{}", v.timestamp())),
         )

taskchampion/src/taskdb.rs

@@ -44,32 +44,30 @@ impl TaskDB {
 
     fn apply_op(txn: &mut dyn TaskStorageTxn, op: &Operation) -> Fallible<()> {
         match op {
-            &Operation::Create { uuid } => {
+            Operation::Create { uuid } => {
                 // insert if the task does not already exist
-                if !txn.create_task(uuid)? {
+                if !txn.create_task(*uuid)? {
                     return Err(Error::DBError(format!("Task {} already exists", uuid)).into());
                 }
             }
-            &Operation::Delete { ref uuid } => {
+            Operation::Delete { ref uuid } => {
                 if !txn.delete_task(uuid)? {
                     return Err(Error::DBError(format!("Task {} does not exist", uuid)).into());
                 }
             }
-            &Operation::Update {
+            Operation::Update {
                 ref uuid,
                 ref property,
                 ref value,
                 timestamp: _,
             } => {
                 // update if this task exists, otherwise ignore
-                if let Some(task) = txn.get_task(uuid)? {
+                if let Some(mut task) = txn.get_task(uuid)? {
-                    let mut task = task.clone();
-                    // TODO: update working_set if this is changing state to or from pending
                     match value {
                         Some(ref val) => task.insert(property.to_string(), val.clone()),
                         None => task.remove(property),
                     };
-                    txn.set_task(uuid.clone(), task)?;
+                    txn.set_task(*uuid, task)?;
                 } else {
                     return Err(Error::DBError(format!("Task {} does not exist", uuid)).into());
                 }
@@ -80,19 +78,19 @@ impl TaskDB {
     }
 
     /// Get all tasks.
-    pub fn all_tasks<'a>(&'a mut self) -> Fallible<Vec<(Uuid, TaskMap)>> {
+    pub fn all_tasks(&mut self) -> Fallible<Vec<(Uuid, TaskMap)>> {
         let mut txn = self.storage.txn()?;
         txn.all_tasks()
     }
 
     /// Get the UUIDs of all tasks
-    pub fn all_task_uuids<'a>(&'a mut self) -> Fallible<Vec<Uuid>> {
+    pub fn all_task_uuids(&mut self) -> Fallible<Vec<Uuid>> {
         let mut txn = self.storage.txn()?;
         txn.all_task_uuids()
     }
 
     /// Get the working set
-    pub fn working_set<'a>(&'a mut self) -> Fallible<Vec<Option<Uuid>>> {
+    pub fn working_set(&mut self) -> Fallible<Vec<Option<Uuid>>> {
         let mut txn = self.storage.txn()?;
         txn.get_working_set()
     }
@@ -124,7 +122,7 @@ impl TaskDB {
             if let Some(uuid) = elt {
                 if let Some(task) = txn.get_task(&uuid)? {
                     if in_working_set(&task) {
-                        new_ws.push(uuid.clone());
+                        new_ws.push(uuid);
                         seen.insert(uuid);
                     }
                 }
@@ -134,10 +132,8 @@ impl TaskDB {
         // Now go hunting for tasks that should be in this list but are not, adding them at the
         // end of the list.
         for (uuid, task) in txn.all_tasks()? {
-            if !seen.contains(&uuid) {
+            if !seen.contains(&uuid) && in_working_set(&task) {
-                if in_working_set(&task) {
+                new_ws.push(uuid);
-                    new_ws.push(uuid.clone());
-                }
             }
         }
 
@@ -186,8 +182,8 @@ impl TaskDB {
                 TaskDB::apply_version(txn.as_mut(), version)?;
             }
 
-            let operations: Vec<Operation> = txn.operations()?.iter().map(|o| o.clone()).collect();
+            let operations: Vec<Operation> = txn.operations()?.to_vec();
-            if operations.len() == 0 {
+            if operations.is_empty() {
                 // nothing to sync back to the server..
                 break;
             }
@@ -195,7 +191,7 @@ impl TaskDB {
             // now make a version of our local changes and push those
             let new_version = Version {
                 version: txn.base_version()? + 1,
-                operations: operations,
+                operations,
             };
             let new_version_str = serde_json::to_string(&new_version).unwrap();
             println!("sending version {:?} to server", new_version.version);
@@ -457,7 +453,7 @@ mod tests {
 
         // add everything to the TaskDB
         for uuid in &uuids {
-            db.apply(Operation::Create { uuid: uuid.clone() })?;
+            db.apply(Operation::Create { uuid: *uuid })?;
         }
         for i in &[0usize, 1, 4] {
             db.apply(Operation::Update {

taskchampion/src/taskstorage/inmemory.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::new_without_default)]
+
 use crate::taskstorage::{Operation, TaskMap, TaskStorage, TaskStorageTxn};
 use failure::{format_err, Fallible};
 use std::collections::hash_map::Entry;
@@ -48,7 +50,7 @@ impl<'t> TaskStorageTxn for Txn<'t> {
 
     fn create_task(&mut self, uuid: Uuid) -> Fallible<bool> {
         if let ent @ Entry::Vacant(_) = self.mut_data_ref().tasks.entry(uuid) {
-            ent.or_insert(TaskMap::new());
+            ent.or_insert_with(TaskMap::new);
             Ok(true)
         } else {
             Ok(false)
@@ -61,11 +63,7 @@ impl<'t> TaskStorageTxn for Txn<'t> {
     }
 
     fn delete_task(&mut self, uuid: &Uuid) -> Fallible<bool> {
-        if let Some(_) = self.mut_data_ref().tasks.remove(uuid) {
+        Ok(self.mut_data_ref().tasks.remove(uuid).is_some())
-            Ok(true)
-        } else {
-            Ok(false)
-        }
     }
 
     fn all_tasks<'a>(&mut self) -> Fallible<Vec<(Uuid, TaskMap)>> {
@@ -73,12 +71,12 @@ impl<'t> TaskStorageTxn for Txn<'t> {
             .data_ref()
             .tasks
             .iter()
-            .map(|(u, t)| (u.clone(), t.clone()))
+            .map(|(u, t)| (*u, t.clone()))
             .collect())
     }
 
     fn all_task_uuids<'a>(&mut self) -> Fallible<Vec<Uuid>> {
-        Ok(self.data_ref().tasks.keys().map(|u| u.clone()).collect())
+        Ok(self.data_ref().tasks.keys().copied().collect())
     }
 
     fn base_version(&mut self) -> Fallible<u64> {
@@ -110,7 +108,7 @@ impl<'t> TaskStorageTxn for Txn<'t> {
 
     fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible<usize> {
         let working_set = &mut self.mut_data_ref().working_set;
-        working_set.push(Some(uuid.clone()));
+        working_set.push(Some(*uuid));
         Ok(working_set.len())
     }
 

taskchampion/src/taskstorage/kv.rs

@@ -13,21 +13,20 @@ struct Key(uuid::Bytes);
 
 impl From<&[u8]> for Key {
     fn from(bytes: &[u8]) -> Key {
-        let key = Key(bytes.try_into().unwrap());
+        Key(bytes.try_into().unwrap())
-        key
     }
 }
 
 impl From<&Uuid> for Key {
     fn from(uuid: &Uuid) -> Key {
-        let key = Key(uuid.as_bytes().clone());
+        let key = Key(*uuid.as_bytes());
         key
     }
 }
 
 impl From<Uuid> for Key {
     fn from(uuid: Uuid) -> Key {
-        let key = Key(uuid.as_bytes().clone());
+        let key = Key(*uuid.as_bytes());
         key
     }
 }
@@ -108,7 +107,7 @@ struct Txn<'t> {
 
 impl<'t> Txn<'t> {
     // get the underlying kv Txn
-    fn kvtxn<'a>(&mut self) -> &mut kv::Txn<'t> {
+    fn kvtxn(&mut self) -> &mut kv::Txn<'t> {
         if let Some(ref mut txn) = self.txn {
             txn
         } else {
@@ -316,7 +315,7 @@ impl<'t> TaskStorageTxn for Txn<'t> {
         kvtxn.set(
             working_set_bucket,
             next_index.into(),
-            Msgpack::to_value_buf(uuid.clone())?,
+            Msgpack::to_value_buf(*uuid)?,
         )?;
         kvtxn.set(
             numbers_bucket,
@@ -387,7 +386,7 @@ mod test {
         let uuid = Uuid::new_v4();
         {
             let mut txn = storage.txn()?;
-            assert!(txn.create_task(uuid.clone())?);
+            assert!(txn.create_task(uuid)?);
             txn.commit()?;
         }
         {
@@ -405,12 +404,12 @@ mod test {
         let uuid = Uuid::new_v4();
         {
             let mut txn = storage.txn()?;
-            assert!(txn.create_task(uuid.clone())?);
+            assert!(txn.create_task(uuid)?);
             txn.commit()?;
         }
         {
             let mut txn = storage.txn()?;
-            assert!(!txn.create_task(uuid.clone())?);
+            assert!(!txn.create_task(uuid)?);
             txn.commit()?;
         }
         Ok(())
@@ -436,10 +435,7 @@ mod test {
         let uuid = Uuid::new_v4();
         {
             let mut txn = storage.txn()?;
-            txn.set_task(
+            txn.set_task(uuid, taskmap_with(vec![("k".to_string(), "v".to_string())]))?;
-                uuid.clone(),
-                taskmap_with(vec![("k".to_string(), "v".to_string())]),
-            )?;
             txn.commit()?;
         }
         {
@@ -472,7 +468,7 @@ mod test {
         let uuid = Uuid::new_v4();
         {
             let mut txn = storage.txn()?;
-            assert!(txn.create_task(uuid.clone())?);
+            assert!(txn.create_task(uuid)?);
             txn.commit()?;
         }
         {

taskchampion/src/taskstorage/mod.rs

@@ -52,10 +52,10 @@ pub trait TaskStorageTxn {
     fn delete_task(&mut self, uuid: &Uuid) -> Fallible<bool>;
 
     /// Get the uuids and bodies of all tasks in the storage, in undefined order.
-    fn all_tasks<'a>(&mut self) -> Fallible<Vec<(Uuid, TaskMap)>>;
+    fn all_tasks(&mut self) -> Fallible<Vec<(Uuid, TaskMap)>>;
 
     /// Get the uuids of all tasks in the storage, in undefined order.
-    fn all_task_uuids<'a>(&mut self) -> Fallible<Vec<Uuid>>;
+    fn all_task_uuids(&mut self) -> Fallible<Vec<Uuid>>;
 
     /// Get the current base_version for this storage -- the last version synced from the server.
     fn base_version(&mut self) -> Fallible<u64>;
@@ -65,7 +65,7 @@ pub trait TaskStorageTxn {
 
     /// Get the current set of outstanding operations (operations that have not been sync'd to the
     /// server yet)
-    fn operations<'a>(&mut self) -> Fallible<Vec<Operation>>;
+    fn operations(&mut self) -> Fallible<Vec<Operation>>;
 
     /// Add an operation to the end of the list of operations in the storage.  Note that this
     /// merely *stores* the operation; it is up to the TaskDB to apply it.

taskchampion/src/taskstorage/operation.rs

@@ -109,12 +109,9 @@ impl Operation {
                 } else if timestamp1 < timestamp2 {
                     // prefer the later modification
                     (None, Some(operation2))
-                } else if timestamp1 > timestamp2 {
-                    // prefer the later modification
-                    //(Some(operation1), None)
-                    (Some(operation1), None)
                 } else {
-                    // arbitrarily resolve in favor of the first operation
+                    // prefer the later modification or, if the modifications are the same,
+                    // just choose one of them
                     (Some(operation1), None)
                 }
             }