fix: Remove incorrectly filtering VS Code cargo task execution resolution by scope #10093

oeed · 2021-08-31T08:46:34Z

This fixes #9093 introduced by #8995.

A filter was present on the function that adds the execution definition to Cargo tasks. This would mean Cargo tasks defined in workspaces (i.e. .code-workspace files) would not be given an execution, leading to a There is no task provider registered for tasks of type "cargo". error as descibed in #9093. I have made a minimum reproduction setup here.

This PR essentially removes that check. The if (scope) { ... } is to handle the case where task.scope === undefined using a deprecated constructor. I'm not sure if that is ever likely to occur and can remove if not needed.

There is some discussion about whether it's necessary to filter the tasks before building them. From my understanding, it shouldn't be needed as we should provide an execution for all cargo tasks; but I'm not overly familiar with VS Code internals so I could be wrong. For more info please see the discussion on #8995

Fixes rust-lang#9093

matklad · 2021-08-31T08:52:52Z

r? @SomeoneToIgnore

oeed · 2021-08-31T08:55:25Z

It's worth noting that I've never touched VS Code extensions before; so take these changes with an huge grain of salt. There's definitely an issue, but maybe isWorkspaceFolder just needs to account for workspace scopes instead? I'm not familiar enough to know.

SomeoneToIgnore · 2021-08-31T09:03:26Z

editors/code/src/tasks.ts

+    }
+    else {
+        // if the original task did not provide a scope retain the original lack of scope
+        return new vscode.Task(


This constructor is considered deprecated: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/vscode/index.d.ts#L6743

@deprecated Use the new constructors that allow specifying a scope for the task.

So looks like VSCode forces everybody to explicitly define a workspace folder for every task.

I'm not entirely thrilled to start using deprecated APIs, so would personally concentrate on deriving the folder instead.
Seems unfair to block you too, so if somebody else feels fine, he can merge the PR.

Ah I see, so was your comment here:

Now, the biggest task is to determine the folder that should apply for your custom configuration since I did not take such cases into account.

about how to handle scope === undefined?

In that case I assumed that it should simply 'add on' the necessary parts to the task, and if the original task didn't have a scope then simply leave it undefined. From what I've seen, the function doesn't need to use the scope and so if it doesn't have one it just passes that lack of scope onwards. But once again, I've little understanding of what's going on here.

Alternatively could the check instead be if (scope === undefined)?

You've got the idea right: it's all about the scope === undefined for tasks that come from user configurations (in your case).

The fix logic can be anything that fixes things for you and preferrably is not deprecated/super hacky 🙂
I see that the scope can be scope: WorkspaceFolder | TaskScope.Global | TaskScope.Workspace so I wonder, if the TaskScope.Workspace variant a right thing to fix your issues?

TaskScope.Workspace is indeed the scope that the tasks are given, the comments on TaskScope.Global seem to imply it's not used currently.

SomeoneToIgnore · 2021-08-31T09:07:40Z

I've tried to elaborate more on the changes that caused this: #8995 (comment)

So looks like VSCode pushes everybody to explicitly specify the workspace folder for every task the plugins create (as it seems from the constructor deprecation mentioned above).

With this requirement, for user defined rust tasks and multiworkspace project, we have to default to some workspace folder, either picking the first (which effectively negates part of the #8995 reason behind described in #8955 (comment)) or find a better way to get that folder somehow.

I'm also not greatly familiar with the VSCode internals (and not able to investigate it closely this year unfortunately), but in general it all feels weird, as if we miss something very simple as "get the current workspace" method in the API for such cases. Or this can be an API design issue too.

oeed · 2021-09-05T22:01:40Z

Any further thoughts on this one? Happy to modify to skip tasks that have an undefined scope if that's the right path?

matklad · 2021-09-06T14:03:46Z

Not dug deep into this, but here's what I'd do:

return undefined if the scope is undefined, as per resolveTask docs
if that causes real problems, raise vscode issue asking why VS Code calls us with undefined scopes at all.

Alternatively, why we are returning a new task here? Can't we just set the fields of the task we are given?

oeed · 2021-09-06T22:03:56Z

My intention with using the deprecated constructor was to essentially only modify the task's exec.

That's a great point though, there's no reason the function can't just set the exec on the existing task to my eyes. Can change it to do that if in agreement.

matklad · 2021-09-06T22:12:38Z

We definitely should not call deprecated functions. Other than that — whatever gets the job done!

…

On Tuesday, 7 September 2021, Oliver Cooper ***@***.***> wrote: My intention with using the deprecated constructor was to essentially just modify the task as is. That's a great point though, there's no reason the function can't just set the exec on the existing task to my eyes. Can change it to do that if in agreement. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10093 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M4UQ2542WQQ3EGWZHLUAU3FNANCNFSM5DDTL6AA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

oeed · 2021-09-07T22:48:07Z

Setting task.execution clears the scope and results in the same error.

The official docs on the subject explicitly make a new task rather and use task.scope ?? vscode.TaskScope.Workspace to compensate for an undefined scope. I've implemented this in the above commit and from my testing works correctly without needing deprecated functions.

So this should be good for a final check I believe.

SomeoneToIgnore · 2021-09-08T07:04:33Z

So, looks like we've ended up with pretty much what was suggested in #10093 (comment) , great that it was as simple as expected.

Thank you for looking into that.
One last thing, if you want to: we can add a comment somewhere around buildCargoTask function to explain what is happening with the scope and why we set it so (your case may not so clear for people who had never dealt with predefined configs, better warn them before they twist the logic on the next refactoring)

But this is minor, feel free to r+ and merge the current changes if you've had enough of this already.

bors d+

bors · 2021-09-08T07:04:34Z

✌️ oeed can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

oeed · 2021-09-08T22:34:45Z

bors r+

bors · 2021-09-08T22:41:21Z

Build succeeded:

10093: fix: Remove incorrectly filtering VS Code cargo task execution resolution by scope r=oeed a=oeed This fixes rust-lang#9093 introduced by rust-lang#8995. A filter was present on the function that adds the execution definition to Cargo tasks. This would mean Cargo tasks defined in workspaces (i.e. `.code-workspace` files) would not be given an execution, leading to a `There is no task provider registered for tasks of type "cargo".` error as descibed in rust-lang#9093. I have made a minimum reproduction setup [here](https://github.com/oeed/ra-workspace). This PR essentially removes that check. The `if (scope) { ... }` is to handle the case where `task.scope === undefined` using a deprecated constructor. I'm not sure if that is ever likely to occur and can remove if not needed. There is some discussion about whether it's necessary to filter the tasks before building them. From my understanding, it shouldn't be needed as we should provide an execution for all `cargo` tasks; but I'm not overly familiar with VS Code internals so I could be wrong. For more info please see [the discussion](rust-lang#8995 (comment)) on rust-lang#8995 Co-authored-by: Oliver Cooper <[email protected]>

lnicola · 2021-09-09T11:04:38Z

changelog fix (first contribution) fix cargo task filtering in Code workspaces

Don't filter cargo tasks by scope

0517247

Fixes rust-lang#9093

oeed changed the title ~~Remove incorrectly filtering VS Code cargo task execution resolution by scope~~ fix: Remove incorrectly filtering VS Code cargo task execution resolution by scope Aug 31, 2021

SomeoneToIgnore reviewed Aug 31, 2021

View reviewed changes

Removed deprecated Task constructor

0016089

Undefined scope comment

d246a5f

bors bot merged commit 22162aa into rust-lang:master Sep 8, 2021

oeed deleted the fix-9093-task-resolution branch September 9, 2021 00:04

fix: Remove incorrectly filtering VS Code cargo task execution resolution by scope #10093

fix: Remove incorrectly filtering VS Code cargo task execution resolution by scope #10093

Uh oh!

Conversation

oeed commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Aug 31, 2021

Uh oh!

oeed commented Aug 31, 2021

Uh oh!

SomeoneToIgnore Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

oeed Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

oeed Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oeed commented Sep 5, 2021

Uh oh!

matklad commented Sep 6, 2021

Uh oh!

oeed commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Sep 6, 2021 via email

Uh oh!

oeed commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomeoneToIgnore commented Sep 8, 2021

Uh oh!

bors bot commented Sep 8, 2021

Uh oh!

oeed commented Sep 8, 2021

Uh oh!

bors bot commented Sep 8, 2021

Uh oh!

lnicola commented Sep 9, 2021

Uh oh!

Uh oh!

oeed commented Aug 31, 2021 •

edited

Loading

SomeoneToIgnore commented Aug 31, 2021 •

edited

Loading

oeed commented Sep 6, 2021 •

edited

Loading

oeed commented Sep 7, 2021 •

edited

Loading