From e563ae8fc125bd0335eab7c2e0ea4e66ff0b5dfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fe=CC=81lix=20Saparelli?= Date: Mon, 28 Oct 2019 01:10:54 +1300 Subject: [PATCH] Enable anti-unwrap lints --- src/cli.rs | 19 ++++++++++++++++-- src/gitignore.rs | 40 +++++++++++++++++++------------------- src/ignore.rs | 38 ++++++++++++++++++------------------ src/lib.rs | 4 ++-- src/notification_filter.rs | 15 ++++++++------ src/process.rs | 20 +++++++++---------- src/run.rs | 4 ++-- src/signal.rs | 4 ++-- 8 files changed, 81 insertions(+), 63 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 0520ccaa..fbea6983 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -11,7 +11,7 @@ //! .cmd(vec!["echo hello world".into()]) //! .paths(vec![".".into()]) //! .build() -//! .unwrap(); +//! .expect("mission failed"); //! ``` use crate::error; @@ -25,8 +25,9 @@ use std::{ /// Arguments to the watcher #[derive(Builder, Clone, Debug)] #[builder(setter(into, strip_option))] +#[builder(build_fn(validate = "Self::validate"))] pub struct Args { - /// List of commands to be executed on change. Will be joined with `&&`. + /// Command to execute in popen3 format (first program, rest arguments). pub cmd: Vec, /// List of paths to watch for changes. pub paths: Vec, @@ -75,6 +76,20 @@ pub struct Args { pub poll_interval: u32, } +impl ArgsBuilder { + fn validate(&self) -> Result<(), String> { + if self.cmd.as_ref().map_or(true, Vec::is_empty) { + return Err("cmd must not be empty".into()); + } + + if self.paths.as_ref().map_or(true, Vec::is_empty) { + return Err("paths must not be empty".into()); + } + + Ok(()) + } +} + #[cfg(target_family = "windows")] pub fn clear_screen() { let _ = Command::new("cmd") diff --git a/src/gitignore.rs b/src/gitignore.rs index e1327981..88b4e855 100644 --- a/src/gitignore.rs +++ b/src/gitignore.rs @@ -117,7 +117,7 @@ impl GitignoreFile { file.read_to_string(&mut contents)?; let lines: Vec<_> = contents.lines().collect(); - let root = path.parent().unwrap(); + let root = path.parent().expect("gitignore file is at filesystem root"); Self::from_strings(&lines, root) } @@ -155,23 +155,19 @@ impl GitignoreFile { self.matches(path) == MatchResult::Ignore } - fn matches(&self, path: &Path) -> MatchResult { - let stripped = path.strip_prefix(&self.root); - if stripped.is_err() { - return MatchResult::None; + fn matches(&self, path: &Path) -> MatchResult { + if let Ok(stripped) = path.strip_prefix(&self.root) { + let matches = self.set.matches(stripped); + if let Some(i) = matches.iter().rev().next() { + let pattern = &self.patterns[*i]; + return match pattern.pattern_type { + PatternType::Whitelist => MatchResult::Whitelist, + PatternType::Ignore => MatchResult::Ignore, + }; + } } - let matches = self.set.matches(stripped.unwrap()); - - if let Some(i) = matches.iter().rev().next() { - let pattern = &self.patterns[*i]; - return match pattern.pattern_type { - PatternType::Whitelist => MatchResult::Whitelist, - PatternType::Ignore => MatchResult::Ignore, - }; - } - - MatchResult::None + MatchResult::None } pub fn root_len(&self) -> usize { @@ -248,7 +244,8 @@ mod tests { } fn build_gitignore(pattern: &str) -> GitignoreFile { - GitignoreFile::from_strings(&[pattern], &base_dir()).unwrap() + GitignoreFile::from_strings(&[pattern], &base_dir()) + .expect("test gitignore file invalid") } #[test] @@ -332,7 +329,8 @@ mod tests { #[test] fn test_empty_file_never_excludes() { - let file = GitignoreFile::from_strings(&[], &base_dir()).unwrap(); + let file = GitignoreFile::from_strings(&[], &base_dir()) + .expect("test gitignore file invalid"); assert!(!file.is_excluded(&base_dir().join("target"))); } @@ -340,7 +338,8 @@ mod tests { #[test] fn test_checks_all_patterns() { let patterns = vec!["target", "target2"]; - let file = GitignoreFile::from_strings(&patterns, &base_dir()).unwrap(); + let file = GitignoreFile::from_strings(&patterns, &base_dir()) + .expect("test gitignore file invalid"); assert!(file.is_excluded(&base_dir().join("target").join("foo.txt"))); assert!(file.is_excluded(&base_dir().join("target2").join("bar.txt"))); @@ -349,7 +348,8 @@ mod tests { #[test] fn test_handles_whitelisting() { let patterns = vec!["target", "!target/foo.txt"]; - let file = GitignoreFile::from_strings(&patterns, &base_dir()).unwrap(); + let file = GitignoreFile::from_strings(&patterns, &base_dir()) + .expect("test gitignore file invalid"); assert!(!file.is_excluded(&base_dir().join("target").join("foo.txt"))); assert!(file.is_excluded(&base_dir().join("target").join("blah.txt"))); diff --git a/src/ignore.rs b/src/ignore.rs index 438182de..12f08124 100644 --- a/src/ignore.rs +++ b/src/ignore.rs @@ -110,7 +110,7 @@ impl IgnoreFile { file.read_to_string(&mut contents)?; let lines: Vec<_> = contents.lines().collect(); - let root = path.parent().unwrap(); + let root = path.parent().expect("ignore file is at filesystem root"); Self::from_strings(&lines, root) } @@ -149,22 +149,18 @@ impl IgnoreFile { } fn matches(&self, path: &Path) -> MatchResult { - let stripped = path.strip_prefix(&self.root); - if stripped.is_err() { - return MatchResult::None; + if let Ok(stripped) = path.strip_prefix(&self.root) { + let matches = self.set.matches(stripped); + if let Some(i) = matches.iter().rev().next() { + let pattern = &self.patterns[*i]; + return match pattern.pattern_type { + PatternType::Whitelist => MatchResult::Whitelist, + PatternType::Ignore => MatchResult::Ignore, + }; + } } - let matches = self.set.matches(stripped.unwrap()); - - if let Some(i) = matches.iter().rev().next() { - let pattern = &self.patterns[*i]; - return match pattern.pattern_type { - PatternType::Whitelist => MatchResult::Whitelist, - PatternType::Ignore => MatchResult::Ignore, - }; - } - - MatchResult::None + MatchResult::None } pub fn root_len(&self) -> usize { @@ -241,7 +237,8 @@ mod tests { } fn build_ignore(pattern: &str) -> IgnoreFile { - IgnoreFile::from_strings(&[pattern], &base_dir()).unwrap() + IgnoreFile::from_strings(&[pattern], &base_dir()) + .expect("test ignore file invalid") } #[test] @@ -325,7 +322,8 @@ mod tests { #[test] fn test_empty_file_never_excludes() { - let file = IgnoreFile::from_strings(&[], &base_dir()).unwrap(); + let file = IgnoreFile::from_strings(&[], &base_dir()) + .expect("test ignore file invalid"); assert!(!file.is_excluded(&base_dir().join("target"))); } @@ -333,7 +331,8 @@ mod tests { #[test] fn test_checks_all_patterns() { let patterns = vec!["target", "target2"]; - let file = IgnoreFile::from_strings(&patterns, &base_dir()).unwrap(); + let file = IgnoreFile::from_strings(&patterns, &base_dir()) + .expect("test ignore file invalid"); assert!(file.is_excluded(&base_dir().join("target").join("foo.txt"))); assert!(file.is_excluded(&base_dir().join("target2").join("bar.txt"))); @@ -342,7 +341,8 @@ mod tests { #[test] fn test_handles_whitelisting() { let patterns = vec!["target", "!target/foo.txt"]; - let file = IgnoreFile::from_strings(&patterns, &base_dir()).unwrap(); + let file = IgnoreFile::from_strings(&patterns, &base_dir()) + .expect("test ignore file invalid"); assert!(!file.is_excluded(&base_dir().join("target").join("foo.txt"))); assert!(file.is_excluded(&base_dir().join("target").join("blah.txt"))); diff --git a/src/lib.rs b/src/lib.rs index 5d843731..d10e29eb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,8 +15,8 @@ deprecated, intra_doc_link_resolution_failure, // missing_docs, -// clippy::option_unwrap_used, -// clippy::result_unwrap_used, + clippy::option_unwrap_used, + clippy::result_unwrap_used, )] #![deny(unsafe_code, clippy::missing_const_for_fn)] #![allow(clippy::default_trait_access, clippy::cognitive_complexity)] diff --git a/src/notification_filter.rs b/src/notification_filter.rs index ab8aaf36..64b48680 100644 --- a/src/notification_filter.rs +++ b/src/notification_filter.rs @@ -31,7 +31,7 @@ impl NotificationFilter { if ignore_path.is_relative() && !i.starts_with('*') { ignore_path = Path::new("**").join(&ignore_path); } - let pattern = ignore_path.to_str().unwrap(); + let pattern = ignore_path.to_str().expect("corrupted memory (string -> path -> string)"); ignore_set_builder.add(Glob::new(pattern)?); debug!("Adding ignore: \"{}\"", pattern); } @@ -83,7 +83,8 @@ mod tests { #[test] fn test_allows_everything_by_default() { let filter = - NotificationFilter::new(&[], &[], gitignore::load(&[]), ignore::load(&[])).unwrap(); + NotificationFilter::new(&[], &[], gitignore::load(&[]), ignore::load(&[])) + .expect("test filter errors"); assert!(!filter.is_excluded(Path::new("foo"))); } @@ -96,7 +97,7 @@ mod tests { gitignore::load(&[]), ignore::load(&[]), ) - .unwrap(); + .expect("test filter errors"); assert!(filter.is_excluded(Path::new("/path/to/test.json"))); assert!(filter.is_excluded(Path::new("test.json"))); @@ -106,7 +107,8 @@ mod tests { fn test_multiple_filters() { let filters = &["*.rs".into(), "*.toml".into()]; let filter = - NotificationFilter::new(filters, &[], gitignore::load(&[]), ignore::load(&[])).unwrap(); + NotificationFilter::new(filters, &[], gitignore::load(&[]), ignore::load(&[])) + .expect("test filter errors"); assert!(!filter.is_excluded(Path::new("hello.rs"))); assert!(!filter.is_excluded(Path::new("Cargo.toml"))); @@ -117,7 +119,8 @@ mod tests { fn test_multiple_ignores() { let ignores = &["*.rs".into(), "*.toml".into()]; let filter = - NotificationFilter::new(&[], ignores, gitignore::load(&[]), ignore::load(&[])).unwrap(); + NotificationFilter::new(&[], ignores, gitignore::load(&[]), ignore::load(&[])) + .expect("test filter errors"); assert!(filter.is_excluded(Path::new("hello.rs"))); assert!(filter.is_excluded(Path::new("Cargo.toml"))); @@ -129,7 +132,7 @@ mod tests { let ignores = &["*.rs".into(), "*.toml".into()]; let filter = NotificationFilter::new(ignores, ignores, gitignore::load(&[]), ignore::load(&[])) - .unwrap(); + .expect("test filter errors"); assert!(filter.is_excluded(Path::new("hello.rs"))); assert!(filter.is_excluded(Path::new("Cargo.toml"))); diff --git a/src/process.rs b/src/process.rs index 81afe01e..f8270822 100644 --- a/src/process.rs +++ b/src/process.rs @@ -94,7 +94,7 @@ mod imp { // but is a little less performant and can cause trouble when using custom signals // (e.g. --signal SIGHUP) let mut command = if no_shell { - let (head, tail) = cmd.split_first().unwrap(); + let (head, tail) = cmd.split_first().expect("cmd was empty"); let mut command = Command::new(head); command.args(tail); command @@ -117,7 +117,7 @@ mod imp { } command.spawn().and_then(|p| { Ok(Self { - pgid: p.id().try_into().unwrap(), + pgid: p.id().try_into().expect("u32 -> i32 failed in process::new"), lock: Mutex::new(false), cvar: Condvar::new(), }) @@ -141,7 +141,7 @@ mod imp { } if finished { - let mut done = self.lock.lock().unwrap(); + let mut done = self.lock.lock().expect("poisoned lock in process::reap"); *done = true; self.cvar.notify_one(); } @@ -166,9 +166,9 @@ mod imp { } pub fn wait(&self) { - let mut done = self.lock.lock().unwrap(); + let mut done = self.lock.lock().expect("poisoned lock in process::wait"); while !*done { - done = self.cvar.wait(done).unwrap(); + done = self.cvar.wait(done).expect("poisoned cvar in process::wait"); } } } @@ -256,7 +256,7 @@ mod imp { let mut command; if no_shell { - let (first, rest) = cmd.split_first().unwrap(); + let (first, rest) = cmd.split_first().expect("command is empty"); command = Command::new(first); command.args(rest); } else { @@ -521,7 +521,7 @@ mod tests { #[test] fn longest_common_path_should_return_correct_value() { let single_path = vec![PathBuf::from("/tmp/random/")]; - let single_result = get_longest_common_path(&single_path).unwrap(); + let single_result = get_longest_common_path(&single_path).expect("failed to get longest common path"); assert_eq!(single_result, "/tmp/random/"); let common_paths = vec![ @@ -531,12 +531,12 @@ mod tests { PathBuf::from("/tmp/logs/fly"), ]; - let common_result = get_longest_common_path(&common_paths).unwrap(); + let common_result = get_longest_common_path(&common_paths).expect("failed to get longest common path"); assert_eq!(common_result, "/tmp/logs"); let diverging_paths = vec![PathBuf::from("/tmp/logs/hi"), PathBuf::from("/var/logs/hi")]; - let diverging_result = get_longest_common_path(&diverging_paths).unwrap(); + let diverging_result = get_longest_common_path(&diverging_paths).expect("failed to get longest common path"); assert_eq!(diverging_result, "/"); let uneven_paths = vec![ @@ -545,7 +545,7 @@ mod tests { PathBuf::from("/tmp/logs/bye"), ]; - let uneven_result = get_longest_common_path(&uneven_paths).unwrap(); + let uneven_result = get_longest_common_path(&uneven_paths).expect("failed to get longest common path"); assert_eq!(uneven_result, "/tmp/logs"); } diff --git a/src/run.rs b/src/run.rs index 812ae010..ae0e9a4b 100644 --- a/src/run.rs +++ b/src/run.rs @@ -172,7 +172,7 @@ impl Handler for ExecHandler { signal::install_handler(move |sig: Signal| { if let Some(lock) = weak_child.upgrade() { - let strong = lock.read().unwrap(); + let strong = lock.read().expect("poisoned lock in install_handler"); if let Some(ref child) = *strong { match sig { Signal::SIGCHLD => child.reap(), // SIGCHLD is special, initiate reap() @@ -299,7 +299,7 @@ fn wait_fs(rx: &Receiver, filter: &NotificationFilter, debounce: u64) -> // signal_process sends signal to process. It waits for the process to exit if wait is true fn signal_process(process: &RwLock>, signal: Option, wait: bool) { - let guard = process.read().unwrap(); + let guard = process.read().expect("poisoned lock in signal_process"); if let Some(ref child) = *guard { if let Some(s) = signal { diff --git a/src/signal.rs b/src/signal.rs index 550fb147..3e7b46c1 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -165,7 +165,7 @@ where } fn invoke(sig: self::Signal) { - if let Some(ref handler) = *CLEANUP.lock().unwrap() { + if let Some(ref handler) = *CLEANUP.lock().expect("poisoned lock in signal::invoke") { handler(sig) } } @@ -174,5 +174,5 @@ fn set_handler(handler: F) where F: Fn(self::Signal) + 'static + Send + Sync, { - *CLEANUP.lock().unwrap() = Some(Box::new(handler)); + *CLEANUP.lock().expect("poisoned lock in signal::set_handler") = Some(Box::new(handler)); }