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 a way for debugger variables to include fields that are expensive/have side-effects and can be lazily evaluated #134450

Closed
DanTup opened this issue Oct 5, 2021 · 17 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented Oct 5, 2021

Many languages have "properties" on classes that require executing code when executed. Users often want to see these in the debug views (Watch, Variables, Evaluation) but executing them can be expensive and/or produce side-effects.

In Dart, we evaluate these by default but have a setting to turn it off - however it's a little hard to discover this if you don't know where to look.

It would be nice if there was some way to return some "placeholder" variables from a variablesRequest that could require some user-action to evaluate. For example, given the code:

class Person {
  String name = "Danny";
  String get email {
    return "foo@example.bar";
  }
}

final a = Person();

When evaluating a, I would like to be able to return a response that includes the value for name but a lazy placeholder for email that the user can trigger to get the value. It should look natural in the UI (eg. not wrapping the value for email in some dummy object):

a
  name:     Danny
  email:    (click to evaluate)

The "click to evaluate" could perhaps just trigger evaluation of the evaluateName on the variable?

This would also potentially allow common some well-known methods like toString() to be included in the list without having to execute them (but providing an easy/convenient way for the user to see them).

@isidorn @weinand @jacob314

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Oct 6, 2021
@weinand weinand added this to the On Deck milestone Oct 6, 2021
@weinand weinand added the feature-request Request for new features or functionality label Oct 6, 2021
@weinand weinand modified the milestones: On Deck, November 2021 Nov 17, 2021
@weinand
Copy link
Contributor

weinand commented Nov 17, 2021

@DanTup this feature can be easily implemented by representing an "expensive property" as a placeholder object that the user can "expand" to trigger the expensive evaluation.
(You've alluded to this approach already in your initial comment: "wrapping the value ... in some dummy object")

VS Code's built-in js-debug uses this approach for JS/TS getters. Here is an example:

class Person {
	name = "Danny";
	get email(): string {
		return "foo@example.bar";
	}
	get address(): Address {
		return new Address();
	}
}
class Address {
	street = "Main Street";
	city = "Zurich";
}

... and here how it looks and behaves:

CleanShot 2021-11-17 at 18 56 14

The screencast clearly shows that the intermediate object is a bit noisy and makes it difficult to grasp the effective data structure. But I still think that the approach is the best what a debug adapter can achieve today within DAP's constrains and existing client UIs.

Proposal:

Instead of inventing new DAP requests I suggest that we try to find a minimal addition (e.g. "a presentation hint") to the variables request so that a client can eliminate the intermediate object from the UI. This will allow existing implementations to profit from a new clean UI without code changes and new implementations can continue to use variables requests and don't have to deal with a new DAP concept.

Some details:

  • a new boolean property lazy on the Variable indicates that the client should create a "placeholder" UI element for it.
  • a new "placeholder" UI element shows an ellipsis ('…') on the right hand side of the value (instead of an expander arrow, aka "twisty"). Clicking on the ellipsis triggers the "lazy loading" by issuing a variables request for the "lazy" variable.
  • a "lazy" variable is expected to return only a single child variable which is used to replace the properties of the placeholder. As a consequence the ellipsis disappears and a twisty is shown if the variable has children.
  • if a client does not understand "lazy" variables, it will continue to work and shows an intermediate variable like js-debug today.

@connor4312 @roblourens what do you think?

@DanTup
Copy link
Contributor Author

DanTup commented Nov 17, 2021

@weinand that approach sounds good to me :-)

@testforstephen
Copy link

  • We can use VariablePresentationHint to add a lazy property to a Variable. But how to send a second Variables request for the "lazy" variable? I don't see that current Variables request allows to add extra property.

  • Does the user have to click twice to expand the children of a lazy variable? Click once to fetch lazy part, click twice to expand its children. If so, that seems inconvenient. Can we just clicking on ellipsis (it could become a button to be more visible) to fetch lazy part?
    image

@weinand
Copy link
Contributor

weinand commented Nov 18, 2021

@testforstephen thanks for your great questions:

  • the debug adapter has to introduce an intermediate "lazy" placeholder variable (as it is done already in js-debug). Today this indirection will be visible in the UI (see screen recording from above). By adding the lazy property to that placeholder, a conforming client (e.g. VS Code) will hide the placeholder in the UI as soon as the lazy operation has returned a result.
  • no, only a single "lazy" click is needed: if the result is a simple variable its value will be shown in the placeholder. If the result is a structured variable its value will be shown in the placeholder and its children will be added expanded under the placeholder. Or in other terms: after the "lazy" click, the placeholder is replaced by the result of the "lazy" evaluation.

@connor4312
Copy link
Member

The "lazy" presentationHint sounds good, and benefits from being backwards and forwards-compatible.

We can also then move the debug.javascript.autoExpandGetters to be some debug.autoExpandLazy setting. We added that since quite a few users complained when we started requiring the second click.

@weinand
Copy link
Contributor

weinand commented Nov 29, 2021

We now have a plan how to implement this. But the implementation will is planned for an upcoming milestone.

@weinand
Copy link
Contributor

weinand commented Feb 10, 2022

@roblourens I've created (and implemented) a DAP feature request microsoft/debug-adapter-protocol#246 for the new lazy flag on the VariablePresentationHint type.

VS Code and the npm modules have been upgraded to include the "lazy" property.

And I made Mock Debug return the "lazy" flag for variables that contain the word "lazy" in their name.

CleanShot 2022-02-10 at 18 08 13@2x

The "🛌 click me to retrieve value..." is just a placeholder for the UI that VS Code should provide.
A "lazy" node should not show an expand/collapse twistie and instead of the "🛌 click ..." some kind of load/eval button. After pressing that button the button should be replaced by the variable value so that it looks identical to a non-lazy variable.

@weinand
Copy link
Contributor

weinand commented Feb 15, 2022

@roblourens I've published a pre-release version (1.49.1) of Mock Debug to the Marketplace. It contains the "lazy flag" support.

@roblourens
Copy link
Member

So the adapter sends a variable with lazy=true and a variablesReference set. The client makes a request for that variable reference and the adapter returns a single variable with no name. Then essentially the client takes the value or variable reference from that new variable and copies it onto the placeholder variable. Right?

It seems like the usage should be spelled out in the DAP more, it's not obvious that they need to return a dummy variable. What does the client do if the adapter sets lazy on a variable that doesn't have a variableReference, or returns multiple variables from the request for that reference?

@roblourens
Copy link
Member

roblourens commented Feb 18, 2022

For an evaluate request, is it up to the adapter to get the real value and not return a lazy variable? Or is it up to the client to immediately resolve a lazy variable in that case?

@weinand
Copy link
Contributor

weinand commented Feb 19, 2022

@roblourens you raise excellent questions

So the adapter sends a variable with lazy=true and a variablesReference set. The client makes a request for that variable reference and the adapter returns a single variable with no name. Then essentially the client takes the value or variable reference from that new variable and copies it onto the placeholder variable. Right?

yes, correct.

It seems like the usage should be spelled out in the DAP more, it's not obvious that they need to return a dummy variable. What does the client do if the adapter sets lazy on a variable that doesn't have a variableReference, or returns multiple variables from the request for that reference?

Agreed. Interestingly some existing debugger extensions already use exactly the proposed approach minus the "lazy" flag. So the existing "variable" request seems to suggest such an approach.
I propose that a "lazy" flag on a non structured variable or on a structured variable with more than one child is ignored, so that everything works as before.

For an evaluate request, is it up to the adapter to get the real value and not return a lazy variable? Or is it up to the client to immediately resolve a lazy variable in that case?

I propose that the client honours the "lazy" flag based on context: if a lazy variable shows up in a tree (e.g. in the debug console or WATCHES view), the client should use the same behavior as in the VARIABLES view. In other contexts (e.g. when copying to clipboard) the client could resolve a lazy variable immediately.

@weinand weinand modified the milestones: On Deck, February 2022 Feb 19, 2022
@roblourens
Copy link
Member

My thinking is that if I took a specific user action to access that property - adding $lazyInteger to a watch, debug console, or on hover, it would show the value immediately and it would not be lazy. But in the variables view which exists without my interaction, they would be lazy. This is actually what I see in chrome devtools. All of these non-lazy methods go through evaluate. (In watch/console/hover, a lazy property in an object that I expand would still have the lazy behavior, I'm just talking about the top level.)

This means that the lazy property on EvaluateResponse would be ignored. That doesn't seem right. Assuming we wouldn't change the DAP, then it seems like the right solution would be to allow a lazy EvaluateResponse but then have the client resolve it immediately.

Actually, presentationHint on EvaluateResponse is currently not referenced anywhere in vscode.

@roblourens
Copy link
Member

For the inline hints

image

I think this is probably the right thing to do - render them with whatever placeholder value the DA sends. Not sure of a better way to handle it, but I can make them refresh when the value is expanded in the variables view, at least

@roblourens
Copy link
Member

On the other hand when you send an "evaluate" request for foo.lazy, it makes sense that the client wants the real value at that moment, and it would probably make some sense to not allow lazy variables returned in an EvaluateResponse if we want to go that route.

@weinand
Copy link
Contributor

weinand commented Feb 21, 2022

My thinking is that if I took a specific user action to access that property - adding $lazyInteger to a watch, debug console, or on hover, it would show the value immediately and it would not be lazy.

yes, correct.

But in the variables view which exists without my interaction, they would be lazy.

yes. And the same applies to non-top-level lazy properties of watch/console/hover too.

This means that the lazy property on EvaluateResponse would be ignored. That doesn't seem right. Assuming we wouldn't change the DAP, then it seems like the right solution would be to allow a lazy EvaluateResponse but then have the client resolve it immediately.

yes, a debug adapter can either return a "lazy" top-level variable from the evaluate request and VS Code will resolve that immediately for all predefined contexts ('watch', 'repl', 'hover', 'clipboard'), or the debug adapter can return the variable without a "lazy" intermediary (in which case VS Code does not have to resolve anything).

inline hints ... render them with whatever placeholder value the DA sends. Not sure of a better way to handle it, but I can make them refresh when the value is expanded in the variables view, at least

yes, if a "lazy" var has a (placeholder) value, we could show that. And if the value is empty we would omit the lazy var from the inline values. Alternatively we could always omit lazy vars from the inline values completely to avoid the visual noise...
Refreshing inline values on expand in the variables view is "a nice to have" only if we show the placeholder value.

@weinand
Copy link
Contributor

weinand commented Mar 1, 2022

@DanTup the February release has a first cut of a "lazy variables" feature.

image

It would be great if you could give it a try and provide feedback.

What you have to do is explained here.

@DanTup
Copy link
Contributor Author

DanTup commented Mar 2, 2022

@weinand nice! I'll try to take a look soon, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Projects
None yet
Development

No branches or pull requests

6 participants
@roblourens @DanTup @weinand @connor4312 @testforstephen and others