Missing requirement to have `"request": "launch"` or `"request": "attach"` in new `startDebugging` request
jonahgraham opened this issue · 17 comments
IIUC In this comment #79 (comment) the startDebugging was specified to require "request": "launch" or "request": "attach" fields. Without them the receiver cannot distinguish if the startDebugging is for launch or attach.
As far as I understand these fields aren't specified here:
debug-adapter-protocol/debugAdapterProtocol.json
Lines 828 to 840 in 1f0a81b
Therefore this issue is to request that request field be made required in StartDebuggingRequestArguments
Thanks @jonahgraham, you're right, this is missing. Will have to look up the right JSON schema way to say this, but I think what we want is LaunchRequestArguments & { request: "launch" } | AttachRequestArguments & { request: "attach" }
Thanks @roblourens - I'm not quite sure how to write it either.
Since I originally raised this @KamasamaK noted that restart request has a similar issue.
debug-adapter-protocol/debugAdapterProtocol.json
Lines 1064 to 1076 in 1f0a81b
In that case, it's the client telling the debuggee to restart, and I think it might be implied that if it was previously a Launch, it's still a launch. The args are just to update the current config with any changes
"The latest version of the `launch` or `attach` configuration."
That is true, but it means that the correct type can't be created by simply parsing the JSON, knowledge of the running state is needed too. Not a problem in Javascript, more of an issue in Java.
LaunchRequestArguments & { request: "launch" }
Something like this:
OneOf: [
{ AllOf: { { ref: LaunchRequestArguments}, { type: object, properties: request: "launch" } } }
{ AllOf: { { ref: Attachrequest .... } }
]
I do somehting similar here: https://github.com/puremourning/vimspector/blob/master/docs/schema/vimspector.schema.json#L86-L108
I did this:
"StartDebuggingRequestArguments": {
"type": "object",
"description": "Arguments for `startDebugging` request.",
"properties": {
"configuration": {
"oneOf": [
{
"allOf": [
{ "$ref": "#/definitions/LaunchRequestArguments" },
{ "type": "object", "properties": { "request": { "type": "string", "enum": [ "launch" ] } } }
]
},
{
"allOf": [
{ "$ref": "#/definitions/AttachRequestArguments" },
{ "type": "object", "properties": { "request": { "type": "string", "enum": [ "attach" ] } } }
]
}
],
"description": "Arguments passed to the new debug session. The arguments must only contain properties understood by the `launch` or `attach` requests of the debug adapter and they must not contain any client-specific properties (e.g. `type`) or client-specific features (e.g. substitutable 'variables')."
}
},
"required": [ "configuration" ]
},
but our spec generator doesn't know how to write a TS interface for this. Before I go try to implement that, I was wondering whether there's a simpler solution.
We could split startDebugging into two requests, which would mirror the two requests for launch and attach? I think this is actually reasonable and consistent.
Or, we could come up with a way to make "launch" and "attach" required properties in Launch/AttachRequestArguments. But obviously this is a breaking change and would require some new capability, is technically redundant, and I don't like this option.
We could split startDebugging into two requests, which would mirror the two requests for launch and attach? I think this is actually reasonable and consistent.
This seems reasonable, and simple.
I'm not a big fan of having two requests, since we all properties we add to startDebugging will get duplicated.
What if we just have the configuration be described as a simple object with request being launch or attach, as in Andre's initial proposal? The only difference between the two sets of args is an optional noDebug on the LaunchRequestArguments, but since this is used only as an instruction from the UI to the DA, specification isn't required in a scenario where the DA is talking "to itself".
What if we just have the configuration be described as a simple object with request being launch or attach
Is that what @roblourens's code snippet in #333 (comment) is?
No, more like
"StartDebuggingRequestArguments": {
"type": "object",
"description": "Arguments for `startDebugging` request.",
"properties": {
"configuration": {
"type": "object",
"additionalProperties": true,
"required": ["request"],
"properties": {
"request": {
"type": "string",
"enum": ["launch", "attach"]
}
},
"description": "Arguments passed to the new debug session. The arguments must only contain properties understood by the `launch` or `attach` requests of the debug adapter and they must not contain any client-specific properties (e.g. `type`) or client-specific features (e.g. substitutable 'variables')."
}
},
"required": [ "configuration" ]
}I am mostly ambivalent about the solution. As long as we can tell whether it is a launch or attach request, as any of the proposals now do, I am fine with it.
That is also fine with me. The way I saw it, bringing in Launch/AttachRequestArguments was mainly about emphasizing the relationship between this event and those events. But the description The arguments must only contain properties understood by the `launch` or `attach` requests of the debug adapter does the same thing. @connor4312 pointed out noDebug doesn't need to be part of the spec here, __restart also isn't relevant here, so we don't actually gain anything from pulling those in.
When I saw this issue, my first reaction was to turn it down because I did not saw a need for enforcing any properties on an DA internal communication mechanism (a DA emitting startDebugging will call into itself so it will definitely "know" what the callee supports). But on second thoughts I realised that the DAP client (e.g. VS Code) might need to know whether the new session is of type "attach" or "launch" (at least to show the correct icon for the "Stop" button). But in 99.9% of all cases it will be "attach" because startDebugging will typically be used in scenarios where the debuggee launches a sub process and the DA wants to attach to the spawned process. But maybe there is a need to use "launch" too.
Anyways, I don't think that making the configuration's request property mandatory is the best approach. DAP does not use a request property anywhere. Instead DAP offers a launch and an attach request. Mapping a configuration property request to these requests is done outside of DAP in the DAP client. Making the request property mandatory would move this connection into DAP without a pressing need.
Couldn't we just add a request argument to the startDebugging request instead?
Then the request argument would be outside/independent of the configuration argument.
That sounds good to me. So that would look like
"StartDebuggingRequestArguments": {
"type": "object",
"description": "Arguments for `startDebugging` request.",
"properties": {
"configuration": {
"type": "object",
"additionalProperties": true,
"description": "Arguments passed to the new debug session. The arguments must only contain properties understood by the `launch` or `attach` requests of the debug adapter and they must not contain any client-specific properties (e.g. `type`) or client-specific features (e.g. substitutable 'variables')."
},
"request": {
"type": "string",
"enum": ["launch", "attach"]
}
},
"required": [ "configuration", "request" ]
}Should I send a PR for that or do you plan to?
Since you have startDebugging on your October plan, I suggest that you create a PR for this.
I was mainly looking at an implementation in vscode, but I can send the PR.
We can roll this back into the original issue #79