Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROBUSTNESS: Give error if condition for control statements have length != 1 #38

Closed
HenrikBengtsson opened this issue Mar 3, 2017 · 7 comments

Comments

@HenrikBengtsson
Copy link
Owner

@HenrikBengtsson HenrikBengtsson commented Mar 3, 2017

Issue

Control statements if(cond) and while(cond) in R gives an error if length(cond) == 0, but if length(cond) > 1, then only a warning is produced, e.g.

> x <- 1:2
> if (x == 1) message("x == 1")
x == 1
Warning message:
In if (x == 1) message("x == 1") :
  the condition has length > 1 and only the first element will be used

By the design of if and while control statements, it makes no sense to use a condition with a length other than one. Because of this there is not logical reason why the above should not be considered an error rather than a warning.

Suggestion

The long-term goal should be that R always produces an error if the length of the condition differs from one. However, due to only a warning has been produces this far, there is a great risk that there exist lots of code that will break immediately if an error is generated. In order to avoid wreaking havoc, a migration from producing a warning to an error may go via an option check.condition with default value FALSE. When FALSE, the current behavior remains and only a warning is produced. With TRUE, an error is produced. R CMD check --as-cran could enable options(check.condition = TRUE) such that all new packages submitted to CRAN must pass this new requirement. This will also allow individual developers to run checks locally.

Patch

A complete patch is available in HenrikBengtsson/r-source@hb-develop...HenrikBengtsson:hb-feature/check-condition. Example:

> options("check.condition")
$check.condition
[1] FALSE
> if (x == 1) message("x == 1")
x == 1
Warning message:
In if (x == 1) message("x == 1") :
  the condition has length > 1 and only the first element will be used

> options(check.condition = TRUE)
> x <- 1:2
> if (x == 1) message("x == 1")
Error in if (x == 1) message("x == 1") : the condition has length > 1

References

@HenrikBengtsson HenrikBengtsson added the bug label Mar 3, 2017
@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Mar 3, 2017

The corresponding SVN patch is:

Index: src/library/base/man/options.Rd
===================================================================
--- src/library/base/man/options.Rd	(revision 72298)
+++ src/library/base/man/options.Rd	(working copy)
@@ -86,6 +86,11 @@
       vector (atomic or \code{\link{list}}) is extended, by something
       like \code{x <- 1:3; x[5] <- 6}.}
 
+    \item{\code{check.condition}:}{logical, defaulting to \code{FALSE}.  If
+      \code{TRUE}, an error is produced whenever the condition to an
+      \code{if} or a \code{while} control statement is of length greater
+      than one.  If \code{FALSE}, a \link{warning} is produced.}
+
     \item{\code{CBoundsCheck}:}{logical, controlling whether
       \code{\link{.C}} and \code{\link{.Fortran}} make copies to check for
       array over-runs on the atomic vector arguments.
@@ -445,6 +450,7 @@
   \tabular{ll}{
     \code{add.smooth} \tab \code{TRUE}\cr
     \code{check.bounds} \tab \code{FALSE}\cr
+    \code{check.condition} \tab \code{FALSE}\cr
     \code{continue} \tab \code{"+ "}\cr
     \code{digits} \tab \code{7}\cr
     \code{echo} \tab \code{TRUE}\cr
Index: src/library/utils/R/completion.R
===================================================================
--- src/library/utils/R/completion.R	(revision 72298)
+++ src/library/utils/R/completion.R	(working copy)
@@ -1304,8 +1304,8 @@
           "plt", "ps", "pty", "smo", "srt", "tck", "tcl", "usr",
           "xaxp", "xaxs", "xaxt", "xpd", "yaxp", "yaxs", "yaxt")
 
-    options <- c("add.smooth", "browser", "check.bounds", "continue",
-	"contrasts", "defaultPackages", "demo.ask", "device",
+    options <- c("add.smooth", "browser", "check.bounds", "check.condition",
+        "continue", "contrasts", "defaultPackages", "demo.ask", "device",
 	"digits", "dvipscmd", "echo", "editor", "encoding",
 	"example.ask", "expressions", "help.search.types",
 	"help.try.all.packages", "htmlhelp", "HTTPUserAgent",
Index: src/main/eval.c
===================================================================
--- src/main/eval.c	(revision 72298)
+++ src/main/eval.c	(working copy)
@@ -1851,9 +1851,13 @@
     Rboolean cond = NA_LOGICAL;
 
     if (length(s) > 1) {
+	int check = asInteger(GetOption1(install("check.condition")));
 	PROTECT(s);	 /* needed as per PR#15990.  call gets protected by warningcall() */
-	warningcall(call,
-		    _("the condition has length > 1 and only the first element will be used"));
+	if(check != NA_INTEGER && check > 0)
+	    errorcall(call, _("the condition has length > 1"));
+	else
+	    warningcall(call,
+			_("the condition has length > 1 and only the first element will be used"));
 	UNPROTECT(1);
     }
     if (length(s) > 0) {
Index: src/main/options.c
===================================================================
--- src/main/options.c	(revision 72298)
+++ src/main/options.c	(working copy)
@@ -65,6 +65,7 @@
  *	"timeout"		./connections.c
 
  *	"check.bounds"
+ *	"check.condition"
  *	"error"
  *	"error.messages"
  *	"show.error.messages"
@@ -248,9 +249,9 @@
     char *p;
 
 #ifdef HAVE_RL_COMPLETION_MATCHES
+    PROTECT(v = val = allocList(22));
+#else
     PROTECT(v = val = allocList(21));
-#else
-    PROTECT(v = val = allocList(20));
 #endif
 
     SET_TAG(v, install("prompt"));
@@ -289,6 +290,10 @@
     SETCAR(v, ScalarLogical(0));	/* no checking */
     v = CDR(v);
 
+    SET_TAG(v, install("check.condition"));
+    SETCAR(v, ScalarLogical(0));	/* no checking */
+    v = CDR(v);
+    
     p = getenv("R_KEEP_PKG_SOURCE");
     R_KeepSource = (p && (strcmp(p, "yes") == 0)) ? 1 : 0;
@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Mar 3, 2017

I've sent an email about this to R-devel 'Control statements with condition with greater than one should give error (not just warning) [PATCH]' on 2017-03-03 (https://mailman.stat.ethz.ch/pipermail/r-devel/2017-March/073817.html)

UPDATE: Hmm... what an awkward title - should have been "... condition of length greater than one ..."

@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Mar 4, 2017

After feedback in the above R-devel thread, dropping R option check.condition and adding environment variable _R_CHECK_CONDITION_.

Patch

New SVN patch:

Index: src/main/eval.c
===================================================================
--- src/main/eval.c	(revision 72303)
+++ src/main/eval.c	(working copy)
@@ -1851,9 +1851,19 @@
     Rboolean cond = NA_LOGICAL;
 
     if (length(s) > 1) {
+	int val = 0; /* warn by default */
+	char *check = getenv("_R_CHECK_CONDITION_");
+	if (check != NULL) {
+	    val = (strcmp("1", check) == 0 ||
+	           strcasecmp("true", check) == 0 ||
+		   strcasecmp("yes", check) == 0);
+        }
 	PROTECT(s);	 /* needed as per PR#15990.  call gets protected by warningcall() */
-	warningcall(call,
-		    _("the condition has length > 1 and only the first element will be used"));
+	if (val)
+	    errorcall(call, _("the condition has length > 1"));
+        else
+	    warningcall(call,
+			_("the condition has length > 1 and only the first element will be used"));
 	UNPROTECT(1);
     }
     if (length(s) > 0) {

Example

$ Rscript --vanilla -e "if (1:2 == 1) 1"
[1] 1
Warning message:
In if (1:2 == 1) 1 :
  the condition has length > 1 and only the first element will be used

$ _R_CHECK_CONDITION_=true Rscript --vanilla -e "if (1:2 == 1) 1"
Error in if (1:2 == 1) 1 : the condition has length > 1
Execution halted

Git repository

The R source with the above patch applied is available in Git repository https://github.com/HenrikBengtsson/r-source/tree/hb-feature/check-condition-env

@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Mar 16, 2017

A version of this is now implemented in R devel SVN rev 72358:

     if (length(s) > 1) {
  	PROTECT(s);	 /* needed as per PR#15990.  call gets protected by warningcall() */
 -	warningcall(call,
 +	char *check = getenv("_R_CHECK_LENGTH_1_CONDITION_");
 +	if((check != NULL) ? StringTrue(check) : FALSE) // warn by default
 +	    errorcall(call, _("the condition has length > 1"));
 +        else
 +	    warningcall(call,
  		    _("the condition has length > 1 and only the first element will be used"));
  	UNPROTECT(1);
      }
@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented May 4, 2017

Setting _R_CHECK_LENGTH_1_CONDITION_=true (e.g. in .Renviron) enables this assertion in R (>= 3.4.0). I'll assume CRAN will eventually turn that on by default, i.e. R CMD check --as-cran will do it. Hopefully by R 3.5.0, it's the new default.

@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Aug 30, 2017

Just for the record (since I stumbled upon it), at least one bug in R itself has already been discovered and fixed:

Errors caught in other packages:

@HenrikBengtsson
Copy link
Owner Author

@HenrikBengtsson HenrikBengtsson commented Nov 26, 2019

2019-11-25: R CMD check --as-cran on R-devel and hence CRAN Submissions will now (r77457) check with:

_R_CHECK_LENGTH_1_CONDITION_=package:_R_CHECK_PACKAGE_NAME_,verbose

per https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17658

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.