fix: clarify ContinuedEvent.allThreadsContinued

[connor4312]

Fixes #513

[mfussenegger]

Shouldn't this also be changed in https://microsoft.github.io/debug-adapter-protocol/specification#Events_Continued ?

debug-adapter-protocol/specification.md

Lines 315 to 319 in 7743416

	/**
	* If `allThreadsContinued` is true, a debug adapter can announce that all
	* threads have continued.
	*/
	allThreadsContinued?: boolean;

Is kind of explicit about only true implying that all threads have continued. Seems to me this is sort of a breaking change?

[connor4312]

That's what this PR changed

I don't view this as breaking: the behavior was intended to be the same as in ContinueResponse, and the description on the ContinuedEvent failed to specify the behavior if allThreadsContinued was omitted. This PR adds clarification for that case.

[mfussenegger]

I'm not sure how you can see this as not breaking.

The text says:

If allThreadsContinued is true, a debug adapter can announce that all threads have continued.

From that follows that if it is not true, only the specified threadId has continued.
not true meaning false or absent - there's no ambiguity here.

Now you redefine absent to mean true, at least 4 years after having had this text.
If I change this now in the client it means any debug adapter not specifying allThreadsContinued explicitly might have a behavior change that could be wrong?

[mfussenegger]

cc @puremourning if I'm reading this right, then vimspector is also affected by this change.

[puremourning]

Thanks @mfussenegger this does look breaking change like to me.

I mean i can't just change that code because the spec was updated! It will break where it currently works and users will legitimately complain, right?

What's the mitigation? Have we changed the spec to match defacto vscode behaviour?

[mfussenegger]

I checked the debug adapters that I care about and it looks like they either already set an explicit value or don't use the continued event so I don't care as much anymore if this is a "clarification" instead of the breaking change it is.

In any case - currently the text on the webpage contradicts the json file and that should definitely be fixed?

[connor4312]

Hey, sorry for the delay.

This is kind of an unfortunate situation. I think the intention is that the behavior should have been the same, just based on the sense in the naming and the behavior implemented in by the original DAP author in VS Code. But at this point, in any direction, some implementor will have to change. Just going down our list of implementors to see what they do:

• nvim: treats missing as false
• vscode: treats missing as true
• eclipse: treats missing as true (ref)
• emacs dape: treats missing as true (ref)
• theia: treats missing as true (ref) (but probably has a bug that it incorrectly memorizes the last present value)
• opensum: treats missing as true (ref)
• VS: seems to ignore allThreadsContinued (🤔)
• emacs dap-mode: seems to ignore allThreadsContinued

I haven't looked further afield outside out list of implementors, but based on what I found it seems like the practical solution is to take this clarification. While I agree the old phrasing is definitely misleading and it may be 'more correct' to have clarified in the opposite direction, I would rather take the path that breaks fewer people and reduces the complexity of the specification (by having the same property name in a similar context work the same way in both places.)

[puremourning]

What’s most baffling to me is how @mfussenegger and I are the only ones that implemented what the spec actually says and somehow everyone else didn’t.

But it’s hard to argue with your conclusion.

So I guess we are supposed to just assume that missing means true now?

[connor4312]

I think the default assumption of missing=false/nully is good. Imo this was an oversight or typographic for this property. I'm guessing other implementors implemented it that way due to the symmetry between continue/continued.

I understand this is frustrating. Ideally, if it was one client who did it this way, I would say this is a client bug and the spec is the point of truth, even if idiosyncratic. But if almost all clients implement it one way (and therefore all DA implementations developed against those clients implement it that way) it's a hard sell to change it.

This property needed clarification, and I don't think there was good way to fix it otherwise, but I'm open to ideas if you can think of anything clever.

[connor4312]

Also: I apologize for just throwing in a PR for this without more discussion beforehand, even if we end in the same place. If you think there is a potential good alternative solution I'm happy to revert this for further discussion before the next release cycle (in ~2 weeks.)

[puremourning]

It’s all good. As I said I think your conclusion is well reasoned, and the symmetry of the result is sound.