IDebugProtocolServer not using existing LaunchRequestArguments

[tgodzik]

LaunchRequestArguments is not used in launch request in IDebugProtocolServer and doesn't look to be used at all.

Is that expected?

https://github.com/eclipse-lsp4j/lsp4j/blame/f235e91fbe2e45f62e185bbb9f6d21bed48eb2b9/org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/services/IDebugProtocolServer.java#L174 uses a map instead.

[jonahgraham]

Perhaps LaunchRequestArguments and launch could be better documented as to why it is that way - but we can't use LaunchRequestArguments in launch because not all the keys are known as they are implementation specific. This is one (of many) places where the stricter type system in Java doesn't map so well to the protocol.

If you have ideas of how to improve things so that partially typed value can be used here, I am open to PRs on this subject. Equally if you think that the wording can be better than "Since launching is debugger/runtime specific, the arguments for this request are not part of this specification." I am also open to PRs on this subject.

[tgodzik]

I think the biggest confusion here was that for most of the methods there is a special parameters class, so I assumed this was the case here. Had it not existed I would probably have to dig in more to see what is going on.

The other option is to have the map inside LaunchRequestArguments for all other keys aside from the known ones.

So in short I see 2 solutions:

• remove or deprecate LaunchRequestArguments altogether
• add the map to LaunchRequestArguments

This all might be related to the fact that we don't actually implement the interfaces currently (we probably should), so if this expected it's save to close the issue.

[jonahgraham]

I suspect there are similar cases in the LSP - I will let one of the LSP maintainers comment about what they do before closing.

remove or deprecate LaunchRequestArguments altogether

I am in favour of deprecating / commenting this.

add the map to LaunchRequestArguments

I don't think this really works out of the box, but I guess it could if we add a custom type adapter that maps known fields to the right values and places the rest of them in the map.

This all might be related to the fact that we don't actually implement the interfaces currently (we probably should), so if this expected it's save to close the issue.

I think that does have an effect here.

[pisv]

I don't think I can really add anything to this discussion, except for noticing that besides launch and attach methods, which both take a Map, there is also a similar restart method, which takes RestartArguments, whose arguments property is either LaunchRequestArguments or AttachRequestArguments.

So, there is one place where the LaunchRequestArguments type is actually used, and that usage seems to ignore the implementation-specific attributes, in which case it doesn't seem correct.

Regarding the LSP side, it looks like the established pattern there is using an Object annotated with @JsonAdapter(JsonElementTypeAdapter.Factory) rather than a Map for representing properties of any type, e.g.

lsp4j/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend

Lines 4947 to 4951 in f235e91

	/**
	* User provided initialization options.
	*/
	@JsonAdapter(JsonElementTypeAdapter.Factory)
	Object initializationOptions

Having said that, I'd be in favour of keeping the existing method signatures for launch and attach to avoid introducing a breaking change in this case.