Bug 14985 - The "debug" facility doesn't work sensibly with if, for, while, and repeat
Summary: The "debug" facility doesn't work sensibly with if, for, while, and repeat
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Language (show other bugs)
Version: R 2.15.0
Hardware: All All
: P5 major
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2012-07-11 20:10 UTC by Radford Neal
Modified: 2013-07-24 12:33 UTC (History)
1 user (show)

See Also:


Attachments
fix for problems with "debug" (6.74 KB, application/octet-stream)
2012-07-11 20:10 UTC, Radford Neal
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Radford Neal 2012-07-11 20:10:14 UTC
Created attachment 1335 [details]
fix for problems with "debug"

The "debug" facility (ie, see help(debug)) doesn't work sensibly with if, for, while, and repeat statements.  What it's supposed to do isn't documented, but what it does do makes no sense of any sort.  This is in R-2.15.0 and R-2.15.1, though it seems to go back a long time.

One major problem is the treatment of loop bodies with and without curly brackets.  It seems that the writer of the code in eval.c intended something sensible, but accidently reversed the test for curly brackets, leading to behaviour that makes no sense.  For instance, 

> f <- function (n) while (n>0) n <- n-1
> debug(f)
> f(10)
debugging in: f(2)
debug: while (n > 0) n <- n - 1
Browse[2]> 
exiting from: f(2)

Note that the loop iterations can't be stepped through.  Whereas,

> f <- function (n) while (n>0) { n <- n-1 }
> debug(f)
> f(2)
debugging in: f(2)
debug: while (n > 0) {
    n <- n - 1
}
Browse[2]> 
debug at #1: n > 0
Browse[2]> 
debug at #1: n <- n - 1
Browse[2]> 
debug at #1: n > 0
Browse[2]> 
debug at #1: n <- n - 1
Browse[2]> 
exiting from: f(2)

The loop condition is treated as a statement to step through.

Other behaviours are also bizarre.

I've attached a patch that fixes this, and cleans up a few other things, and that documents the implemented behaviour in help(debug).
Comment 1 Duncan Murdoch 2012-07-11 20:37:43 UTC
I agree it might be nice if the braces didn't have the effect they do, but I don't see why you find the display of the condition to be "bizarre".  It needs to be evaluated at the top of the loop each time; why not give the user a chance to examine the variables involved (or modify them) before doing it?
Comment 2 Radford Neal 2012-07-11 22:08:22 UTC
(In reply to comment #1)
> I agree it might be nice if the braces didn't have the effect they do, but I
> don't see why you find the display of the condition to be "bizarre".  It needs
> to be evaluated at the top of the loop each time; why not give the user a
> chance to examine the variables involved (or modify them) before doing it?

Well, to start, it isn't consistent with the behaviour with "if" statements, where there is no stopping at evaluation of the condition.  And in general, the debug facility does not stop at every sub-expression, only at statements.

If you want something more bizarre, however, try a "for" loop:

> f <- function () { a <- 0; for (i in 1:3) { a <- a+i }; a }
> debug(f)
> f()
debugging in: f()
debug at #1: {
    a <- 0
    for (i in 1:3) {
        a <- a + i
    }
    a
}
Browse[2]> 
debug at #1: a <- 0
Browse[2]> 
debug at #1: for (i in 1:3) {
    a <- a + i
}
Browse[2]> 
debug at #1: i
Browse[2]> 
debug at #1: a <- a + i
Browse[2]> 
debug at #1: i
Browse[2]> 
debug at #1: a <- a + i
Browse[2]> 
debug at #1: i
Browse[2]> 
debug at #1: a <- a + i
Browse[2]> 
debug at #1: a
Browse[2]> 
exiting from: f()
[1] 6

It stops at the variable "i", even though it is not being evaluated.  For good measure, "i" also has the wrong value.

It's obvious that the code involved has never been tested.  Much of the problem is just getting the test for curly brackets backwards, by accidently omitting a "!" (which is correctly present for the "if" statement code).
Comment 3 Radford Neal 2012-07-12 00:48:26 UTC
> That doesn't sound like an answer.  Maybe for loops are bad, but what's 
> wrong with the behaviour for while loops?

Since the documentation doesn't say anything about what's supposed to happen, you can argue that anything at all is "correct".  And maybe that's what everyone thought all these years as debug did silly things - whatever it does must be right!  

But that doesn't mean that it's actually sensible.  The user ought to have some consistent model of what is going to happen, which would seem most reasonably to be that it stops at statements only - ie, things in curly brackets, or that are the uncurly-bracketted parts of "if", "for", "while", or "repeat".  There's no reason for it to stop at the while condition, since the user can see what happened in evaluation of that condition when it stops at the next statement (assuming the condition doesn't have side effects).

And it's quite clear that the behaviour was not actually intended by whoever wrote the code - it occurs as an unintended consequence of their having gotten the check for curly braces backwards.
Comment 4 Duncan Murdoch 2012-07-12 04:26:32 UTC
On 12-07-11 5:08 PM, r-bugs@r-project.org wrote:
> https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=14985
>
> --- Comment #2 from Radford Neal<radfordneal@gmail.com>  2012-07-11 17:08:22 EDT ---
> (In reply to comment #1)
>> I agree it might be nice if the braces didn't have the effect they do, but I
>> don't see why you find the display of the condition to be "bizarre".  It needs
>> to be evaluated at the top of the loop each time; why not give the user a
>> chance to examine the variables involved (or modify them) before doing it?
> Well, to start, it isn't consistent with the behaviour with "if" statements,
> where there is no stopping at evaluation of the condition.  And in general, the
> debug facility does not stop at every sub-expression, only at statements.


That doesn't sound like an answer.  Maybe for loops are bad, but what's 
wrong with the behaviour for while loops?

Duncan Murdoch
>
> If you want something more bizarre, however, try a "for" loop:
>
>> f<- function () { a<- 0; for (i in 1:3) { a<- a+i }; a }
>> debug(f)
>> f()
> debugging in: f()
> debug at #1: {
>      a<- 0
>      for (i in 1:3) {
>          a<- a + i
>      }
>      a
> }
> Browse[2]>
> debug at #1: a<- 0
> Browse[2]>
> debug at #1: for (i in 1:3) {
>      a<- a + i
> }
> Browse[2]>
> debug at #1: i
> Browse[2]>
> debug at #1: a<- a + i
> Browse[2]>
> debug at #1: i
> Browse[2]>
> debug at #1: a<- a + i
> Browse[2]>
> debug at #1: i
> Browse[2]>
> debug at #1: a<- a + i
> Browse[2]>
> debug at #1: a
> Browse[2]>
> exiting from: f()
> [1] 6
>
> It stops at the variable "i", even though it is not being evaluated.  For good
> measure, "i" also has the wrong value.
>
> It's obvious that the code involved has never been tested.  Much of the problem
> is just getting the test for curly brackets backwards, by accidently omitting a
> "!" (which is correctly present for the "if" statement code).
>



Comment 5 Duncan Murdoch 2012-07-12 18:57:45 UTC
I wasn't writing to be argumentative, I was just asking a question.  I 
am not planning on acting on this bug report; someone else will have to 
take it up.  (I'm not saying that most of what you've fixed shouldn't be 
fixed, just that I don't have time to go through it all and check it.)

Duncan Murdoch

On 11/07/2012 7:48 PM, r-bugs@r-project.org wrote:
> https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=14985
>
> --- Comment #3 from Radford Neal <radfordneal@gmail.com> 2012-07-11 19:48:26 EDT ---
>
> > That doesn't sound like an answer.  Maybe for loops are bad, but what's
> > wrong with the behaviour for while loops?
>
> Since the documentation doesn't say anything about what's supposed to happen,
> you can argue that anything at all is "correct".  And maybe that's what
> everyone thought all these years as debug did silly things - whatever it does
> must be right!
>
> But that doesn't mean that it's actually sensible.  The user ought to have some
> consistent model of what is going to happen, which would seem most reasonably
> to be that it stops at statements only - ie, things in curly brackets, or that
> are the uncurly-bracketted parts of "if", "for", "while", or "repeat".  There's
> no reason for it to stop at the while condition, since the user can see what
> happened in evaluation of that condition when it stops at the next statement
> (assuming the condition doesn't have side effects).
>
> And it's quite clear that the behaviour was not actually intended by whoever
> wrote the code - it occurs as an unintended consequence of their having gotten
> the check for curly braces backwards.
>



Comment 6 Duncan Murdoch 2013-07-23 18:26:58 UTC
I've been looking at the browser, and I don't agree with some of your recommended changes.  In particular, I think the browser should not run through whole loops without breaking.  Here are the details of how I would (will?) change things:

1.  if (test) statement

I think it is correct to break before the if (at which point the user can examine the test), and again before the statement.  The latter makes it obvious to the user whether the test was TRUE or not.   I don't like the fact that it prints NULL when the test is FALSE, but I don't think I'd change that.

2.  if (test) { statement1; statement2 }

Currently this stops before the if, and before statement1 and statement 2 (or before NULL if test is FALSE).  Again, I think it's okay, except possibly for displaying NULL.

3.  for (i in vals) statement

Currently this stops before the loop, but not within it.  I would prefer it to stop before evaluating the statement each time, for the same reason as above.

4.  for (i in vals) { statement1; statement2 }

Currently this prints i before doing the statements; I agree that's useless.
It should stop before the loop, and then before statement1, statement2, statement1, etc.

5.  while (test) statement

Currently this acts like 2; I'd prefer it to stop before the statement each time it is evaluated.  It should handle the test like 5 below.

6.  while (test) {statement1; statement2}

Currently this stops before the loop, and before the test, and before each statement.  I think this is once too many:  it should stop before the loop, then on statement1, then statement2, then before the test, then statement1, statement2, etc.  This way the user has a chance to examine the variables involved in the test before it is executed.  It should skip this on the first time through the loop, since the user has already been shown it at the beginning.

7.  repeat statement;

This breaks before the repeat, but not before the statement. It is so rarely used that it probably doesn't matter, but I'd prefer it to stop before the 2nd and subsequent evaluations of the statement.

8.  repeat { statement1; statement2 }

Currently this stops before the repeat, then before the {, then before statement1, then statement2, then the {, etc.  I don't think it should stop before the {, but the others are okay.
Comment 7 Duncan Murdoch 2013-07-23 18:30:28 UTC
I changed the numbering of my points but didn't update the text in 5.  It should read


5.  while (test) statement

Currently this acts like 3; I'd prefer it to stop before the statement each
time it is evaluated.  It should handle the test like 6 below.
Comment 8 Radford Neal 2013-07-23 20:14:12 UTC
Comparing your proposal to what is implemented in pqR, it seems to differ in two ways.  First, you want to stop and display tests, except not the test for an "if", and not the test for a "while" the first time, only the second and later times. Second, you want to treat an "if" statement with no "else" part as if it had an "else" part that was "NULL".

Neither of these differences from pqR seem desirable to me.  The second, printing a "NULL" statement that does not appear in the user's program, seems to have no purpose, and seems certain to confuse the user.  The first, printing the tests before they are evaluated, would be useful when a test has side effects, so you can't tell what happened in it once the following statement is reached. But on the other hand, it makes the model of what the debugger is doing less clear to the user, and increases the number of steps that are gone through when debugging, which is often rather large already.

Of course, one could implement a much more sophisticated debugging facility that shows more source context to make clear where you are in the program, etc.  But I think a minimal fix to the buggy version should follow the simple model that it just stops at every statement (using the usual user view of what constitutes a "statement"), which I believe is what the programmer who wrote the code actually intended.
Comment 9 Duncan Murdoch 2013-07-23 20:31:13 UTC
But the proposal *is* to stop before evaluation of the test for both if and while, just not to do it twice.  The user can see the test when the browser stops at the beginning of the statement, so there's no need to do it again.  I do think it's useful to do it once.

Regarding if (FALSE) displaying NULL:  I could easily be convinced to drop it if nobody steps up to defend it.
Comment 10 Radford Neal 2013-07-23 23:18:39 UTC
Your proposal would indeed pause before each evaluation of the while condition, but the first pause would look different from the later ones.

Looking at an actual example might help.  Consider the following setup:

  > f <- function (x) {
  +   while (x > 2) x <- sqrt(x)
  +   x
  + }
  > debug(f)

Here is what pqR does if you just keep hitting "Return":

  > f(20)
  debugging in: f(20)
  debug at #1: {
      while (x > 2) x <- sqrt(x)
      x
  }
  Browse[2]>
  debug at #2: while (x > 2) x <- sqrt(x)
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #3: x
  Browse[2]>
  exiting from: f(20)
  [1] 1.454215

Here is your proposal:

  > f(20)
  debugging in: f(20)
  debug at #1: {
      while (x > 2) x <- sqrt(x)
      x
  }
  Browse[2]>
  debug at #2: while (x > 2) x <- sqrt(x)
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #2: x > 2
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #2: x > 2
  Browse[2]>
  debug at #2: x <- sqrt(x)
  Browse[2]>
  debug at #2: x > 2
  Browse[2]>
  debug at #3: x
  Browse[2]>
  exiting from: f(20)
  [1] 1.454215

I think this is both too long-winded, and a bit confusing: Why didn't we see x > 2 for the first iteration?  And if we're seeing x > 2, why don't we see the result of this comparision?  (In contrast, it's pretty natural to not see the values of statements, since most don't have their values printed when typed at the R prompt.)
Comment 11 Duncan Murdoch 2013-07-24 00:40:44 UTC
I won't argue against your straw man.
Comment 12 Duncan Murdoch 2013-07-24 12:33:50 UTC
Changes now committed to R-devel along with other debugging improvements.