Fix accidently starting all onTaskType extensions when running any task; fixes #175821

[Mai-Lapyst]

This fixes issue #175821 by only starting extensions of the task's type when trying to reload task after a workspace save.

The original issue falsly is marked as closed, the bug itself isn't resolved; this PR fixes it for good.

To test them:

• open vscode and manually install any extension that has an onTaskType activation event such as rust-lang.rust-analyzer.
• create an new empty folder with nothing else than an package.json with following content:

  {
    "scripts": {
      "test": "echo 'sucessfull runned npm-task'"
    }
  }

• now press F1 and use Tasks: Run Task, select npm (to discover npm tasks) and run npm: test
• vscode now also activates the rust extension which was previously not loaded; this is indicated by the new rust-analyzer item in the statusbar which only gets added once the extension is loaded

[winstliu]

Does this also fix the issue where launch configs with prelaunchTask will enable everything as well? That one seems to be going through a different code path.

[Mai-Lapyst]

Does this also fix the issue where launch configs with prelaunchTask will enable everything as well?

It should; when testing with following configuration, no additional extensions are activated:

• .vscode/task.json:

  {
      "version": "2.0.0",
      "tasks": [
    	  {
    		  "type": "npm",
    		  "script": "test",
    		  "group": "test",
    		  "problemMatcher": [],
    		  "label": "run 'npm: test'",
    	  }
      ]
  }
  

• .vscode/launch.json:

  {
      "version": "0.2.0",
      "configurations": [
          {
              "type": "node",
              "request": "launch",
              "name": "Launch Program",
              "skipFiles": [
                  "<node_internals>/**"
              ],
              "program": "${workspaceFolder}/t.js",
              "preLaunchTask": "run 'npm: test'"
          }
      ]
  }
  

[meganrogge]

I tried your above steps and am not seeing the extension get activated when I run an npm task (no status bar item). When I run a command from that extension, I do see it.

Screen.Recording.2023-03-30.at.11.57.10.AM.mov

[meganrogge]

I see it work when I use a different extension with onTaskType though. Looks like they've removed that

[winstliu]

Whoops, I forgot my own repro steps :D. It needs to be a preLaunchTask set to ${defaultBuildTask}, and then one of the tasks in tasks.json needs to have

"group": {
  "kind": "build",
  "isDefault": true
}

Even if it doesn't fix that case, this is still a good improvement!

(Also, I suppose for clarity: I do not work on VS Code despite the "Member" tag)

[Mai-Lapyst]

It needs to be a preLaunchTask set to ${defaultBuildTask}

Looked into it and it's not the task that is the problem; it's the variable: when launching, it first tries to resolve all variables present inside the config:

vscode/src/vs/workbench/contrib/debug/browser/debugService.ts

Line 479 in b9202b6

let resolvedConfig = await this.substituteVariables(launch, configByProviders);

When i replace the line above with following code, it works:

let resolvedConfig = configByProviders;
resolvedConfig.preLaunchTask = 'run \'npm: test\'';

I think it's best to fix this issue in another PR, or should it better be included in this one?

[Mai-Lapyst]

Digged depper into this and it seems that the current behaviour regarding ${defaultBuildTask} is correct:

vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

Lines 305 to 306 in b9202b6

	this._configurationResolverService.contributeVariable('defaultBuildTask', async (): Promise<string | undefined> => {
	let tasks = await this._getTasksForGroup(TaskGroup.Build);

When resolving ${defaultBuildTask}, vscode needs to find all buildtasks first to then filter out the default ones; since all extensions can contribute tasks, all extensions need to be launched here.

I think it's best to discuss this behaviour and any possible change in this in a new issue.

[meganrogge]

Yes @Mai-Lapyst you are correct in your understanding and that is why I said in the original issue that the behavior was expected.

[alexr00]

I didn't test it out, but the change looks reasonable!