Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
I'm just throwing this out there, but maybe CRAN (or some other testing machinery) should get a "strict testing" build of R in which suppressWarnings is reassigned to identity... Or maybe suppressWarnings could check for, eg an environment variable and turn itself into a no-op when present, so that build systems could be used to test for this. ~G On Tue, Mar 7, 2017 at 11:45 AM, Karl Millar via R-devel < r-devel@r-project.org> wrote: > Is there anything that actually requires R core members to manually do > significant amounts of work here? > > IIUC, you can do a CRAN run to detect the broken packages, and a simple > script can collect the emails of the affected maintainers, so you can send > a single email to them all. If authors don't respond by fixing their > packages, then those packages should be archived, since there's high > probability of those packages being buggy anyway. > > If you expect a non-trivial amount of questions regarding this change from > the affected package maintainers, then you can create a FAQ page for it, > which you can fill in as questions arrive, so you don't get too many > duplicated questions. > > Karl > > On Mon, Mar 6, 2017 at 4:51 AM, Martin Maechler < > maech...@stat.math.ethz.ch> > wrote: > > > > Michael Lawrence> > > on Sat, 4 Mar 2017 12:20:45 -0800 writes: > > > > > Is there really a need for these complications? Packages > > > emitting this warning are broken by definition and should be fixed. > > > > I agree and probably Henrik, too. > > > > (Others may disagree to some extent .. and find it convenient > > that R does translate 'if(x)' to 'if(x[1])' for them albeit > > with a warning .. ) > > > > > Perhaps we could "flip the switch" in a test > > > environment and see how much havoc is wreaked and whether > > > authors are sufficiently responsive? > > > > > Michael > > > > As we have > 10'000 packages on CRAN alonce, and people have > > started (mis)using suppressWarnings(.) in many places, there > > may be considerably more packages affected than we optimistically > assume... > > > > As R core member who would "flip the switch" I'd typically then > > have to be the one sending an e-mail to all package maintainers > > affected and in this case I'm very reluctant to volunteer > > for that and so, I'd prefer the environment variable where R > > core and others can decide how to use it .. for a while .. until > > the flip is switched for all. > > > > or have I overlooked an issue? > > > > Martin > > > > > On Sat, Mar 4, 2017 at 12:04 PM, Martin Maechler > > > > >> wrote: > > > > >> > Henrik Bengtsson > > > >> on Fri, 3 Mar 2017 10:10:53 -0800 writes: > > >> > > >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham > > > >> wrote: >>> But, how you propose a > > >> warning-to-error transition >>> should be made without > > >> wreaking havoc? Just flip the >>> switch in R-devel and > > >> see CRAN and Bioconductor packages >>> break overnight? > > >> Particularly Bioconductor devel might >>> become > > >> non-functional (since at times it requires >>> R-devel). > > >> For my own code / packages, I would be able >>> to handle > > >> such a change, but I'm completely out of >>> control if > > >> one of the package I'm depending on does not >>> provide > > >> a quick fix (with the only option to remove >>> package > > >> tests for those dependencies). > > >> >> > > >> >> Generally, a package can not be on CRAN if it has any > > >> >> warnings, so I don't think this change would have any > > >> >> impact on CRAN packages. Isn't this also true for >> > > >> bioconductor? > > >> > > >> > Having a tests/warn.R file with: > > >> > > >> > warning("boom") > > >> > > >> > passes through R CMD check --as-cran unnoticed. > > >> > > >> Yes, indeed.. you are right Henrik that many/most R > > >> warning()s would not produce R CMD check 'WARNING's .. > > >> > > >> I think Hadley and I fell into the same mental pit of > > >> concluding that such warning()s from > > >> if() ... would not currently happen > > >> in CRAN / Bioc packages and hence turning them to errors > > >> would not have a direct effect. > > >> > > >> With your 2nd e-mail of saying that you'd propose such an > > >> option only for a few releases of R you've indeed > > >> clarified your intent to me. OTOH, I would prefer using > > >> an environment variable (as you've proposed as an > > >> alternative) which is turned "active" at the beginning > > >> only manually or for the "CRAN incoming" checks of the > > >> CRAN team (and bioconductor submission checks?) and > > >> later for '--as-cran' etc until it eventually becomes the > > >> unconditional behavior of R
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
Is there anything that actually requires R core members to manually do significant amounts of work here? IIUC, you can do a CRAN run to detect the broken packages, and a simple script can collect the emails of the affected maintainers, so you can send a single email to them all. If authors don't respond by fixing their packages, then those packages should be archived, since there's high probability of those packages being buggy anyway. If you expect a non-trivial amount of questions regarding this change from the affected package maintainers, then you can create a FAQ page for it, which you can fill in as questions arrive, so you don't get too many duplicated questions. Karl On Mon, Mar 6, 2017 at 4:51 AM, Martin Maechlerwrote: > > Michael Lawrence > > on Sat, 4 Mar 2017 12:20:45 -0800 writes: > > > Is there really a need for these complications? Packages > > emitting this warning are broken by definition and should be fixed. > > I agree and probably Henrik, too. > > (Others may disagree to some extent .. and find it convenient > that R does translate 'if(x)' to 'if(x[1])' for them albeit > with a warning .. ) > > > Perhaps we could "flip the switch" in a test > > environment and see how much havoc is wreaked and whether > > authors are sufficiently responsive? > > > Michael > > As we have > 10'000 packages on CRAN alonce, and people have > started (mis)using suppressWarnings(.) in many places, there > may be considerably more packages affected than we optimistically assume... > > As R core member who would "flip the switch" I'd typically then > have to be the one sending an e-mail to all package maintainers > affected and in this case I'm very reluctant to volunteer > for that and so, I'd prefer the environment variable where R > core and others can decide how to use it .. for a while .. until > the flip is switched for all. > > or have I overlooked an issue? > > Martin > > > On Sat, Mar 4, 2017 at 12:04 PM, Martin Maechler > > >> wrote: > > >> > Henrik Bengtsson > > >> on Fri, 3 Mar 2017 10:10:53 -0800 writes: > >> > >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham > > >> wrote: >>> But, how you propose a > >> warning-to-error transition >>> should be made without > >> wreaking havoc? Just flip the >>> switch in R-devel and > >> see CRAN and Bioconductor packages >>> break overnight? > >> Particularly Bioconductor devel might >>> become > >> non-functional (since at times it requires >>> R-devel). > >> For my own code / packages, I would be able >>> to handle > >> such a change, but I'm completely out of >>> control if > >> one of the package I'm depending on does not >>> provide > >> a quick fix (with the only option to remove >>> package > >> tests for those dependencies). > >> >> > >> >> Generally, a package can not be on CRAN if it has any > >> >> warnings, so I don't think this change would have any > >> >> impact on CRAN packages. Isn't this also true for >> > >> bioconductor? > >> > >> > Having a tests/warn.R file with: > >> > >> > warning("boom") > >> > >> > passes through R CMD check --as-cran unnoticed. > >> > >> Yes, indeed.. you are right Henrik that many/most R > >> warning()s would not produce R CMD check 'WARNING's .. > >> > >> I think Hadley and I fell into the same mental pit of > >> concluding that such warning()s from > >> if() ... would not currently happen > >> in CRAN / Bioc packages and hence turning them to errors > >> would not have a direct effect. > >> > >> With your 2nd e-mail of saying that you'd propose such an > >> option only for a few releases of R you've indeed > >> clarified your intent to me. OTOH, I would prefer using > >> an environment variable (as you've proposed as an > >> alternative) which is turned "active" at the beginning > >> only manually or for the "CRAN incoming" checks of the > >> CRAN team (and bioconductor submission checks?) and > >> later for '--as-cran' etc until it eventually becomes the > >> unconditional behavior of R (and the env.variable is no > >> longer used). > >> > >> Martin > >> > >> __ > >> R-devel@r-project.org mailing list > >> https://stat.ethz.ch/mailman/listinfo/r-devel > >> > > > [[alternative HTML version deleted]] > > __ > R-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel > [[alternative HTML version deleted]] __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
> Michael Lawrence> on Sat, 4 Mar 2017 12:20:45 -0800 writes: > Is there really a need for these complications? Packages > emitting this warning are broken by definition and should be fixed. I agree and probably Henrik, too. (Others may disagree to some extent .. and find it convenient that R does translate 'if(x)' to 'if(x[1])' for them albeit with a warning .. ) > Perhaps we could "flip the switch" in a test > environment and see how much havoc is wreaked and whether > authors are sufficiently responsive? > Michael As we have > 10'000 packages on CRAN alonce, and people have started (mis)using suppressWarnings(.) in many places, there may be considerably more packages affected than we optimistically assume... As R core member who would "flip the switch" I'd typically then have to be the one sending an e-mail to all package maintainers affected and in this case I'm very reluctant to volunteer for that and so, I'd prefer the environment variable where R core and others can decide how to use it .. for a while .. until the flip is switched for all. or have I overlooked an issue? Martin > On Sat, Mar 4, 2017 at 12:04 PM, Martin Maechler > > wrote: >> > Henrik Bengtsson > >> on Fri, 3 Mar 2017 10:10:53 -0800 writes: >> >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham > >> wrote: >>> But, how you propose a >> warning-to-error transition >>> should be made without >> wreaking havoc? Just flip the >>> switch in R-devel and >> see CRAN and Bioconductor packages >>> break overnight? >> Particularly Bioconductor devel might >>> become >> non-functional (since at times it requires >>> R-devel). >> For my own code / packages, I would be able >>> to handle >> such a change, but I'm completely out of >>> control if >> one of the package I'm depending on does not >>> provide >> a quick fix (with the only option to remove >>> package >> tests for those dependencies). >> >> >> >> Generally, a package can not be on CRAN if it has any >> >> warnings, so I don't think this change would have any >> >> impact on CRAN packages. Isn't this also true for >> >> bioconductor? >> >> > Having a tests/warn.R file with: >> >> > warning("boom") >> >> > passes through R CMD check --as-cran unnoticed. >> >> Yes, indeed.. you are right Henrik that many/most R >> warning()s would not produce R CMD check 'WARNING's .. >> >> I think Hadley and I fell into the same mental pit of >> concluding that such warning()s from >> if() ... would not currently happen >> in CRAN / Bioc packages and hence turning them to errors >> would not have a direct effect. >> >> With your 2nd e-mail of saying that you'd propose such an >> option only for a few releases of R you've indeed >> clarified your intent to me. OTOH, I would prefer using >> an environment variable (as you've proposed as an >> alternative) which is turned "active" at the beginning >> only manually or for the "CRAN incoming" checks of the >> CRAN team (and bioconductor submission checks?) and >> later for '--as-cran' etc until it eventually becomes the >> unconditional behavior of R (and the env.variable is no >> longer used). >> >> Martin >> >> __ >> R-devel@r-project.org mailing list >> https://stat.ethz.ch/mailman/listinfo/r-devel >> > [[alternative HTML version deleted]] __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
Here's a patch that enables the error if _R_CHECK_CONDITION_={1,true,TRUE,yes,YES}. $ svn diff 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) { An alternative is to make _R_CHECK_CONDITION_=false the default behavior. With this env variable, I think it'll be easier for a developer to temporarily work around broken dependencies until fixed. It will also help identify broken dependencies. For instance, as soon as one is identified it can be wrapped up in an: with_faulty_conditions(x <- pkg::foo(...)) and additionally faulty if/while statements can be detected. Your package will still "work" until downstream packages are fixed. Here, with_faulty_conditions <- function(expr, envir = parent.frame()) { oenv <- Sys.getenv("_R_CHECK_CONDITION_") on.exit(Sys.setenv("_R_CHECK_CONDITION_" = oenv)) Sys.setenv("_R_CHECK_CONDITION_" = FALSE) eval(substitute(expr), envir = envir) } /Henrik On Sat, Mar 4, 2017 at 12:20 PM, Michael Lawrencewrote: > Is there really a need for these complications? Packages emitting this > warning are broken by definition and should be fixed. Perhaps we could "flip > the switch" in a test environment and see how much havoc is wreaked and > whether authors are sufficiently responsive? > > Michael > > On Sat, Mar 4, 2017 at 12:04 PM, Martin Maechler > wrote: >> >> > Henrik Bengtsson >> > on Fri, 3 Mar 2017 10:10:53 -0800 writes: >> >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham >> > wrote: >> >>> But, how you propose a warning-to-error transition >> >>> should be made without wreaking havoc? Just flip the >> >>> switch in R-devel and see CRAN and Bioconductor packages >> >>> break overnight? Particularly Bioconductor devel might >> >>> become non-functional (since at times it requires >> >>> R-devel). For my own code / packages, I would be able >> >>> to handle such a change, but I'm completely out of >> >>> control if one of the package I'm depending on does not >> >>> provide a quick fix (with the only option to remove >> >>> package tests for those dependencies). >> >> >> >> Generally, a package can not be on CRAN if it has any >> >> warnings, so I don't think this change would have any >> >> impact on CRAN packages. Isn't this also true for >> >> bioconductor? >> >> > Having a tests/warn.R file with: >> >> > warning("boom") >> >> > passes through R CMD check --as-cran unnoticed. >> >> Yes, indeed.. you are right Henrik that many/most R warning()s would >> not produce R CMD check 'WARNING's .. >> >> I think Hadley and I fell into the same mental pit of concluding >> that such warning()s from if() ... >> would not currently happen in CRAN / Bioc packages and hence >> turning them to errors would not have a direct effect. >> >> With your 2nd e-mail of saying that you'd propose such an option >> only for a few releases of R you've indeed clarified your intent >> to me. >> OTOH, I would prefer using an environment variable (as you've >> proposed as an alternative) which is turned "active" at the >> beginning only manually or for the "CRAN incoming" checks of >> the CRAN team (and bioconductor submission checks?) >> and later for '--as-cran' etc until it eventually becomes the >> unconditional behavior of R (and the env.variable is no longer used). >> >> Martin >> >> __ >> R-devel@r-project.org mailing list >> https://stat.ethz.ch/mailman/listinfo/r-devel > > __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
Is there really a need for these complications? Packages emitting this warning are broken by definition and should be fixed. Perhaps we could "flip the switch" in a test environment and see how much havoc is wreaked and whether authors are sufficiently responsive? Michael On Sat, Mar 4, 2017 at 12:04 PM, Martin Maechlerwrote: > > Henrik Bengtsson > > on Fri, 3 Mar 2017 10:10:53 -0800 writes: > > > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham > > wrote: > >>> But, how you propose a warning-to-error transition > >>> should be made without wreaking havoc? Just flip the > >>> switch in R-devel and see CRAN and Bioconductor packages > >>> break overnight? Particularly Bioconductor devel might > >>> become non-functional (since at times it requires > >>> R-devel). For my own code / packages, I would be able > >>> to handle such a change, but I'm completely out of > >>> control if one of the package I'm depending on does not > >>> provide a quick fix (with the only option to remove > >>> package tests for those dependencies). > >> > >> Generally, a package can not be on CRAN if it has any > >> warnings, so I don't think this change would have any > >> impact on CRAN packages. Isn't this also true for > >> bioconductor? > > > Having a tests/warn.R file with: > > > warning("boom") > > > passes through R CMD check --as-cran unnoticed. > > Yes, indeed.. you are right Henrik that many/most R warning()s would > not produce R CMD check 'WARNING's .. > > I think Hadley and I fell into the same mental pit of concluding > that such warning()s from if() ... > would not currently happen in CRAN / Bioc packages and hence > turning them to errors would not have a direct effect. > > With your 2nd e-mail of saying that you'd propose such an option > only for a few releases of R you've indeed clarified your intent > to me. > OTOH, I would prefer using an environment variable (as you've > proposed as an alternative) which is turned "active" at the > beginning only manually or for the "CRAN incoming" checks of > the CRAN team (and bioconductor submission checks?) > and later for '--as-cran' etc until it eventually becomes the > unconditional behavior of R (and the env.variable is no longer used). > > Martin > > __ > R-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel > [[alternative HTML version deleted]] __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
> Henrik Bengtsson> on Fri, 3 Mar 2017 10:10:53 -0800 writes: > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham > wrote: >>> But, how you propose a warning-to-error transition >>> should be made without wreaking havoc? Just flip the >>> switch in R-devel and see CRAN and Bioconductor packages >>> break overnight? Particularly Bioconductor devel might >>> become non-functional (since at times it requires >>> R-devel). For my own code / packages, I would be able >>> to handle such a change, but I'm completely out of >>> control if one of the package I'm depending on does not >>> provide a quick fix (with the only option to remove >>> package tests for those dependencies). >> >> Generally, a package can not be on CRAN if it has any >> warnings, so I don't think this change would have any >> impact on CRAN packages. Isn't this also true for >> bioconductor? > Having a tests/warn.R file with: > warning("boom") > passes through R CMD check --as-cran unnoticed. Yes, indeed.. you are right Henrik that many/most R warning()s would not produce R CMD check 'WARNING's .. I think Hadley and I fell into the same mental pit of concluding that such warning()s from if() ... would not currently happen in CRAN / Bioc packages and hence turning them to errors would not have a direct effect. With your 2nd e-mail of saying that you'd propose such an option only for a few releases of R you've indeed clarified your intent to me. OTOH, I would prefer using an environment variable (as you've proposed as an alternative) which is turned "active" at the beginning only manually or for the "CRAN incoming" checks of the CRAN team (and bioconductor submission checks?) and later for '--as-cran' etc until it eventually becomes the unconditional behavior of R (and the env.variable is no longer used). Martin __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickhamwrote: >> But, how you propose a warning-to-error transition should be made >> without wreaking havoc? Just flip the switch in R-devel and see CRAN >> and Bioconductor packages break overnight? Particularly Bioconductor >> devel might become non-functional (since at times it requires >> R-devel). For my own code / packages, I would be able to handle such >> a change, but I'm completely out of control if one of the package I'm >> depending on does not provide a quick fix (with the only option to >> remove package tests for those dependencies). > > Generally, a package can not be on CRAN if it has any warnings, so I > don't think this change would have any impact on CRAN packages. Isn't > this also true for bioconductor? Having a tests/warn.R file with: warning("boom") passes through R CMD check --as-cran unnoticed. Same with: if (sample(2) == 1) message("It's your lucky day today!") /Henrik PS. Does testthat signal that? > > Hadley > > -- > http://hadley.nz __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
On Fri, Mar 3, 2017 at 9:22 AM, Martin Maechlerwrote: >> Henrik Bengtsson >> on Fri, 3 Mar 2017 00:52:16 -0800 writes: > > > I'd like to propose that the whenever the length of condition passed > > to an if or a while statement differs from one, an error is produced > > rather than just a warning as today: > > >> 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 > > > There are probably legacy reasons for why this is accepted by R in the > > first place, but I cannot imagine than anyone wants to use an if/while > > statement this way on purpose. The warning about this misuse, was > > introduced in November 2002 (R-devel thread 'vector arguments to > > if()'; > https://stat.ethz.ch/pipermail/r-devel/2002-November/025537.html). > > yes, before, there was *no* warning at all and so the problem existed > in several partly important R packages. > > Now is a different time, I agree, and I even tend to agree we > should make this an error... probably however not for the > upcoming R 3.4.0 (in April which is somewhat soon) but rather > for the next version. > > > > Below is patch (also attached) that introduces option > > 'check.condition' such that when TRUE, > > ouch ouch ouch! There are many sayings starting with > "The way to hell " > > Here: > > The way to R hell starts (or "widens", your choice) by > introducing options() that influence basic language semantics > > !! > > For robust code you will start to test all code of R for all > different possible combinations of these options set I am > sure you would not want this. You only want to test with check.condition = TRUE. No new code, package updates etc should be allowed to only pass if they have to use check.condition = FALSE. > > No --- don't even think of allowing an option for something such basic! But, how you propose a warning-to-error transition should be made without wreaking havoc? Just flip the switch in R-devel and see CRAN and Bioconductor packages break overnight? Particularly Bioconductor devel might become non-functional (since at times it requires R-devel). For my own code / packages, I would be able to handle such a change, but I'm completely out of control if one of the package I'm depending on does not provide a quick fix (with the only option to remove package tests for those dependencies). My idea is that with this option, then this can be tested at runtime locally by users and developers (cf. warnPartialMatchArgs), but also via R CMD check. It would also provide CRAN with a way to check it on incoming submissions as on the test farm - eventually all CRAN packages pass without errors. This option would only exist for a number of R releases (first default to FALSE, then TRUE) and then eventually be deprecated and removed. Does this clarify my design? As an alternative to an option, one could use an environment variable R_CHECK_CONDITION that is a bit "hidden" from misuse. /Henrik > > Martin Maechler > ETH Zurich (and R Core) > > > it will generate an error > > rather than a warning (default). This option allows for a smooth > > migration as it can be added to 'R CMD check --as-cran' and developers > > can give time to check and fix their packages. Eventually, > > check.condition=TRUE can become the new default. > > > With options(check.condition = TRUE), one gets: > > >> x <- 1:2 > >> if (x == 1) message("x == 1") > > Error in if (x == 1) message("x == 1") : the condition has length > 1 > > > and > > >> while (x < 2) message("x < 2") > > Error in while (x < 2) message("x < 2") : the condition has length > 1 > > > > 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}
Re: [Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
> Henrik Bengtsson> on Fri, 3 Mar 2017 00:52:16 -0800 writes: > I'd like to propose that the whenever the length of condition passed > to an if or a while statement differs from one, an error is produced > rather than just a warning as today: >> 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 > There are probably legacy reasons for why this is accepted by R in the > first place, but I cannot imagine than anyone wants to use an if/while > statement this way on purpose. The warning about this misuse, was > introduced in November 2002 (R-devel thread 'vector arguments to > if()'; https://stat.ethz.ch/pipermail/r-devel/2002-November/025537.html). yes, before, there was *no* warning at all and so the problem existed in several partly important R packages. Now is a different time, I agree, and I even tend to agree we should make this an error... probably however not for the upcoming R 3.4.0 (in April which is somewhat soon) but rather for the next version. > Below is patch (also attached) that introduces option > 'check.condition' such that when TRUE, ouch ouch ouch! There are many sayings starting with "The way to hell " Here: The way to R hell starts (or "widens", your choice) by introducing options() that influence basic language semantics !! For robust code you will start to test all code of R for all different possible combinations of these options set I am sure you would not want this. No --- don't even think of allowing an option for something such basic! Martin Maechler ETH Zurich (and R Core) > it will generate an error > rather than a warning (default). This option allows for a smooth > migration as it can be added to 'R CMD check --as-cran' and developers > can give time to check and fix their packages. Eventually, > check.condition=TRUE can become the new default. > With options(check.condition = TRUE), one gets: >> x <- 1:2 >> if (x == 1) message("x == 1") > Error in if (x == 1) message("x == 1") : the condition has length > 1 > and >> while (x < 2) message("x < 2") > Error in while (x < 2) message("x < 2") : the condition has length > 1 > 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
[Rd] Control statements with condition with greater than one should give error (not just warning) [PATCH]
I'd like to propose that the whenever the length of condition passed to an if or a while statement differs from one, an error is produced rather than just a warning as today: > 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 There are probably legacy reasons for why this is accepted by R in the first place, but I cannot imagine than anyone wants to use an if/while statement this way on purpose. The warning about this misuse, was introduced in November 2002 (R-devel thread 'vector arguments to if()'; https://stat.ethz.ch/pipermail/r-devel/2002-November/025537.html). Below is patch (also attached) that introduces option 'check.condition' such that when TRUE, it will generate an error rather than a warning (default). This option allows for a smooth migration as it can be added to 'R CMD check --as-cran' and developers can give time to check and fix their packages. Eventually, check.condition=TRUE can become the new default. With options(check.condition = TRUE), one gets: > x <- 1:2 > if (x == 1) message("x == 1") Error in if (x == 1) message("x == 1") : the condition has length > 1 and > while (x < 2) message("x < 2") Error in while (x < 2) message("x < 2") : the condition has length > 1 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; I'm happy to file this via https://bugs.r-project.org, if preferred. /Henrik Index: