setBreakpoints is called with sourceModified:false when source is dirty and new breakpoint is added

[DanTup]

It's not clear from the DAP spec exactly what the intended behaviour is here, so I've filed https://github.com/microsoft/debug-adapter-protocol/issues/376. However, this part definitely seems like a bug.

I added a breakpoint to a file, modified the file, and then added another. At no point did I save the file.

In the DAP log, I see that VS Code called setBreakpoints after I added the second breakpoint but with sourceModified: false. This seems to be incorrect.. The debug adapter now thinks these breakpoints are for a saved file, but they are not.

[11:08:17] [DAP] [Info] ==> {"command":"setBreakpoints","arguments":{"source":{"name":"package:flutter_counter/main.dart","path":"/Users/danny/Desktop/dart_samples/flutter_counter/lib/main.dart","sourceReference":0},"lines":[14],"breakpoints":[{"line":14}],"sourceModified":false},"type":"request","seq":8}
[11:08:21] [DAP] [Info] ==> {"command":"setBreakpoints","arguments":{"source":{"name":"package:flutter_counter/main.dart","path":"/Users/danny/Desktop/dart_samples/flutter_counter/lib/main.dart","sourceReference":0},"lines":[14,15],"breakpoints":[{"line":14},{"line":15}],"sourceModified":false},"type":"request","seq":9}

[roblourens]

DAP says it "indicates that the underlying source has been modified which results in new breakpoint locations."

So if you type above the breakpoints, pushing them down, I expect it to set sourceModified to true. Otherwise we are assuming that the original line numbers are still valid. Does that make sense?

[DanTup]

@roblourens in my testing above, sourceModified was never set to true, despite modifying files.

It also sounds to me like it means "if this flag is set, the locations are based on a dirty/unsaved file". However, in https://github.com/microsoft/vscode/issues/8077#issuecomment-230244837 @weinand says:

• source is dirty, all breakpoint modifications are tracked but no setBreakpoints requests are issued until the next save of the source. In the meantime breakpoints are shown as unverified. After the save a single setBreakpoint request with the attribute sourceModified set to true is issued.

Which seems different - it sounds like the file has been saved, and the flag is set to indicate it was modified since some other time (the start of the debug session? since the breakpoints were added?).

I think this firstly needs to be more explicit (does modified mean dirty, or does it mean modified since some other time? which is most useful may depend on whether a debug is hot reloading code during the sessions lifetime), but secondly based on my log above (and being unable to trigger sourceModified: true) it seems like this might not be working as intended here either way.

[roblourens]

I think back then it worked the way you describe. Now it works the way I describe which I think matches DAP, I am not sure whether DAP changed. In your gif, you only typed below the breakpoints so their locations did not change.

[DanTup]

which I think matches DAP

It's not clear to me what matching DAP means, because DAP only has that very vague line you quoted above.

I think the spec needs clarifying since there are at least two possible interpretations, but additionally VS Code's current behaviour doesn't seem to match either (specifically, I cannot produce a case where VS Code sends sourceModified: true whether the file is dirty/unsaved, or was just modified since starting).

[roblourens]

I think we could make the DAP description a little more clear, but what I see is that if I type above a breakpoint so that its original line number becomes invalid, then we send setBreakpoints with sourceModified: true

[DanTup]

hmm, that's not the behaviour I'm seeing (see the GIF and log above.. the log was captured while recording that GIF). Are there any conditions that should affect this?

If it is intended to work the way you describe (and we can figure out why it's not working for me), does Code additionally send a setBreakpoints request with sourceModified: false when the file is saved (and for subsequent breakpoint changes)? I really think this needs to be explicit in the spec - it's very difficult for DAs to do the right thing when things aren't documented explicitly (plus, Code behaviour often changes and it's not clear whether it's a Code bug or the DA is wrong if it's not explicit).

[roblourens]

Agreed about the spec being more explicit, but what I see in your gif is that your breakpoints are above the place in the file that you changed, so their line numbers are considered to be the same, and I think that this is what which results in new breakpoint locations in the spec is describing.

[DanTup]

@roblourens this doesn't seem to make a difference. Here I added a breakpoint, modified the file above it and then added another breakpoint, but I still get breakpoint requests with sourceModified: true and line numbers that do not match the file that the VM is running:

[11:27:51] [DAP] [Info] ==> {"command":"setBreakpoints","arguments":{"source":{"name":"package:flutter_counter/main.dart","path":"/Users/danny/Desktop/dart_samples/flutter_counter/lib/main.dart","sourceReference":0},"lines":[18],"breakpoints":[{"line":18}],"sourceModified":false},"type":"request","seq":8}
[11:27:51] [DAP] [Info] <== {"seq":14789,"type":"response","request_seq":8,"command":"setBreakpoints","success":true,"body":{"breakpoints":[{"verified":true}]}}
[11:27:55] [DAP] [Info] ==> {"command":"setBreakpoints","arguments":{"source":{"name":"package:flutter_counter/main.dart","path":"/Users/danny/Desktop/dart_samples/flutter_counter/lib/main.dart","sourceReference":0},"lines":[14,20],"breakpoints":[{"line":14},{"line":20}],"sourceModified":false},"type":"request","seq":9}
[11:27:55] [DAP] [Info] <== {"seq":14799,"type":"response","request_seq":9,"command":"setBreakpoints","success":true,"body":{"breakpoints":[{"verified":true},{"verified":false}]}}

In the second request includes:

[{"line":14},{"line":20}],"sourceModified":false

But line 20 is not the line where this breakpoint was according to the VM, but VS Code has not given any indication to the DA that this file has been modified, so there's no way the DA can handle this. As far as it can tell, the user deleted the breakpoint on line 18 and added another on line 20.

I agree the spec definitely needs improving, but I also think VS Code's current behaviour doesn't make any sense either.

[roblourens]

Ok, yeah there is an issue with VS Code's behavior too. What I said before was partially correct, you should be getting an event with sourceModified=true once the file is saved. But that isn't remembered, and anything that triggers a normal setBreakpoints, like adding a new breakpoint, will send the event with sourceModified=false again. I need to look into this a bit.

In your scenario did you save the file?

[DanTup]

Nope, no saving in the above.

I am interested in saving though... I'm trying to make sure the workflow is good when we're hot reloading. I don't think the current VS Code APIs are going to be good enough though, which is why I think the behaviour needs to be very explicit.

I think the behaviour wanted by a DA that is hot reloading is different to one that is not hot reloading. And one that is reloading on save may be different to user-trigger hot reloads (or, one that is hot reloading on-save, but may skip some reloads due to compile errors) 😬