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

[Theodore Lytras]

Στις Τετ 10 Οκτ 2012, ο/η Dirk Eddelbuettel έγραψε:
> There is a boolean in RInside.cpp that you need to flip.  Eventually, we
> could (if it is useful) support a flag at instantiation of the RObject.
[snip]
> | I browsed the code, and a few lines down I see:
> | 
> | case PARSE_ERROR:
> |    Rf_error("Parse Error: \"%s\"\n", line.c_str());
> |    UNPROTECT(2);
> |    return 1;
> |    break;
> | 
> | thus it appears that parse errors are already handled. Even better:
> | handled with a return code (I don't like exceptions that much). So I
> | guess the only problem is that we're not in interactive mode, correct?
> 
> In theory. In practice, I think, it just doesn't get there.
> 
> | What kind of work/extensions do you suggest this code needs? (I could
> | try to work a little on that, and learn a little about the insides of
> | R in the process).
> 
> I think it is pretty much this switch block we need to get to work
> properly.
> 
> Dirk

Hi Dirk,

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. 

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

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.

Regards,

Theodore
-------------- next part --------------
A non-text attachment was scrubbed...
Name: interactive.diff
Type: text/x-patch
Size: 2527 bytes
Desc: not available
URL: <http://lists.r-forge.r-project.org/pipermail/rcpp-devel/attachments/20121011/2d9e41e2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simplified.diff
Type: text/x-patch
Size: 767 bytes
Desc: not available
URL: <http://lists.r-forge.r-project.org/pipermail/rcpp-devel/attachments/20121011/2d9e41e2/attachment-0003.bin>