dgkf / debugadapter

Debug Adapter Protocol implementation for R
GNU General Public License v3.0
18 stars 2 forks source link

Robustness of stdout parsing #13

Open sebffischer opened 1 year ago

sebffischer commented 1 year ago

I think the mechanism of reading stdout from the process that runs browser browser() - which is currently done using read_until_browse_prompt function - should be replaced with something else. It is possible that something calls print("Browse[1] ") and then things would break badly. Probably this does not have too high priority but I wanted to mention it.

A solution would be that the process that runs browser() does not redirect its output directly, but that it gets the commands that it should execute wrapped in a function. For example, the browser() process does not get the expression print(x) into its stdin, but send_back_to_child(print(x)). And the send_backt_to_child (should have a weirder name to avoid any name clashes) captures the output and then sends it back to the child via a pipe.

What do you think?

To do this, one would have to check before sending the command whether the command that is being sent is a standard R expression to be evaluated or a browser control symbol such as "n" or "cont" etc.

sebffischer commented 1 year ago

Maybe a simpler solution is to add a TaskCallback to the process that runs the browser that does something like cat(".______before_next_browser_propt\n"), that can be used in the read_until_browse_prompt function.

dgkf commented 1 year ago

Maybe a simpler solution is to add a TaskCallback

This is a great idea! It was weird to discover that task callbacks get called while in browser(), but maybe we can exploit it to handle some debugging steps.

Just to document the behavior a bit:

But this might be enough for us. For anything that isn't a debugger command (n, cont, etc), we already have the hook we need to execute arbitrary code as a task callback. For anything that is a debugger command, we can monitor the input to hide a debugger update. Since this is a much narrower set of inputs, this would be much easier to handle holistically.


example:

x <- function() { browser(); 1 }
addTaskCallback(function(...) { cat("<synchronize debugger here [", sys.nframe(), "]>", "\n", sep = ""); TRUE })
x()
# Called from: x()                 # 1. when using `shadow_browser`, inject sync code
# Browse[1]> "test"
# [1] "test"
# <synchronize debugger here [2]>  # 2.2 automatically sync on user code input
# Browse[1]>                       # 2.1 no entry, same as `n`, need to inject sync code
# debug at #1: [1] 1
# Browse[2]> 
# [1] 1
# <synchronize debugger here [1]>  # 3. can self-terminate callback at top level (nframe == 1)
# >
  1. Just as it is now, kick off a new task callback when a browser starts
  2. We'd still need to capture user input, but instead of blindly reading until the next browser prompt we only check for whether a debug keyword was entered
    1. If a keyword is entered, perform the keyword (n, cont, etc) and sync manually, hiding execution from the user
    2. Otherwise, the task callback handles the syncing - no extra work needed
  3. Self terminate the task callback when we reach the top level
dgkf commented 3 months ago

Now that we have the browser hook, we can much more easily capture browser steps (n, c), but we still need the task callback to emit environment update messages, so this will still be a useful suggestion.