Clippy

[alexsnaps]

I don't know that you actually want this, but did a run of cargo clippy --fix and followed up with a rustfmt... Now, clippy seems to highlight a few warnings still...

`cel-interpreter` (lib) generated 9 warnings

The Arc one also caught my attention when working on #48 ... Correct me if I'm wrong, but there is no support for multithreading in CEL, is there? So are you planning on making the interpreter mutlithreaded eventually? Or should I take a stab at making all the Arc just Rc?

``` warning: bound is defined in more than one place --> interpreter/src/context.rs:118:37 | 118 | pub fn add_function(&mut self, name: &str, value: F) | ^ 119 | where 120 | F: Handler + 'static, | ^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations = note: `#[warn(clippy::multiple_bound_locations)]` on by default warning: use of deprecated method `chrono::DateTime::::timestamp_nanos`: use `timestamp_nanos_opt()` instead --> interpreter/src/objects.rs:634:38 | 634 | Value::Timestamp(v) => v.timestamp_nanos() > 0, | ^^^^^^^^^^^^^^^ | = note: `#[warn(deprecated)]` on by default warning: field `0` is never read --> interpreter/src/magic.rs:201:17 | 201 | pub struct List(pub Arc>); | ---- ^^^^^^^^^^^^^^^^^^^ | | | field in this struct | = note: `List` has a derived impl for the trait `Clone`, but this is intentionally ignored during dead code analysis = note: `#[warn(dead_code)]` on by default help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field | 201 | pub struct List(()); | ~~ warning: function `test_script` is never used --> interpreter/src/testing.rs:6:15 | 6 | pub(crate) fn test_script(script: &str, ctx: Option) -> ResolveResult { | ^^^^^^^^^^^ warning: method `clone` can be confused for the standard trait method `std::clone::Clone::clone` --> interpreter/src/context.rs:135:5 | 135 | / pub fn clone(&self) -> Context { 136 | | Context::Child { 137 | | parent: self, 138 | | variables: Default::default(), 139 | | } 140 | | } | |_____^ | = help: consider implementing the trait `std::clone::Clone` or choosing a less ambiguous method name = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait = note: `#[warn(clippy::should_implement_trait)]` on by default warning: usage of an `Arc` that is not `Send` and `Sync` --> interpreter/src/functions.rs:274:25 | 274 | Value::List(Arc::new(values)) | ^^^^^^^^^^^^^^^^ | = note: `Arc>` is not `Send` and `Sync` as `Vec` is neither `Send` nor `Sync` = help: if the `Arc` will not used be across threads replace it with an `Rc` = help: otherwise make `Vec` `Send` and `Sync` or consider a wrapper type such as `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync = note: `#[warn(clippy::arc_with_non_send_sync)]` on by default warning: usage of an `Arc` that is not `Send` and `Sync` --> interpreter/src/functions.rs:308:25 | 308 | Value::List(Arc::new(values)) | ^^^^^^^^^^^^^^^^ | = note: `Arc>` is not `Send` and `Sync` as `Vec` is neither `Send` nor `Sync` = help: if the `Arc` will not used be across threads replace it with an `Rc` = help: otherwise make `Vec` `Send` and `Sync` or consider a wrapper type such as `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync warning: usage of an `Arc` that is not `Send` and `Sync` --> interpreter/src/ser.rs:254:24 | 254 | Ok(Value::List(Arc::new(self.vec))) | ^^^^^^^^^^^^^^^^^^ | = note: `Arc>` is not `Send` and `Sync` as `Vec` is neither `Send` nor `Sync` = help: if the `Arc` will not used be across threads replace it with an `Rc` = help: otherwise make `Vec` `Send` and `Sync` or consider a wrapper type such as `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync warning: usage of an `Arc` that is not `Send` and `Sync` --> interpreter/src/ser.rs:303:51 | 303 | let map = HashMap::from_iter([(self.name, Arc::new(self.vec))]); | ^^^^^^^^^^^^^^^^^^ | = note: `Arc>` is not `Send` and `Sync` as `Vec` is neither `Send` nor `Sync` = help: if the `Arc` will not used be across threads replace it with an `Rc` = help: otherwise make `Vec` `Send` and `Sync` or consider a wrapper type such as `Mutex` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync ```

[clarkmcc]

This is excellent, thank you!

CEL programs actually are multi-threaded (https://github.com/clarkmcc/cel-rust/blob/master/example/src/threads.rs) hence the Arc. They actually used to be Rc back in the day!

[alexsnaps]

So either these instances would suffice as plain Rcs or clippy is wrong... Should I leave things as they are for these? I can quickly try and see if rustc then complains if they really need to be Sync/Send ...

[alexsnaps]

Added a few clean ups, also misread the clippy issue with these Arc, the problem is the what is put in them that's not Sync/Send. So I guess so either T should be made Send + Sync.

I'll look in the Clone one: warning: method clone can be confused for the standard trait method std::clone::Clone::clone, which I think is the lifetime issue, yes?

Feel free to disagree with any of the above changes (kept them isolated in discrete commits)...

[clarkmcc]

So I guess so either T should be made Send + Sync.

Ah, found it. Map used an Rc still under the hood.

Index: interpreter/src/objects.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/objects.rs b/interpreter/src/objects.rs
--- a/interpreter/src/objects.rs    (revision 45c883d5fc4c8a26f200ce8bbf3d8ae75ab2624a)
+++ b/interpreter/src/objects.rs    (date 1718806273985)
@@ -16,7 +16,7 @@

 #[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 +126,7 @@
             new_map.insert(k.into(), v.into());
         }
         Map {
-            map: Rc::new(new_map),
+            map: Arc::new(new_map),
         }
     }
 }
@@ -516,7 +516,10 @@
                     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) {
@@ -686,7 +689,7 @@
                 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),
@@ -860,6 +863,5 @@
         let program = Program::compile("size == 50").unwrap();
         let value = program.execute(&context).unwrap();
         assert_eq!(value, false.into());
-
     }
 }

I'll look in the Clone one: warning: method clone can be confused for the standard trait method std::clone::Clone::clone, which I think is the lifetime issue, yes?

Yes I've seen this one. I'd be open to renaming to .fork if you would like. Under the hood the function takes one context and returns a new context that has a reference to the original context, sort of like setting up a tree of contexts. I wrote up and explanation of how and why here if it's helpful. In any case, fork seems like an accurate name for what's happening. Thoughts?

[alexsnaps]

Yes I've seen this one. I'd be open to renaming to .fork if you would like. Under the hood the function takes one context and returns a new context that has a reference to the original context, sort of like setting up a tree of contexts. I wrote up and explanation of how and why here if it's helpful. In any case, fork seems like an accurate name for what's happening. Thoughts?

Probably not .clone() indeed. So we're talking about inner scopes here, right? Always lexical ones I think in CEL? So should that just be a .inner_scope() or .add_new_inner_scope() or something? Fork isn't quite right neither, as it stacks rather than fork it... Tho... 🤔

Only two hard things in computer science, eh?! 😉

I'd go with .fork() over .clone() for sure tho...

[clarkmcc]

Always lexical ones I think in CEL?

Correct, yes I'd be good with either suggestion! Take your pick.

[alexsnaps]

Dunno if having rustfmt check & clippy at the github action level is something you'd want, but I took the freedom to add it... Can easily drop the commit if needed.

Finally the Arc issue was because Value wasn't Sync + Send, because Map wasn't as it held an Rc...

Now confused by the test run failures... They reported rustc error don't point to the code as I have it 🤔

[alexsnaps]

Passes here... confused

[alexsnaps]

Rebased 🤦 Sorry for all the noise... hadn't updated my own fork(s)/clone(s) with my own changes... So this is ready for review/merge, either with or without github actions changes