Bug 16385 - Unary logical operators are all treated as "not"
Summary: Unary logical operators are all treated as "not"
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Low-level (show other bugs)
Version: R 3.2.0
Hardware: Other Linux
: P5 enhancement
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2015-05-14 14:10 UTC by Barry Rowlingson
Modified: 2015-05-18 14:53 UTC (History)
2 users (show)

See Also:


Attachments
Patch which adds arity check to do_logic (408 bytes, patch)
2015-05-14 18:38 UTC, Gabriel Becker
Details | Diff
Fix patch (402 bytes, patch)
2015-05-14 19:03 UTC, Gabriel Becker
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Rowlingson 2015-05-14 14:10:32 UTC
As expected:

> `&`(TRUE,TRUE)
[1] TRUE
> `&`(TRUE,TRUE,TRUE)
Error: binary operations require two arguments

but:

> `&`(TRUE)
[1] FALSE
> `|`(TRUE)
[1] FALSE

Possibly because all logical operators with one arg are treated as "not" operators, via line in logic.c around here:

https://github.com/wch/r-source/blob/ed415a8431b32e079100f50a846e4769aeb54d5a/src/main/logic.c#L59
Comment 1 Gabriel Becker 2015-05-14 18:38:44 UTC
Created attachment 1827 [details]
Patch which adds arity check to do_logic

the added checkArity call protects against unary calls to `&` and `|`, and against non-unary calls to `!`. 

This needs to happen before the special casing for scalar logical unary calls (which assumes without checking that op is `!`), so can't happen within the lunary/lbinary calls near the end of the function
Comment 2 Gabriel Becker 2015-05-14 19:03:18 UTC
Created attachment 1828 [details]
Fix patch

I called checkArity with the signature of Rf_checkArity (including the call) in the previous patch, which was incorrect. This patch is tested and works.

Arity check needs to happen before the special casing for unary ops (assumed to be `!` ) on simple logical scalars.
Comment 3 Martin Maechler 2015-05-15 13:06:09 UTC
Also note the *wrong* error message in this case:

> `!`()
Error: binary operations require two arguments
> 

I do wonder why the current code has been organized the way it is, instead of using the usual checkArity().
Possibly for historical reasons --- remember that in S, the "!" had had a double use of also allowing to do system()-like calls.

If using checkArity() as Gabriel proposes, the rest of the logic could also be simplified a bit, I'd say.

OTOH, if we do use checkArity(),  the nice error message

  "binary operations require two arguments"

is replaced by something like

  "3 arguments passed to '&' which requires 2"

which is more accurate for advanced programmeRs and computer science literates,
but *is* more cryptic for the average useR.
But then, for the regular operator (as opposed to function) use, the parser already protects such a useR from getting into these muddy waters at all:
> &TRUE
Error: unexpected '&' in "&"
> FALSE ! TRUE
Error: unexpected '!' in "FALSE !"
> 

---

There's more:  Gabriel said "tested"... well, not really: You did not run R's own tests successfully:  These are listed as S3 generic primitives and so
 &.foo <- function(x, ...) { ........ }
has worked and this is detected by our checks.
Consequently, the checkArity() test should *not* happen before dispatch...

I'm currently testing things
Comment 4 Martin Maechler 2015-05-15 23:33:20 UTC
More simplifications were possible, and moving the DispatchGroup() call earlier was necessary.

The change is committed to R-devel... and planned to be moved to R 3.2.0 patched after a few days of waiting.