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

GotoTargetsArguments should have thread id #38

Open
karthiknadig opened this issue Mar 27, 2019 · 3 comments
Open

GotoTargetsArguments should have thread id #38

karthiknadig opened this issue Mar 27, 2019 · 3 comments
Labels
feature-request Request for new features or functionality

Comments

@karthiknadig
Copy link
Member

Currently GotoTargetsArguments provides only source, line, and column. This is not enough to identify if a given line is valid for the user to move there. It should have threadId as an argument. This will allow the debugger to validate.

# test.py
def foo():
    print('A')  # suppose thread 2 is stopped here

def bar():
    print('B')  # suppose thread 1 is stopped here
    print('C')

It is valid for thread 1 to move within the same block to print('C'). It is not valid for thread 2 to move to print('C'). There is no way to tell which thread the user wants to move from the arguments of gotoTargets request. The goto request will fail for thread 2, but this can be avoided.

For the screenshot below, I forced a blank targets response. This allows IDE to show changes to cursor indicating the move invalid
image

@gregg-miskelly
Copy link
Member

gregg-miskelly commented Mar 27, 2019

Note: You have a screen shot of VS, but I don't think VS could provide you that information (at least not without a bunch of work) even if a thread id was added to the protocol as at the time that the UI asks the engine for the corresponding engine API (IDebugProgram2.EnumCodeContexts]) it doesn't know a thread.

Is there a problem with just failing the goto request instead?

@int19h
Copy link

int19h commented Mar 27, 2019

It's what we're doing currently, so this is mostly about better UX.

It seems that most DAP implementations should be able to provide the thread in these circumstances, since they are issuing a goto request with that information immediately after. So I think it would be nice to have it in the protocol at least as an optional field, to encourage future implementations to provide it. The adapter can then try to do the best it can based on the information it has.

@weinand weinand self-assigned this Mar 27, 2019
@Trass3r
Copy link

Trass3r commented Sep 2, 2020

For the screenshot below, I forced a blank targets response. This allows IDE to show changes to cursor indicating the move invalid

An extension of this is described in https://blog.jetbrains.com/idea/2020/08/jump-to-any-line-while-debugging/ (Limitations paragraph). They apparently color the lines based on how dangerous it is to jump to that line.

@weinand weinand added the feature-request Request for new features or functionality label Nov 9, 2020
@weinand weinand removed their assignment Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

5 participants