R devel - Mar 2017 - 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 Maechler <maechler at
stat.math.ethz.ch
> wrote:
> >>>>> Henrik Bengtsson <henrik.bengtsson at gmail.com>
> >>>>>     on Fri, 3 Mar 2017 10:10:53 -0800 writes:
>
>     > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham
>     > <h.wickham at gmail.com> 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(<length-larger-one>)  ...
> 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 at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>
	[[alternative HTML version deleted]]

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 Lawrence
<lawrence.michael at gene.com> wrote:
> 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
> <maechler at stat.math.ethz.ch> wrote:
>>
>> >>>>> Henrik Bengtsson <henrik.bengtsson at
gmail.com>
>> >>>>>     on Fri, 3 Mar 2017 10:10:53 -0800 writes:
>>
>>     > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham
>>     > <h.wickham at gmail.com> 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(<length-larger-one>)  ...
>> 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 at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
>

>>>>> Michael Lawrence <lawrence.michael at gene.com>
>>>>>     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
    > <maechler at stat.math.ethz.ch
    >> wrote:

    >> >>>>> Henrik Bengtsson <henrik.bengtsson at
gmail.com> >>>>>
    >> on Fri, 3 Mar 2017 10:10:53 -0800 writes:
    >> 
    >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham >
    >> <h.wickham at gmail.com> 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(<length-larger-one>) ...  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 at r-project.org mailing list
    >> https://stat.ethz.ch/mailman/listinfo/r-devel
    >> 

    > 	[[alternative HTML version deleted]]

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 <maechler at
stat.math.ethz.ch>
wrote:
> >>>>> Michael Lawrence <lawrence.michael at gene.com>
> >>>>>     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
>     > <maechler at stat.math.ethz.ch
>     >> wrote:
>
>     >> >>>>> Henrik Bengtsson <henrik.bengtsson at
gmail.com> >>>>>
>     >> on Fri, 3 Mar 2017 10:10:53 -0800 writes:
>     >>
>     >> > On Fri, Mar 3, 2017 at 9:55 AM, Hadley Wickham >
>     >> <h.wickham at gmail.com> 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(<length-larger-one>) ...  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 at r-project.org mailing list
>     >> https://stat.ethz.ch/mailman/listinfo/r-devel
>     >>
>
>     >   [[alternative HTML version deleted]]
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>
	[[alternative HTML version deleted]]