Provide default implementation for stepIn and stepOut redirecting to

apupier commented 1 year ago

fixes #743

apupier commented 1 year ago

it failed only on Windows with somethign which seems unrelated to my changes:

Execution failed for task ':org.eclipse.lsp4j.jsonrpc:test'.
> There were failing tests. See the report at: file:///D:/a/lsp4j/lsp4j/org.eclipse.lsp4j.jsonrpc/build/reports/tests/test/index.html

jonahgraham commented 1 year ago

it failed only on Windows with somethign which seems unrelated to my changes:

Passes now on re-run. I'll review soon.

apupier commented 9 months ago

up! :-)

(Not urgent but I was checking my list of opened PR)

jonahgraham commented 9 months ago

Sorry - fell off my list I guess. Thank you for pinging!

I don't think this change is correct, here are my thoughts

IDebugProtocolServer provides simply an interface, the behaviour is defined by the adapter itself. All the other methods don't provide such a default. That is consistent with implementation decision in https://github.com/eclipse-lsp4j/lsp4j/tree/main/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/services too
I assume the reason for this change is this line of the spec "If the request cannot step into a target, stepIn behaves like the next request." but that decision about doing a next vs stepIn is normally a context dependent decision by the adapter rather that an always thing. e.g. I don't think anything more than a trivial debugger would work without an implemented stepIn
the stepOut is not documented to fall back to next, although I suppose that would be an acceptable behaviour if the debugger can tell ahead of time that stepOut won't work. In practice debuggers like gdb can't tell ahead of time the stepOut won't work and will just continue never hitting a stop point

All that said, I don't think that there is much harm in this change, but I don't really want to set a precedent of adding logic into lsp4j.

WDYT?

jonahgraham commented 5 months ago

I'm going to close this - feel free to re-open if you feel that is correct after reviewing my above comment

eclipse-lsp4j / lsp4j

Provide default implementation for stepIn and stepOut redirecting to #744