Conversation
DavisVaughan
left a comment
There was a problem hiding this comment.
I do think there is an actual bug with the fact that condition should be evaluated before incrementing the hit condition value.
And we should probably think about implementing as a bare number vs a boolean expression, as I mention below
But overall looks quite nice and is something I've definitely wanted in RStudio for a long long time!
| // here, but only containing high-level information such as `search()` | ||
| // contents and `ls(rho)`. | ||
| if !self.debug_is_debugging && !matches!(info.kind, PromptKind::InputRequest) { | ||
| self.debug_dap.lock().unwrap().reset(); |
There was a problem hiding this comment.
So we do this every time we come through read-console at top level?
Is there any way to only do it after we exit the debugger?
Just curious. I know it's probably not performance sensitive at all right now, I'm just wondering if we can only call this when we really need it.
There was a problem hiding this comment.
This is the right place, for this purpose we only know a debug session has finished when we come back to a non-debug prompt.
| return Ok(RObject::from(false).sexp); | ||
| } | ||
|
|
||
| let hit_count = dap.increment_hit_count(&uri, id); |
There was a problem hiding this comment.
Just a random thought that this function is called ps_should_break and that makes me think its a simple yes/no kind of thing, but man it's doing way more now! I wonder if there is a better name we can come up with.
There was a problem hiding this comment.
Good point, I had the same feeling. It's now handle_breakpoint().
| injected: false, | ||
| condition: bp.condition.clone(), | ||
| log_message: bp.log_message.clone(), | ||
| hit_condition: bp.hit_condition.clone(), |
There was a problem hiding this comment.
I would not be opposed to parsing hit_condition as an integer here so you don't have to do it at each should-break step
There was a problem hiding this comment.
That'd be less user friendly because then users don't get parse error messages in a breakpoint block in the Console.
| @@ -497,6 +498,8 @@ impl<R: Read, W: Write> DapServer<R, W> { | |||
| injected: false, | |||
| condition: bp.condition.clone(), | |||
| log_message: bp.log_message.clone(), | |||
There was a problem hiding this comment.
I'd like to call out that the docs suggest
The expression that controls how many hits of the breakpoint are ignored.
To me that means if you set hit_condition = 2, then 2 hits should be ignored, and only on the 3rd does it stop.
But you said its a >= implementation, and that seems to go against that?
There was a problem hiding this comment.
Adapters generally use >= or == (and some don't allow simple numbers). Since I've opted for requiring numbers, it makes more sense to go with the former. That's also what's most generally useful.
| // Must drop before calling back into R to avoid deadlock | ||
| drop(dap); | ||
|
|
||
| if let Some(ref hit_condition) = hit_condition { |
There was a problem hiding this comment.
Ooooh I believe you have an ordering error!
According to the docs
* If both this property and `condition` are specified, `hitCondition` should
* be evaluated only if the `condition` is met, and the debug adapter should
* stop only if both conditions are met.
So IIUC, this whole section should come after your should_break early exit!
i.e. if my condition is x > 5 and then i have a hitCondition of 2, then when x <= 5 it should not be incrementing the hitCondition!
This would make for a good test!
| drop(dap); | ||
|
|
||
| if let Some(ref hit_condition) = hit_condition { | ||
| match hit_condition.trim().parse::<u64>() { |
There was a problem hiding this comment.
I also want to call out that I'm not sure that interpreting this as a simple number is how most people implement hit conditions
It looks like a common way to do this is to allow the user to supply a hit condition of == 5, where you fill in the LHS of the binary expression there with the current hitCount.
Then users could supply > 5 as well, for "break every time after 5 hits"
You'd support a limited set of options, like ==, >, >=, <, <=, and that would maybe be enough.
There was a problem hiding this comment.
Then again, some people do what you did
There was a problem hiding this comment.
yep the agent initially implemented a small parser along these lines, but that seemed a bit overkill to me. I thought we'd start with a simple >= interpretation.
1fb6c9b to
48fb3aa
Compare
c46455e to
542663f
Compare
04f45bb to
22a9677
Compare
542663f to
e559ab2
Compare
Branched from #1086
Addresses posit-dev/positron#12360
Adds support for DAP hit count breakpoints. When a breakpoint has a
hitCondition, the breakpoint location must be reached at least that many times before it fires. The value is interpreted as a plain integer with>=semantics. For more complex hit patterns, users can combine hit counts with conditional breakpoints.Hit counts are reset when
ReadConsolereturns to a top-level (non-browser) prompt, meaning the execution that may have hit breakpoints is complete. This way counts survive across continue/step within a debug session but start fresh for new sessions.Hit count integrates with the existing breakpoint feature chain. Hit counts with conditions require both to be satisfied, and hit counts with log messages only emit output once the threshold is reached. If the hit count can't be parsed, the error message is propagated to the Console in breakpoint fences with a clickable link.
QA Notes
Tested on the backend side.
Hit counts should behave well on their own or in combination with logpoints and conditional breakpoints. Parse errors appear in the console.