Bug 15534 - deparse(complex) can produce invalid syntax, e.g. 1+Infi.
deparse(complex) can produce invalid syntax, e.g. 1+Infi.
Status: CLOSED FIXED
Product: R
Classification: Unclassified
Component: Language
R 3.0.2
All All
: P5 minor
Assigned To: R-core
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-07 19:36 UTC by Alex Bertram
Modified: 2014-01-01 20:10 UTC (History)
2 users (show)

See Also:


Attachments
Defect correction and unit tests (3.79 KB, patch)
2013-11-07 19:36 UTC, Alex Bertram
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Bertram 2013-11-07 19:36:10 UTC
Created attachment 1508 [details]
Defect correction and unit tests

deparse(complex(r=1,imaginary=Inf)) produces invalid syntax: 1+Infi.

There is no way to specify a literal complex value with a non-finite imaginary component, so deparse() should return "complex(real=1, i=Inf)" instead.

A patch against r64518 is attached with a fix and unit tests to verify correctness.
Comment 1 Duncan Murdoch 2013-11-14 20:35:32 UTC
I agree there's a problem, but I have some problems with the supplied patch.

1.  I don't think Print.h should be modified.  This change should be local to EncodeComplex and/or vector2buff.  (The former is used in all print routines, the latter only in deparsing, but it can make use of EncodeElement which calls EncodeComplex.)

2.  EncodeComplex needs some kind of fix, because the same parsing error you report affects regular output as well, e.g. 

> complex(r=1,imaginary=Inf)
[1] 1+Infi

3.  I think your patch changes too much, e.g. it converts complex(r=Inf, imaginary=1) to the function call, even though "Inf+1i" parses with no errors.
You do need to be careful, because NaN + 1i is not the same as
complex(real=NaN, complex=1).

4.  Regular display should be willing to display things that are correct but not reparsable (e.g. NaN+1i is reasonable in a regular print statement).  Deparsing doesn't promise reparsing is possible, but we do try to do that when it doesn't make the display too ugly.  I think the full function call notation is too ugly to use in regular printing, but it may be reasonable to use it in deparsing.
Comment 2 Alex Bertram 2013-11-19 22:07:34 UTC
Thanks for the feedback! A few questions and then I'll submit an updated patch:

1) I modified Print.h to add a seperate method for specifically deparsing as opposed to printing complex numbers, because I believe that EncodeComplex is also called for regular printing, which I agree should continue to use the more readable 1+Infi output. Is there some other way to modify EncodeComplex to distinguish between these two call paths without modifying Print.h?

2) I think that complex(r=1,i=Inf) should print "1+Infi", right? In this case we can't modify EncodeComplex because it's called by print, correct?

3) Agreed, will fix

4) Agreed
Comment 3 Benjamin Tyner 2013-11-22 13:45:46 UTC
Note that this isn't limited to complex; for example:

   > deparse(charToRaw("hello world"))
   [1] "c(68, 65, 6c, 6c, 6f, 20, 77, 6f, 72, 6c, 64)"

is equally invalid syntax.
Comment 4 Alex Bertram 2013-11-22 14:10:52 UTC
(In reply to Benjamin Tyner from comment #3)

I believe that issue (#PR#15369) is fixed by r62883 in June, it's definitely fixed in the trunk but not sure which release. 

Building from the trunk, I get:

> deparse(charToRaw("hello world"))
[1] "as.raw(c(0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x20, 0x77, 0x6f, 0x72, "
[2] "0x6c, 0x64))"                                                   




> Note that this isn't limited to complex; for example:
> 
>    > deparse(charToRaw("hello world"))
>    [1] "c(68, 65, 6c, 6c, 6f, 20, 77, 6f, 72, 6c, 64)"
> 
> is equally invalid syntax.
Comment 5 Benjamin Tyner 2013-11-23 03:39:06 UTC
(In reply to Alex Bertram from comment #4)
> (In reply to Benjamin Tyner from comment #3)
> 
> I believe that issue (#PR#15369) is fixed by r62883 in June, it's definitely
> fixed in the trunk but not sure which release. 
> 
> Building from the trunk, I get:
> 
> > deparse(charToRaw("hello world"))
> [1] "as.raw(c(0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x20, 0x77, 0x6f, 0x72, "
> [2] "0x6c, 0x64))"                                                   
> 
> 
> 
> 
> > Note that this isn't limited to complex; for example:
> > 
> >    > deparse(charToRaw("hello world"))
> >    [1] "c(68, 65, 6c, 6c, 6f, 20, 77, 6f, 72, 6c, 64)"
> > 
> > is equally invalid syntax.

My mistake, thanks Alex. I do get the same output in 3.0.2 as well as in 3.1.0 (r64286).
Comment 6 Duncan Murdoch 2014-01-01 20:10:08 UTC
I have simplified this a bit, and will commit to R-devel and R-patched soon.