Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide default implementation for stepIn and stepOut redirecting to #744

Conversation

apupier
Copy link
Contributor

@apupier apupier commented Jul 6, 2023

next

This is the behavior mentioned in the protocol specification.

fixes #743

next

This is the behavior mentioned in the protocol specification.

fixes eclipse-lsp4j#743

Signed-off-by: Aurélien Pupier <apupier@redhat.com>
@apupier
Copy link
Contributor Author

apupier commented Jul 6, 2023

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
Copy link
Contributor

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

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

@apupier
Copy link
Contributor Author

apupier commented Oct 20, 2023

up! :-)

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

@jonahgraham
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement default behavior for stepIn and stepOut to nextStep
2 participants