microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.94k stars 29.18k forks source link

Code action not working even though command is invoked #173736

Open luabud opened 1 year ago

luabud commented 1 year ago

Type: Bug

Even though this problem happens with our Python related extensions, I don't think it's something related to them control as the commands seem to be triggered properly 🤔

Repro steps:

  1. Install the pre-release versions of our Python, Pylint and isort extensions
  2. Create a new Python file with import os, sys and save it (e.g. hello.py)
  3. When all the extensions activate, you will see a few warnings over that line and quick fixes: image
  4. Select Pylint: Run organize imports

Nothing happens. Now if you run the Organize Imports command from the command palette, it works as expected. The problem is the Pylint extension seems to be triggering the right command, if you open the Developer Tools it shows the right command was triggered:

image

These are the logs for when the organize imports command is run:

image

VS Code version: Code - Insiders 1.76.0-insider (25514d899f4bd877803defb213719f8af3d997d3, 2023-02-07T05:22:19.428Z) OS version: Windows_NT x64 10.0.22621 Modes: Sandboxed: Yes

System Info |Item|Value| |---|---| |CPUs|11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz (8 x 2995)| |GPU Status|2d_canvas: enabled
canvas_oop_rasterization: disabled_off
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_renderer: enabled_on
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: disabled_off| |Load (avg)|undefined| |Memory (System)|31.71GB (15.09GB free)| |Process Argv|--user-data-dir nonexisting3232 --extensions-dir nonexisting3232/extfolder --crash-reporter-id 11494669-52ca-4f2c-aa0e-29172189cc8e| |Screen Reader|no| |VM|0%|
Extensions (10) Extension|Author (truncated)|Version ---|---|--- isort|ms-|2023.9.10371012 pylint|ms-|2023.1.10381730 python|ms-|2023.3.10381854 vscode-pylance|ms-|2023.2.13 jupyter|ms-|2023.2.1000391021 jupyter-keymap|ms-|1.0.0 jupyter-renderers|ms-|1.0.14 vscode-jupyter-cell-tags|ms-|0.1.6 vscode-jupyter-slideshow|ms-|0.1.5 cpptools|ms-|1.14.1
A/B Experiments ``` vsliv695:30137379 vsins829:30139715 vsliv368cf:30146710 vsreu685:30147344 python383cf:30185419 vspor879:30202332 vspor708:30202333 vspor363:30204092 vslsvsres303:30308271 pythonvspyl392:30422396 pythontb:30258533 vsc_aa:30263845 pythonptprofiler:30281269 vshan820:30294714 pythondataviewer:30285072 vscod805:30301674 bridge0708:30335490 bridge0723:30353136 cmake_vspar411:30581797 vsaa593:30376534 pythonvs932:30404738 cppdebug:30492333 vsclangdf:30492506 c4g48928:30535728 dsvsc012cf:30540253 pynewext54:30618038 pylantcb52:30590116 pyindex848:30611229 nodejswelcome1:30587009 pyind779:30611226 pythonsymbol12:30651887 2i9eh265:30646982 showlangstatbar:30657381 ```
luabud commented 1 year ago

Interestingly enough, it only doesn't work if I use the lightbulbs in the editor. They work from the problems window:

codeactionbugfeb2023

mjbvz commented 1 year ago

The pylint quick fix here returns a code action that has command = 'editor.action.organizeImports'. When this command is run as part of applying the code action, it seems that no source.organizeImports code action is returned to VS Code. That's why you see the notification

However when source.organizeImports is invoked manually using Source: Organize imports from the command palette, then I do see an code action returned (specifically isort: Organize Imports )

I think this needs a bit more investigation on the python side. Specifically try seeing why isort does not return a code action in the first case. It's still possible there's a bug with VS Code here but we need more investigation first to say what the bug is

@luabud Can you please follow up and move this issue over to isort if necessary


As an aside: a code action using editor.action.organizeImports as a command is fishy. Can pylint return a code action that does the edit itself instead of delegating like this?

luabud commented 1 year ago

Thanks @mjbvz, I'm moving this to the isort repo while we investigate further. Would you mind elaborating why delegating the action is fishy? Because we're investing in making it easier for Python tools to create extensions for VS Code and integrate with our existing ones, the idea is that we would trigger organize imports with whatever tools users decide to use. This way it would work the same whether users were using the isort extension or e.g. the Ruff one.

luabud commented 1 year ago

and speaking of Ruff, the same issue is happening when using it. I'll debug the extension later and see if I find more clues on what's going on

karthiknadig commented 1 year ago

@mjbvz isort extension does not receive a code action request in the above scenario. In the second case it does get the code action request. isort registers for sourceOrganizeImports and QuickFix

Also, the reason why pylint does not do the edit, is because how people configure their imports to be organized is handled by isort. Pylint detects these issues and usually the way there are addressed is by running isort, ruff, ufmt, or ssort etc, which ever extension user has installed.

This is how it came to be: Let us say linter identifies a problem. There is already an existing command that handles this, and users have to run that command to address the problem. So, when they see a problem in the problems window, and we tell users to run a command. That is an extra step, they could just click on the command directly in the problems window itself. From this context, there should be no difference whether the command was run by user manually or triggered from Code action.

We do the same thing when pylint detects that content is not formatted properly. We provide code action command to format document. Which formatter has to be used is something that users can decide by installing the corresponding extension.

mjbvz commented 1 year ago

Would you mind elaborating why delegating the action is fishy?

I have two concerns:

mjbvz commented 1 year ago

@karthiknadig Ok, I debugged this more and it does seem like a VS Code issue.

First I see a code actions request for source.organizeImports going into isort code:

Screenshot 2023-02-07 at 3 04 55 PM

However this code action request is cancelled so we end up discarding the result. This happens because the original code action completes, which itself then triggers a request for quick fixes because new ones may now be available:

https://github.com/microsoft/vscode/blob/7e54126f9e85baa6f1f1ca3bceb1db67c85b406b/src/vs/editor/contrib/codeAction/browser/codeActionCommands.ts#L119

I don't think we ever considered a code action triggering another code action request while the original code action is still being applied


Will see about fixing this on the VS Code side but my recommendation would be to move off of this pattern for the reasons I outlined above

mjbvz commented 1 year ago

Minimal repo using code action provider samples:

/*---------------------------------------------------------
 * Copyright (C) Microsoft Corporation. All rights reserved.
 *--------------------------------------------------------*/

import * as vscode from 'vscode';

const COMMAND = 'code-actions-sample.command';

export function activate(context: vscode.ExtensionContext) {

    const emojiDiagnostics = vscode.languages.createDiagnosticCollection("emoji");
    context.subscriptions.push(emojiDiagnostics);

    context.subscriptions.push(
        vscode.languages.registerCodeActionsProvider('markdown', new ActionProvider(), {
            providedCodeActionKinds: ActionProvider.providedCodeActionKinds
        }),
        vscode.languages.registerCodeActionsProvider('markdown', new OrganizeImportsProvider(), {
            providedCodeActionKinds: OrganizeImportsProvider.providedCodeActionKinds
        })
    );

    context.subscriptions.push(
        vscode.commands.registerCommand(COMMAND, () => vscode.env.openExternal(vscode.Uri.parse('https://unicode.org/emoji/charts-12.0/full-emoji-list.html')))
    );
}

/**
 * Provides code actions for converting :) to a smiley emoji.
 */
class ActionProvider implements vscode.CodeActionProvider {

    static providedCodeActionKinds = [
        vscode.CodeActionKind.QuickFix
    ];

    provideCodeActions(document: vscode.TextDocument, range: vscode.Range | vscode.Selection, context: vscode.CodeActionContext, token: vscode.CancellationToken): vscode.ProviderResult<(vscode.CodeAction | vscode.Command)[]> {

        const action = new vscode.CodeAction('Indirectly organize imports', vscode.CodeActionKind.QuickFix);
        action.command = { command: 'editor.action.organizeImports', title: 'Learn more about emojis', tooltip: 'This will open the unicode emoji page.' };
        return [action];
    }
}

class OrganizeImportsProvider implements vscode.CodeActionProvider {

    static providedCodeActionKinds = [
        vscode.CodeActionKind.SourceOrganizeImports
    ];

    async provideCodeActions(document: vscode.TextDocument, range: vscode.Range | vscode.Selection, context: vscode.CodeActionContext, token: vscode.CancellationToken) {
        console.log('provideCodeActions organize imports' , token.isCancellationRequested);

        token.onCancellationRequested(() => {
            console.log('cancelled');
        });

        await new Promise(resolve => setTimeout(resolve, 10));

        const action = new vscode.CodeAction('Source organize', vscode.CodeActionKind.SourceOrganizeImports);
        action.command = { command: COMMAND, title: 'Learn more about emojis', tooltip: 'This will open the unicode emoji page.' };
        return [action];
    }
}
shubhamjain0594 commented 7 months ago

Any updates on this one? Still facing this issue.

justschen commented 7 months ago

@shubhamjain0594 sorry, will give this another look this coming iteration since the work around the trigger is slightly related to some other work done this iteration.

voxlz commented 4 months ago

Just ran into this a well.