Proposal: allow conditional exception breakpoints

[connor4312]

Refs: microsoft/vscode#104453 which requests the ability to set exception breakpoints which are caught conditionally.

I think the implementation can be straightforward from a DAP perspective, with the addition of a new switch in the ExceptionBreakpointsFilter

export interface ExceptionBreakpointsFilter {
  /**
   * Whether the user can set a condition for this exception.
   */
  supportsCondition?: boolean;

I think the logical place would be in the exceptionOptions, however it doesn't look like VS Code uses this property and it hasn't been referenced or updated in this repo was initially pushed to Github. Would this be a reason to start using it?

export interface ExceptionOptions {
  /**
   * An optional expression for conditional breakpoints. It may only be
   * present 'supportsCondition' was given in the breakpoint filter.
   */
  condition?: string;

cc @isidorn

[weinand]

What DAP already provides:

Currently DAP provides two independent mechanisms for configuring exception behaviour:

• ExceptionBreakpointsFilters cover sets of exceptions in an abstract way: DAP provides a way to return "ExceptionBreakpointsFilters" that the DAP client presents in a checkbox-based UI with a DAP provided label and internal ID. When debugging starts, the DAP client passes the IDs of all checked filters to the debug adapter via the setExceptionBreakpoints request. The semantics of the filters is defined in and only known to the debug adapter. This mechanism is useful if the number of filters is small. Here is an example from the JavaScript debuggers:
  
• ExceptionOptions can be used to configure exception behavior if there are many exceptions organised in categories or even as a (class-)tree. With this a DAP client could show a UI like this (from VS 2019):
  
  In contrast to the ExceptionBreakpointsFilters mechanism, DAP does not provide protocol to retrieve the set of all exceptions because it is assumed that the set of exceptions is either statically configured or it is dynamically determined by some language server (but not by the debugger or runtime). For this reason it is not possible to use IDs for communicating configured exceptions back to the debug adapter in the setExceptionBreakpoints request. Instead the selected exceptions (or whole subtrees thereof) are expressed with their names in a "glob-pattern" like way with the path property of ExceptionOptions and one or more ExceptionPathSegments. In addition DAP clients cannot easily provide a generic exception configuration UI because that would rely on some addition services outside of the DAP.
  (For details about the design discussions that ultimately lead to the ExceptionOptions spec please see the original feature request).

How does the new request for "conditional exceptions" fit into the existing mechanisms?

In general supporting an optional conditional makes sense both on the ExceptionBreakpointsFilters and on ExceptionOptions.

In the former case the problem is that it is not possible to communicate a condition back into the debug adapter because setExceptionBreakpoints only expects to receive filter IDs. And since filters are not reified, it is not possible to add a condition to them.

For the latter case (ExceptionOptions) it is easy to add a condition property as suggested in the original comment. But here the problem is that a single "conditional exception" doesn't fit well into a mechanism that was designed to support "hundreds of exceptions" and where no generic UI is available.

Proposal

see below: #137 (comment)

[connor4312]

Thank you for the context. That makes sense. I could actually see exceptionoptions being useful for js-debug in browser breakpoints, but that user experience that VS has is not very Code-like...

Probably the least ugly way to enable this for filter-based exceptions would be adding supportsCondition to ExceptionBreakpointsFilter, and then deprecate SetExceptionBreakpointsParams.filters in favor of some new field filterOptions: { filter: string; condition?: string }[];. (I can't think of a good alternative name for this property at the moment.)

Alternately, add a new conditions: string[] in SetExceptionBreakpointsParams whose indices match up with the filters, but this is awkward to deal with.

[weinand]

@connor4312 thanks a lot for your feedback.

Here is a proposal:
(I've marked everything that is new with the word "NEW")

First the capability that controls whether a client is allowed to use filter options (i.e. for now only a "condition")
I'm not yet sure whether we need a capability for the other direction as well.

interface Capabilities {
  // ...

  /**
   * NEW: The debug adapter supports 'filterOptions' as an argument on the 'setExceptionBreakpoints' request.
   */
  supportsExceptionFilterOptions?: boolean;

  // ...
}

The center of the feature is to allow passing "filter options" instead of just "filter IDs" to the setExceptionBreakpoints request. Since DAs need to support the filters property on old clients anyway, a DA should support both arguments in parallel.

interface SetExceptionBreakpointsArguments {
  /**
   * IDs of checked exception options. The set of IDs is returned via the 'exceptionBreakpointFilters' capability.
   */
  filters: string[];

  /**
   * NEW: Configuration options for exception filters returned by the 'exceptionBreakpointFilters' capability.
   * This attribute is only honored by a debug adapter if the capability 'supportsExceptionFilterOptions' is true.
   */
  filterOptions: ExceptionFilterOptions[];

  /**
   * Configuration options for selected exceptions.
   * The attribute is only honored by a debug adapter if the capability 'supportsExceptionOptions' is true.
   */
  exceptionOptions?: ExceptionOptions[];
}

The filters returned from the exceptionBreakpointFilters capability can now return a supportsCondition property to enable a "condition" edit UI in the client:

interface ExceptionBreakpointsFilter {
  /**
   * The internal ID of the filter. This value is passed to the setExceptionBreakpoints request.
   */
  filter: string;
  
  /**
   * The name of the filter. This will be shown in the UI.
   */
  label: string;
  
  /**
   * Initial value of the filter. If not specified a value 'false' is assumed.
   */
  default?: boolean;

  /**
   * NEW: Whether the user can set a condition for this exception or filter.
   */
  supportsCondition?: boolean;
}

The ExceptionFilterOptions for now is just a pair of the filter ID and the condition property. We might add more options in the future.

// NEW
interface ExceptionFilterOptions {
  /**
   * ID of exception filter returned by the 'exceptionBreakpointFilters' capability.
   */
  filterID: string;

  /**
   * An optional expression for conditional exceptions.
   * The exception will break into the debugger if the result of the condition is true. 
   */
  condition?: string;
}

@isidorn @connor4312 I'd appreciate your feedback.

[isidorn]

This makes good sense to me from the protocol point of view.
I am just not sure if the supportsCondition attribute is needed on each ExceptionBreakpointFilter, since the Adapter can already say if it supports via a capability. This would give a more fine grained access that each Exception breakpoint can specify this, I am just not sure if this is needed.
Also looks like something which is easy to miss by the debug adapter, and if left out the default value would be that it does not support it.

For the VS Code side we would have to discuss how to show the UI for specifying the condition on an exception breakpoint. @connor4312 and me alreay started this discussion in another issue (can not find it now). Somewhat similar to microsoft/vscode#3646

[connor4312]

This makes sense in general to me.

• I am just not sure if the supportsCondition attribute is needed on each ExceptionBreakpointFilter, since the Adapter can already say if it supports via a capability

  I agree that one or the other would be preferable. I'm ambivalent on which one; in js I will support both easily enough. Some languages can have other limitations, so having it in ExceptionBreakpointsFilter may be better.

• Should both filters and filterOptions, since only one of them will need to be provided now?

• nit: in DAP otherwise I notice we use camel casing on "ID", e.g. threadId or dataId, so probably filterId is better for consistency

[weinand]

Thanks for the feedback!

I am just not sure if the supportsCondition attribute is needed on each ExceptionBreakpointFilter, since the Adapter can already say if it supports via a capability.

Currently there is no capability for this. Please note that we can never extend an existing capability to cover new cases because that would break backward compatibility. So we would have to introduce a new capability for this.

I agree that one or the other would be preferable. I'm ambivalent on which one;

I'm leaning towards the current proposal for the following reason:
the ExceptionBreakpointsFilter already has configuration properties for individual filters (i.e. on the "instance" level); introducing a new configuration property supportsConditionalFilters on the "class" level would introduce an unnecessary inconsistency (and would be less flexible).

Should both filters and filterOptions, since only one of them will need to be provided now

Since we will have to support the "filters" argument for all old clients that don't know about the new "filterOptions" argument, the only question is whether a debug adapter accepts both or only one. From an implementation perspective it is probably simpler to support both by having "filters" just calling "filterOptions". If we want to support only one, then we need a client capability that tells the debug adapter what to expect. I don't think that this is worth the effort.

camel casing on "ID"

good find, I'll fix the proposal.

[weinand]

Here is the updated proposal:

First the capability that controls whether a client is allowed to use filter options (i.e. for now only a "condition"):

interface Capabilities {
  // ...

  /**
   * The debug adapter supports 'filterOptions' as an argument on the 'setExceptionBreakpoints' request.
   */
  supportsExceptionFilterOptions?: boolean;

  // ...
}

The center of the feature is to allow passing "filter options" instead of just "filter Ids" to the setExceptionBreakpoints request. Since DAs need to support the current filters property on old clients anyway, a DA should support both arguments in parallel.

interface SetExceptionBreakpointsArguments {
  /**
   * Ids of checked exception options. The set of Ids is returned via the 'exceptionBreakpointFilters' capability.
   */
  filters: string[];

  /**
   * Configuration options for exception filters returned by the 'exceptionBreakpointFilters' capability.
   * This attribute is only honored by a debug adapter if the capability 'supportsExceptionFilterOptions' is true.
   */
  filterOptions: ExceptionFilterOptions[];

  /**
   * Configuration options for selected exceptions.
   * The attribute is only honored by a debug adapter if the capability 'supportsExceptionOptions' is true.
   */
  exceptionOptions?: ExceptionOptions[];
}

The filters returned from the exceptionBreakpointFilters capability can now return a supportsCondition property to enable a "condition" edit UI in the client:

interface ExceptionBreakpointsFilter {
  /**
   * The internal Id of the filter. This value is passed to the setExceptionBreakpoints request.
   */
  filter: string;
  
  /**
   * The name of the filter. This will be shown in the UI.
   */
  label: string;
  
  /**
   * Initial value of the filter. If not specified a value 'false' is assumed.
   */
  default?: boolean;

  /**
   * Whether the user can set a condition for this exception or filter.
   */
  supportsCondition?: boolean;
}

The ExceptionFilterOptions for now is just a pair of the filter ID and the condition property. We might add more options in the future.

interface ExceptionFilterOptions {
  /**
   * Id of an exception filter returned by the 'exceptionBreakpointFilters' capability.
   */
  filterId: string;

  /**
   * An optional expression for conditional exceptions.
   * The exception will break into the debugger if the result of the condition is true. 
   */
  condition?: string;
}

[isidorn]

Looks good to me

[justingrant]

Is the format of the exception condition documented anywhere? I ended up at this issue from a link in the docs, but I can't find any docs on the condition syntax itself. Specifically, is there an ability to write a condition that will only break only exceptions that are NOT thrown from files in the node_modules folder, or that are thrown from a particular set of files?

[connor4312]

The format is language-specific. In JavaScript, we take any JavaScript expression with error being the error that was thrown. We don't currently expose information about the location of the exception, however you can probably use skipFiles to do what you're after. Exceptions that are thrown and handled within skipped code will not cause the debugger to pause.