Add a `reason` for why breakpoints are not verified about debug-adapter-protocol HOT 12 CLOSED

This could be done with the addition of an optional reason on the Breakpoint.

from debug-adapter-protocol.

Is using verified:false temporarily until the location is resolved a valid use

Yea, it is. I think your behavior is pretty common and nvim-dap may want to update their verbiage. Alternatively Dart could provide a message stating something like "Code not yet loaded".

from debug-adapter-protocol.

Confusion about unverified breakpoints has come up a few times since we started using verified in this way (for ex. Dart-Code/Dart-Code#4734). Users are used to always seeing red breakpoints (when we didn't support lazy-resolving them) and now see grey breakpoints and think they're broken.

The spec also has wording that suggests verified: false means they failed to verify rather than they might not yet be verified:

   * This is shown to the user and can be used to explain why a breakpoint could
   * not be verified.

I feel like there might be value in having "not yet verified" as a first-class state, allowing editors to distinguish between:

• not yet verified (grey)
• verified (red)
• failed to verify (error !?)

We can certainly add "Breakpoint not yet verified" to the tooltip (in message), but I think making the difference between failures and lazy-verified breakpoints visible without having to hover would be much better.

from debug-adapter-protocol.

@connor4312 that sounds reasonable to me. Should I file a separate issue, or should we tweak this one to be for that (I think adding the reason would address this issue by making it clearer that this is a valid use).

I've had a number of issues raised recently around confusion about grey breakpoints because we only recently started resolving them correctly. I don't want to revert that change (because I think it provides useful information for breakpoints in invalid locations), but I do think the current behaviour (particularly when people are so used to red breakpoints always) is pretty confusing.

from debug-adapter-protocol.

Let's tweak this issue to support that. I propose the addition of a property, with some initial minimal states. I don't think we need a verified reason, it doesn't add any extra information.

interface Breakpoint {
  /**
   * A machine-readable explanation of why a breakpoint may not be verified.
   * If a breakpoint is verified or a specific reason is not known, the adapter
   * should omit this property. Possible values include:
   * 
   *  - `pending`: Indicates a breakpoint might be verified in the future, but
   *    the adapter cannot verify it in the current state. 
   *  - `failed`: Indicates a breakpoint was not able to be verified, and the
   *    adapter does not believe it can be verified without intervention.
   */
  reason?: 'pending' | 'failed';

from debug-adapter-protocol.

That looks good to me :-)

from debug-adapter-protocol.

and the

• adapter does not believe it can be verified without intervention.

Can the adapter really know this? Isn't it possible in most runtimes that a new module could be loaded at any moment where this breakpoint can be bound?

from debug-adapter-protocol.

Can the adapter really know this? Isn't it possible in most runtimes that a new module could be loaded at any moment where this breakpoint can be bound?

If "most" runtimes allow this but not "all", wouldn't that suggest some could?

I don't know the reason but the spec already has this:

  /**
   * A message about the state of the breakpoint.
   * This is shown to the user and can be used to explain why a breakpoint could
   * not be verified.
   */
  message?: string;

However this is just a string so clients can't use it to visually distinguish between reasons. If every unverified breakpoint actually means "not verified yet", then I wonder why message existed in the first place?

from debug-adapter-protocol.

For conditional breakpoints and friends, parse errors in the breakpoint expression can be shown in the message. This would be one use case of the "intervention required" state.

from debug-adapter-protocol.

If "most" runtimes allow this but not "all", wouldn't that suggest some could?

Can the Dart debugger accurately know when all files are loaded, and are new files not allowed to be dynamically loaded later?

from debug-adapter-protocol.

Can the Dart debugger accurately know when all files are loaded, and are new files not allowed to be dynamically loaded later?

I don't think so. Although @connor4312 gave some examples above of cases where breakpoints might be "definitely invalid" versus "not yet resolved". I think users should be able to tell in VS Code the difference between a "breakpoint that is not going to work" and a "breakpoint that is not yet resolved because this script isn't compiled".

from debug-adapter-protocol.

parse errors in the breakpoint expression

This makes sense to me, and invalid log messages, and also maybe there are times when the DA knows that the requested location will never be valid. I'm fine with the proposal

from debug-adapter-protocol.