Repository: extendr/extendr
Pull request #205 (2021-04-10 13:08:32)

Fix more clippy warnings.
Fix remaining clippy warnings.

I've now added `cargo clippy` to my pre-commit hook and I suggest that others do the same.

I didn't agree with one of the clippy checks and put an #[allow()] block in.

andy-thomason: I thought that I should include you in the conversation as you have been working hard.

Patch:
diff --git a/extendr-api/src/functions.rs b/extendr-api/src/functions.rs
index 8bef740d6..ed3160e1c 100644
--- a/extendr-api/src/functions.rs
+++ b/extendr-api/src/functions.rs
@@ -105,7 +105,7 @@ pub fn empty_env() -> Environment {
 /// use extendr_api::prelude::*;
 /// test! {
 ///     global_env().set_local(sym!(x), "hello");
-///     assert_eq!(base_env().local(sym!(+)), Ok(r!(Primitive::from_str("+"))));
+///     assert_eq!(base_env().local(sym!(+)), Ok(r!(Primitive::from_string("+"))));
 /// }
 /// ```
 pub fn base_env() -> Environment {
@@ -237,8 +237,8 @@ pub fn eval_string(code: &str) -> Result<Robj> {
 /// ```
 pub fn eval_string_with_params(code: &str, values: &[&Robj]) -> Result<Robj> {
     single_threaded(|| {
-        let env = Environment::new(global_env());
-        for (i, &v) in values.into_iter().enumerate() {
+        let env = Environment::new_with_parent(global_env());
+        for (i, &v) in values.iter().enumerate() {
             let key = Symbol::from_string(format!("param.{}", i));
             env.set_local(key, v);
         }
diff --git a/extendr-api/src/lang_macros.rs b/extendr-api/src/lang_macros.rs
index 006a51cf3..00362ecdb 100644
--- a/extendr-api/src/lang_macros.rs
+++ b/extendr-api/src/lang_macros.rs
@@ -102,7 +102,6 @@ macro_rules! append_lang {
 /// test! {
 ///     let vec = call!("c", 1.0, 2.0, 3.0).unwrap();
 ///     assert_eq!(vec, r!([1., 2., 3.]));
-///     assert_eq!(vec.is_owned(), true);
 ///
 ///     let list = call!("list", a=1, b=2).unwrap();
 ///     assert_eq!(list.len(), 2);
diff --git a/extendr-api/src/ownership.rs b/extendr-api/src/ownership.rs
index e9282937d..b51a16afd 100644
--- a/extendr-api/src/ownership.rs
+++ b/extendr-api/src/ownership.rs
@@ -21,12 +21,12 @@ lazy_static! {
     static ref OWNERSHIP: Mutex<Ownership> = Mutex::new(Ownership::new());
 }
 
-pub unsafe fn protect(sexp: SEXP) {
+pub(crate) unsafe fn protect(sexp: SEXP) {
     let mut own = OWNERSHIP.lock().expect("protect failed");
     own.protect(sexp);
 }
 
-pub unsafe fn unprotect(sexp: SEXP) {
+pub(crate) unsafe fn unprotect(sexp: SEXP) {
     let mut own = OWNERSHIP.lock().expect("unprotect failed");
     own.unprotect(sexp);
 }
diff --git a/extendr-api/src/robj/from_robj.rs b/extendr-api/src/robj/from_robj.rs
index 737ecb912..882e90e44 100644
--- a/extendr-api/src/robj/from_robj.rs
+++ b/extendr-api/src/robj/from_robj.rs
@@ -167,7 +167,7 @@ impl<'a> FromRobj<'a> for HashMap<String, Robj> {
     fn from_robj(robj: &'a Robj) -> std::result::Result<Self, &'static str> {
         if let Some(iter) = robj.as_list().map(|l| l.iter()) {
             Ok(iter
-                .map(|(k, v)| (k.to_string(), v.to_owned()))
+                .map(|(k, v)| (k.to_string(), v))
                 .collect::<HashMap<String, Robj>>())
         } else {
             Err("expected a list")
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index 876a41b9c..6ccd292e5 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -179,11 +179,11 @@ impl Robj {
     ///     assert_eq!(sym!(xyz).rtype(), RType::Symbol);
     ///     assert_eq!(r!(Pairlist::from_pairs(vec![("a", r!(1))])).rtype(), RType::Pairlist);
     ///     assert_eq!(R!("function() {}")?.rtype(), RType::Function);
-    ///     assert_eq!(Environment::new(global_env()).rtype(), RType::Enviroment);
+    ///     assert_eq!(Environment::new_with_parent(global_env()).rtype(), RType::Enviroment);
     ///     assert_eq!(lang!("+", 1, 2).rtype(), RType::Language);
-    ///     assert_eq!(r!(Primitive::from_str("if")).rtype(), RType::Special);
-    ///     assert_eq!(r!(Primitive::from_str("+")).rtype(), RType::Builtin);
-    ///     assert_eq!(r!(Character::from_str("hello")).rtype(), RType::Character);
+    ///     assert_eq!(r!(Primitive::from_string("if")).rtype(), RType::Special);
+    ///     assert_eq!(r!(Primitive::from_string("+")).rtype(), RType::Builtin);
+    ///     assert_eq!(r!(Character::from_string("hello")).rtype(), RType::Character);
     ///     assert_eq!(r!(TRUE).rtype(), RType::Logical);
     ///     assert_eq!(r!(1).rtype(), RType::Integer);
     ///     assert_eq!(r!(1.0).rtype(), RType::Real);
@@ -661,30 +661,6 @@ impl Robj {
             Robj::from(())
         }
     }
-
-    /// Return true if the object is owned by this wrapper.
-    /// If so, it will be released when the wrapper drops.
-    /// ```
-    /// use extendr_api::prelude::*;
-    /// test! {
-    ///    let owned = r!(1);      // Allocated vector.
-    ///    let borrowed = r!(());  // R_NilValue
-    ///    assert_eq!(owned.is_owned(), true);
-    ///    assert_eq!(borrowed.is_owned(), false);
-    /// }
-    /// ```
-    pub fn is_owned(&self) -> bool {
-        matches!(self, Robj::Owned(_))
-    }
-
-    // Convert the Robj to an owned one.
-    #[doc(hidden)]
-    pub fn to_owned(self) -> Robj {
-        match self {
-            Robj::Owned(_) => self,
-            _ => unsafe { new_owned(self.get()) },
-        }
-    }
 }
 
 /// Generic access to typed slices in an Robj.
@@ -1060,7 +1036,7 @@ impl std::fmt::Debug for Robj {
             BUILTINSXP => write!(f, "r!(Builtin())"),
             CHARSXP => {
                 let c = Character::try_from(self.clone()).unwrap();
-                write!(f, "r!(Character::from_str({:?}))", c.as_str())
+                write!(f, "r!(Character::from_string({:?}))", c.as_str())
             }
             LGLSXP => {
                 let slice = self.as_logical_slice().unwrap();
diff --git a/extendr-api/src/robj/try_from_robj.rs b/extendr-api/src/robj/try_from_robj.rs
index ef9fefe03..8aa793c7b 100644
--- a/extendr-api/src/robj/try_from_robj.rs
+++ b/extendr-api/src/robj/try_from_robj.rs
@@ -174,7 +174,7 @@ impl TryFrom<Robj> for HashMap<String, Robj> {
     fn try_from(robj: Robj) -> Result<Self> {
         if let Some(iter) = robj.as_list().map(|l| l.iter()) {
             Ok(iter
-                .map(|(k, v)| (k.to_string(), v.to_owned()))
+                .map(|(k, v)| (k.to_string(), v))
                 .collect::<HashMap<String, Robj>>())
         } else {
             Err(Error::ExpectedList(robj))
@@ -187,9 +187,7 @@ impl TryFrom<Robj> for HashMap<&str, Robj> {
 
     fn try_from(robj: Robj) -> Result<Self> {
         if let Some(iter) = robj.as_list().map(|l| l.iter()) {
-            Ok(iter
-                .map(|(k, v)| (k, v.to_owned()))
-                .collect::<HashMap<&str, Robj>>())
+            Ok(iter.map(|(k, v)| (k, v)).collect::<HashMap<&str, Robj>>())
         } else {
             Err(Error::ExpectedList(robj))
         }
diff --git a/extendr-api/src/wrapper/character.rs b/extendr-api/src/wrapper/character.rs
index 69c7160a4..c075e1b2b 100644
--- a/extendr-api/src/wrapper/character.rs
+++ b/extendr-api/src/wrapper/character.rs
@@ -7,7 +7,7 @@ use super::*;
 /// ```
 /// use extendr_api::prelude::*;
 /// test! {
-///     let chr = r!(Character::from_str("xyz"));
+///     let chr = r!(Character::from_string("xyz"));
 ///     assert_eq!(chr.as_character().unwrap().as_str(), "xyz");
 /// }
 /// ```
@@ -19,7 +19,7 @@ pub struct Character {
 
 impl Character {
     /// Make a character object from a string.
-    pub fn from_str(val: &str) -> Self {
+    pub fn from_string(val: &str) -> Self {
         unsafe {
             Character {
                 robj: new_owned(str_to_character(val)),
diff --git a/extendr-api/src/wrapper/environment.rs b/extendr-api/src/wrapper/environment.rs
index c089d9b88..d86894a42 100644
--- a/extendr-api/src/wrapper/environment.rs
+++ b/extendr-api/src/wrapper/environment.rs
@@ -6,21 +6,20 @@ pub struct Environment {
 }
 
 impl Environment {
-    /// Create a new, empty environment parented on global_env()
+    /// Create a new, empty environment.
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///     let env = Environment::new(global_env());
+    ///     let env = Environment::new_with_parent(global_env());
     ///     assert_eq!(env.len(), 0);
     /// }
     /// ```
-    pub fn new(parent: Environment) -> Self {
+    pub fn new_with_parent(parent: Environment) -> Self {
         // 14 is a reasonable default.
         Environment::new_with_capacity(parent, 14)
     }
 
-    /// Create a new, empty environment parented on global_env()
-    /// with a reserved size.
+    /// Create a new, empty environment with a reserved size.
     ///
     /// This function will guess the hash table size if required.
     /// Use the Env{} wrapper for more detail.
@@ -55,6 +54,7 @@ impl Environment {
     ///     assert_eq!(env.len(), 100);
     /// }
     /// ```
+    #[allow(clippy::wrong_self_convention)]
     pub fn from_pairs<NV>(parent: Environment, names_and_values: NV) -> Self
     where
         NV: IntoIterator,
@@ -142,7 +142,7 @@ impl Environment {
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///     let env = Environment::new(global_env());
+    ///     let env = Environment::new_with_parent(global_env());
     ///     env.set_local(sym!(x), "harry");
     ///     env.set_local(sym!(x), "fred");
     ///     assert_eq!(env.local(sym!(x)), Ok(r!("fred")));
@@ -162,7 +162,7 @@ impl Environment {
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///     let env = Environment::new(global_env());
+    ///     let env = Environment::new_with_parent(global_env());
     ///     env.set_local(sym!(x), "fred");
     ///     assert_eq!(env.local(sym!(x)), Ok(r!("fred")));
     /// }
diff --git a/extendr-api/src/wrapper/mod.rs b/extendr-api/src/wrapper/mod.rs
index bd0c6111a..8f09efb3a 100644
--- a/extendr-api/src/wrapper/mod.rs
+++ b/extendr-api/src/wrapper/mod.rs
@@ -161,8 +161,8 @@ impl Robj {
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///     let fred = r!(Character::from_str("fred"));
-    ///     assert_eq!(fred.as_character(), Some(Character::from_str("fred")));
+    ///     let fred = r!(Character::from_string("fred"));
+    ///     assert_eq!(fred.as_character(), Some(Character::from_string("fred")));
     /// }
     /// ```
     pub fn as_character(&self) -> Option<Character> {
diff --git a/extendr-api/src/wrapper/primitive.rs b/extendr-api/src/wrapper/primitive.rs
index 8be4b2bf5..6009d4c71 100644
--- a/extendr-api/src/wrapper/primitive.rs
+++ b/extendr-api/src/wrapper/primitive.rs
@@ -6,8 +6,8 @@ use super::*;
 /// ```
 /// use extendr_api::prelude::*;
 /// test! {
-///     let builtin = r!(Primitive::from_str("+"));
-///     let special = r!(Primitive::from_str("if"));
+///     let builtin = r!(Primitive::from_string("+"));
+///     let special = r!(Primitive::from_string("if"));
 /// }
 /// ```
 ///
@@ -21,13 +21,13 @@ impl Primitive {
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///     let builtin = r!(Primitive::from_str("+")?);
-    ///     let special = r!(Primitive::from_str("if")?);
+    ///     let builtin = r!(Primitive::from_string("+")?);
+    ///     let special = r!(Primitive::from_string("if")?);
     ///     assert_eq!(builtin.rtype(), RType::Builtin);
     ///     assert_eq!(special.rtype(), RType::Special);
     /// }
     /// ```
-    pub fn from_str(val: &str) -> Result<Self> {
+    pub fn from_string(val: &str) -> Result<Self> {
         single_threaded(|| unsafe {
             // Primitives have a special "SYMVALUE" entry in their symbol.
             let sym = Symbol::from_string(val);
diff --git a/extendr-macros/src/pairlist.rs b/extendr-macros/src/pairlist.rs
index f9f5209d4..1fec10fca 100644
--- a/extendr-macros/src/pairlist.rs
+++ b/extendr-macros/src/pairlist.rs
@@ -20,7 +20,7 @@ pub fn pairlist(item: TokenStream) -> TokenStream {
         })
         .collect::<Vec<Expr>>();
 
-    if pairs.len() == 0 {
+    if pairs.is_empty() {
         TokenStream::from(quote!(Pairlist::from(())))
     } else {
         TokenStream::from(quote!(


Test patch:
diff --git a/extendr-api/src/robj/tests.rs b/extendr-api/src/robj/tests.rs
index 71ef39333..8678bdff6 100644
--- a/extendr-api/src/robj/tests.rs
+++ b/extendr-api/src/robj/tests.rs
@@ -22,7 +22,7 @@ fn test_debug() {
 
         // Wrappers
         assert_eq!(format!("{:?}", r!(Symbol::from_string("x"))), "sym!(x)");
-        assert_eq!(format!("{:?}", r!(Character::from_str("x"))), "r!(Character::from_str(\"x\"))");
+        assert_eq!(format!("{:?}", r!(Character::from_string("x"))), "r!(Character::from_string(\"x\"))");
         assert_eq!(
             format!("{:?}", r!(Language::from_values(&[r!(Symbol::from_string("x"))]))),
             "r!(Language::from_values([sym!(x)]))"
@@ -86,7 +86,6 @@ fn test_from_robj() {
         assert_eq!(hmap_owned, hmap1);
         assert_eq!(hmap_borrowed, hmap2);
 
-        assert!(hmap_owned["a"].is_owned());
         assert_eq!(hmap_owned["a"], Robj::from(1));
         assert_eq!(hmap_owned["b"], Robj::from(2));
 
@@ -180,7 +179,6 @@ fn test_try_from_robj() {
         assert_eq!(hmap_owned, hmap1);
         assert_eq!(hmap_borrowed, hmap2);
 
-        assert!(hmap_owned["a"].is_owned());
         assert_eq!(hmap_owned["a"], Robj::from(1));
         assert_eq!(hmap_owned["b"], Robj::from(2));