Apply problem matchers specified by task provider

[spoenemann]

What it does

Fixes #8755. The problemMatchers array from the Task object created by the task provider is copied to the problemMatcher field in TaskDto to be transferred to the frontend.

Caveat: problemMatcher is not yet defined in TaskDto, but it has a catch-all signature [key: string]: any. Adding that field explicitly to TaskDto leads to problems because the type is also used to transfer task data from frontend to backend, and the respective field in TaskCustomization is not compatible.

How to test

See reproduction steps in #8755.

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

[JanKoehnlein]

LGTM

[vince-fugnitto]

@spoenemann the plugin-ext unit-tests will need to be adjusted for the tasks, it is currently failing.

[spoenemann]

I fixed the test, but don't see why the build is still failing.

I also added a problemMatcher field with any type to TaskDto. As mentioned before, giving it a proper type would require to duplicate a lot of structures, which is not necessary at this point.

[vince-fugnitto]

I fixed the test, but don't see why the build is still failing.

I restarted the build, the typescript tests fail intermittently: #8360.

[spoenemann]

Thanks @vince-fugnitto!

Any opinion on the TaskDto interface extension? Ok to continue with the current compromise (added problemMatcher property, but with any type)?

[RomanNikitenko]

Hello!
I see you provided the changes for fromTask() function.
Should toTask() function have the corresponding changes?

[RomanNikitenko]

I mean fromTask and toTask functions are used for communication between theia and a plugin.
If problemMatchers were added to fromTask function - should we add some logic to toTask function?
It makes sense to check that problemMatchers are not lost at converting from TaskDto to theia.Task.

toTask function is used at resolving a task, for example.
I tried to check it using your extension, but looks like resolveTask() method is not executed for your custom resolver.

[spoenemann]

I added problemMatcher to the destructured properties of TaskDto to avoid including it in the taskDefinition. But looking at what VS Code does, I think we shouldn't copy that to the Task at this point. I added the following code to my example project:

https://github.com/spoenemann/task-problem-matcher-example/blob/f48f5823afce15bc01dfc2c24b3d869d5dbe03a3/src/extension.ts#L25-L33

It prints the problemMatchers when resolveTask is called. I tested that with the following entry in tasks.json:

{
  "version": "2.0.0",
  "tasks": [
    {
      "type": "mytask",
      "label": "Foo",
      "problemMatcher": "$mymatcher"
    }
  ]
}

When running that task, VS Code shows a message "problemMatchers: []", so the value of the problemMatcher property is not passed to the task resolver. One reason for this might be that the properties are incompatible in this direction. Here's the description of problemMatcher in tasks.json:

The problem matcher(s) to use. Can either be a string or a problem matcher definition or an array of strings and problem matchers.

This matches what we have in our TaskCustomization interface.

When I tried to run that task in Theia, I found another problem (unrelated to this change): the task resolver is not called, or at least I don't see any info message popping up. Is this a known problem?

[RomanNikitenko]

I'll test it today and write the results here

[RomanNikitenko]

I investigated the problem:

• task system resolves a task using registered resolvers
• task service takes the corresponding resolver by task type
• the problem is: the custom task from the extension comes with type shell, mytask type is present as taskType field in the configuration
• as result - ProcessTaskResolver is used for resolving the task, not custom resolver from the extension

[RomanNikitenko]

About VS Code behavior - it's really strange for me, because it looks like resolving a task works for me if I run a task from recently used section only - resolving a task doesn't work if I do it from contributed section

[RomanNikitenko]

@spoenemann
I commented above:

It makes sense to check that problemMatchers are not lost at converting from TaskDto to theia.Task

because task service resolves a task before running and precisely resolved task configuration is passed to server for running. So I was worried that problemMatchers can be lost on the resolving step.
I explored code more close and I found that problemMatchers come on server within TaskConfiguration and RunTaskOption objects, but actually task server reads problemMatchers from RunTaskOption object. So - regarding my comment above - there are should be no problems with converting task configuration.

Also I added a log and checked that with your changes problemMatchers come to task server:

I tested according to Steps to reproduce section of #8755 - it fixes the issue - dialog to pick a problem matcher is NOT shown for the mytask: Print a warning task.

But I faced with another problem - task system does not display the dialog to pick problem matcher for any detected task.

Could you check that use case on your side?

[spoenemann]

Good catch @RomanNikitenko! I fixed that by checking hasProblemMatchers to determine whether a problem matcher was passed to the Task constructor. I had to cast to a local type definition because that field is not included in the Task type.

Could you verify the fix?

[RomanNikitenko]

interface ExtendedTask extends theia.Task2

was added within the PR, but I didn't find the interface on VS Code side.
I think it's better to keep objects close to VS Code implementation.

The interface is used like:

if ((task as ExtendedTask).hasProblemMatchers) {

We could do it like:

if ((task as types.Task).hasProblemMatchers) {

In that case we could avoid adding the additional interface.
wdyt?

[spoenemann]

Ok changed that. I wasn't aware of types.Task.

If there are no objections, I'll merge this later today.

[RomanNikitenko]

Tested:

• using test extension from Steps to reproduce section of Problem matchers specified by task provider are not respected #8755 - task system doesn't ask about problem matchers at running mytask: Print a warning task
• for detected npm:install and npm:build tasks - task system asks about problem matchers if they are not provided by task provider

I didn't notice any regression.