[Rcpp-devel] Catching parse errors with RInside::parseEval()

[Dirk Eddelbuettel]

Hi Theodore,

On 11 October 2012 at 00:39, Theodore Lytras wrote:
| I studied RInside.cpp, made a few trial-and-error tests, and it appears to 
| work for me now - parseEval() doesn't crash when fed with syntactically 
| incorrect R code.
| 
| I am attaching a patch I made with quilt (interactive.diff), so you can see 
| what I did.
| 
| Turns out, that boolean you were talking about (on line 152) did no good. 
| Leaving it to false results in a crash ("Execution halted"), switching it to 
| true results in a crash plus a segmentation fault!
| 
| Turns out also that the switch statement is indeed evaluated. It gets there. 
| What halts execution is indeed the Rf_error() function; changing that to 
| Rf_warning() solves the problem, and allows parseEval() to return 1 and 
| parseEvalQ() to throw its exception. 

Good catch.
 
| One also needs to call mb_m.rewind() afterwards, or the MemBuf is not cleared 
| and subsequent calls to parseEval() fail as well.

Very good, that's along the lines that I was thinking about.
 
| As a result I added a boolean to RInside.h (interactiveMode) along with a 
| setter function (setInteractive(bool)), in order to enable this behaviour 
| instead of the old one.
| 
| Now it works perfectly for me!
| 
| As an alternative, we could get rid of the boolean altogether and also get rid 
| of the Rf_error() or Rf_warning(). I don't think it can ever be good to "halt 
| execution" when dealing with syntactically incorrect R code. Returning an 
| error code or throwing an exception is better. And the Rf_warning() is also 
| unnecessary, if case PARSE_NULL: and case PARSE_ERROR: return their own error 
| code (eg. 2 and 3). No code using RInside should be affected by this, since 
| until now the error code was not returned and execution was halted at 
| Rf_error().
| 
| So if you aggree with that, you can apply instead the other patch I'm 
| attaching (simplified.diff). I guess that's the best solution.
| 
| Hope this helps a bit! It sure solved my problem.

That helps a ton. I glanced briefly at the simpler patch, and I will start
with that.

Thanks for all this -- this is greatly appreciated.

Cheers, Dirk
 
| Regards,
| 
| Theodore
| 
| ----------------------------------------------------------------------
| Index: RInside/inst/include/RInside.h
| ===================================================================
| --- RInside.orig/inst/include/RInside.h	2010-07-09 23:59:14.000000000 +0300
| +++ RInside/inst/include/RInside.h	2012-10-10 19:25:37.302217289 +0300
| @@ -32,6 +32,7 @@
|      Rcpp::Environment global_env ;
|      
|      bool verbose_m;				// private switch
| +    bool interactiveMode;	// if true, parseEval() will not cause crash when evaluating a line with syntax errors
|  
|      void init_tempdir(void);
|      void init_rand(void);
| @@ -56,6 +57,8 @@
|      int  parseEval(const std::string & line, SEXP &ans); // parse line, return in ans; error code rc
|      void parseEvalQ(const std::string & line);		 // parse line, no return (throws on error)
|  
| +    void setInteractive(const bool interactive) { interactiveMode = interactive; }
| +    
|      class Proxy {
|  	public:
|  	    Proxy(SEXP xx): x(xx) { };
| Index: RInside/src/RInside.cpp
| ===================================================================
| --- RInside.orig/src/RInside.cpp	2012-09-08 04:00:46.000000000 +0300
| +++ RInside/src/RInside.cpp	2012-10-10 19:30:08.427279004 +0300
| @@ -57,7 +57,7 @@
|  
|  RInside::RInside()
|  #ifdef RINSIDE_CALLBACKS
| -    : callbacks(0)
| +    : interactiveMode(false), callbacks(0)
|  #endif
|  {
|      initialize( 0, 0, false );
| @@ -99,7 +99,7 @@
|  
|  RInside::RInside(const int argc, const char* const argv[], const bool loadRcpp)
|  #ifdef RINSIDE_CALLBACKS
| -: callbacks(0)
| +: interactiveMode(false), callbacks(0)
|  #endif
|  {
|      initialize( argc, argv, loadRcpp );
| @@ -333,14 +333,28 @@
|          // need to read another line
|          break;
|      case PARSE_NULL:
| -        Rf_error("%s: ParseStatus is null (%d)\n", programName, status);
| -        UNPROTECT(2);
| -        return 1;
| +        if (interactiveMode) {
| +	    Rf_warning("%s: ParseStatus is null (%d)\n", programName, status);
| +	    UNPROTECT(2);
| +	    mb_m.rewind();
| +	    return 1;
| +	} else {
| +	    Rf_error("%s: ParseStatus is null (%d)\n", programName, status);
| +	    UNPROTECT(2);
| +	    return 1;
| +	}
|          break;
|      case PARSE_ERROR:
| -        Rf_error("Parse Error: \"%s\"\n", line.c_str());
| -        UNPROTECT(2);
| -        return 1;
| +	if (interactiveMode) {
| +	    Rf_warning("Parse Error: \"%s\"\n", line.c_str());
| +	    UNPROTECT(2);
| +	    mb_m.rewind();
| +	    return 1;
| +	} else {
| +	    Rf_error("Parse Error: \"%s\"\n", line.c_str());
| +	    UNPROTECT(2);
| +	    return 1;
| +	}
|          break;
|      case PARSE_EOF:
|          Rf_error("%s: ParseStatus is eof (%d)\n", programName, status);
| 
| ----------------------------------------------------------------------
| Index: RInside/src/RInside.cpp
| ===================================================================
| --- RInside.orig/src/RInside.cpp	2012-10-11 00:30:43.228525248 +0300
| +++ RInside/src/RInside.cpp	2012-10-11 00:32:01.676564524 +0300
| @@ -333,14 +333,14 @@
|          // need to read another line
|          break;
|      case PARSE_NULL:
| -        Rf_error("%s: ParseStatus is null (%d)\n", programName, status);
|          UNPROTECT(2);
| -        return 1;
| +	mb_m.rewind();
| +        return 2;
|          break;
|      case PARSE_ERROR:
| -        Rf_error("Parse Error: \"%s\"\n", line.c_str());
|          UNPROTECT(2);
| -        return 1;
| +	mb_m.rewind();
| +        return 3;
|          break;
|      case PARSE_EOF:
|          Rf_error("%s: ParseStatus is eof (%d)\n", programName, status);

-- 
Dirk Eddelbuettel | edd at debian.org | http://dirk.eddelbuettel.com