Honor debug stop from configuration providers #10999

alvsan09 · 2022-04-06T21:29:44Z

What it does

Debug configuration providers can indicate to stop debugging by
returning undefined or null on either of the functions

resolveDebugConfiguration or
resolveDebugConfigurationWithSubstitutedVariables

The vscode API
https://code.visualstudio.com/api/references/vscode-api#DebugConfigurationProvider
specifies different handling for undefined and null as follows:

"Returning the value 'undefined' prevents the debug session from
starting. Returning the value 'null' prevents the debug session from
starting and opens the underlying debug configuration instead."

This implementation focuses on passing the undefined or null
response from the extension to the debug session manager where the
indication from the extension is handled.

Signed-off-by: Alvaro Sanchez-Leon alvaro.sanchez-leon@ericsson.com

How to test

Download the following extension and place it under your theia/plugins folder
https://github.com/alvsan09/debug-activation-events/blob/master/on-debug-resolve-null.vsix
Start theia
Testing behavior for an extension returning undefined:
From the command palette (Ctrl + shft + P) , type debug , note trailing space is needed.
Select configuration named Create JavaScript Debug Terminal
Verify that no debug session is started and that a single JavaScript Debug Terminal instance is created.
This verifies the fix for issue: resolveDebugConfiguration returning 'undefined' or 'null' does not comply to vscode API #10998
Testing behavior for an extension returning null:
From the command palette (Ctrl + shft + P) , type debug , note trailing space is needed.
Select configuration named Resolve config to null (this configuration is provided by the test extension loaded in step 1)
Verify that no debug session is started and that the launch.json file is opened.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

packages/plugin/src/theia.d.ts

packages/plugin-ext/src/plugin/node/debug/debug.ts

colin-grant-work

The code looks good and the provided tests work. @vince-fugnitto mentioned VSCode's ProviderResult type, and I think it might make sense to introduce something similar in our packages/core/src/common/types.ts file to reduce the number of times we have to write | undefined | null?

alvsan09 · 2022-04-12T20:48:24Z

I think it might make sense to introduce something similar in our packages/core/src/common/types.ts file to reduce the number of times we have to write | undefined | null?]

Hi @colin-grant-work,
I introduced a new type under common so the result can be passed across all the way to the browser,

The new type is under a new commit so we can discuss this particular update separately!

I have updated all the related returns accordingly, let me know what you think !

vince-fugnitto · 2022-04-13T17:48:17Z

packages/core/src/common/types.ts

@@ -130,3 +130,5 @@ export namespace ArrayUtils {
 export function unreachable(_never: never, message: string = 'unhandled case'): never {
    throw new Error(message);
 }
+
+export type ResultPromise<T> = Promise<T | undefined | null>;


@alvsan09 I would probably keep the same name ProviderResult unless you have a strong reason to deviate? I don't think ResultPromise would be a good name for this type.

I don't think we benefit from this type at all at them moment, I would remove it.

Specifically I don't think it's super useful to use for implementations, which is its main use right now.

Looking at VS Code they have a ProviderResult used in definitions, but they never mention it in implementations.

paul-marechal · 2022-04-13T18:18:11Z

packages/core/src/common/types.ts

@@ -130,3 +130,5 @@ export namespace ArrayUtils {
 export function unreachable(_never: never, message: string = 'unhandled case'): never {
    throw new Error(message);
 }
+
+export type ResultPromise<T> = Promise<T | undefined | null>;


I don't think we benefit from this type at all at them moment, I would remove it.

packages/debug/src/browser/debug-session-manager.ts

alvsan09 · 2022-04-13T21:58:53Z

@colin-grant-work, @paul-marechal, @vince-fugnitto,
After offline discussions with Paul and Vince, there seems to be a preference to not introduce the new type,
so I have reset the tip of the branch to the previous commit (before the introduction of the new type),

Colin had already approved that commit, @vince-fugnitto and @paul-marechal any pending items before approving this PR?

Debug configuration providers can indicate to stop debugging by returning `undefined` or `null` on either of the functions - `resolveDebugConfiguration` or - `resolveDebugConfigurationWithSubstitutedVariables` The vscode API https://code.visualstudio.com/api/references/vscode-api#DebugConfigurationProvider specifies different handling for `undefined` and `null` as follows: "Returning the value 'undefined' prevents the debug session from starting. Returning the value 'null' prevents the debug session from starting and opens the underlying debug configuration instead." This implementation focuses on passing the `undefined` or `null` response from the extension to the debug session manager where the indication from the extension is handled. Signed-off-by: Alvaro Sanchez-Leon <alvaro.sanchez-leon@ericsson.com>

alvsan09 · 2022-04-13T22:11:39Z

Re-based to latest master

colin-grant-work · 2022-04-14T21:50:42Z

My approval stands, with or without the additional type. 👍

vince-fugnitto

LGTM!

alvsan09 force-pushed the asl/stop_from_debug_providers branch from e085336 to 917d913 Compare April 6, 2022 21:32

alvsan09 requested a review from colin-grant-work April 6, 2022 21:34

alvsan09 added vscode issues related to VSCode compatibility debug issues that related to debug functionality labels Apr 6, 2022

vince-fugnitto reviewed Apr 7, 2022

View reviewed changes

packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/node/debug/debug.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/node/debug/debug.ts Outdated Show resolved Hide resolved

alvsan09 force-pushed the asl/stop_from_debug_providers branch from 5ba001c to 4678360 Compare April 7, 2022 17:20

alvsan09 requested a review from vince-fugnitto April 11, 2022 16:02

colin-grant-work approved these changes Apr 12, 2022

View reviewed changes

alvsan09 force-pushed the asl/stop_from_debug_providers branch from 1f29607 to 6eab3ff Compare April 12, 2022 20:41

vince-fugnitto reviewed Apr 13, 2022

View reviewed changes

paul-marechal reviewed Apr 13, 2022

View reviewed changes

alvsan09 force-pushed the asl/stop_from_debug_providers branch from 6eab3ff to 5b1e75b Compare April 13, 2022 21:51

alvsan09 force-pushed the asl/stop_from_debug_providers branch from 5b1e75b to 4214de2 Compare April 13, 2022 22:09

vince-fugnitto approved these changes Apr 18, 2022

View reviewed changes

alvsan09 merged commit e6b57ba into master Apr 18, 2022

alvsan09 deleted the asl/stop_from_debug_providers branch April 18, 2022 14:37

github-actions bot added this to the 1.25.0 milestone Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor debug stop from configuration providers #10999

Honor debug stop from configuration providers #10999

alvsan09 commented Apr 6, 2022

colin-grant-work left a comment

alvsan09 commented Apr 12, 2022 •

edited

vince-fugnitto Apr 13, 2022

paul-marechal Apr 13, 2022

paul-marechal Apr 13, 2022

paul-marechal Apr 13, 2022

alvsan09 commented Apr 13, 2022

alvsan09 commented Apr 13, 2022

colin-grant-work commented Apr 14, 2022

vince-fugnitto left a comment

Honor debug stop from configuration providers #10999

Honor debug stop from configuration providers #10999

Conversation

alvsan09 commented Apr 6, 2022

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work left a comment

Choose a reason for hiding this comment

alvsan09 commented Apr 12, 2022 • edited

vince-fugnitto Apr 13, 2022

Choose a reason for hiding this comment

paul-marechal Apr 13, 2022

Choose a reason for hiding this comment

paul-marechal Apr 13, 2022

Choose a reason for hiding this comment

paul-marechal Apr 13, 2022

Choose a reason for hiding this comment

alvsan09 commented Apr 13, 2022

alvsan09 commented Apr 13, 2022

colin-grant-work commented Apr 14, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

alvsan09 commented Apr 12, 2022 •

edited