RFC: refactor breakpoints #259

KristofferC · 2019-04-02T11:21:11Z

The current breakpoint system immediately "lowers" the high level breakpoints given by a user (a high level breakpoint is for example a breakpoint for a method, or for a file:line) into a set of "lower level breakpoints" (given by a statement index into a FrameCode). This has a major drawback:

After breakpoint insertion we do not know why a breakpoint was set in a FrameCode. Was it because it was a file:line breakpoint or because it was set on a method or a function etc?
- This makes it hard for a UI to show the active breakpoints. If a user sets a breakpoint on convert you don't want to show 60 breakpoints. From a users p.o.v, they added one breakpoint. That breakpoint can apply to multiple places, but it is still one. A user likely want to toggle / remove all the places where this breakpoint applies collectively.
- We cannot insert breakpoint in new FrameCodes that are being created. That means we lose breakpoints if Revise decide to reevaluate a method because e.g. you fixed your bug and changed a + to a -. It means you can't set a breakpoint on foo and define a new foo and have the breakpoint apply.
- For file:line breakpoints, we need to know at insertion time what signature exists at that point. This can be difficult and currently needs Revise

This PR introduces two "high level breakpoint" structs called BreakpointSignature (for breakpoints on methods, signatures etc) and BreakpointFileLocation for breakpoints on file locations. Each breakpoint invocation will add one of these to a list of breakpoints _breakpoints. Upon adding a breakpoint, we look through all FrameCode we have created so far and insert the breakpoint if it matches. Upon creation of FrameCode's we insert the breakpoint if it matches. Each breakpoint struct has a field applications::Vector{BreakpointRef} which is added to as soon as we add a breakpoint to a FrameCode. This allows toggling, removal etc by just looping through this list.

This PR should be fairly non-breaking (doesn't break Debugger, but likely breaks Juno).

This PR enables the file:line breakpoints unconditionally (aka even if Revise is not loaded).

codecov-io · 2019-04-02T11:47:26Z

Codecov Report

Merging #259 into master will increase coverage by 0.04%.
The diff coverage is 73.26%.

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   88.03%   88.07%   +0.04%     
==========================================
  Files          11       11              
  Lines        1671     1719      +48     
==========================================
+ Hits         1471     1514      +43     
- Misses        200      205       +5

Impacted Files	Coverage Δ
src/construct.jl	`92.27% <100%> (+0.09%)`	⬆️
src/JuliaInterpreter.jl	`95.83% <100%> (+0.18%)`	⬆️
src/utils.jl	`84.61% <100%> (-1.1%)`	⬇️
src/types.jl	`72.91% <50%> (-11.7%)`	⬇️
src/breakpoints.jl	`87.9% <80.7%> (+12.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c7ef57...9a8f168. Read the comment docs.

timholy

This looks great, with a couple of big-picture issues worthy of careful consideration.

src/breakpoints.jl

timholy · 2019-04-02T14:42:15Z

src/types.jl

+struct BreakpointSignature <: AbstractBreakpoint
+    f # Method or function
+    sig::Union{Nothing, Type}
+    line::Int # 0 is a sentinel for first statement


How about making this a lineoffset from the first statement? That may facilitate more robust identity after revision.

Could you give an example? If Revise has revised it, we create a new FrameCode which will have updated line numbers.

This is conditioned on what I thought was your goal to try to preserve breakpoints through a revision. (You may be abandoning that goal, IIUC.) But suppose you wanted to preserve "break at the second line of the body" when you revise (1) a single + to a - somewhere in the body of your method, and (2) also make other changes elsewhere in the same source file that alter the absolute line numbers. Then if BreakpointSignature stored a line offset rather than an absolute line number, it would be unaffected by the fact that unrelated changes occurred elsewhere in the same file. Note that the user could specify the line numbers in absolute terms, I'm just proposing that when one creates the BreakpointSignature one should convert it to an offset.

I don't know. Currently I feel like just keeping the line number is the most straight forward. FWIW, I don't see breakpoints on signatures at a given line to be used that much. Just adding file:line is likely more ergonomic.

timholy · 2019-04-02T14:43:13Z

src/breakpoints.jl

-    state = bp[]
-    bp.framecode.breakpoints[bp.stmtidx] = BreakpointState(!state.isactive, state.condition)
+
+function add_breakpoint_if_match!(framecode::FrameCode, bp::AbstractBreakpoint)


Should we do this only for BreakpointSignatures? My biggest concern regards my last comment in #256 (comment): what to do about cases where users specified a breakpoint by file/line and then revised the file, thus changing the line numbers. Perhaps a more robust approach would be to convert BreakpointFileLocations to BreakpointSignatures as soon as they resolve?

Hm, perhaps a BreakpointFileLocation should only be able to bind once to a method and if the method is reevaluated you need to re-add the breakpoint. It would be similar to the current situation except that you can now do "late binding", i.e. add a breakpoint to a file:line before it is included.

But the same thing sort of applies to a breakpoint to a method or signature with a line number. Should those also only trigger once?

Yes, I'm fine with triggering only once. In Juno one might imagine seeing the breakpoint vanish when you save the file, so it should be very easy for users to compensate.

Hm, how would Juno know the breakpoint is "invalid"/"stale"? The breakpoint instance will still be there in _breakpoints, it is just that when we interpret the method, another FrameCode will be executed.

FWIW, I think Juno should implement location based breakpoints on a level higher than here. In IDEs the breakpoint typically to follow along with edits, so if you insert a line above a breakpoint, the breakpoint will move down a line (together with the rest of the text). Clearly, we cannot breakpoint(file, line) and remove every time a user enter a line, so we need to "apply" the current set of line breakpoints when the debugger is started, at the locations they currently are.

cc @pfitzseb

Revise could fire a callback notifying Juno of deleted methods. Though I guess revise() will run when the user tries the next command, not when the user saves the file. So the appropriate spot may no longer be visible...

It's also not only Revise that can redefine methods. Ctrl + enter in Juno or just re-including a file for example. Losing breakpoints because you reinclude a file would perhaps be annoying.

src/breakpoints.jl

KristofferC · 2019-04-04T08:19:13Z

Happy for more comments here. @pfitzseb would be good to know how you feel about this with regards to Juno integration.

pfitzseb · 2019-04-04T14:49:20Z

Ok, so greedy users (e.g. me) want at least the following:

Instant (and correct) feedback when setting a breakpoint.
If I have set a breakpoint on a certain line then I'd like to keep it at that line (meaning the actual code, not the line number) no matter what happens.
Have that breakpoint work no matter how often the method containing it is redefined/has it's name changed/whatever.

AFAICT, 1) works well with the current implementation of breakpoints, 2) kinda works on the Julia side but not the GUI side right now (because the breakpoints line number isn't updated, but maybe Revise had a hiccup). 3) fails.

The new implementation makes 1) basically impossible because we can only know if a breakpoint works once we hit it, right? 2) and 3) would work with this PR as long as a prospective breakpoint can bind to any number of methods.

I'll think more about this tomorrow, but for now I don't see a good way to have all three of the options above at the same time.

KristofferC · 2019-04-04T14:56:31Z

Instant (and correct) feedback when setting a breakpoint.

What kind of feedback? Breakpoints are set through the Juno interface right (I think it almost has to be) so you would have all information there.
You shouldn't even need to have JuliaInterpreter loaded to set breakpoints before starting execution.

If I have set a breakpoint on a certain line then I'd like to keep it at that line (meaning the actual code, not the line number) no matter what happens.

Yes, you would have to move the breakpoint with editing of the code which is a front end problem. Moving around the code like this should not call into JuliaInterpreter. Until debugging has started, it is just a visual thing.

When the debugger starts, you need to set the breakpoints at the line they currently are.
When the debugger exits, you should probably clear all location breakpoints.

Have that breakpoint work no matter how often the method containing it is redefined/has it's name changed/whatever.

Yes, that should work with this PR since we reinsert the breakpoints for new FrameCode's.

pfitzseb · 2019-04-04T15:08:38Z

What kind of feedback? Breakpoints are set through the Juno interface right (I think it almost has to be) so you would have all information there.
You shouldn't even need to have JuliaInterpreter loaded to set breakpoints before starting execution.

Hm. Can we guarantee that we can add a valid breakpoint wherever a user might click? What happens on empty lines/comments/outside of functions?

Yes, you would have to move the breakpoint with editing of the code which is a front end problem.

Sure, the GUI side of this is (almost) trivial.

When the debugger starts, you need to set the breakpoints at the line they currently are.
When the debugger exits, you should probably clear all location breakpoints.

Right, makes sense.

KristofferC · 2019-04-04T15:14:05Z

Hm. Can we guarantee that we can add a valid breakpoint wherever a user might click? What happens on empty lines/comments/outside of functions?

Lines outside of functions will never be reached since they will never be "activated".

For comments and empty lines, I have to look exactly into the current implementation but my guess is that it takes the next statement after the line.

Looking at PyCharm, it doesn't let you set breakpoints on empty lines or lines with comments. But they do full parsing of the code etc,

KristofferC · 2019-04-05T11:32:57Z

Pushed some fixes and changes to path handling (and corresponding tests). So you can set a breakpoint by just giving e.g. the file name. For absolute paths (e.g. from an IDE) this shouldn't change anything.

KristofferC · 2019-04-05T11:54:36Z

Also pushed a commit to clear caches, showing that the breakpoint system still works and fixes #262

timholy

A couple more minor comments.

src/types.jl

src/breakpoints.jl

timholy · 2019-04-06T09:39:18Z

To me it seems the main outstanding issues are (1) implementing pieces that need to go in Juno and (2) deciding exactly how we want these to work when methods or files are revised. For (2), I'd propose we add a new test file, revise.jl, for which Revise is a test dependency and in which we simulate various changes that might come from people using an editor other than Juno/Atom. We can then use these tests to decide what policy we want to adopt. If you agree with this proposal, I can probably help write these tests. (Many models can be found in Revise's runtests.jl file, of course.)

KristofferC · 2019-04-06T10:47:31Z

deciding exactly how we want these to work when methods or files are revised.

I've been thinking about this during the last couple of days and all I feel is:

There is no good choice when all you get is the snapshot of a new method. From an IDE it is easy since you just let the breakpoints flow with the editing of the text. I don't feel strongly about what should happen after Revise as long as I can set a breakpoint in a arbitrary file without having to track it first.

KristofferC · 2019-04-07T11:50:53Z

PR updated based on last comments.

KristofferC · 2019-04-09T10:28:50Z

@pfitzseb, It would be interesting to see how Juno works on top of this PR. So, whenever you have the time, it would be valuable to try it out and see how things work.

pfitzseb · 2019-04-09T13:21:17Z

I'll try this as soon as I have some time (hopefully tomorrow). Last days have been very busy for me, sorry.

KristofferC · 2019-04-21T08:14:50Z

So, anything left to do here for me?

I kind of want to get this in because I think it provides a better breakpoint experience than the current. and I want to add a UI for breakpoints in Debugger.jl but I don't want to put in the time to do that unless this is likely to be mergeable in a reasonable timeframe.

pfitzseb · 2019-04-21T08:36:38Z

Uh, sorry -- I was sure I left a comment here...

So yeah, I have a Atom.jl branch that uses this PR and it feels pretty good to use. Imho this is good to go as is.
We could add some convenience methods for e.g. getting the location of a breakpoint and stuff like that (basically just accessor functions for the various kinds of breakpoints this introduces), but I don't think that should block this PR.

KristofferC · 2019-05-06T14:47:00Z

@timholy How fine grained breakpoints do you want to use for Rebugger? Is it ok to have breakpoint in Rebugger set a breakpoint at file:line or do you need more fine grained control (e.g. statement index level control on specific methods).

timholy · 2019-05-07T08:22:27Z

File/line is fine. Thanks!

KristofferC · 2019-05-08T09:18:12Z

Added docs for the new types.

src/breakpoints.jl

Co-Authored-By: Tim Holy <tim.holy@gmail.com>

KristofferC · 2019-05-13T13:26:59Z

@pfitzseb, do you have a branch with the Atom update to use this PR? If so, I could try it out (would probably need installation instructions though).

pfitzseb · 2019-05-13T13:36:54Z

I have something that's compatible with these changes, but it's not quite where I want it to be. Will ping you once I have PRs up though.

KristofferC force-pushed the kc/refactor_breakpoints branch from 18e79b0 to b6490ff Compare April 2, 2019 11:23

KristofferC mentioned this pull request Apr 2, 2019

WIP: file:line breakpoints without Revise #256

Closed

3 tasks

KristofferC force-pushed the kc/refactor_breakpoints branch from b6490ff to 3be7854 Compare April 2, 2019 11:37

KristofferC force-pushed the kc/refactor_breakpoints branch 3 times, most recently from eeddca2 to 4ea763d Compare April 2, 2019 14:03

timholy reviewed Apr 2, 2019

View reviewed changes

KristofferC force-pushed the kc/refactor_breakpoints branch from 971f1e2 to 4fc42cd Compare April 2, 2019 20:19

timholy mentioned this pull request Apr 3, 2019

The interpreter is #265-ing #262

Closed

KristofferC commented Apr 4, 2019

View reviewed changes

src/breakpoints.jl Outdated Show resolved Hide resolved

KristofferC force-pushed the kc/refactor_breakpoints branch from cfc8d0f to d7f8afb Compare April 5, 2019 11:31

KristofferC force-pushed the kc/refactor_breakpoints branch 2 times, most recently from 64e58e4 to 1fd7dea Compare April 5, 2019 12:27

timholy reviewed Apr 6, 2019

View reviewed changes

src/types.jl Outdated Show resolved Hide resolved

src/types.jl Outdated Show resolved Hide resolved

src/breakpoints.jl Outdated Show resolved Hide resolved

KristofferC mentioned this pull request Apr 23, 2019

manipulating breakpoints #278

Closed

KristofferC added 6 commits April 30, 2019 12:08

refactor breakpoints

eeca8d6

fix 265

e3071a6

rename field applications to instances

85e975f

fix type on line in BreakpointFileLocation

a6215b8

add docs and test for relative paths

4734e20

fixes

6b7a76d

KristofferC force-pushed the kc/refactor_breakpoints branch from 138317d to 6b7a76d Compare April 30, 2019 13:09

fix symlinks

9a8f168

KristofferC mentioned this pull request Apr 30, 2019

fix Revise breakpoint toggling with new breakpoint implementation in JuliaInterpreter timholy/Revise.jl#297

Merged

type some arguments and fields a bit stricter

ad2bc1a

KristofferC mentioned this pull request May 6, 2019

Add a breakpoint UI JuliaDebug/Debugger.jl#181

Merged

KristofferC mentioned this pull request May 7, 2019

Couldn't make Revise work with Juno timholy/Revise.jl#296

Closed

KristofferC added 4 commits May 7, 2019 17:55

don't keep old FrameCode instances alive

64daa99

update Juno link

8ed2213

add some docs

18d28a6

update docs

e035a68

KristofferC added 2 commits May 8, 2019 12:11

fix printing for breakpoints

74c74df

fix printing of signature with line

73e1c58

timholy approved these changes May 11, 2019

View reviewed changes

src/breakpoints.jl Outdated Show resolved Hide resolved

Update src/breakpoints.jl

1d50ad0

Co-Authored-By: Tim Holy <tim.holy@gmail.com>

KristofferC merged commit cbabccf into master May 13, 2019

This was referenced May 13, 2019

Calling Regexes during FrameCode creation time can corrupt the regex call we are interpreting #255

Closed

Update version to 0.6.0 #284

Merged

KristofferC deleted the kc/refactor_breakpoints branch May 13, 2019 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: refactor breakpoints #259

RFC: refactor breakpoints #259

KristofferC commented Apr 2, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading

timholy left a comment

timholy Apr 2, 2019

KristofferC Apr 4, 2019

timholy Apr 4, 2019

KristofferC Apr 5, 2019

timholy Apr 2, 2019

KristofferC Apr 2, 2019

KristofferC Apr 2, 2019

timholy Apr 2, 2019

KristofferC Apr 2, 2019 •

edited

Loading

timholy Apr 2, 2019

KristofferC Apr 2, 2019

KristofferC commented Apr 4, 2019

pfitzseb commented Apr 4, 2019

KristofferC commented Apr 4, 2019 •

edited

Loading

pfitzseb commented Apr 4, 2019

KristofferC commented Apr 4, 2019 •

edited

Loading

KristofferC commented Apr 5, 2019 •

edited

Loading

KristofferC commented Apr 5, 2019

timholy left a comment

timholy commented Apr 6, 2019

KristofferC commented Apr 6, 2019

KristofferC commented Apr 7, 2019

KristofferC commented Apr 9, 2019

pfitzseb commented Apr 9, 2019

KristofferC commented Apr 21, 2019

pfitzseb commented Apr 21, 2019

KristofferC commented May 6, 2019 •

edited

Loading

timholy commented May 7, 2019

KristofferC commented May 8, 2019

KristofferC commented May 13, 2019

pfitzseb commented May 13, 2019

RFC: refactor breakpoints #259

RFC: refactor breakpoints #259

Conversation

KristofferC commented Apr 2, 2019 • edited Loading

codecov-io commented Apr 2, 2019 • edited Loading

Codecov Report

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Apr 4, 2019

pfitzseb commented Apr 4, 2019

KristofferC commented Apr 4, 2019 • edited Loading

pfitzseb commented Apr 4, 2019

KristofferC commented Apr 4, 2019 • edited Loading

KristofferC commented Apr 5, 2019 • edited Loading

KristofferC commented Apr 5, 2019

timholy left a comment

Choose a reason for hiding this comment

timholy commented Apr 6, 2019

KristofferC commented Apr 6, 2019

KristofferC commented Apr 7, 2019

KristofferC commented Apr 9, 2019

pfitzseb commented Apr 9, 2019

KristofferC commented Apr 21, 2019

pfitzseb commented Apr 21, 2019

KristofferC commented May 6, 2019 • edited Loading

timholy commented May 7, 2019

KristofferC commented May 8, 2019

KristofferC commented May 13, 2019

pfitzseb commented May 13, 2019

KristofferC commented Apr 2, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading

KristofferC Apr 2, 2019 •

edited

Loading

KristofferC commented Apr 4, 2019 •

edited

Loading

KristofferC commented Apr 4, 2019 •

edited

Loading

KristofferC commented Apr 5, 2019 •

edited

Loading

KristofferC commented May 6, 2019 •

edited

Loading