extendr / extendr

R extension library for rust designed to be familiar to R users.
https://extendr.github.io
MIT License
422 stars 41 forks source link

Reduce the number of `unwrap`s #341

Open Ilia-Kosenkov opened 2 years ago

Ilia-Kosenkov commented 2 years ago

I was researching how we handle matrices right now, and I came up with this example (during a Discord discussion)

rextendr::rust_function(
  "fn mat() -> Robj {
      Robj::try_from(
        &extendr_api::prelude::Array::from_shape_vec(
          (3, 2), 
          vec![1., 2., 3., 4., 5., 6.]
        )
        .unwrap()
      )
      .unwrap()
  }",
 extendr_deps = list(
   `extendr-api` = list(
     git = "https://github.com/extendr/extendr",
     features = array("ndarray")
    )
  )
)
#> i build directory: 'C:/Users/[redacted]/AppData/Local/Temp/RtmpSS2kt0/file3bcc760499f'
#> v Writing 'C:/Users/[redacted]/AppData/Local/Temp/RtmpSS2kt0/file3bcc760499f/target/extendr_wrappers.R'.

mat()
#>      [,1] [,2]
#> [1,]    1    2
#> [2,]    3    4
#> [3,]    5    6

Created on 2021-12-18 by the reprex package (v2.0.1)

I am not fluent in Rust error handling, but I wonder if we can allow for ? operator usage? It would simplify a lot of code (in demos or when you need to fail fast). I tried modifying this example, but failed to achieve any results because there are no conversions for extendr_api::Error from error types produced, e.g., by ndarray.

andy-thomason commented 2 years ago

If the function returned a Result<> you should be able to use ?.

I haven't checked the state of Result handling recently, but please give it a try.

Also, using Doubles instead of Robj would avoid the conversion.

We should look again at the Matrix implementation as it is lagging the rest.

andy-thomason commented 2 years ago

We should definitely include error conversions for ndarray types in the implmentation.

andy-thomason commented 2 years ago

We could also generate ndarray ArrayRefs from Integers and Doubles.

andy-thomason commented 2 years ago

Maybe a "to_matrix/to_array" call for the vector types to add a class and dims.

multimeric commented 2 years ago

Firstly, ndarray has some nice macros for creating vectors. So for example we can replace:

extendr_api::prelude::Array::from_shape_vec(    
          (3, 2),    
          vec![1., 2., 3., 4., 5., 6.]    
        )

with

ndarray::array![[1., 2., 3.], [4., 5., 6.]]

Fruther, if we use this macro, we don't actually need to unwrap the array creation. Then, we can also make the function return a Result, giving us the following fairly clean code:

rextendr::rust_function(    
  "fn mat() -> Result<Robj> {    
      Ok(Robj::try_from(&ndarray::array![[1., 2., 3.], [4., 5., 6.]])?)   
  }",    
 extendr_deps = list(
   `extendr-api` = list(
     git = "https://github.com/extendr/extendr",
     features = array("ndarray")
    ),
    `ndarray` = "0.15.3"
  )
) 

Okay so in this example using ? may have actually made it more verbose because we have to add the Ok and the Result signature, but this will make code cleaner once we get many more unwraps needed.


Now the main usability issues I see are:

  1. We can't convert between ndarray errors and extendr errors.

This is evident when you take your original code (without the macro), and try to use ?:

rextendr::rust_function(    
  "fn mat() -> Result<Robj> {    
      Ok(Robj::try_from( 
        &extendr_api::prelude::Array::from_shape_vec(    
          (3, 2),    
          vec![1., 2., 3., 4., 5., 6.]    
        )?    
      )?)   
  }",    
 extendr_deps = list(
   `extendr-api` = list(
     git = "https://github.com/extendr/extendr",
     features = array("ndarray")
    )
  )
)    

Currently this throws:

✖ error[E0277]: `?` couldn't convert the error to `extendr_api::Error`
   --> src/lib.rs:8:10
    |
3   | fn mat() -> Result<Robj> {    
    |             ------------ expected `extendr_api::Error` because of this
...
8   |         )?    
    |          ^ the trait `From<ShapeError>` is not implemented for `extendr_api::Error`
    |

I actually wonder if we could solve this more generally, not just by implementing a conversion trait from ndarray errors, but actually implementing a conversion trait from any foreign Error to our Error. We could do this by adding another Error enum type (such as ForeignError) that holds a boxed foreign error. This would improve usability a whole lot.

  1. Implement the ndarray conversion traits not just for references

In my first example, you may have missed that I did this: Ok(Robj::try_from(&ndarray::array![[1., 2., 3.], [4., 5., 6.]])?). Currently if I don't use & the compiler will fail and throw a very unhelpful error, and it's simply because in my PR I only implemented this trait for &ArrayBase, which is kind of annoying now that I think about it.

multimeric commented 2 years ago

Actually it turns out we already have impl From<Box<dyn std::error::Error>> for Error so there should be a cleaner solution to the above using from() or into()