Clippy (#50)

* Auto clippy fixes

* cargo fmt

* Clippy: Avoid using deprecated method

* Clippy: Define bounds in one location

* Clippy: Delete dead code

* Clippy: Test code as cfg(test)

* Clippy: Renamed Context::clone(&self) to .new_inner_scope

* Make Value Sync + Send

* Added cargo fmt & clippy checks to github actions

* Rebase changes

.github/workflows/rust.yml

@@ -16,7 +16,11 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       - uses: Swatinem/rust-cache@v2
+      - name: Format
+        run: cargo fmt --all -- --check
       - name: Build
         run: cargo build --verbose
+      - name: Clippy
+        run: cargo clippy --all-features --all-targets -- -D warnings
       - name: Run tests
         run: cargo test --verbose

interpreter/src/context.rs

@@ -115,7 +115,7 @@ impl<'a> Context<'a> {
         }
     }
 
-    pub fn add_function<T: 'static, F: 'static>(&mut self, name: &str, value: F)
+    pub fn add_function<T: 'static, F>(&mut self, name: &str, value: F)
     where
         F: Handler<T> + 'static,
     {
@@ -132,7 +132,7 @@ impl<'a> Context<'a> {
         Value::resolve_all(exprs, self)
     }
 
-    pub fn clone(&self) -> Context {
+    pub fn new_inner_scope(&self) -> Context {
         Context::Child {
             parent: self,
             variables: Default::default(),

interpreter/src/functions.rs

@@ -6,7 +6,7 @@ use crate::resolvers::{Argument, Resolver};
 use crate::ExecutionError;
 use cel_parser::Expression;
 use chrono::{DateTime, Duration, FixedOffset};
-use regex::{Error, Regex};
+use regex::Regex;
 use std::cmp::Ordering;
 use std::convert::TryInto;
 use std::sync::Arc;
@@ -283,7 +283,7 @@ pub fn map(
     match this {
         Value::List(items) => {
             let mut values = Vec::with_capacity(items.len());
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for item in items.iter() {
                 ptx.add_variable_from_value(ident.clone(), item.clone());
                 let value = ptx.resolve(&expr)?;
@@ -316,7 +316,7 @@ pub fn filter(
     match this {
         Value::List(items) => {
             let mut values = Vec::with_capacity(items.len());
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for item in items.iter() {
                 ptx.add_variable_from_value(ident.clone(), item.clone());
                 if let Value::Bool(true) = ptx.resolve(&expr)? {
@@ -350,7 +350,7 @@ pub fn all(
 ) -> Result<bool> {
     return match this {
         Value::List(items) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for item in items.iter() {
                 ptx.add_variable_from_value(&ident, item);
                 if let Value::Bool(false) = ptx.resolve(&expr)? {
@@ -360,7 +360,7 @@ pub fn all(
             Ok(true)
         }
         Value::Map(value) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for key in value.map.keys() {
                 ptx.add_variable_from_value(&ident, key);
                 if let Value::Bool(false) = ptx.resolve(&expr)? {
@@ -393,7 +393,7 @@ pub fn exists(
 ) -> Result<bool> {
     match this {
         Value::List(items) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for item in items.iter() {
                 ptx.add_variable_from_value(&ident, item);
                 if let Value::Bool(true) = ptx.resolve(&expr)? {
@@ -403,7 +403,7 @@ pub fn exists(
             Ok(false)
         }
         Value::Map(value) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             for key in value.map.keys() {
                 ptx.add_variable_from_value(&ident, key);
                 if let Value::Bool(true) = ptx.resolve(&expr)? {
@@ -437,7 +437,7 @@ pub fn exists_one(
 ) -> Result<bool> {
     match this {
         Value::List(items) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             let mut exists = false;
             for item in items.iter() {
                 ptx.add_variable_from_value(&ident, item);
@@ -451,7 +451,7 @@ pub fn exists_one(
             Ok(exists)
         }
         Value::Map(value) => {
-            let mut ptx = ftx.ptx.clone();
+            let mut ptx = ftx.ptx.new_inner_scope();
             let mut exists = false;
             for key in value.map.keys() {
                 ptx.add_variable_from_value(&ident, key);
@@ -516,20 +516,20 @@ pub fn max(Arguments(args): Arguments) -> Result<Value> {
                 None => Err(ExecutionError::ValuesNotComparable(acc.clone(), x.clone())),
             }
         })
-        .map(|v| v.clone())
+        .cloned()
 }
 
 /// A wrapper around [`parse_duration`] that converts errors into [`ExecutionError`].
 /// and only returns the duration, rather than returning the remaining input.
 fn _duration(i: &str) -> Result<Duration> {
-    let (_, duration) = parse_duration(i)
-        .map_err(|e| ExecutionError::function_error("duration", &e.to_string()))?;
+    let (_, duration) =
+        parse_duration(i).map_err(|e| ExecutionError::function_error("duration", e.to_string()))?;
     Ok(duration)
 }
 
 fn _timestamp(i: &str) -> Result<DateTime<FixedOffset>> {
     DateTime::parse_from_rfc3339(i)
-        .map_err(|e| ExecutionError::function_error("timestamp", &e.to_string()))
+        .map_err(|e| ExecutionError::function_error("timestamp", e.to_string()))
 }
 
 #[cfg(test)]

interpreter/src/lib.rs

@@ -1,7 +1,6 @@
 extern crate core;
 
 use cel_parser::parse;
-use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::sync::Arc;
 use thiserror::Error;
@@ -20,8 +19,8 @@ pub mod objects;
 mod resolvers;
 mod ser;
 pub use ser::to_value;
+#[cfg(test)]
 mod testing;
-use crate::testing::test_script;
 use magic::FromContext;
 
 pub mod extractors {

interpreter/src/macros.rs

@@ -35,13 +35,13 @@ macro_rules! impl_conversions {
                 }
             }
 
-            impl crate::magic::IntoResolveResult for $target_type {
+            impl $crate::magic::IntoResolveResult for $target_type {
                 fn into_resolve_result(self) -> ResolveResult {
                     Ok($value_variant(self))
                 }
             }
 
-            impl crate::magic::IntoResolveResult for Result<$target_type, ExecutionError> {
+            impl $crate::magic::IntoResolveResult for Result<$target_type, ExecutionError> {
                 fn into_resolve_result(self) -> ResolveResult {
                     self.map($value_variant)
                 }
@@ -66,7 +66,7 @@ macro_rules! impl_handler {
             impl<F, $($t,)* R> Handler<($($t,)*)> for F
             where
                 F: Fn($($t,)*) -> R + Clone,
-                $($t: for<'a, 'context> crate::FromContext<'a, 'context>,)*
+                $($t: for<'a, 'context> $crate::FromContext<'a, 'context>,)*
                 R: IntoResolveResult,
             {
                 fn call(self, _ftx: &mut FunctionContext) -> ResolveResult {
@@ -80,7 +80,7 @@ macro_rules! impl_handler {
             impl<F, $($t,)* R> Handler<(WithFunctionContext, $($t,)*)> for F
             where
                 F: Fn(&FunctionContext, $($t,)*) -> R + Clone,
-                $($t: for<'a, 'context> crate::FromContext<'a, 'context>,)*
+                $($t: for<'a, 'context> $crate::FromContext<'a, 'context>,)*
                 R: IntoResolveResult,
             {
                 fn call(self, _ftx: &mut FunctionContext) -> ResolveResult {

interpreter/src/magic.rs

@@ -197,24 +197,6 @@ impl From<Identifier> for String {
     }
 }
 
-#[derive(Clone)]
-pub struct List(pub Arc<Vec<Value>>);
-
-impl FromValue for List {
-    fn from_value(value: &Value) -> Result<Self, ExecutionError>
-    where
-        Self: Sized,
-    {
-        match value {
-            Value::List(list) => Ok(List(list.clone())),
-            _ => Err(ExecutionError::UnexpectedType {
-                got: format!("{:?}", value),
-                want: "list".to_string(),
-            }),
-        }
-    }
-}
-
 /// An argument extractor that extracts all the arguments passed to a function, resolves their
 /// expressions and returns a vector of [`Value`]. This is useful for functions that accept a
 /// variable number of arguments rather than known arguments and types (for example a `sum`

interpreter/src/objects.rs

@@ -11,12 +11,11 @@ use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::convert::{TryFrom, TryInto};
 use std::fmt::{Display, Formatter};
-use std::rc::Rc;
 use std::sync::Arc;
 
 #[derive(Debug, PartialEq, Clone)]
 pub struct Map {
-    pub map: Rc<HashMap<Key, Value>>,
+    pub map: Arc<HashMap<Key, Value>>,
 }
 
 impl PartialOrd for Map {
@@ -126,7 +125,7 @@ impl<K: Into<Key>, V: Into<Value>> From<HashMap<K, V>> for Map {
             new_map.insert(k.into(), v.into());
         }
         Map {
-            map: Rc::new(new_map),
+            map: Arc::new(new_map),
         }
     }
 }
@@ -256,13 +255,13 @@ impl PartialEq for Value {
             (Value::Int(a), Value::UInt(b)) => a
                 .to_owned()
                 .try_into()
-                .and_then(|a: u64| Ok(a == *b))
+                .map(|a: u64| a == *b)
                 .unwrap_or(false),
             (Value::Int(a), Value::Float(b)) => (*a as f64) == *b,
             (Value::UInt(a), Value::Int(b)) => a
                 .to_owned()
                 .try_into()
-                .and_then(|a: i64| Ok(a == *b))
+                .map(|a: i64| a == *b)
                 .unwrap_or(false),
             (Value::UInt(a), Value::Float(b)) => (*a as f64) == *b,
             (Value::Float(a), Value::Int(b)) => *a == (*b as f64),
@@ -289,15 +288,15 @@ impl PartialOrd for Value {
             (Value::Int(a), Value::UInt(b)) => Some(
                 a.to_owned()
                     .try_into()
-                    .and_then(|a: u64| Ok(a.cmp(b)))
+                    .map(|a: u64| a.cmp(b))
                     // If the i64 doesn't fit into a u64 it must be less than 0.
                     .unwrap_or(Ordering::Less),
             ),
             (Value::Int(a), Value::Float(b)) => (*a as f64).partial_cmp(b),
             (Value::UInt(a), Value::Int(b)) => Some(
                 a.to_owned()
                     .try_into()
-                    .and_then(|a: i64| Ok(a.cmp(b)))
+                    .map(|a: i64| a.cmp(b))
                     // If the u64 doesn't fit into a i64 it must be greater than i64::MAX.
                     .unwrap_or(Ordering::Greater),
             ),
@@ -516,7 +515,10 @@ impl<'a> Value {
                     let value = Value::resolve(v, ctx)?;
                     map.insert(key, value);
                 }
-                Value::Map(Map { map: Rc::from(map) }).into()
+                Value::Map(Map {
+                    map: Arc::from(map),
+                })
+                .into()
             }
             Expression::Ident(name) => {
                 if ctx.has_function(&***name) {
@@ -631,7 +633,7 @@ impl<'a> Value {
             Value::Bool(v) => *v,
             Value::Null => false,
             Value::Duration(v) => v.num_nanoseconds().map(|n| n != 0).unwrap_or(false),
-            Value::Timestamp(v) => v.timestamp_nanos() > 0,
+            Value::Timestamp(v) => v.timestamp_nanos_opt().unwrap_or_default() > 0,
             Value::Function(_, _) => false,
         }
     }
@@ -686,7 +688,7 @@ impl ops::Add<Value> for Value {
                 for (k, v) in r.map.iter() {
                     new.insert(k.clone(), v.clone());
                 }
-                Value::Map(Map { map: Rc::new(new) })
+                Value::Map(Map { map: Arc::new(new) })
             }
             (Value::Duration(l), Value::Duration(r)) => Value::Duration(l + r),
             (Value::Timestamp(l), Value::Duration(r)) => Value::Timestamp(l + r),

interpreter/src/testing.rs

@@ -5,5 +5,5 @@ use crate::Program;
 /// Tests the provided script and returns the result. An optional context can be provided.
 pub(crate) fn test_script(script: &str, ctx: Option<Context>) -> ResolveResult {
     let program = Program::compile(script).unwrap();
-    program.execute(&ctx.unwrap_or(Context::default()))
+    program.execute(&ctx.unwrap_or_default())
 }

parser/src/lib.rs

@@ -134,10 +134,7 @@ mod tests {
 
     #[test]
     fn simple_str() {
-        assert_parse_eq(
-            r#"'foobar'"#,
-            Atom(String("foobar".to_string().into()).into()),
-        );
+        assert_parse_eq(r#"'foobar'"#, Atom(String("foobar".to_string().into())));
         println!("{:?}", parse(r#"1 == '1'"#))
     }
 
@@ -317,7 +314,7 @@ mod tests {
                     Expression::Atom(Int(1)),
                 ),
                 (
-                    Expression::Atom(String("b".to_string().into())).into(),
+                    Expression::Atom(String("b".to_string().into())),
                     Expression::Atom(Int(2)),
                 ),
             ]),
@@ -335,7 +332,7 @@ mod tests {
                         Expression::Atom(Int(1)),
                     ),
                     (
-                        Expression::Atom(String("b".to_string().into())).into(),
+                        Expression::Atom(String("b".to_string().into())),
                         Expression::Atom(Int(2)),
                     ),
                 ])),

parser/src/parse.rs

@@ -88,11 +88,11 @@ pub fn parse_string(s: &str) -> Result<String, ParseError> {
     let mut chars = s.chars().enumerate();
     let res = String::with_capacity(s.len());
 
-    return match chars.next() {
+    match chars.next() {
         Some((_, c)) if c == 'r' || c == 'R' => parse_raw_string(&mut chars, res),
         Some((_, c)) if c == '\'' || c == '"' => parse_quoted_string(s, &mut chars, res, c),
         _ => Err(ParseError::MissingOpeningQuote),
-    };
+    }
 }
 
 fn parse_raw_string(chars: &mut Enumerate<Chars>, mut res: String) -> Result<String, ParseError> {