r_top_level_exec() and r_parse_vector()

[romainfrancois]

Related to #109

This adds r_top_level_exec() as a rust wrapper around R_ToplevelExec():

    let out = r_top_level_exec(|| {
        // code that may jump
    });

out is a Result< {whatever the lambda returns}, {Error = Error::TopLevelExecError}>

    let mut ps : ParseStatus = 0;
    let mut protect = RProtect::new();
    let r_code = protect.add(crate::r_string!(code));

    let mut out: SEXP = R_NilValue;

    let out = r_top_level_exec(|| {
        R_ParseVector(r_code, -1, &mut ps, R_NilValue);
    });

Maybe this could be pub unsafe fn r_top_level_exec<F, R>(mut f: F -> R) -> Result<R> where F: FnMut() -> R instead so that we'd just get the returned value from the block, but I could not figure it out yet.

This also add r_parse_vector() around R_ParseVector():

#[allow(non_upper_case_globals)]
pub unsafe fn r_parse_vector(code: String) -> ParseResult {

    let mut ps : ParseStatus = 0;
    let mut protect = RProtect::new();
    let r_code = protect.add(crate::r_string!(code));

    let lambda = || {
        R_ParseVector(r_code, -1, &mut ps, R_NilValue)
    };

    match r_top_level_exec(lambda) {
        Err(_) => ParseResult::Error{
            message: geterrmessage()
        },
        Ok(out) => {
            match ps {
                ParseStatus_PARSE_OK => ParseResult::Ok(out),
                ParseStatus_PARSE_INCOMPLETE => ParseResult::Incomplete(),
                _ => {
                    ParseResult::SyntaxError{
                        message: CStr::from_ptr(R_ParseErrorMsg.as_ptr()).to_string_lossy().to_string(),
                        line: R_ParseError as i32
                    }
                }
            }
        }
    }
}

This has issues:

• because of how R_ParseVector() works, there are two different paths to get an error:
  • the usual path i.e. *42 gives a PARSE_INCOMPLETE that we translate here to a SyntaxError. This gives an error message and a line.
  • after the fact tests, i.e. 42 + _ gives a PARSE_OK but then throws an error (the reason we have to capture it), this gets translated to an Error. This only gives an error message (from geterrmessage())

The other issue is that r_top_level_exec() still prints the error, i.e. when running the tests I'm getting:

So maybe r_top_level_exec() is not the right tool here, unless we can get rid of the error somehow ?

[kevinushey]

I'll try to give a more thorough review later, but what do you think about using a Result type instead of a boolean for the return value?

    let result = r_top_level_exec(|| {
        // code that may jump
    });
    match result { /* Ok() or Err() */}

I'll have to read further to understand if we can really safely transmute Rust closures (or other FnMut's?) in the way being done here.

It may also be considering whether tryCatch() is ultimately the best approach, because then we could catch the (R-level) condition being created and thrown here:

https://github.com/wch/r-source/blob/068a6a0d9d60b1ce42abc879900038649f4a929c/src/main/gram.c#L6553-L6640

[romainfrancois]

(yes, definitely) made it so r_top_level_exec returns a Result now whose type is whatever the closure returns. This might need anther Error variant though.

[romainfrancois]

I'll have a look at adding a similar wrapper around R_TryCatch() and/or R_TryCatchError() so that we have both tools and can choose which one works best for the given situation.

(this is the kind of exercise that is perfect here because I get to practice quite a few rust concepts, but still make something potentially useful)

[kevinushey]

LGTM; do you have any reference material discussing the casts being done to get a C function pointer from a Rust closure and when / why that's safe?

[kevinushey]

Should this be the base environment? (in case someone has defined conditionMessage() in the global environment)

[romainfrancois]

Yeah probably, this also assumes that conditionMessage() won't itself jump, but I think this is fair.

[romainfrancois]

Added the r_try_catch_error function that wraps around R_tryCatchError(). It takes a closure that returns a SEXP and returns a Result<Ok = RObject, Error = TryCatchError>

• In the ok case, we get the result of R_tryCatchError() as an RObject
• In the error case, the TryCatchError wraps the condition as a RObject.

Example from the tst:

let out = r_try_catch_error(|| {
    let msg = CString::new("ouch").unwrap();
    Rf_error(unsafe {msg.as_ptr()});

    R_NilValue
});

I've re-implemented r_parse_vector() using it:

#[allow(non_upper_case_globals)]
pub unsafe fn r_parse_vector(code: String) -> ParseResult {

    let mut ps : ParseStatus = 0;
    let mut protect = RProtect::new();
    let r_code = protect.add(crate::r_string!(code));

    let lambda = || {
        R_ParseVector(r_code, -1, &mut ps, R_NilValue)
    };

    match r_try_catch_error(lambda) {
        Err(_) => ParseResult::Error{
            message: geterrmessage()
        },

        Ok(out) => {
            match ps {
                ParseStatus_PARSE_OK => ParseResult::Ok(*out),
                ParseStatus_PARSE_INCOMPLETE => ParseResult::Incomplete(),
                _ => {
                    ParseResult::SyntaxError{
                        message: CStr::from_ptr(R_ParseErrorMsg.as_ptr()).to_string_lossy().to_string(),
                        line: R_ParseError as i32
                    }
                }
            }
        }
    }
}

The error case here can definitely be improved to grab information from the condition thrown from raiseParseError() https://github.com/r-devel/r-svn/blob/385e818c892fd8a37b8658f4a837dc0be000dd74/src/main/gram.c#L6553 i.e. value, filename, lineno and colno.

Err(_) => ParseResult::Error{
     message: geterrmessage()
}

[romainfrancois]

do you have any reference material discussing the casts being done to get a C function pointer from a Rust closure and when / why that's safe.

Not really, I found let closure: &mut &mut dyn FnMut() = unsafe { mem::transmute(arg) }; on stack overflow (I'll find the question again). I think the double &mut is what does it.

[romainfrancois]

A thing that bothers me here is that the closure must return a SEXP, i.e.

let out = r_try_catch_error(|| {
    let msg = CString::new("ouch").unwrap();
    Rf_error(unsafe {msg.as_ptr()});

    R_NilValue
});

It would be nice to be able to avoid the R_NilValue at the end, and have an r_try_catch_error overload that deals with functions returning () so that we could:

let out = r_try_catch_error(|| {
    let msg = CString::new("ouch").unwrap();
    Rf_error(unsafe {msg.as_ptr()});
})

[romainfrancois]

Related article: https://adventures.michaelfbryan.com/posts/rust-closures-in-ffi/

[romainfrancois]

Also following up after #118 this now has:

• r_strings() generic that knows how to make a STRSXP (wrapped in a RObject) from a bunch of things (whatever implements the ToRStrings trait): &str; String, &[&str], Vec<&str>, &[String], Vec<String>.
• PartialEq<> for RObject implemented for things that can be converted to a CHARSXP (as defined by the ToCharSxp trait)

Because of that, I've been able to implement the r_try_catch and r_try_catch_error variants on top of r_try_catch_finally():

pub unsafe fn r_try_catch<F, R, S>(fun: F, classes: S) -> std::result::Result<RObject, RError> where F: FnMut() -> R, MaybeSEXP: From<R>, S : ToRStrings {
    r_try_catch_finally(fun, classes, ||{} )
}

pub unsafe fn r_try_catch_error<F, R>(fun: F) -> std::result::Result<RObject, RError> where F: FnMut() -> R, MaybeSEXP: From<R> {
    r_try_catch_finally(fun, "error", ||{} )
}

I think it's fine to have both RObject::from() and r_strings() to make a STRSXP, but r_strings() makes it visually clearer what's happening.

[romainfrancois]

Also, I removed r_top_level_exec() but it would be easy to bring back.

[kevinushey]

Overall LGTM, only main concern is avoiding usage of unwrap() in a couple cases.

[romainfrancois]

This still needs a little bit of work, but made it so r_try_catch_error() has this signature:

pub unsafe fn r_try_catch_error<F, R>(fun: F) -> Result<RObject>
where
    F: FnMut() -> R,
    RObject: From<R>

i.e. it takes a closure that returns something that can be converted to an RObject and returns it on success. On failure it returns a Error::TryCatchError

[kevinushey]

LGTM; nice work!

[romainfrancois]

Thanks. I'll follow up with a pr that calls r_parse_vector from higher up.